Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1666588
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v1 1/1] gpio: gpio-wcove: Fix GPIO control register offset calculation |
| Date | 2017-06-15 11:10 +0200 |
| Message-ID | <tSyWS-yc-29@gated-at.bofh.it> (permalink) |
| References | <tSob8-22d-9@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Thu, Jun 15, 2017 at 12:39 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> According to Whiskey Cove PMIC GPIO controller specification, for GPIO
> pins 0-12, GPIO input and output register control address range from,
>
> 0x4e44-0x4e50 for GPIO outputs control register
>
> 0x4e51-0x4e5d for GPIO input control register
>
> But, currently when calculating the GPIO register offsets in to_reg()
> function, all GPIO pins in the same bank uses the same GPIO control
> register address. This logic is incorrect. This patch fixes this
> issue.
>
> This patch also adds support to selectively skip register modification
> for virtual GPIOs.
>
> In case of Whiskey Cove PMIC, ACPI code may use up 94 virtual GPIOs.
> These virtual GPIOs are used by the ACPI code as means to access various
> non GPIO bits of PMIC. So for these virtual GPIOs, we don't need to
> manipulate the physical GPIO pin register. A similar patch has been
> merged recently by Hans for Crystal Cove PMIC GPIO driver. You can
> find more details about it in Commit 9a752b4c9ab9 ("gpio: crystalcove:
> Do not write regular gpio registers for virtual GPIOs")
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reported-by: Jukka Laitinen <jukka.laitinen@intel.com>
It seems it should have a Fixes tag.
> static inline unsigned int to_reg(int gpio, enum ctrl_register reg_type)
> {
> unsigned int reg;
> - int bank;
>
> - if (gpio < BANK0_NR_PINS)
> - bank = 0;
> - else if (gpio < BANK0_NR_PINS + BANK1_NR_PINS)
> - bank = 1;
> - else
> - bank = 2;
> + if (gpio >= WCOVE_GPIO_NUM)
> + return -EOPNOTSUPP;
How this can happen?
>
> if (reg_type == CTRL_IN)
> - reg = GPIO_IN_CTRL_BASE + bank;
> + /*
> + * GPIO input control registers
> + * (one per pin): 0x4e51 - 0x4e5d
> + */
Noise.
> + reg = GPIO_IN_CTRL_BASE + gpio;
> else
> - reg = GPIO_OUT_CTRL_BASE + bank;
> + /* GPIO output control registers
> + * (one per pin): 0x4e44 - 0x4e50
> + */
Wrong multi-line comment and noise overall.
If you wish to leave the comments, put them on top of the function as
its description.
> + reg = GPIO_OUT_CTRL_BASE + gpio;
>
> return reg;
> }
> @@ -145,7 +147,10 @@ static void wcove_update_irq_mask(struct wcove_gpio *wg, int gpio)
>
> static void wcove_update_irq_ctrl(struct wcove_gpio *wg, int gpio)
> {
> - unsigned int reg = to_reg(gpio, CTRL_IN);
> + int reg = to_reg(gpio, CTRL_IN);
> +
> + if (reg < 0)
> + return;
Since above comment this change would gone.
> + int reg = to_reg(gpio, CTRL_OUT);
> +
> + if (reg < 0)
> + return 0;
>
> - return regmap_write(wg->regmap, to_reg(gpio, CTRL_OUT),
> - CTLO_INPUT_SET);
> + return regmap_write(wg->regmap, reg, CTLO_INPUT_SET);
Ditto.
> + int reg = to_reg(gpio, CTRL_OUT);
>
> - return regmap_write(wg->regmap, to_reg(gpio, CTRL_OUT),
> - CTLO_OUTPUT_SET | value);
> + if (reg < 0)
> + return 0;
> +
> + return regmap_write(wg->regmap, reg, CTLO_OUTPUT_SET | value);
Ditto.
> + int ret, reg = to_reg(gpio, CTRL_OUT);
Don't fit such variable on one line.
> +
> + if (reg < 0)
> + return 0;
>
> - ret = regmap_read(wg->regmap, to_reg(gpio, CTRL_OUT), &val);
> + ret = regmap_read(wg->regmap, reg, &val);
This would gone after addressing first comment.
> - int ret;
> + int ret, reg = to_reg(gpio, CTRL_IN);
> +
> + if (reg < 0)
> + return 0;
>
> - ret = regmap_read(wg->regmap, to_reg(gpio, CTRL_IN), &val);
> + ret = regmap_read(wg->regmap, reg, &val);
Ditto.
> + int reg = to_reg(gpio, CTRL_OUT);
> +
> + if (reg < 0)
> + return;
>
> if (value)
> - regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT), 1, 1);
> + regmap_update_bits(wg->regmap, reg, 1, 1);
> else
> - regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT), 1, 0);
> + regmap_update_bits(wg->regmap, reg, 1, 0);
Ditto.
> + int reg = to_reg(gpio, CTRL_OUT);
> +
> + if (reg < 0)
> + return 0;
>
> switch (pinconf_to_config_param(config)) {
> case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> - return regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT),
> - CTLO_DRV_MASK, CTLO_DRV_OD);
> + return regmap_update_bits(wg->regmap, reg, CTLO_DRV_MASK,
> + CTLO_DRV_OD);
> case PIN_CONFIG_DRIVE_PUSH_PULL:
> - return regmap_update_bits(wg->regmap, to_reg(gpio, CTRL_OUT),
> - CTLO_DRV_MASK, CTLO_DRV_CMOS);
> + return regmap_update_bits(wg->regmap, reg, CTLO_DRV_MASK,
> + CTLO_DRV_CMOS);
Ditto.
> + if (data->hwirq >= WCOVE_GPIO_NUM)
> + return 0;
> +
How could it happen?
> + if (data->hwirq >= WCOVE_GPIO_NUM)
> + return;
Ditto.
> + if (data->hwirq >= WCOVE_GPIO_NUM)
> + return;
Ditto.
--
With Best Regards,
Andy Shevchenko
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v1 1/1] gpio: gpio-wcove: Fix GPIO control register offset calculation sathyanarayanan.kuppuswamy@linux.intel.com - 2017-06-14 23:40 +0200
Re: [PATCH v1 1/1] gpio: gpio-wcove: Fix GPIO control register offset calculation Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-15 11:10 +0200
Re: [PATCH v1 1/1] gpio: gpio-wcove: Fix GPIO control register offset calculation sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-06-15 23:00 +0200
csiph-web