Re: [PATCH 1/5] max44000: Initial commit

[Jonathan Cameron <jic23@kernel.org>]

On 18/04/16 11:32, Mark Brown wrote:
> On Sun, Apr 17, 2016 at 09:36:10AM +0100, Jonathan Cameron wrote:
>> On 11/04/16 16:08, Crestez Dan Leonard wrote:
> 
> Please leave blank lines between paragraphs, it makes things much easier
> to read.
> 
>>> Would it be
>>> acceptable to just expand the REGMASK_* into a large or-ing of (1 <<
>>> MAX44000_REG_*)? Then it would be clear in the source what's going on
>>> but binary will be the same.
> 
>> Would be interesting to see but I doubt the optimized code will be that
>> different, and the switch is pretty much the 'standard' way of handling
>> these long register lists cleanly.
> 
>> Often it comes down to doing things the way people expect them to
>> be done as that makes review easier for a tiny possible cost in
>> run time.
> 
> You can also specify ranges of registers if the map mostly has large
> blocks of contiguous registers, a switch statement tends to be easier
> and is probably more efficient for most register maps.
> 
>>>>>> +    .use_single_rw    = 1,
>>>>>> +    .cache_type    = REGCACHE_FLAT,
> 
>>>> This always seems like a good idea, but tends to cause issues.
>>>> FLAT is really only meant for very high performance devices, you
>>>> are probably better with something else here.  If you are doing this
>>>> deliberately to make the below writes actually occur, then please
>>>> add a comment here.
> 
>>> I used REGCACHE_FLAT because my device has a very small number of
>>> registers and I assume it uses less memory. Honestly it would make
>>> sense for regmap to include a REGCACHE_AUTO cache_type and pick the
>>> cache implementation automatically based on number of registers.
> 
>> I've fallen for that one in the past as well.  AUTO would indeed
>> be good if it was easy to do.
> 
> It's extremely easy to do.  Unless you've got a good reason to do
> anything else you should always be using an rbtree.  The core would
> never select anything else.
> 
>>> Yes. It would not work otherwise since the regmap cache is explicitly
>>> initialized with my listed defaults.
>>> As far as I can tell regmap_write will always write to the hardware.
> 
>> Interesting and counter intuitive if true...
> 
> No, if the driver asked to write then we write.  If the driver wants to
> do a read/modify/write cycle it should use regmap_update_bits().
> 
>>> If the device had a reset command I should have used that, right?
>>> What is happening is that I am implementing a reset command in
>>> software.
> 
>> Not necessarily.  Lots of drivers don't - but instead have their interfaces
>> reflect their current state on startup.  Reset's are often there to get
>> the internal state of the device cleaned up if it is in an unknowable state
>> rather than to get the defaults to any particular state.  They are always
>> read from the hardware or a known good cache when queried from userspace
>> anyway.
> 
> That's not entirely it.  Doing a reset is often faster than rewriting
> the entire register map and is more robust against undocumented
> registers or things the driver didn't think about which means that
> the behaviour is going to be more consistent.
Hmm. Fair enough on the undocumented register argument...
>