Message ID | 20240417-mcp251xfd-gpio-feature-v1-0-bc0c61fd0c80@ew.tq-group.com |
---|---|
Headers | show |
Series | can: mcp251xfd: add gpio functionality | expand |
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, ®, 1, > + data + data_offset, 2); > + reg += 3; > + /* Write IOCON[31:24] */ > + mcp251xfd_regmap_crc_gather_write(context, ®, 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, ®, 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, ®, 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, ®, 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, ®, 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