[PATCH 0/4] net: use new GPIO line value setter callbacks

[PATCH 0/4] net: 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 implemented under drivers/net/.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (4):
      net: dsa: vsc73xx: use new GPIO line value setter callbacks
      net: dsa: mt7530: use new GPIO line value setter callbacks
      net: can: mcp251x: use new GPIO line value setter callbacks
      net: phy: qca807x: use new GPIO line value setter callbacks

 drivers/net/can/spi/mcp251x.c          | 16 ++++++++++------
 drivers/net/dsa/mt7530.c               |  6 ++++--
 drivers/net/dsa/vitesse-vsc73xx-core.c | 10 +++++-----
 drivers/net/phy/qcom/qca807x.c         | 10 ++++------
 4 files changed, 23 insertions(+), 19 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250527-gpiochip-set-rv-net-9e7b91a42f7c

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

[PATCH 1/4] net: dsa: vsc73xx: 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/net/dsa/vitesse-vsc73xx-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index f18aa321053d75f34544267528d68ade37264e89..4f9687ab3b2bc1cc61946eef33b7d08f779db8c6 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -2258,14 +2258,14 @@ static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	return !!(val & BIT(offset));
 }
 
-static void vsc73xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
-			     int val)
+static int vsc73xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			    int val)
 {
 	struct vsc73xx *vsc = gpiochip_get_data(chip);
 	u32 tmp = val ? BIT(offset) : 0;
 
-	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_SYSTEM, 0,
-			    VSC73XX_GPIO, BIT(offset), tmp);
+	return vsc73xx_update_bits(vsc, VSC73XX_BLOCK_SYSTEM, 0,
+				   VSC73XX_GPIO, BIT(offset), tmp);
 }
 
 static int vsc73xx_gpio_direction_output(struct gpio_chip *chip,
@@ -2317,7 +2317,7 @@ static int vsc73xx_gpio_probe(struct vsc73xx *vsc)
 	vsc->gc.parent = vsc->dev;
 	vsc->gc.base = -1;
 	vsc->gc.get = vsc73xx_gpio_get;
-	vsc->gc.set = vsc73xx_gpio_set;
+	vsc->gc.set_rv = vsc73xx_gpio_set;
 	vsc->gc.direction_input = vsc73xx_gpio_direction_input;
 	vsc->gc.direction_output = vsc73xx_gpio_direction_output;
 	vsc->gc.get_direction = vsc73xx_gpio_get_direction;

-- 
2.48.1

Re: [PATCH 1/4] net: dsa: vsc73xx: use new GPIO line value setter callbacks

[Linus Walleij]

On Tue, Jun 10, 2025 at 2:38 PM Bartosz Golaszewski <brgl@bgdev.pl> 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>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/4] net: dsa: mt7530: 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/net/dsa/mt7530.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index df213c37b4fe6ab85fc5b01c62297a35b7b6b2ed..e5bed4237ff4c46457b46598f07b65d0daa84ae9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2112,7 +2112,7 @@ mt7530_gpio_get(struct gpio_chip *gc, unsigned int offset)
 	return !!(mt7530_read(priv, MT7530_LED_GPIO_DATA) & bit);
 }
 
-static void
+static int
 mt7530_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
 {
 	struct mt7530_priv *priv = gpiochip_get_data(gc);
@@ -2122,6 +2122,8 @@ mt7530_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
 		mt7530_set(priv, MT7530_LED_GPIO_DATA, bit);
 	else
 		mt7530_clear(priv, MT7530_LED_GPIO_DATA, bit);
+
+	return 0;
 }
 
 static int
@@ -2185,7 +2187,7 @@ mt7530_setup_gpio(struct mt7530_priv *priv)
 	gc->direction_input = mt7530_gpio_direction_input;
 	gc->direction_output = mt7530_gpio_direction_output;
 	gc->get = mt7530_gpio_get;
-	gc->set = mt7530_gpio_set;
+	gc->set_rv = mt7530_gpio_set;
 	gc->base = -1;
 	gc->ngpio = 15;
 	gc->can_sleep = true;

