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

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,

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>

...

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

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

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

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

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

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

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

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

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

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

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

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

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

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