[PATCH 0/4] can: mcp251xfd: add gpio functionality

[PATCH 0/4] can: mcp251xfd: add gpio functionality

[Gregor Herburger]

Hi all,

The mcp251xfd allows two pins to be configured as GPIOs. This series
adds support for this feature.

The GPIO functionality is controlled with the IOCON register which has
an erratum. The second patch is to work around this erratum. I am not
sure if the place for the check and workaround in
mcp251xfd_regmap_crc_write is correct or if the check could be bypassed
with a direct call to mcp251xfd_regmap_crc_gather_write. If you have a
better suggestion where to add the check please let me know.

Patch 1 fixes a unwanted wakeup of the chip
Patch 2 is the fix/workaround for the aforementioned erratum
Patch 3 adds the gpio support
Patch 4 updates dt-binding

---
Gregor Herburger (4):
      can: mcp251xfd: stop timestamp before sending chip to sleep
      can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround for errata 5
      can: mcp251xfd: add gpio functionality
      dt-binding: can: mcp251xfd: add gpio-controller property

 .../bindings/net/can/microchip,mcp251xfd.yaml      |   2 +
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c     | 139 ++++++++++++++++++++-
 drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c   |  56 ++++++++-
 .../net/can/spi/mcp251xfd/mcp251xfd-timestamp.c    |   5 +-
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h          |   4 +
 5 files changed, 200 insertions(+), 6 deletions(-)
---
base-commit: 1fdad13606e104ff103ca19d2d660830cb36d43e
change-id: 20240417-mcp251xfd-gpio-feature-29a1bf6acb54

Best regards,
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 1/4] can: mcp251xfd: stop timestamp before sending chip to sleep

[Gregor Herburger]

MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip
is send to sleep and the timestamp workqueue is not stopped chip is
waked by SPI transfer of mcp251xfd_timestamp_read.

So before sending chip to sleep stop timestamp otherwise the
mcp251xfd_timestamp_read callback would wake chip up.

Also there are error paths in mcp251xfd_chip_start where workqueue has
not been initialized but mcp251xfd_chip_stop is called. So check for
initialized func before canceling delayed_work.

Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c      | 1 +
 drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 1d9057dc44f2..eb699288c076 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -744,6 +744,7 @@ static void mcp251xfd_chip_stop(struct mcp251xfd_priv *priv,
 
 	mcp251xfd_chip_interrupts_disable(priv);
 	mcp251xfd_chip_rx_int_disable(priv);
+	mcp251xfd_timestamp_stop(priv);
 	mcp251xfd_chip_sleep(priv);
 }
 
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
index 712e09186987..537c31890838 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
@@ -67,5 +67,8 @@ void mcp251xfd_timestamp_init(struct mcp251xfd_priv *priv)
 
 void mcp251xfd_timestamp_stop(struct mcp251xfd_priv *priv)
 {
-	cancel_delayed_work_sync(&priv->timestamp);
+	struct work_struct *work = &priv->timestamp.work;
+
+	if (work->func)
+		cancel_delayed_work_sync(&priv->timestamp);
 }

-- 
2.34.1

Re: [PATCH 1/4] can: mcp251xfd: stop timestamp before sending chip to sleep

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On 17.04.2024 15:43:54, Gregor Herburger wrote:
> MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip
> is send to sleep and the timestamp workqueue is not stopped chip is
> waked by SPI transfer of mcp251xfd_timestamp_read.
> 
> So before sending chip to sleep stop timestamp otherwise the
> mcp251xfd_timestamp_read callback would wake chip up.
> 
> Also there are error paths in mcp251xfd_chip_start where workqueue has
> not been initialized but mcp251xfd_chip_stop is called. So check for
> initialized func before canceling delayed_work.

Can you move the mcp251xfd_timestamp_init() (which starts the
timestamping worker) into mcp251xfd_chip_start() to keep things
symmetrical? I think then you don't need to check for "work->func" in
mcp251xfd_timestamp_stop().

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Re: [PATCH 1/4] can: mcp251xfd: stop timestamp before sending chip to sleep

[Gregor Herburger]

On Wed, Apr 24, 2024 at 10:24:58AM +0200, Marc Kleine-Budde wrote:
> On 17.04.2024 15:43:54, Gregor Herburger wrote:
> > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip
> > is send to sleep and the timestamp workqueue is not stopped chip is
> > waked by SPI transfer of mcp251xfd_timestamp_read.
> > 
> > So before sending chip to sleep stop timestamp otherwise the
> > mcp251xfd_timestamp_read callback would wake chip up.
> > 
> > Also there are error paths in mcp251xfd_chip_start where workqueue has
> > not been initialized but mcp251xfd_chip_stop is called. So check for
> > initialized func before canceling delayed_work.
> 
> Can you move the mcp251xfd_timestamp_init() (which starts the
> timestamping worker) into mcp251xfd_chip_start() to keep things
> symmetrical? I think then you don't need to check for "work->func" in
> mcp251xfd_timestamp_stop().
> 
Hi Marc,

