On Wed. 17 Apr. 2024 at 22:45, Gregor Herburger
<gregor.herburger@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
> 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);
