From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH 2/4] gpiolib: add bitmask for valid GPIO lines Date: Fri, 1 Dec 2017 17:08:17 +0530 Message-ID: <68a11ee4-d0c6-0f4e-3631-125cc4e7a1bd@codeaurora.org> References: <1510096056-13765-1-git-send-email-timur@codeaurora.org> <1510096056-13765-3-git-send-email-timur@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1510096056-13765-3-git-send-email-timur@codeaurora.org> Content-Language: en-US Sender: linux-arm-msm-owner@vger.kernel.org To: Timur Tabi , Mika Westerberg , thierry.reding@gmail.com, Stephen Boyd Cc: linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, Linus Walleij , Andy Shevchenko , david.brown@linaro.org, andy.gross@linaro.org, Bjorn Andersson , nbasu@codeaurora.org, vivek.gautam@codeaurora.org List-Id: linux-gpio@vger.kernel.org Hi, On 11/08/2017 04:37 AM, Timur Tabi wrote: > Add support for specifying that some GPIOs within a range are unavailable. > Some systems have a sparse list of GPIOs, where a range of GPIOs is > specified (usually 0 to n-1), but some subset within that range is > absent or unavailable for whatever reason. > > To support this, allow drivers to specify a bitmask of GPIOs that > are present or absent. Gpiolib will then block access to those that > are absent. > > Signed-off-by: Timur Tabi > --- > drivers/gpio/gpiolib.c | 43 +++++++++++++++++++++++++++++++++++-------- > include/linux/gpio/driver.h | 2 ++ > 2 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 60553af4c004..c32387936cdd 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1481,22 +1481,36 @@ static struct gpio_chip *find_chip_by_name(const char *name) > > static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip) > { > - if (!gpiochip->irq_need_valid_mask) > - return 0; > + if (gpiochip->irq_need_valid_mask) { > + gpiochip->irq_valid_mask = > + kcalloc(BITS_TO_LONGS(gpiochip->ngpio), > + sizeof(long), GFP_KERNEL); > + if (!gpiochip->irq_valid_mask) > + return -ENOMEM; > > - gpiochip->irq_valid_mask = kcalloc(BITS_TO_LONGS(gpiochip->ngpio), > - sizeof(long), GFP_KERNEL); > - if (!gpiochip->irq_valid_mask) > - return -ENOMEM; > + /* Assume by default all GPIOs are valid */ > + bitmap_fill(gpiochip->irq_valid_mask, gpiochip->ngpio); > + } > > - /* Assume by default all GPIOs are valid */ > - bitmap_fill(gpiochip->irq_valid_mask, gpiochip->ngpio); > + if (gpiochip->line_need_valid_mask) { > + gpiochip->line_valid_mask = > + kcalloc(BITS_TO_LONGS(gpiochip->ngpio), > + sizeof(long), GFP_KERNEL); > + if (!gpiochip->line_valid_mask) > + return -ENOMEM; > + > + /* Assume by default all GPIOs are valid */ > + bitmap_fill(gpiochip->line_valid_mask, gpiochip->ngpio); > + } > > return 0; > } > > static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip) > { > + kfree(gpiochip->line_valid_mask); > + gpiochip->line_valid_mask = NULL; > + > kfree(gpiochip->irq_valid_mask); > gpiochip->irq_valid_mask = NULL; > } > @@ -1510,6 +1524,15 @@ static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip, > return test_bit(offset, gpiochip->irq_valid_mask); > } > > +static bool gpiochip_irqchip_line_valid(const struct gpio_chip *gpiochip, > + unsigned int offset) > +{ > + /* No mask means all valid */ > + if (likely(!gpiochip->line_valid_mask)) > + return true; > + return test_bit(offset, gpiochip->line_valid_mask); > +} > + > /** > * gpiochip_set_cascaded_irqchip() - connects a cascaded irqchip to a gpiochip > * @gpiochip: the gpiochip to set the irqchip chain to > @@ -3320,6 +3343,10 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev, > return desc; > } > > + /* Make sure the GPIO is valid before we request it. */ > + if (!gpiochip_irqchip_line_valid(desc->gdev->chip, idx)) > + return ERR_PTR(-EACCES); We're hitting an issue here ^ when a consumer calls the devm_gpiod_get() API. This API internally sets idx in gpiod_get_index to always 0. For example, in the call sequence below, the idx argument to gpiochip_irqchip_line_valid is always 0: devm_gpiod_get(dev, con_id, flags) devm_gpiod_get_index(dev, con_id, 0, flags) gpiod_get_index(dev, con_id, 0, flags) if (!gpiochip_irqchip_line_valid(desc->gdev->chip, 0)) return ERR_PTR(-EACCES); Therefore, with these patches, if we create a sparse gpio map, and the 0th gpio is set to "not accessible", all gpio requests end up failing with EACCESS because idx is always passed as 0. In gpiochip_irqchip_line_valid, shouldn't we test the nth bit (that corresponds to this gpio_desc) in chip->line_valid_mask instead of idx passed above, which is always 0? Thanks, Archit > + > status = gpiod_request(desc, con_id); > if (status < 0) > return ERR_PTR(status); > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index 424e5139ff10..853828ccabc8 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -173,6 +173,8 @@ struct gpio_chip { > bool irq_nested; > bool irq_need_valid_mask; > unsigned long *irq_valid_mask; > + bool line_need_valid_mask; > + unsigned long *line_valid_mask; > struct lock_class_key *lock_key; > #endif > > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project