I realise now I confused mcp251xfd_timestamp_init with
mcp251xfd_chip_timestamp_init.

The only call chip mcp251xfd_chip_stop without call to
mcp251xfd_timestamp_stop is from mcp251xfd_handle_cerrif.

So it should be sufficient to stop the worker there and the check for
"work->func" can be also omitted.

Best regards,
Gregor
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 1/4] can: mcp251xfd: stop timestamp before sending chip to sleep

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On 17.04.2024 15:43:54, Gregor Herburger wrote:
> MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip
> is send to sleep and the timestamp workqueue is not stopped chip is
> waked by SPI transfer of mcp251xfd_timestamp_read.

How does the Low-Power Mode affect the GPIO lines? Is there a difference
if the device is only in sleep mode?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Re: [PATCH 1/4] can: mcp251xfd: stop timestamp before sending chip to sleep

[Gregor Herburger]

On Wed, Apr 24, 2024 at 01:54:54PM +0200, Marc Kleine-Budde wrote:
> On 17.04.2024 15:43:54, Gregor Herburger wrote:
> > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip
> > is send to sleep and the timestamp workqueue is not stopped chip is
> > waked by SPI transfer of mcp251xfd_timestamp_read.
> 
> How does the Low-Power Mode affect the GPIO lines? Is there a difference
> if the device is only in sleep mode?

The MCP251XFD_REG_IOCON is cleared when leaving Low-Power Mode. This is
why I implemented regcache.

Best regards
Gregor
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 1/4] can: mcp251xfd: stop timestamp before sending chip to sleep

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

On 25.04.2024 07:17:11, Gregor Herburger wrote:
> On Wed, Apr 24, 2024 at 01:54:54PM +0200, Marc Kleine-Budde wrote:
> > On 17.04.2024 15:43:54, Gregor Herburger wrote:
> > > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip
> > > is send to sleep and the timestamp workqueue is not stopped chip is
> > > waked by SPI transfer of mcp251xfd_timestamp_read.
> > 
> > How does the Low-Power Mode affect the GPIO lines? Is there a difference
> > if the device is only in sleep mode?
> 
> The MCP251XFD_REG_IOCON is cleared when leaving Low-Power Mode. This is
> why I implemented regcache.

But that means you have to power the chip if a GPIO is requested. You
have to power up the chip in the request() callback and power it down in
the free() callback.

I've 2 patches laying around, one that moves the timestamp
init/start/stop into the chip_start/stop. And another one that moves the
soft reset and basic configuration of the chip into the runtime pm
functions. I have to make both patches compatible and send them to the
list. Feel free to pick them up and integrate them into your series.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Re: [PATCH 1/4] can: mcp251xfd: stop timestamp before sending chip to sleep

[Gregor Herburger]

On Thu, Apr 25, 2024 at 08:29:13AM +0200, Marc Kleine-Budde wrote:
> On 25.04.2024 07:17:11, Gregor Herburger wrote:
> > On Wed, Apr 24, 2024 at 01:54:54PM +0200, Marc Kleine-Budde wrote:
> > > On 17.04.2024 15:43:54, Gregor Herburger wrote:
> > > > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip
> > > > is send to sleep and the timestamp workqueue is not stopped chip is
> > > > waked by SPI transfer of mcp251xfd_timestamp_read.
> > > 
> > > How does the Low-Power Mode affect the GPIO lines? Is there a difference
> > > if the device is only in sleep mode?
> > 
> > The MCP251XFD_REG_IOCON is cleared when leaving Low-Power Mode. This is
> > why I implemented regcache.
> 
> But that means you have to power the chip if a GPIO is requested. You
> have to power up the chip in the request() callback and power it down in
> the free() callback.

Ah I see. Currently the GPIO rigister is cached and only written to the
chip if the netdevice is set up. I think to have a more generic gpio controller
the chip should wake up when the GPIO is requested. Also the chip should
not go to sleep while GPIO is requested and netdevice is set down.

> I've 2 patches laying around, one that moves the timestamp
> init/start/stop into the chip_start/stop. And another one that moves the
> soft reset and basic configuration of the chip into the runtime pm
> functions. I have to make both patches compatible and send them to the
> list. Feel free to pick them up and integrate them into your series.

I will have a look at them.
> 
> regards,
> Marc
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Best regards
Gregor
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround for errata 5

[Gregor Herburger]

According to Errata DS80000789E 5 writing IOCON register using one SPI
write command clears LAT0/LAT1.

Errata Fix/Work Around suggests to write registers with single byte write
instructions. However, it seems that every write to the second byte
causes the overrite of LAT0/LAT1.

Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.

Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index 92b7bc7f14b9..ab4e372baffb 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
@@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
 	return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
 }
 
