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:
signature.asc
Description: PGP signature