-- 
2.48.1

[PATCH 3/4] net: can: mcp251x: 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/net/can/spi/mcp251x.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip,
 	return 0;
 }
 
-static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
-			     int value)
+static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			    int value)
 {
 	struct mcp251x_priv *priv = gpiochip_get_data(chip);
 	u8 mask, val;
@@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 
 	priv->reg_bfpctrl &= ~mask;
 	priv->reg_bfpctrl |= val;
+
+	return 0;
 }
 
-static void
+static int
 mcp251x_gpio_set_multiple(struct gpio_chip *chip,
 			  unsigned long *maskp, unsigned long *bitsp)
 {
@@ -561,7 +563,7 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip,
 	val = FIELD_PREP(BFPCTRL_BFS_MASK, val);
 
 	if (!mask)
-		return;
+		return 0;
 
 	mutex_lock(&priv->mcp_lock);
 	mcp251x_write_bits(priv->spi, BFPCTRL, mask, val);
@@ -569,6 +571,8 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip,
 
 	priv->reg_bfpctrl &= ~mask;
 	priv->reg_bfpctrl |= val;
+
+	return 0;
 }
 
 static void mcp251x_gpio_restore(struct spi_device *spi)
@@ -594,8 +598,8 @@ static int mcp251x_gpio_setup(struct mcp251x_priv *priv)
 	gpio->get_direction = mcp251x_gpio_get_direction;
 	gpio->get = mcp251x_gpio_get;
 	gpio->get_multiple = mcp251x_gpio_get_multiple;
-	gpio->set = mcp251x_gpio_set;
-	gpio->set_multiple = mcp251x_gpio_set_multiple;
+	gpio->set_rv = mcp251x_gpio_set;
+	gpio->set_multiple_rv = mcp251x_gpio_set_multiple;
 	gpio->base = -1;
 	gpio->ngpio = ARRAY_SIZE(mcp251x_gpio_names);
 	gpio->names = mcp251x_gpio_names;

-- 
2.48.1

Re: [PATCH 3/4] net: can: mcp251x: use new GPIO line value setter callbacks

[Vincent Mailhol]

On 10/06/2025 at 21:37, 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>
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This does not match the address with which you sent the patch: brgl@bgdev.pl

> ---
>  drivers/net/can/spi/mcp251x.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip,
>  	return 0;
>  }
>  
> -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> -			     int value)
> +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			    int value)
>  {
>  	struct mcp251x_priv *priv = gpiochip_get_data(chip);
>  	u8 mask, val;
> @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>  
>  	priv->reg_bfpctrl &= ~mask;
>  	priv->reg_bfpctrl |= val;
> +
> +	return 0;

mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write()
which can fail.

For this change to really make sense, the return value of mcp251x_spi_write()
should be propagated all the way around.

>  }
>  
> -static void
> +static int
>  mcp251x_gpio_set_multiple(struct gpio_chip *chip,
>  			  unsigned long *maskp, unsigned long *bitsp)
>  {
> @@ -561,7 +563,7 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip,
>  	val = FIELD_PREP(BFPCTRL_BFS_MASK, val);
>  
>  	if (!mask)
> -		return;
> +		return 0;
>  
>  	mutex_lock(&priv->mcp_lock);
>  	mcp251x_write_bits(priv->spi, BFPCTRL, mask, val);
> @@ -569,6 +571,8 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip,
>  
>  	priv->reg_bfpctrl &= ~mask;
>  	priv->reg_bfpctrl |= val;
> +
> +	return 0;

Same as above.

>  }
>  
>  static void mcp251x_gpio_restore(struct spi_device *spi)
> @@ -594,8 +598,8 @@ static int mcp251x_gpio_setup(struct mcp251x_priv *priv)
>  	gpio->get_direction = mcp251x_gpio_get_direction;
>  	gpio->get = mcp251x_gpio_get;
>  	gpio->get_multiple = mcp251x_gpio_get_multiple;
> -	gpio->set = mcp251x_gpio_set;
> -	gpio->set_multiple = mcp251x_gpio_set_multiple;
> +	gpio->set_rv = mcp251x_gpio_set;
> +	gpio->set_multiple_rv = mcp251x_gpio_set_multiple;
>  	gpio->base = -1;
>  	gpio->ngpio = ARRAY_SIZE(mcp251x_gpio_names);
>  	gpio->names = mcp251x_gpio_names;
> 

