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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

On Tue, Sep 22, 2020 at 03:56:38PM +0200, Marc Kleine-Budde wrote:
> On 9/22/20 2:13 PM, Mark Brown wrote:

> > This feels like a non-idiomatic way of doing this - usually you'd
> > enumerate then allocate the extra maps (using regmap_reinit_cache() to
> > replace the regmap used to do the enumeration).

> I have implemented two regmap clients for this driver. One does transfers with
> CRC, the other one not. Both are REGCACHE_NONE.

Hrm, right.  The assumption when that was written was that nothing
fundamental would change about the regmap format but we should probably
extend to allow that since it's the general idea.  However it looks like
that might not be needed here.

> For autodetection the driver initializes the regmap which does transfers with
> CRC. Then it detects the chip variant and maybe that variant works properly
> without CRC. The CRC regmap is not needed anymore, so the buffer used to
> linearize the data, is freed.

That flow sounds sensible but the way the code is written it's really
not clear that this is what's going on since there's only one init
function with no indication that it'll be invoked multiple times, the
logic for detection is in a completely separate place and functions by
changing the quirk flags underneath this code - just reading the regmap
handling alone you'd be hard pressed to spot that this is what's going
on.  It might be clearer to have an explicit function to generate the
regmap for detection and then call the current function _reinit(),
which would also allow it to be a bit more explicit about the
transitions (if it only needs to handle the CRC->no CRC transition).
Failing that at least some comments about what's going on would be
useful, even just a "this will be called twice because...".

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