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@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, &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);