Yours sincerely,
Vincent Mailhol

Re: [PATCH 3/4] net: can: mcp251x: use new GPIO line value setter callbacks

[Bartosz Golaszewski]

On Tue, Jun 10, 2025 at 3:55 PM Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
>
> On 10/06/2025 at 21:37, 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>
>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This does not match the address with which you sent the patch: brgl@bgdev.pl
>
> > ---
> >  drivers/net/can/spi/mcp251x.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> > index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644
> > --- a/drivers/net/can/spi/mcp251x.c
> > +++ b/drivers/net/can/spi/mcp251x.c
> > @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip,
> >       return 0;
> >  }
> >
> > -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > -                          int value)
> > +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > +                         int value)
> >  {
> >       struct mcp251x_priv *priv = gpiochip_get_data(chip);
> >       u8 mask, val;
> > @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> >
> >       priv->reg_bfpctrl &= ~mask;
> >       priv->reg_bfpctrl |= val;
> > +
> > +     return 0;
>
> mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write()
> which can fail.
>
> For this change to really make sense, the return value of mcp251x_spi_write()
> should be propagated all the way around.
>

I don't know this code so I followed the example of the rest of the
codebase where the result of this function is never checked - even in
functions that do return values. I didn't know the reason for this and
so didn't want to break anything as I have no means of testing it.

Can you confirm that you really want the result to be checked here?

Bart

Re: [PATCH 3/4] net: can: mcp251x: use new GPIO line value setter callbacks

[Vincent Mailhol]

On 10/06/2025 at 23:05, Bartosz Golaszewski wrote:
> On Tue, Jun 10, 2025 at 3:55 PM Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> wrote:
>>
>> On 10/06/2025 at 21:37, 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>
>>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> This does not match the address with which you sent the patch: brgl@bgdev.pl
>>
>>> ---
>>>  drivers/net/can/spi/mcp251x.c | 16 ++++++++++------
>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
>>> index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644
>>> --- a/drivers/net/can/spi/mcp251x.c
>>> +++ b/drivers/net/can/spi/mcp251x.c
>>> @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip,
>>>       return 0;
>>>  }
>>>
>>> -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>>> -                          int value)
>>> +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>>> +                         int value)
>>>  {
>>>       struct mcp251x_priv *priv = gpiochip_get_data(chip);
>>>       u8 mask, val;
>>> @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>>>
>>>       priv->reg_bfpctrl &= ~mask;
>>>       priv->reg_bfpctrl |= val;
>>> +
>>> +     return 0;
>>
>> mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write()
>> which can fail.
>>
>> For this change to really make sense, the return value of mcp251x_spi_write()
>> should be propagated all the way around.
>>
> 
> I don't know this code so I followed the example of the rest of the
> codebase where the result of this function is never checked - even in
> functions that do return values. I didn't know the reason for this and
> so didn't want to break anything as I have no means of testing it.

The return value of mcp251x_spi_write() is used in mcp251x_hw_reset(). In other
locations, mcp251x_spi_write() is only used in functions which return void, so
obviously, the return value is not checked.

> Can you confirm that you really want the result to be checked here?

That's the point of those new gpio setters, isn't it? If we do not check the
result, I do not understand the purpose of the migration.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 3/4] net: can: mcp251x: use new GPIO line value setter callbacks

[Bartosz Golaszewski]

