diff mbox series

[RFC/RFT] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Message ID 20250219102750.38519-1-brgl@bgdev.pl (mailing list archive)
State New
Headers show
Series [RFC/RFT] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction() | expand

Commit Message

Bartosz Golaszewski Feb. 19, 2025, 10:27 a.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Since commit 9d846b1aebbe ("gpiolib: check the return value of
gpio_chip::get_direction()") we check the return value of the
get_direction() callback as per its API contract. This driver returns
-EINVAL if the pin in question is set to one of the alternative
(non-GPIO) functions. This isn't really an error that should be
communicated to GPIOLIB so default to returning the "safe" value of
INPUT in this case. The GPIO subsystem does not have the notion of
"unknown" direction.

Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()")
Reported-by: Mark Brown <broonie@kernel.org>
Closes: https://lore.kernel.org/all/Z7VFB1nST6lbmBIo@finisterre.sirena.org.uk/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Marek Szyprowski Feb. 19, 2025, 11:16 a.m. UTC | #1 
On 19.02.2025 11:27, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Since commit 9d846b1aebbe ("gpiolib: check the return value of
> gpio_chip::get_direction()") we check the return value of the
> get_direction() callback as per its API contract. This driver returns
> -EINVAL if the pin in question is set to one of the alternative
> (non-GPIO) functions. This isn't really an error that should be
> communicated to GPIOLIB so default to returning the "safe" value of
> INPUT in this case. The GPIO subsystem does not have the notion of
> "unknown" direction.
>
> Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()")
> Reported-by: Mark Brown <broonie@kernel.org>
> Closes: https://lore.kernel.org/all/Z7VFB1nST6lbmBIo@finisterre.sirena.org.uk/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Closes: 
https://lore.kernel.org/all/dfe03f88-407e-4ef1-ad30-42db53bbd4e4@samsung.com/
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/pinctrl/bcm/pinctrl-bcm2835.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index cc1fe0555e19..eaeec096bc9a 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -346,14 +346,14 @@ static int bcm2835_gpio_get_direction(struct gpio_chip *chip, unsigned int offse
>   	struct bcm2835_pinctrl *pc = gpiochip_get_data(chip);
>   	enum bcm2835_fsel fsel = bcm2835_pinctrl_fsel_get(pc, offset);
>   
> -	/* Alternative function doesn't clearly provide a direction */
> -	if (fsel > BCM2835_FSEL_GPIO_OUT)
> -		return -EINVAL;
> +	if (fsel == BCM2835_FSEL_GPIO_OUT)
> +		return GPIO_LINE_DIRECTION_OUT;
>   
> -	if (fsel == BCM2835_FSEL_GPIO_IN)
> -		return GPIO_LINE_DIRECTION_IN;
> -
> -	return GPIO_LINE_DIRECTION_OUT;
> +	/*
> +	 * Alternative function doesn't clearly provide a direction. Default
> +	 * to INPUT.
> +	 */
> +	return GPIO_LINE_DIRECTION_IN;
>   }
>   
>   static void bcm2835_gpio_set(struct gpio_chip *chip, unsigned offset, int value)

Best regards
Mark Brown Feb. 19, 2025, 2:27 p.m. UTC | #2 
On Wed, Feb 19, 2025 at 11:27:50AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Since commit 9d846b1aebbe ("gpiolib: check the return value of
> gpio_chip::get_direction()") we check the return value of the
> get_direction() callback as per its API contract. This driver returns
> -EINVAL if the pin in question is set to one of the alternative
> (non-GPIO) functions. This isn't really an error that should be
> communicated to GPIOLIB so default to returning the "safe" value of
> INPUT in this case. The GPIO subsystem does not have the notion of
> "unknown" direction.

I see this was already tested for these specific boards.  I've also
found that Avenger96 is failing with bisect pointing to the same commit
this is fixing:

    https://lava.sirena.org.uk/scheduler/job/1126314

as is the Libretech Potato:

    https://lava.sirena.org.uk/scheduler/job/1126285

neither of which produce any output before dying, they'll not be fixed
by this change.  Seems like an audit of the drivers might be in order?
Bartosz Golaszewski Feb. 19, 2025, 2:29 p.m. UTC | #3 
On Wed, Feb 19, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Feb 19, 2025 at 11:27:50AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Since commit 9d846b1aebbe ("gpiolib: check the return value of
> > gpio_chip::get_direction()") we check the return value of the
> > get_direction() callback as per its API contract. This driver returns
> > -EINVAL if the pin in question is set to one of the alternative
> > (non-GPIO) functions. This isn't really an error that should be
> > communicated to GPIOLIB so default to returning the "safe" value of
> > INPUT in this case. The GPIO subsystem does not have the notion of
> > "unknown" direction.
>
> I see this was already tested for these specific boards.  I've also
> found that Avenger96 is failing with bisect pointing to the same commit
> this is fixing:
>
>     https://lava.sirena.org.uk/scheduler/job/1126314
>
> as is the Libretech Potato:
>
>     https://lava.sirena.org.uk/scheduler/job/1126285
>
> neither of which produce any output before dying, they'll not be fixed
> by this change.  Seems like an audit of the drivers might be in order?

Right. I don't know if they return EINVAL or some other error so let
me prepare a change that will not bail-out but simply warn on
get_direction() errors in gpiochip_add_data() instead.

This patch can still go upstream IMO.

Bart
Stefan Wahren Feb. 19, 2025, 3:46 p.m. UTC | #4 
Am 19.02.25 um 11:27 schrieb Bartosz Golaszewski:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Since commit 9d846b1aebbe ("gpiolib: check the return value of
> gpio_chip::get_direction()") we check the return value of the
> get_direction() callback as per its API contract. This driver returns
> -EINVAL if the pin in question is set to one of the alternative
> (non-GPIO) functions. This isn't really an error that should be
> communicated to GPIOLIB so default to returning the "safe" value of
> INPUT in this case. The GPIO subsystem does not have the notion of
> "unknown" direction.
>
> Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()")
> Reported-by: Mark Brown <broonie@kernel.org>
> Closes: https://lore.kernel.org/all/Z7VFB1nST6lbmBIo@finisterre.sirena.org.uk/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

Thanks
diff mbox series

Patch

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index cc1fe0555e19..eaeec096bc9a 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -346,14 +346,14 @@  static int bcm2835_gpio_get_direction(struct gpio_chip *chip, unsigned int offse
 	struct bcm2835_pinctrl *pc = gpiochip_get_data(chip);
 	enum bcm2835_fsel fsel = bcm2835_pinctrl_fsel_get(pc, offset);
 
-	/* Alternative function doesn't clearly provide a direction */
-	if (fsel > BCM2835_FSEL_GPIO_OUT)
-		return -EINVAL;
+	if (fsel == BCM2835_FSEL_GPIO_OUT)
+		return GPIO_LINE_DIRECTION_OUT;
 
-	if (fsel == BCM2835_FSEL_GPIO_IN)
-		return GPIO_LINE_DIRECTION_IN;
-
-	return GPIO_LINE_DIRECTION_OUT;
+	/*
+	 * Alternative function doesn't clearly provide a direction. Default
+	 * to INPUT.
+	 */
+	return GPIO_LINE_DIRECTION_IN;
 }
 
 static void bcm2835_gpio_set(struct gpio_chip *chip, unsigned offset, int value)