[PATCH 0/2] pinctrl: cirrus: use new GPIO line value setter callbacks

[PATCH 0/2] pinctrl: cirrus: use new GPIO line value setter callbacks

[Bartosz Golaszewski]

Commit 98ce1eb1fd87e ("gpiolib: introduce gpio_chip setters that return
values") added new line setter callbacks to struct gpio_chip. They allow
to indicate failures to callers. We're in the process of converting all
GPIO controllers to using them before removing the old ones. This series
converts all GPIO chips in cirrus pin control drivers.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (2):
      pinctrl: cirrus: lochnagar: use new GPIO line value setter callbacks
      pinctrl: cirrus: cs42l43: use new GPIO line value setter callbacks

 drivers/pinctrl/cirrus/pinctrl-cs42l43.c   | 21 +++++++++++++--------
 drivers/pinctrl/cirrus/pinctrl-lochnagar.c | 25 +++++++++++++------------
 2 files changed, 26 insertions(+), 20 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250612-gpiochip-set-rv-pinctrl-cirrus-2335675707ce

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 1/2] pinctrl: cirrus: lochnagar: use new GPIO line value setter callbacks

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

struct gpio_chip now has callbacks for setting line values that return
an integer, allowing to indicate failures. Convert the driver to using
them.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/cirrus/pinctrl-lochnagar.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/cirrus/pinctrl-lochnagar.c b/drivers/pinctrl/cirrus/pinctrl-lochnagar.c
index 0f32866a4aef2c84b99f3c56374d8dfd9150b024..dcc0a2f3c7dd56d3975c54ef3dc2726b938e4480 100644
--- a/drivers/pinctrl/cirrus/pinctrl-lochnagar.c
+++ b/drivers/pinctrl/cirrus/pinctrl-lochnagar.c
@@ -1058,13 +1058,12 @@ static const struct pinctrl_desc lochnagar_pin_desc = {
 	.confops = &lochnagar_pin_conf_ops,
 };
 
-static void lochnagar_gpio_set(struct gpio_chip *chip,
-			       unsigned int offset, int value)
+static int lochnagar_gpio_set(struct gpio_chip *chip,
+			      unsigned int offset, int value)
 {
 	struct lochnagar_pin_priv *priv = gpiochip_get_data(chip);
 	struct lochnagar *lochnagar = priv->lochnagar;
 	const struct lochnagar_pin *pin = priv->pins[offset].drv_data;
-	int ret;
 
 	value = !!value;
 
@@ -1075,29 +1074,31 @@ static void lochnagar_gpio_set(struct gpio_chip *chip,
 	case LN_PTYPE_MUX:
 		value |= LN2_OP_GPIO;
 
-		ret = lochnagar_pin_set_mux(priv, pin, value);
+		return lochnagar_pin_set_mux(priv, pin, value);
 		break;
 	case LN_PTYPE_GPIO:
 		if (pin->invert)
 			value = !value;
 
-		ret = regmap_update_bits(lochnagar->regmap, pin->reg,
-					 BIT(pin->shift), value << pin->shift);
+		return regmap_update_bits(lochnagar->regmap, pin->reg,
+					  BIT(pin->shift),
+					  value << pin->shift);
 		break;
 	default:
-		ret = -EINVAL;
 		break;
 	}
 
-	if (ret < 0)
-		dev_err(chip->parent, "Failed to set %s value: %d\n",
-			pin->name, ret);
+	return -EINVAL;
 }
 
 static int lochnagar_gpio_direction_out(struct gpio_chip *chip,
 					unsigned int offset, int value)
 {
-	lochnagar_gpio_set(chip, offset, value);
+	int ret;
+
+	ret = lochnagar_gpio_set(chip, offset, value);
+	if (ret)
+		return ret;
 
 	return pinctrl_gpio_direction_output(chip, offset);
 }
@@ -1160,7 +1161,7 @@ static int lochnagar_pin_probe(struct platform_device *pdev)
 	priv->gpio_chip.request = gpiochip_generic_request;
 	priv->gpio_chip.free = gpiochip_generic_free;
 	priv->gpio_chip.direction_output = lochnagar_gpio_direction_out;
-	priv->gpio_chip.set = lochnagar_gpio_set;
+	priv->gpio_chip.set_rv = lochnagar_gpio_set;
 	priv->gpio_chip.can_sleep = true;
 	priv->gpio_chip.parent = dev;
 	priv->gpio_chip.base = -1;

-- 
2.48.1

[PATCH 2/2] pinctrl: cirrus: cs42l43: use new GPIO line value setter callbacks

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