On Tue, Jun 10, 2025 at 5:48 PM Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
>
> On 10/06/2025 at 23:05, Bartosz Golaszewski wrote:
> > On Tue, Jun 10, 2025 at 3:55 PM Vincent Mailhol
> > <mailhol.vincent@wanadoo.fr> wrote:
> >>
> >> On 10/06/2025 at 21:37, 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>
> >>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> This does not match the address with which you sent the patch: brgl@bgdev.pl
> >>
> >>> ---
> >>>  drivers/net/can/spi/mcp251x.c | 16 ++++++++++------
> >>>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> >>> index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644
> >>> --- a/drivers/net/can/spi/mcp251x.c
> >>> +++ b/drivers/net/can/spi/mcp251x.c
> >>> @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip,
> >>>       return 0;
> >>>  }
> >>>
> >>> -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> >>> -                          int value)
> >>> +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> >>> +                         int value)
> >>>  {
> >>>       struct mcp251x_priv *priv = gpiochip_get_data(chip);
> >>>       u8 mask, val;
> >>> @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> >>>
> >>>       priv->reg_bfpctrl &= ~mask;
> >>>       priv->reg_bfpctrl |= val;
> >>> +
> >>> +     return 0;
> >>
> >> mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write()
> >> which can fail.
> >>
> >> For this change to really make sense, the return value of mcp251x_spi_write()
> >> should be propagated all the way around.
> >>
> >
> > I don't know this code so I followed the example of the rest of the
> > codebase where the result of this function is never checked - even in
> > functions that do return values. I didn't know the reason for this and
> > so didn't want to break anything as I have no means of testing it.
>
> The return value of mcp251x_spi_write() is used in mcp251x_hw_reset(). In other
> locations, mcp251x_spi_write() is only used in functions which return void, so
> obviously, the return value is not checked.
>

Wait, after a second look GPIO callbacks (including those that return
a value like request()) use mcp251x_write_bits() which has no return
value. It probably should propagate what mcp251x_spi_write() returns
but that's material for a different series. The goal of this one is to
use the new setters treewide and drop the old ones from struct
gpio_chip.

Bart

Re: [PATCH 3/4] net: can: mcp251x: use new GPIO line value setter callbacks

[Vincent Mailhol]

On 11/06/2025 at 01:05, Bartosz Golaszewski wrote:
> On Tue, Jun 10, 2025 at 5:48 PM Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> wrote:
>>
>> On 10/06/2025 at 23:05, Bartosz Golaszewski wrote:
>>> On Tue, Jun 10, 2025 at 3:55 PM Vincent Mailhol
>>> <mailhol.vincent@wanadoo.fr> wrote:
>>>>
>>>> On 10/06/2025 at 21:37, 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>
>>>>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> This does not match the address with which you sent the patch: brgl@bgdev.pl
>>>>
>>>>> ---
>>>>>  drivers/net/can/spi/mcp251x.c | 16 ++++++++++------
>>>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
>>>>> index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644
>>>>> --- a/drivers/net/can/spi/mcp251x.c
>>>>> +++ b/drivers/net/can/spi/mcp251x.c
>>>>> @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip,
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>>>>> -                          int value)
>>>>> +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>>>>> +                         int value)
>>>>>  {
>>>>>       struct mcp251x_priv *priv = gpiochip_get_data(chip);
>>>>>       u8 mask, val;
>>>>> @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>>>>>
>>>>>       priv->reg_bfpctrl &= ~mask;
>>>>>       priv->reg_bfpctrl |= val;
>>>>> +
>>>>> +     return 0;
>>>>
>>>> mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write()
>>>> which can fail.
>>>>
>>>> For this change to really make sense, the return value of mcp251x_spi_write()
>>>> should be propagated all the way around.
>>>>
>>>
>>> I don't know this code so I followed the example of the rest of the
>>> codebase where the result of this function is never checked - even in
>>> functions that do return values. I didn't know the reason for this and
>>> so didn't want to break anything as I have no means of testing it.
>>
>> The return value of mcp251x_spi_write() is used in mcp251x_hw_reset(). In other
>> locations, mcp251x_spi_write() is only used in functions which return void, so
>> obviously, the return value is not checked.
>>
> 
> Wait, after a second look GPIO callbacks (including those that return
> a value like request()) use mcp251x_write_bits() which has no return
> value.

Yes. Read again my first message:

  mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write()
  which can fail.

My point is that the grand father can fail.

> It probably should propagate what mcp251x_spi_write() returns

Exactly what I asked for :)

