Re: [PATCH v53 2/6] can: mcp25xxfd: add regmap infrastructure

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2407 bytes --]

On 9/21/20 9:33 PM, Mark Brown wrote:
> On Fri, Sep 18, 2020 at 07:25:32PM +0200, Marc Kleine-Budde wrote:
> 
>> +static int
>> +mcp25xxfd_regmap_nocrc_update_bits(void *context, unsigned int reg,
>> +				   unsigned int mask, unsigned int val)
>> +{
> 
> This doesn't look like the hardware has an update_bits() operation so
> why is there a driver specific implementation?

It's an optimization.

The registers of the device are 32 bits in general, but support 8 bit read/write
operations. So if only bits in the e.g. 2nd 8 bit are changed, both in the read
and write operation we can save 3 bytes.

Further in some registers the driver only changes all writeable bits in the
respective 8bit part of the register at the same time. This means we can skip
the read completely.

>> +static int
>> +mcp25xxfd_regmap_init_nocrc(struct mcp25xxfd_priv *priv)
>> +{
>> +	if (!priv->map_nocrc) {
>> +		struct regmap *map;
>> +
>> +		map = devm_regmap_init(&priv->spi->dev, &mcp25xxfd_bus_nocrc,
>> +				       priv->spi, &mcp25xxfd_regmap_nocrc);
> 
> Why all these checks to see if things might already be allocated?
> 
>> +static void mcp25xxfd_regmap_destroy_nocrc(struct mcp25xxfd_priv *priv)
>> +{
>> +	if (priv->map_buf_nocrc_rx) {
>> +		devm_kfree(&priv->spi->dev, priv->map_buf_nocrc_rx);
>> +		priv->map_buf_nocrc_rx = NULL;
>> +	}
> 
> Why explicitly free managed allocations like this?

The driver covers two chips, the mcp2517fd and the mcp2518fd. Due to erratas we
need different quirks for these variants. Further you can configure if regmap
should use transfers with or without CRC checks (separately for register access,
RAM access in RX and TX path).

It's possible to distinguish between chip variants by reading/writing a
register, so we first init the regmap with CRC mode, autodetect the chip
variant, and then re-init the regmap with the quirks of that detected variant.

The mcp25xxfd regmap init is written so that resources that are allocated first,
but not needed due to different quirks are then freed (both regmap and the
buffers for rx/tx).

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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