+static int
+mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
+{
+	const size_t data_offset = sizeof(__be16) +
+		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
+	u16 reg = *(u16 *)data;
+
+	/* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
+	 *
+	 * According to Errata DS80000789E 5 writing IOCON register using one
+	 * SPI write command clears LAT0/LAT1.
+	 *
+	 * Errata Fix/Work Around suggests to write registers with single byte
+	 * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
+	 * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
+	 */
+
+	/* Write IOCON[15:0] */
+	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
+					  data + data_offset, 2);
+	reg += 3;
+	/* Write IOCON[31:24] */
+	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
+					  data + data_offset + 3, 1);
+
+	return 0;
+}
+
 static int
 mcp251xfd_regmap_crc_write(void *context,
 			   const void *data, size_t count)
 {
 	const size_t data_offset = sizeof(__be16) +
 		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
+	u16 reg = *(u16 *)data;
 
-	return mcp251xfd_regmap_crc_gather_write(context,
+	if (reg == MCP251XFD_REG_IOCON)
+		return mcp251xfd_regmap_crc_write_iocon(context,
+						 data, count);
+	else
+		return mcp251xfd_regmap_crc_gather_write(context,
 						 data, data_offset,
 						 data + data_offset,
 						 count - data_offset);

-- 
2.34.1

Re: [PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround for errata 5

[Simon Horman]

On Wed, Apr 17, 2024 at 03:43:55PM +0200, Gregor Herburger wrote:
> According to Errata DS80000789E 5 writing IOCON register using one SPI
> write command clears LAT0/LAT1.
> 
> Errata Fix/Work Around suggests to write registers with single byte write
> instructions. However, it seems that every write to the second byte
> causes the overrite of LAT0/LAT1.

nit: overwrite

Flagged by ./scripts/checkpatch.pl --codespell

> 
> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.
> 
> Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>

...

Re: [PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround for errata 5

[Vincent MAILHOL]

On Wed. 17 Apr. 2024 at 22:45, Gregor Herburger
<gregor.herburger@ew.tq-group.com> wrote:
> According to Errata DS80000789E 5 writing IOCON register using one SPI
> write command clears LAT0/LAT1.
>
> Errata Fix/Work Around suggests to write registers with single byte write
> instructions. However, it seems that every write to the second byte
> causes the overrite of LAT0/LAT1.
>
> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.
>
> Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> index 92b7bc7f14b9..ab4e372baffb 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> @@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
>         return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
>  }
>
> +static int
> +mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
                                                                            ^^^^
count is never used.

> +{
> +       const size_t data_offset = sizeof(__be16) +
> +               mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +       u16 reg = *(u16 *)data;

This line made me scratch my head a lot.

When I see a void * parameter named data, I expect this to be a memory
region. Here, if I got this correctly, data is just a pointer to a u16
which represents the low bit of a register.

So, if you are not passing an address to a memory region but just a
single scalar, why the void *? Wouldn't it be better to just do:

  mcp251xfd_regmap_crc_write_iocon(void *context, u16 reg)

> +       /* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
> +        *
> +        * According to Errata DS80000789E 5 writing IOCON register using one
> +        * SPI write command clears LAT0/LAT1.
> +        *
> +        * Errata Fix/Work Around suggests to write registers with single byte
> +        * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
> +        * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
                                                                  ^^^^^^^^
clearing

> +        */
> +
> +       /* Write IOCON[15:0] */
> +       mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +                                         data + data_offset, 2);
> +       reg += 3;
> +       /* Write IOCON[31:24] */
> +       mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +                                         data + data_offset + 3, 1);
> +
> +       return 0;
> +}
> +
>  static int
>  mcp251xfd_regmap_crc_write(void *context,
>                            const void *data, size_t count)

This also uses the const void* data, except that here, this is kind of
forced by the prototype of the write() callback function from struct
regmap_bus. Also, count is properly used.

>  {
>         const size_t data_offset = sizeof(__be16) +
>                 mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +       u16 reg = *(u16 *)data;
>
> -       return mcp251xfd_regmap_crc_gather_write(context,
> +       if (reg == MCP251XFD_REG_IOCON)
> +               return mcp251xfd_regmap_crc_write_iocon(context,
> +                                                data, count);

After changing the prototype of mcp251xfd_regmap_crc_write_iocon(),
this would then become:

                return mcp251xfd_regmap_crc_write_iocon(context, reg);

> +       else
> +               return mcp251xfd_regmap_crc_gather_write(context,
>                                                  data, data_offset,
>                                                  data + data_offset,
>                                                  count - data_offset);

Re: [PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround for errata 5

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4190 bytes --]

On 17.04.2024 15:43:55, Gregor Herburger wrote:
> According to Errata DS80000789E 5 writing IOCON register using one SPI
> write command clears LAT0/LAT1.
> 
> Errata Fix/Work Around suggests to write registers with single byte write
> instructions. However, it seems that every write to the second byte
> causes the overrite of LAT0/LAT1.

This change doesn't use single byte write instructions.

> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.

I discovered that erratum, it's described in
mcp251xfd_chip_rx_int_enable():

	/* Configure GPIOs:
	 * - PIN0: GPIO Input
	 * - PIN1: GPIO Input/RX Interrupt
	 *
	 * PIN1 must be Input, otherwise there is a glitch on the
	 * rx-INT line. It happens between setting the PIN as output
	 * (in the first byte of the SPI transfer) and configuring the
	 * PIN as interrupt (in the last byte of the SPI transfer).
	 */

The problem is that the SPI writes 1 byte at a time, starting at the
lower address. The chip updates the GPIO pin's status after each written
byte.

This may leads to a glitch if you have an external pull up. The power on
default auf the chip is GPIO/input, the GPIO line is not driven by the
chip and with the external pull up this will result in a high level.

If you configure the GPIO as an output/high, the driver first writes
bits 0...7, which results in the GPIO line being configured as an
output; the subsequent bits 8...15 configure the level of the GPIO
line.

This change doesn't take care of this.

I'm not sure, if it's better to have 2 dedicated writes to IOCON in the
driver or try to hide it here in the regmap.

> Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> index 92b7bc7f14b9..ab4e372baffb 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> @@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
>  	return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
>  }
>  
> +static int
> +mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
> +{
> +	const size_t data_offset = sizeof(__be16) +
> +		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +	u16 reg = *(u16 *)data;
> +
> +	/* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
> +	 *
> +	 * According to Errata DS80000789E 5 writing IOCON register using one
> +	 * SPI write command clears LAT0/LAT1.
> +	 *
> +	 * Errata Fix/Work Around suggests to write registers with single byte
> +	 * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
> +	 * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
> +	 */
> +
> +	/* Write IOCON[15:0] */
> +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +					  data + data_offset, 2);

You write 15:0 in 1 go here.

> +	reg += 3;
> +	/* Write IOCON[31:24] */
> +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +					  data + data_offset + 3, 1);
> +
> +	return 0;
> +}
> +
>  static int
>  mcp251xfd_regmap_crc_write(void *context,
>  			   const void *data, size_t count)
>  {
>  	const size_t data_offset = sizeof(__be16) +
>  		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +	u16 reg = *(u16 *)data;
>  
> -	return mcp251xfd_regmap_crc_gather_write(context,
> +	if (reg == MCP251XFD_REG_IOCON)
> +		return mcp251xfd_regmap_crc_write_iocon(context,
> +						 data, count);
> +	else
> +		return mcp251xfd_regmap_crc_gather_write(context,
>  						 data, data_offset,
>  						 data + data_offset,
>  						 count - data_offset);

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Re: [PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write(): workaround for errata 5

[Gregor Herburger]

On Wed, Apr 24, 2024 at 01:51:37PM +0200, Marc Kleine-Budde wrote:
> On 17.04.2024 15:43:55, Gregor Herburger wrote:
> > According to Errata DS80000789E 5 writing IOCON register using one SPI
> > write command clears LAT0/LAT1.
> > 
> > Errata Fix/Work Around suggests to write registers with single byte write
> > instructions. However, it seems that every write to the second byte
> > causes the overrite of LAT0/LAT1.
> 
> This change doesn't use single byte write instructions.

Yes, because this is not necessary. Single byte write instructions
wont't fix the problem. The microchip errata sheet is wrong or at least
misleading expressed.

From my observation single byte insctructions won't fix the problem. No
write to bits [16:24] does fix the problem.

I talked to Thomas Kopp from Microchip about that and he confirmed my
observations.
> 
> > Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.
> 
> I discovered that erratum, it's described in
> mcp251xfd_chip_rx_int_enable():
> 
> 	/* Configure GPIOs:
> 	 * - PIN0: GPIO Input
> 	 * - PIN1: GPIO Input/RX Interrupt
> 	 *
> 	 * PIN1 must be Input, otherwise there is a glitch on the
> 	 * rx-INT line. It happens between setting the PIN as output
> 	 * (in the first byte of the SPI transfer) and configuring the
> 	 * PIN as interrupt (in the last byte of the SPI transfer).
> 	 */
> 
> The problem is that the SPI writes 1 byte at a time, starting at the
> lower address. The chip updates the GPIO pin's status after each written
> byte.
> 
> This may leads to a glitch if you have an external pull up. The power on
> default auf the chip is GPIO/input, the GPIO line is not driven by the
> chip and with the external pull up this will result in a high level.
> 
> If you configure the GPIO as an output/high, the driver first writes
> bits 0...7, which results in the GPIO line being configured as an
> output; the subsequent bits 8...15 configure the level of the GPIO
> line.
> 
> This change doesn't take care of this.

I'm not sure if this is the same problem. Anyway, with this fix we didn't
see any glitches on the gpio lines.
> 
> I'm not sure, if it's better to have 2 dedicated writes to IOCON in the
> driver or try to hide it here in the regmap.

What would be the alternative? Maybe add a mcp251xfd_write_iocon
function to the driver and call there regmap_update_bits twice?

> 
> > Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> > ---
> >  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> > index 92b7bc7f14b9..ab4e372baffb 100644
> > --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> > +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> > @@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
> >  	return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
> >  }
> >  
> > +static int
> > +mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
> > +{
> > +	const size_t data_offset = sizeof(__be16) +
> > +		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> > +	u16 reg = *(u16 *)data;
> > +
> > +	/* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
> > +	 *
> > +	 * According to Errata DS80000789E 5 writing IOCON register using one
> > +	 * SPI write command clears LAT0/LAT1.
> > +	 *
> > +	 * Errata Fix/Work Around suggests to write registers with single byte
> > +	 * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
> > +	 * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
> > +	 */
> > +
> > +	/* Write IOCON[15:0] */
> > +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> > +					  data + data_offset, 2);
> 
> You write 15:0 in 1 go here.
See above.
> 
> > +	reg += 3;
> > +	/* Write IOCON[31:24] */
> > +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> > +					  data + data_offset + 3, 1);
> > +
> > +	return 0;
> > +}
> > +
> >  static int
> >  mcp251xfd_regmap_crc_write(void *context,
> >  			   const void *data, size_t count)
> >  {
> >  	const size_t data_offset = sizeof(__be16) +
> >  		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> > +	u16 reg = *(u16 *)data;
> >  
> > -	return mcp251xfd_regmap_crc_gather_write(context,
> > +	if (reg == MCP251XFD_REG_IOCON)
> > +		return mcp251xfd_regmap_crc_write_iocon(context,
> > +						 data, count);
> > +	else
> > +		return mcp251xfd_regmap_crc_gather_write(context,
> >  						 data, data_offset,
> >  						 data + data_offset,
> >  						 count - data_offset);
> 
> Marc
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |


Best regards,
Gregor
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 3/4] can: mcp251xfd: add gpio functionality

[Gregor Herburger]

The mcp251xfd devices allow two pins to be configured as gpio. Add this
functionality to driver.

Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c   | 138 ++++++++++++++++++++++-
 drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c |  21 +++-
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h        |   4 +
 3 files changed, 159 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index eb699288c076..5ba9fd0af4b6 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -16,6 +16,7 @@
 #include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/device.h>
+#include <linux/gpio/driver.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
@@ -366,6 +367,8 @@ static int mcp251xfd_chip_wake(const struct mcp251xfd_priv *priv)
 
 static inline int mcp251xfd_chip_sleep(const struct mcp251xfd_priv *priv)
 {
+	int ret;
+
 	if (priv->pll_enable) {
 		u32 osc;
 		int err;
@@ -381,7 +384,12 @@ static inline int mcp251xfd_chip_sleep(const struct mcp251xfd_priv *priv)
 		priv->spi->max_speed_hz = priv->spi_max_speed_hz_slow;
 	}
 
-	return mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_SLEEP);
+	ret = mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_SLEEP);
+
+	regcache_cache_only(priv->map_reg, true);
+	regcache_mark_dirty(priv->map_reg);
+
+	return ret;
 }
 
 static int mcp251xfd_chip_softreset_do(const struct mcp251xfd_priv *priv)