> but that's material for a different series.

Why? Are you going to do this other series?

If the answer is no, then please just do it here. Propagating the error in
mcp251x_write_bits() is a three line change. Am I asking for too much?

> The goal of this one is to
> use the new setters treewide and drop the old ones from struct
> gpio_chip.

Yours sincerely,
Vincent Mailhol

[PATCH 4/4] net: phy: qca807x: 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/net/phy/qcom/qca807x.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/qcom/qca807x.c b/drivers/net/phy/qcom/qca807x.c
index 1af6b5ead74bf615ad155e396b0ecc6fe5636e8c..bc480710c2bd27d621dad3b5595f0a292a4c72c1 100644
--- a/drivers/net/phy/qcom/qca807x.c
+++ b/drivers/net/phy/qcom/qca807x.c
@@ -377,7 +377,7 @@ static int qca807x_gpio_get(struct gpio_chip *gc, unsigned int offset)
 	return FIELD_GET(QCA807X_GPIO_FORCE_MODE_MASK, val);
 }
 
-static void qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+static int qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
 {
 	struct qca807x_gpio_priv *priv = gpiochip_get_data(gc);
 	u16 reg;
@@ -390,14 +390,12 @@ static void qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int valu
 	val |= QCA807X_GPIO_FORCE_EN;
 	val |= FIELD_PREP(QCA807X_GPIO_FORCE_MODE_MASK, value);
 
-	phy_write_mmd(priv->phy, MDIO_MMD_AN, reg, val);
+	return phy_write_mmd(priv->phy, MDIO_MMD_AN, reg, val);
 }
 
 static int qca807x_gpio_dir_out(struct gpio_chip *gc, unsigned int offset, int value)
 {
-	qca807x_gpio_set(gc, offset, value);
-
-	return 0;
+	return qca807x_gpio_set(gc, offset, value);
 }
 
 static int qca807x_gpio(struct phy_device *phydev)
@@ -425,7 +423,7 @@ static int qca807x_gpio(struct phy_device *phydev)
 	gc->get_direction = qca807x_gpio_get_direction;
 	gc->direction_output = qca807x_gpio_dir_out;
 	gc->get = qca807x_gpio_get;
-	gc->set = qca807x_gpio_set;
+	gc->set_rv = qca807x_gpio_set;
 
 	return devm_gpiochip_add_data(dev, gc, priv);
 }

-- 
2.48.1

Re: [PATCH 4/4] net: phy: qca807x: use new GPIO line value setter callbacks

[Andrew Lunn]

> -static void qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> +static int qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
>  {
>  	struct qca807x_gpio_priv *priv = gpiochip_get_data(gc);
>  	u16 reg;
> @@ -390,14 +390,12 @@ static void qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int valu
>  	val |= QCA807X_GPIO_FORCE_EN;
>  	val |= FIELD_PREP(QCA807X_GPIO_FORCE_MODE_MASK, value);
>  
> -	phy_write_mmd(priv->phy, MDIO_MMD_AN, reg, val);
> +	return phy_write_mmd(priv->phy, MDIO_MMD_AN, reg, val);
>  }

The full function is:

static void qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
{
	struct qca807x_gpio_priv *priv = gpiochip_get_data(gc);
	u16 reg;
	int val;

	reg = QCA807X_MMD7_LED_FORCE_CTRL(offset);

	val = phy_read_mmd(priv->phy, MDIO_MMD_AN, reg);
	val &= ~QCA807X_GPIO_FORCE_MODE_MASK;
	val |= QCA807X_GPIO_FORCE_EN;
	val |= FIELD_PREP(QCA807X_GPIO_FORCE_MODE_MASK, value);

	phy_write_mmd(priv->phy, MDIO_MMD_AN, reg, val);
}

The phy_read_mmd() could also fail and return an error code.

    Andrew

---
pw-bot: cr