| Message ID | 20250610-gpiochip-set-rv-net-v1-3-35668dd1c76f@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: use new GPIO line value setter callbacks | expand |
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
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
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
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;