Message-ID: <20160418103212.GQ3217@sirena.org.uk>
Date:	Mon, 18 Apr 2016 11:32:12 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Jonathan Cameron <jic23@...nel.org>
Cc:	Crestez Dan Leonard <leonard.crestez@...el.com>,
	Peter Meerwald-Stadler <pmeerw@...erw.net>,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Daniel Baluta <daniel.baluta@...el.com>
Subject: Re: [PATCH 1/5] max44000: Initial commit

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.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)

Powered by blists - more mailing lists