On 10/06/2025 at 23:05, Bartosz Golaszewski wrote:

On Tue, Jun 10, 2025 at 3:55 PM Vincent Mailhol
[off-list ref] wrote:

quoted

On 10/06/2025 at 21:37, Bartosz Golaszewski wrote:

quoted

From: Bartosz Golaszewski <redacted>

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 <redacted>

                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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

quoted

---
 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