From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [PATCH] gpio: crystalcove: Do not write regular gpio registers for virtual GPIOs Date: Mon, 15 May 2017 11:57:05 +0300 Message-ID: <1494838625.6967.48.camel@linux.intel.com> References: <20170513123953.22142-1-hdegoede@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Return-path: Received: from mga05.intel.com ([192.55.52.43]:14657 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933490AbdEOJAV (ORCPT ); Mon, 15 May 2017 05:00:21 -0400 In-Reply-To: <20170513123953.22142-1-hdegoede@redhat.com> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Hans de Goede , Mika Westerberg , Heikki Krogerus , Linus Walleij , Alexandre Courbot Cc: linux-gpio@vger.kernel.org, Aaron Lu On Sat, 2017-05-13 at 14:39 +0200, Hans de Goede wrote: > The Crystal Cove PMIC has 16 real GPIOs but the ACPI code for devices > with this PMIC may address up to 95 GPIOs, these extra GPIOs are > called virtual GPIOs and are used by the ACPI code as a method of > accessing various non GPIO bits of PMIC. > > Commit dcdc3018d635 ("gpio: crystalcove: support virtual GPIO") added > dummy support for these to avoid a bunch of ACPI errors, but instead > of > ignoring writes / reads to them by doing: > > if (gpio >= CRYSTALCOVE_GPIO_NUM) > return 0; > > It accidentally introduced the following wrong check: > > if (gpio > CRYSTALCOVE_VGPIO_NUM) > return 0; > > Which means that attempts by the ACPI code to access these gpios > causes some arbitrary gpio to get touched through for example > GPIO1P0CTLO + gpionr % 8. > > Since we do support input/output (but not interrupts) on the 0x5e > virtual GPIO, this commit makes to_reg return -ENOTSUPP for > unsupported > virtual GPIOs so as to not have to check for (gpio >= > CRYSTALCOVE_GPIO_NUM > && gpio != 0x5e) everywhere and to make it easier to add support for > more > virtual GPIOs in the future. > > It then adds a check for to_reg returning an error to all callers > where > this may happen fixing the ACPI code accessing virtual GPIOs > accidentally > causing changes to real GPIOs. Looks fine to me. FWIW: Reviewed-by: Andy Shevchenko P.S. Should it have Fixes tag? > > Cc: Aaron Lu > Signed-off-by: Hans de Goede > --- >  drivers/gpio/gpio-crystalcove.c | 54 +++++++++++++++++++++++++++----- > --------- >  1 file changed, 36 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio- > crystalcove.c > index 2197368cc899..e60156ec0c18 100644 > --- a/drivers/gpio/gpio-crystalcove.c > +++ b/drivers/gpio/gpio-crystalcove.c > @@ -90,8 +90,18 @@ static inline int to_reg(int gpio, enum > ctrl_register reg_type) >  { >   int reg; >   > - if (gpio == 94) > - return GPIOPANELCTL; > + if (gpio >= CRYSTALCOVE_GPIO_NUM) { > + /* > +  * Virtual GPIO called from ACPI, for now we only > support > +  * the panel ctl. > +  */ > + switch (gpio) { > + case 0x5e: > + return GPIOPANELCTL; > + default: > + return -EOPNOTSUPP; > + } > + } >   >   if (reg_type == CTRL_IN) { >   if (gpio < 8) > @@ -130,36 +140,36 @@ static void crystalcove_update_irq_ctrl(struct > crystalcove_gpio *cg, int gpio) >  static int crystalcove_gpio_dir_in(struct gpio_chip *chip, unsigned > gpio) >  { >   struct crystalcove_gpio *cg = gpiochip_get_data(chip); > + int reg = to_reg(gpio, CTRL_OUT); >   > - if (gpio > CRYSTALCOVE_VGPIO_NUM) > + if (reg < 0) >   return 0; >   > - return regmap_write(cg->regmap, to_reg(gpio, CTRL_OUT), > -     CTLO_INPUT_SET); > + return regmap_write(cg->regmap, reg, CTLO_INPUT_SET); >  } >   >  static int crystalcove_gpio_dir_out(struct gpio_chip *chip, unsigned > gpio, >       int value) >  { >   struct crystalcove_gpio *cg = gpiochip_get_data(chip); > + int reg = to_reg(gpio, CTRL_OUT); >   > - if (gpio > CRYSTALCOVE_VGPIO_NUM) > + if (reg < 0) >   return 0; >   > - return regmap_write(cg->regmap, to_reg(gpio, CTRL_OUT), > -     CTLO_OUTPUT_SET | value); > + return regmap_write(cg->regmap, reg, CTLO_OUTPUT_SET | > value); >  } >   >  static int crystalcove_gpio_get(struct gpio_chip *chip, unsigned > gpio) >  { >   struct crystalcove_gpio *cg = gpiochip_get_data(chip); > - int ret; >   unsigned int val; > + int ret, reg = to_reg(gpio, CTRL_IN); >   > - if (gpio > CRYSTALCOVE_VGPIO_NUM) > + if (reg < 0) >   return 0; >   > - ret = regmap_read(cg->regmap, to_reg(gpio, CTRL_IN), &val); > + ret = regmap_read(cg->regmap, reg, &val); >   if (ret) >   return ret; >   > @@ -170,14 +180,15 @@ static void crystalcove_gpio_set(struct > gpio_chip *chip, >    unsigned gpio, int value) >  { >   struct crystalcove_gpio *cg = gpiochip_get_data(chip); > + int reg = to_reg(gpio, CTRL_OUT); >   > - if (gpio > CRYSTALCOVE_VGPIO_NUM) > + if (reg < 0) >   return; >   >   if (value) > - regmap_update_bits(cg->regmap, to_reg(gpio, > CTRL_OUT), 1, 1); > + regmap_update_bits(cg->regmap, reg, 1, 1); >   else > - regmap_update_bits(cg->regmap, to_reg(gpio, > CTRL_OUT), 1, 0); > + regmap_update_bits(cg->regmap, reg, 1, 0); >  } >   >  static int crystalcove_irq_type(struct irq_data *data, unsigned type) > @@ -185,6 +196,9 @@ static int crystalcove_irq_type(struct irq_data > *data, unsigned type) >   struct crystalcove_gpio *cg = >   gpiochip_get_data(irq_data_get_irq_chip_data(data)); >   > + if (data->hwirq >= CRYSTALCOVE_GPIO_NUM) > + return 0; > + >   switch (type) { >   case IRQ_TYPE_NONE: >   cg->intcnt_value = CTLI_INTCNT_DIS; > @@ -235,8 +249,10 @@ static void crystalcove_irq_unmask(struct > irq_data *data) >   struct crystalcove_gpio *cg = >   gpiochip_get_data(irq_data_get_irq_chip_data(data)); >   > - cg->set_irq_mask = false; > - cg->update |= UPDATE_IRQ_MASK; > + if (data->hwirq < CRYSTALCOVE_GPIO_NUM) { > + cg->set_irq_mask = false; > + cg->update |= UPDATE_IRQ_MASK; > + } >  } >   >  static void crystalcove_irq_mask(struct irq_data *data) > @@ -244,8 +260,10 @@ static void crystalcove_irq_mask(struct irq_data > *data) >   struct crystalcove_gpio *cg = >   gpiochip_get_data(irq_data_get_irq_chip_data(data)); >   > - cg->set_irq_mask = true; > - cg->update |= UPDATE_IRQ_MASK; > + if (data->hwirq < CRYSTALCOVE_GPIO_NUM) { > + cg->set_irq_mask = true; > + cg->update |= UPDATE_IRQ_MASK; > + } >  } >   >  static struct irq_chip crystalcove_irqchip = { -- Andy Shevchenko Intel Finland Oy