struct gpio_chip now has callbacks for setting line values that return
an integer, allowing to indicate failures. Convert the driver to using
them.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/cirrus/pinctrl-cs42l43.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/cirrus/pinctrl-cs42l43.c b/drivers/pinctrl/cirrus/pinctrl-cs42l43.c
index 628b60ccc2b07dc77e36da8919436fa348749e0c..29ed985273eb47d06f6cf5e6e41078deae4cc2bb 100644
--- a/drivers/pinctrl/cirrus/pinctrl-cs42l43.c
+++ b/drivers/pinctrl/cirrus/pinctrl-cs42l43.c
@@ -483,7 +483,8 @@ static int cs42l43_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	return ret;
 }
 
-static void cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+static int cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			    int value)
 {
 	struct cs42l43_pin *priv = gpiochip_get_data(chip);
 	unsigned int shift = offset + CS42L43_GPIO1_LVL_SHIFT;
@@ -493,23 +494,27 @@ static void cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset, int va
 		offset + 1, str_high_low(value));
 
 	ret = pm_runtime_resume_and_get(priv->dev);
-	if (ret) {
-		dev_err(priv->dev, "Failed to resume for set: %d\n", ret);
-		return;
-	}
+	if (ret)
+		return ret;
 
 	ret = regmap_update_bits(priv->regmap, CS42L43_GPIO_CTRL1,
 				 BIT(shift), value << shift);
 	if (ret)
-		dev_err(priv->dev, "Failed to set gpio%d: %d\n", offset + 1, ret);
+		return ret;
 
 	pm_runtime_put(priv->dev);
+
+	return 0;
 }
 
 static int cs42l43_gpio_direction_out(struct gpio_chip *chip,
 				      unsigned int offset, int value)
 {
-	cs42l43_gpio_set(chip, offset, value);
+	int ret;
+
+	ret = cs42l43_gpio_set(chip, offset, value);
+	if (ret)
+		return ret;
 
 	return pinctrl_gpio_direction_output(chip, offset);
 }
@@ -550,7 +555,7 @@ static int cs42l43_pin_probe(struct platform_device *pdev)
 	priv->gpio_chip.direction_output = cs42l43_gpio_direction_out;
 	priv->gpio_chip.add_pin_ranges = cs42l43_gpio_add_pin_ranges;
 	priv->gpio_chip.get = cs42l43_gpio_get;
-	priv->gpio_chip.set = cs42l43_gpio_set;
+	priv->gpio_chip.set_rv = cs42l43_gpio_set;
 	priv->gpio_chip.label = dev_name(priv->dev);
 	priv->gpio_chip.parent = priv->dev;
 	priv->gpio_chip.can_sleep = true;

-- 
2.48.1

Re: [PATCH 1/2] pinctrl: cirrus: lochnagar: use new GPIO line value setter callbacks

[Charles Keepax]

On Thu, Jun 12, 2025 at 02:19:53PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> struct gpio_chip now has callbacks for setting line values that return
> an integer, allowing to indicate failures. Convert the driver to using
> them.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> @@ -1075,29 +1074,31 @@ static void lochnagar_gpio_set(struct gpio_chip *chip,
>  	case LN_PTYPE_MUX:
>  		value |= LN2_OP_GPIO;
>  
> -		ret = lochnagar_pin_set_mux(priv, pin, value);
> +		return lochnagar_pin_set_mux(priv, pin, value);
>  		break;
>  	case LN_PTYPE_GPIO:
>  		if (pin->invert)
>  			value = !value;
>  
> -		ret = regmap_update_bits(lochnagar->regmap, pin->reg,
> -					 BIT(pin->shift), value << pin->shift);
> +		return regmap_update_bits(lochnagar->regmap, pin->reg,
> +					  BIT(pin->shift),
> +					  value << pin->shift);
>  		break;
>  	default:
> -		ret = -EINVAL;
>  		break;
>  	}
>  
> -	if (ret < 0)
> -		dev_err(chip->parent, "Failed to set %s value: %d\n",
> -			pin->name, ret);
> +	return -EINVAL;

Nit: I would be tempted just to move this return up into the
default case, the other two cases return so its slightly more
symmetical that way. But I don't feel super strongly:

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH 2/2] pinctrl: cirrus: cs42l43: use new GPIO line value setter callbacks

[Charles Keepax]