@@ -389,6 +397,8 @@ static int mcp251xfd_chip_softreset_do(const struct mcp251xfd_priv *priv)
 	const __be16 cmd = mcp251xfd_cmd_reset();
 	int err;
 
+	regcache_cache_only(priv->map_reg, false);
+
 	/* The Set Mode and SPI Reset command only works if the
 	 * controller is not in Sleep Mode.
 	 */
@@ -401,7 +411,12 @@ static int mcp251xfd_chip_softreset_do(const struct mcp251xfd_priv *priv)
 		return err;
 
 	/* spi_write_then_read() works with non DMA-safe buffers */
-	return spi_write_then_read(priv->spi, &cmd, sizeof(cmd), NULL, 0);
+	err = spi_write_then_read(priv->spi, &cmd, sizeof(cmd), NULL, 0);
+	if (err)
+		return err;
+
+	/* After reset restore cached register values to hardware */
+	return regcache_sync(priv->map_reg);
 }
 
 static int mcp251xfd_chip_softreset_check(const struct mcp251xfd_priv *priv)
@@ -1772,6 +1787,119 @@ static int mcp251xfd_register_check_rx_int(struct mcp251xfd_priv *priv)
 	return 0;
 }
 
+static const char * const mcp251xfd_gpio_names[] = {"GPIO0", "GPIO1"};
+
+static int mcp251xfd_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct mcp251xfd_priv *priv = gpiochip_get_data(chip);
+	u32 pin_mask = MCP251XFD_REG_IOCON_PM0 << offset;
+
+	return regmap_update_bits(priv->map_reg, MCP251XFD_REG_IOCON,
+				  pin_mask, pin_mask);
+}
+
+static int mcp251xfd_gpio_get_direction(struct gpio_chip *chip,
+					unsigned int offset)
+{
+	struct mcp251xfd_priv *priv = gpiochip_get_data(chip);
+	u32 mask = MCP251XFD_REG_IOCON_TRIS0 << offset;
+	u32 val;
+
+	regmap_read(priv->map_reg, MCP251XFD_REG_IOCON, &val);
+
+	if (mask & val)
+		return GPIO_LINE_DIRECTION_IN;
+
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mcp251xfd_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct mcp251xfd_priv *priv = gpiochip_get_data(chip);
+	u32 mask = MCP251XFD_REG_IOCON_GPIO0 << offset;
+	u32 val;
+
+	regcache_drop_region(priv->map_reg, MCP251XFD_REG_IOCON, MCP251XFD_REG_IOCON);
+	regmap_read(priv->map_reg, MCP251XFD_REG_IOCON, &val);
+
+	return !!(mask & val);
+}
+
+static int mcp251xfd_gpio_direction_output(struct gpio_chip *chip,
+					   unsigned int offset, int value)
+{
+	struct mcp251xfd_priv *priv = gpiochip_get_data(chip);
+	u32 dir_mask = MCP251XFD_REG_IOCON_TRIS0 << offset;
+	u32 val_mask = MCP251XFD_REG_IOCON_LAT0 << offset;
+	u32 val;
+
+	if (value)
+		val = val_mask;
+	else
+		val = 0;
+
+	return regmap_update_bits(priv->map_reg, MCP251XFD_REG_IOCON,
+				  dir_mask | val_mask, val);
+}
+
+static int mcp251xfd_gpio_direction_input(struct gpio_chip *chip,
+					  unsigned int offset)
+{
+	struct mcp251xfd_priv *priv = gpiochip_get_data(chip);
+	u32 dir_mask = MCP251XFD_REG_IOCON_TRIS0 << offset;
+
+	return regmap_update_bits(priv->map_reg, MCP251XFD_REG_IOCON,
+				  dir_mask, dir_mask);
+}
+
+static void mcp251xfd_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			       int value)
+{
+	struct mcp251xfd_priv *priv = gpiochip_get_data(chip);
+	u32 val_mask = MCP251XFD_REG_IOCON_LAT0 << offset;
+	u32 val;
+	int ret;
+
+	if (value)
+		val = val_mask;
+	else
+		val = 0;
+
+	ret = regmap_update_bits(priv->map_reg, MCP251XFD_REG_IOCON,
+				 val_mask, val);
+	if (ret)
+		dev_warn(&priv->spi->dev,
+			 "Failed to set GPIO %u: %d\n", offset, ret);
+}
+
+static int mcp251fdx_gpio_setup(struct mcp251xfd_priv *priv)
+{
+	struct gpio_chip *gc = &priv->gc;
+
+	if (!device_property_present(&priv->spi->dev, "gpio-controller"))
+		return 0;
+
+	if (priv->rx_int)
+		return dev_err_probe(&priv->spi->dev, -EINVAL,
+				     "Can't configure gpio-controller with RX-INT!\n");
+
+	gc->label = dev_name(&priv->spi->dev);
+	gc->parent = &priv->spi->dev;
+	gc->owner = THIS_MODULE;
+	gc->request = mcp251xfd_gpio_request;
+	gc->get_direction = mcp251xfd_gpio_get_direction;
+	gc->direction_output = mcp251xfd_gpio_direction_output;
+	gc->direction_input = mcp251xfd_gpio_direction_input;
+	gc->get = mcp251xfd_gpio_get;
+	gc->set = mcp251xfd_gpio_set;
+	gc->base = -1;
+	gc->can_sleep = true;
+	gc->ngpio = ARRAY_SIZE(mcp251xfd_gpio_names);
+	gc->names = mcp251xfd_gpio_names;
+
+	return devm_gpiochip_add_data(&priv->spi->dev, gc, priv);
+}
+
 static int
 mcp251xfd_register_get_dev_id(const struct mcp251xfd_priv *priv, u32 *dev_id,
 			      u32 *effective_speed_hz_slow,
@@ -2142,6 +2270,12 @@ static int mcp251xfd_probe(struct spi_device *spi)
 	if (err)
 		goto out_free_candev;
 
+	err = mcp251fdx_gpio_setup(priv);
+	if (err) {
+		dev_err_probe(&spi->dev, err, "Failed to register gpio-controller.\n");
+		goto out_free_candev;
+	}
+
 	err = mcp251xfd_register(priv);
 	if (err) {
 		dev_err_probe(&spi->dev, err, "Failed to detect %s.\n",
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index ab4e372baffb..868c424f22a4 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
@@ -447,6 +447,21 @@ static const struct regmap_access_table mcp251xfd_reg_table = {
 	.n_yes_ranges = ARRAY_SIZE(mcp251xfd_reg_table_yes_range),
 };
 
+static bool mcp251xfd_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MCP251XFD_REG_ECCCON:
+	case MCP251XFD_REG_DEVID:
+	case MCP251XFD_REG_NBTCFG:
+	case MCP251XFD_REG_DBTCFG:
+	case MCP251XFD_REG_TDC:
+	case MCP251XFD_REG_TSCON:
+	case MCP251XFD_REG_IOCON:
+		return false;
+	}
+	return true;
+}
+
 static const struct regmap_config mcp251xfd_regmap_nocrc = {
 	.name = "nocrc",
 	.reg_bits = 16,
@@ -456,7 +471,8 @@ static const struct regmap_config mcp251xfd_regmap_nocrc = {
 	.max_register = 0xffc,
 	.wr_table = &mcp251xfd_reg_table,
 	.rd_table = &mcp251xfd_reg_table,
-	.cache_type = REGCACHE_NONE,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = mcp251xfd_volatile_reg,
 	.read_flag_mask = (__force unsigned long)
 		cpu_to_be16(MCP251XFD_SPI_INSTRUCTION_READ),
 	.write_flag_mask = (__force unsigned long)
@@ -483,7 +499,8 @@ static const struct regmap_config mcp251xfd_regmap_crc = {
 	.max_register = 0xffc,
 	.wr_table = &mcp251xfd_reg_table,
 	.rd_table = &mcp251xfd_reg_table,
-	.cache_type = REGCACHE_NONE,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = mcp251xfd_volatile_reg,
 };
 
 static const struct regmap_bus mcp251xfd_bus_crc = {
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index 24510b3b8020..e2ab486862d8 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -14,6 +14,7 @@
 #include <linux/can/core.h>
 #include <linux/can/dev.h>
 #include <linux/can/rx-offload.h>
+#include <linux/gpio/driver.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
@@ -660,6 +661,9 @@ struct mcp251xfd_priv {
 
 	struct mcp251xfd_devtype_data devtype_data;
 	struct can_berr_counter bec;
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gc;
+#endif
 };
 
 #define MCP251XFD_IS(_model) \

-- 
2.34.1

Re: [PATCH 3/4] can: mcp251xfd: add gpio functionality

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On 17.04.2024 15:43:56, Gregor Herburger wrote:
> The mcp251xfd devices allow two pins to be configured as gpio. Add this
> functionality to driver.

Can you please move the introduction of regmap cache into a separate
patch.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] can: mcp251xfd: add gpio functionality

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On 17.04.2024 15:43:56, Gregor Herburger wrote:
> The mcp251xfd devices allow two pins to be configured as gpio. Add this
> functionality to driver.

Fails to build if CONFIG_GPIOLIB is not enabled.

|   CC [M]  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.o
| drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c: In function ‘mcp251fdx_gpio_setup’:
| drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c:1877:39: error: ‘struct mcp251xfd_priv’ has no member named ‘gc’; did you mean ‘cc’?
|  1877 |         struct gpio_chip *gc = &priv->gc;
|       |                                       ^~
|       |                                       cc

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] can: mcp251xfd: add gpio functionality

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3131 bytes --]

On 17.04.2024 15:43:56, Gregor Herburger wrote:
> The mcp251xfd devices allow two pins to be configured as gpio. Add this
> functionality to driver.
> 
> Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c   | 138 ++++++++++++++++++++++-
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c |  21 +++-
>  drivers/net/can/spi/mcp251xfd/mcp251xfd.h        |   4 +
>  3 files changed, 159 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index eb699288c076..5ba9fd0af4b6 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c

[...]

> +static int mcp251fdx_gpio_setup(struct mcp251xfd_priv *priv)
> +{
> +	struct gpio_chip *gc = &priv->gc;
> +
> +	if (!device_property_present(&priv->spi->dev, "gpio-controller"))
> +		return 0;
> +
> +	if (priv->rx_int)
> +		return dev_err_probe(&priv->spi->dev, -EINVAL,
> +				     "Can't configure gpio-controller with RX-INT!\n");
> +
> +	gc->label = dev_name(&priv->spi->dev);
> +	gc->parent = &priv->spi->dev;
> +	gc->owner = THIS_MODULE;
> +	gc->request = mcp251xfd_gpio_request;
> +	gc->get_direction = mcp251xfd_gpio_get_direction;
> +	gc->direction_output = mcp251xfd_gpio_direction_output;
> +	gc->direction_input = mcp251xfd_gpio_direction_input;
> +	gc->get = mcp251xfd_gpio_get;
> +	gc->set = mcp251xfd_gpio_set;

Please also implement the get_multiple and set_multiple callbacks.

> +	gc->base = -1;
> +	gc->can_sleep = true;
> +	gc->ngpio = ARRAY_SIZE(mcp251xfd_gpio_names);
> +	gc->names = mcp251xfd_gpio_names;
> +
> +	return devm_gpiochip_add_data(&priv->spi->dev, gc, priv);
> +}
> +
>  static int
>  mcp251xfd_register_get_dev_id(const struct mcp251xfd_priv *priv, u32 *dev_id,
>  			      u32 *effective_speed_hz_slow,
> @@ -2142,6 +2270,12 @@ static int mcp251xfd_probe(struct spi_device *spi)

[...]

> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> index 24510b3b8020..e2ab486862d8 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> @@ -14,6 +14,7 @@
>  #include <linux/can/core.h>
>  #include <linux/can/dev.h>
>  #include <linux/can/rx-offload.h>
> +#include <linux/gpio/driver.h>

please keep the includes alphabetically sorted.

>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/netdevice.h>
> @@ -660,6 +661,9 @@ struct mcp251xfd_priv {
>  
>  	struct mcp251xfd_devtype_data devtype_data;
>  	struct can_berr_counter bec;
> +#ifdef CONFIG_GPIOLIB
> +	struct gpio_chip gc;
> +#endif
>  };
>  
>  #define MCP251XFD_IS(_model) \

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] can: mcp251xfd: add gpio functionality

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On 17.04.2024 15:43:56, Gregor Herburger wrote:
> The mcp251xfd devices allow two pins to be configured as gpio. Add this
> functionality to driver.
> 
> Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c   | 138 ++++++++++++++++++++++-
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c |  21 +++-
>  drivers/net/can/spi/mcp251xfd/mcp251xfd.h        |   4 +
>  3 files changed, 159 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index eb699288c076..5ba9fd0af4b6 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c

[...]

> +static int mcp251fdx_gpio_setup(struct mcp251xfd_priv *priv)
> +{
> +	struct gpio_chip *gc = &priv->gc;
> +
> +	if (!device_property_present(&priv->spi->dev, "gpio-controller"))
> +		return 0;
> +
> +	if (priv->rx_int)
> +		return dev_err_probe(&priv->spi->dev, -EINVAL,
> +				     "Can't configure gpio-controller with RX-INT!\n");

Can you enhance the DT binding to reflect this?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] can: mcp251xfd: add gpio functionality

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

On 24.04.2024 11:35:59, Marc Kleine-Budde wrote:
> On 17.04.2024 15:43:56, Gregor Herburger wrote:
> > The mcp251xfd devices allow two pins to be configured as gpio. Add this
> > functionality to driver.
> > 
> > Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> > ---
> >  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c   | 138 ++++++++++++++++++++++-
> >  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c |  21 +++-
> >  drivers/net/can/spi/mcp251xfd/mcp251xfd.h        |   4 +
> >  3 files changed, 159 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > index eb699288c076..5ba9fd0af4b6 100644
> > --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> 
> [...]
> 
> > +static int mcp251fdx_gpio_setup(struct mcp251xfd_priv *priv)
> > +{
> > +	struct gpio_chip *gc = &priv->gc;
> > +
> > +	if (!device_property_present(&priv->spi->dev, "gpio-controller"))
> > +		return 0;
> > +
> > +	if (priv->rx_int)
> > +		return dev_err_probe(&priv->spi->dev, -EINVAL,
> > +				     "Can't configure gpio-controller with RX-INT!\n");
> 
> Can you enhance the DT binding to reflect this?

Another option would be to check if RX-INT is configured in the
mcp251xfd_gpio_request() callback and refuse to request GPIO1.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 4/4] dt-binding: can: mcp251xfd: add gpio-controller property

[Gregor Herburger]

The mcp251xfd has two pins that can be used as gpio. Add gpio-controller
property to binding description.

Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
---
 Documentation/devicetree/bindings/net/can/microchip,mcp251xfd.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp251xfd.yaml b/Documentation/devicetree/bindings/net/can/microchip,mcp251xfd.yaml
index 2a98b26630cb..e8a3ad4b1231 100644
--- a/Documentation/devicetree/bindings/net/can/microchip,mcp251xfd.yaml
+++ b/Documentation/devicetree/bindings/net/can/microchip,mcp251xfd.yaml
@@ -49,6 +49,8 @@ properties:
       Must be half or less of "clocks" frequency.
     maximum: 20000000
 
+  gpio-controller: true
+
 required:
   - compatible
   - reg

-- 
2.34.1

Re: [PATCH 4/4] dt-binding: can: mcp251xfd: add gpio-controller property

[Rob Herring]

On Wed, 17 Apr 2024 15:43:57 +0200, Gregor Herburger wrote:
> The mcp251xfd has two pins that can be used as gpio. Add gpio-controller
> property to binding description.
> 
> Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
> ---
>  Documentation/devicetree/bindings/net/can/microchip,mcp251xfd.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/can/microchip,mcp251xfd.yaml: properties: '#gpio-cells' is a dependency of 'gpio-controller'
	from schema $id: http://devicetree.org/meta-schemas/gpios.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240417-mcp251xfd-gpio-feature-v1-4-bc0c61fd0c80@ew.tq-group.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 4/4] dt-binding: can: mcp251xfd: add gpio-controller property

[Krzysztof Kozlowski]

On 17/04/2024 15:43, Gregor Herburger wrote:
> The mcp251xfd has two pins that can be used as gpio. Add gpio-controller
> property to binding description.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

Also, please test the patch.

Best regards,
Krzysztof