On Thu, Jun 12, 2025 at 02:19:54PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> struct gpio_chip now has callbacks for setting line values that return
> an integer, allowing to indicate failures. Convert the driver to using
> them.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> +static int cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			    int value)
>  {
>  	struct cs42l43_pin *priv = gpiochip_get_data(chip);
>  	unsigned int shift = offset + CS42L43_GPIO1_LVL_SHIFT;
> @@ -493,23 +494,27 @@ static void cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset, int va
>  		offset + 1, str_high_low(value));
>  
>  	ret = pm_runtime_resume_and_get(priv->dev);
> -	if (ret) {
> -		dev_err(priv->dev, "Failed to resume for set: %d\n", ret);
> -		return;
> -	}
> +	if (ret)
> +		return ret;

Is there a reason to lose the error message here? Always nice to
know which of the two paths failed when things go bad.

Thanks,
Charles
>  
>  	ret = regmap_update_bits(priv->regmap, CS42L43_GPIO_CTRL1,
>  				 BIT(shift), value << shift);
>  	if (ret)
> -		dev_err(priv->dev, "Failed to set gpio%d: %d\n", offset + 1, ret);
> +		return ret;
>  
>  	pm_runtime_put(priv->dev);
> +
> +	return 0;
>  }

Re: [PATCH 2/2] pinctrl: cirrus: cs42l43: use new GPIO line value setter callbacks

[Bartosz Golaszewski]

On Thu, Jun 12, 2025 at 2:36 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Thu, Jun 12, 2025 at 02:19:54PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > struct gpio_chip now has callbacks for setting line values that return
> > an integer, allowing to indicate failures. Convert the driver to using
> > them.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> > +static int cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > +                         int value)
> >  {
> >       struct cs42l43_pin *priv = gpiochip_get_data(chip);
> >       unsigned int shift = offset + CS42L43_GPIO1_LVL_SHIFT;
> > @@ -493,23 +494,27 @@ static void cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset, int va
> >               offset + 1, str_high_low(value));
> >
> >       ret = pm_runtime_resume_and_get(priv->dev);
> > -     if (ret) {
> > -             dev_err(priv->dev, "Failed to resume for set: %d\n", ret);
> > -             return;
> > -     }
> > +     if (ret)
> > +             return ret;
>
> Is there a reason to lose the error message here? Always nice to
> know which of the two paths failed when things go bad.
>

No reason other than being in line with most other drivers which
typically just return a value without a message. I don't care much, we
can restore it.

Bart

> Thanks,
> Charles
> >
> >       ret = regmap_update_bits(priv->regmap, CS42L43_GPIO_CTRL1,
> >                                BIT(shift), value << shift);
> >       if (ret)
> > -             dev_err(priv->dev, "Failed to set gpio%d: %d\n", offset + 1, ret);
> > +             return ret;
> >
> >       pm_runtime_put(priv->dev);
> > +
> > +     return 0;
> >  }

Re: [PATCH 2/2] pinctrl: cirrus: cs42l43: use new GPIO line value setter callbacks

[Charles Keepax]

On Thu, Jun 12, 2025 at 02:45:45PM +0200, Bartosz Golaszewski wrote:
> On Thu, Jun 12, 2025 at 2:36 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > On Thu, Jun 12, 2025 at 02:19:54PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > struct gpio_chip now has callbacks for setting line values that return
> > > an integer, allowing to indicate failures. Convert the driver to using
> > > them.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > > +static int cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > > +                         int value)
> > >  {
> > >       struct cs42l43_pin *priv = gpiochip_get_data(chip);
> > >       unsigned int shift = offset + CS42L43_GPIO1_LVL_SHIFT;
> > > @@ -493,23 +494,27 @@ static void cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset, int va
> > >               offset + 1, str_high_low(value));
> > >
> > >       ret = pm_runtime_resume_and_get(priv->dev);
> > > -     if (ret) {
> > > -             dev_err(priv->dev, "Failed to resume for set: %d\n", ret);
> > > -             return;
> > > -     }
> > > +     if (ret)
> > > +             return ret;
> >
> > Is there a reason to lose the error message here? Always nice to
> > know which of the two paths failed when things go bad.
> >
> 
> No reason other than being in line with most other drivers which
> typically just return a value without a message. I don't care much, we
> can restore it.
> 

I guess it doesn't generally fail, but I am probably more
comfortable leaving it. So lets go with that if you are easy
either way. With that:

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH 0/2] pinctrl: cirrus: use new GPIO line value setter callbacks

[Linus Walleij]

On Thu, Jun 12, 2025 at 2:19 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Commit 98ce1eb1fd87e ("gpiolib: introduce gpio_chip setters that return
> values") added new line setter callbacks to struct gpio_chip. They allow
> to indicate failures to callers. We're in the process of converting all
> GPIO controllers to using them before removing the old ones. This series
> converts all GPIO chips in cirrus pin control drivers.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Patches applied!

Yours,
Linus Walleij