Message-ID: <20181026080051.GK4870@dell>
Date:   Fri, 26 Oct 2018 09:00:51 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     Charles Keepax <ckeepax@...nsource.cirrus.com>,
        mturquette@...libre.com, sboyd@...nel.org, broonie@...nel.org,
        linus.walleij@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
        lgirdwood@...il.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com,
        linux-clk@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 2/5] mfd: lochnagar: Add support for the Cirrus Logic
 Lochnagar

On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

> On 25/10/18 12:42, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > > On 25/10/18 09:26, Charles Keepax wrote:
> > > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > > From: Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
> > > > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > > > ...
> > > > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > > > +};
> > > > > 
> > > > > Why do you need to specify each register value?
> > > > 
> > > > The way regmap operates it needs to know the starting value of
> > > > each register. It will use this to initialise the cache and to
> > > > determine if writes need to actually update the hardware on
> > > > cache_syncs after devices have been powered back up.
> > 
> > That sounds crazy to me.  Some devices have thousands of registers.
> 
> Largely the point. How long do you think it would take to populate the
> cache if you had to read thousands of registers over I2C? Boot time matters.
> Deferring it until it's touched can create various unpredictable and
> annoying behaviour later, for example if a lot of cache entries are
> written while the chip is asleep and the initial values weren't known
> then a suspend/resume cannot filter out writes that are setting the
> register to its default (which regmap does to avoid unnecessary bus traffic).
> So the resume could have a large amount of unnecessary overhead writing
> registers to a value they already have or reading the initial values of
> those registers.

One more register read when initially writing to a register and one
more when resuming doesn't sound like a vast amount of over-head.

> > At a line per register, that's thousands of lines of code/cruft.
> > Especially seeing as most (sane?) register layouts I've seen default
> > to zero.
> 
> Not a valid generalization. And it's not a question of sanity, the purpose
> of the register and the overhead to setup a use-case also matter.
> There are many reasons why the default of a register might not be zero.
> Take a look at drivers/mfd/wm5110-tables.c, a lot of the registers don't
> have a default of zero (and that's only the registers accessed by the driver.)
> It's particularly true of registers that affect things like voltage and
> current sources, zero might be a very inappropriate default - even dangerous.
> Also enable bits, if some things must power-up enabled and others don't, unless
> you want a chip that has a confusing mix of inverted and non-inverted enable
> bits. Another side to this is to reduce the number of writes to enable _typical_
> behaviour - if an IP block has say 100 registers and you have to write all of
> them to make it work that's a lot of overhead compared to them defaulting to
> typical values used 99.9% of the time and you only need to write one or two
> registers to use it.

Not sure what you think I was suggesting above.  If the default values
are actually non-zero that's fine - we'll either leave them as they
are (if they are never changed, in which case Regmap doesn't even need
to know about them), document only those (non-zero) ones or wait until
they are read for the first time, then populate the cache.

Setting up the cache manually also sounds like a vector for potential
failure.  At least if you were to cache dynamically on first write
(either on start-up or after sleep) then the actual value will be
cached, rather than what a piece of C code says it should be.

>   Then default values can be changed at the leisure of the
> > s/w.
> 
> Potentially with a lot of overhead, especially on those chips with thousands
> of registers to set to useful non-zero values before you can use it.
> 
> Lochnagar doesn't have that many registers but convention and consistency also
> comes into play. Regmap is used in a particular way and it helps people a lot
> if every driver using it follows the convention.

Precisely my point.  Lochnagar is a small device yet it's required to
submit hundreds of lines of Regmap tables.  Imagine what that would
look like for a large device.

> > Even if it is absolutely critical that you have to supply these to
> > Regmap up-front, instead of on first use/read, why can't you just
> > supply the oddball non-zero ones?
> 
> If you aren't happy with the regmap subsystem you could send some
> patches to change it to what you would be happy with (and patch the ~1300
> drivers that use it)

That may well happen.  This is the pre-patch discussion.

Apologies that it had to happen on your submission, but this is that
alerted me to the issue(s).

> Like any kernel subsystem it has an API that we have to obey to be able to
> use it.

Again, this isn't about Lochnagar.

> > > > > > +static const struct reg_sequence lochnagar1_patch[] = {
> > > > > > +	{ 0x40, 0x0083 },
> > > > > > +	{ 0x46, 0x0001 },
> > > > > > +	{ 0x47, 0x0018 },
> > > > > > +	{ 0x50, 0x0000 },
> > > > > > +};
> > > > > 
> > > > > I'm really not a fan of these so call 'patches'.
> > > > > 
> > > > > Can't you set the registers up proper way?
> > > > > 
> > > > 
> > > > I will see if we could move any out of here or define any of the
> > > > registers but as we have discussed before it is not always possible.
> > > > 
> > > 
> > > Also patches generally come out of hardware tuning/qualification/tools
> > > as this list of address,value. So it's easy for people to dump an update
> > > into the driver as a trivial copy-paste but more work if they have to
> > > reverse-engineer the patch list from hardware/datasheet into what each
> > > line "means" and then find the relevant lines of code to change. It's also
> > > much easier to answer the question "Have these hardware patches been
> > > applied to the driver?" if we have them in the original documented format.
> > > It just makes people's lives more difficult if they have to search around
> > > the code to try to find something that looks like the originally specified
> > > patch list. We don't use them just as a lazy way to setup some registers.
> > 
> > I understand why they normally exist (sometimes people are just lazy
> > too) (Mark: BTW chicken-bits sound delicious!).  They're just ugly
> > from an Open Source PoV.
> 
> In my opinion a lot of the source code in Linux is much uglier than
> these tables.

Right.  I usually comment on those (when I see them) too.

Besides, just because some people committing murder, doesn't mean
other people shouldn't go to jail for stealing a car.

> > > > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > > > ...
> > > > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > > > ...
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > 
> > > > > This is getting silly now.  Can't you use ranges?
> > > > 
> > > > I can if you feel strongly about it? But it does make the drivers
> > > > much more error prone and significantly more annoying to work
> > > > with. I find it is really common to be checking that a register
> > > > is handled correctly through the regmap callbacks and it is nice
> > > > to just be able to grep for that. Obviously this won't work for
> > > > all devices/regmaps as well since many will not have consecutive
> > > > addresses on registers, for example having multi-byte registers
> > > > that are byte addressed.
> > > > 
> > > > How far would you like me to take this as well? Is it just the
> > > > numeric registers you want ranges for ie.
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > > > 
> > > > Or is it all consecutive registers even if they are unrelated
> > > > (exmaple is probably not accurate as I haven't checked the
> > > > addresses):
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > > > 
> > > > I don't mind the first at all but the second is getting really
> > > > horrible in my opinion.
> > 
> > My issue is that we have one end of the scale, where contributors are
> > submitting patches, trying to remove a line of code here, a line of
> > code there, then there are patches like this one which describe the
> > initial value, readable status, writable status and volatile status of
> > each and every register.
> 
> If we could add support for new devices by removing lines of code that
> would be cool :). Eventually Linux would support every piece of hardware
> and be zero lines of code.

I'm starting to think that you've missed the point. ;)

> > The API is obscene and needs a re-work IMHO.
> > 
> > I really hope we do not really have to list every register, but *if we
> > absolutely must*, let's do it once:
> > 
> >    REG_ADDRESS, WRV, INITIAL_VALUE
> 
> To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
> so it's not our responsibility if you don't like it. Mark Brown is the maintainer.

Sounds very much like you are saying, "it's not Cirrus' fault,
therefore it is not my problem"?  Which is a terrible attitude.

Remember that the Linux kernel is an open community.  Anyone should be
free to discuss any relevant issue.  If we decide to take this
off-line, which is likely, then so be it.  In the mean time, as a
contributor to this community project, it's absolutely your
responsibly to help discuss and potentially solve wider issues than
just your lines of code.

> Submit your patches to Mark and the owners of those ~1300 drivers to propose
> changes to regmap that you would be happy with.

Quoting myself from above:

  "That may well happen.  This is the pre-patch discussion.

  Apologies that it had to happen on your submission, but this is that
  alerted me to the issue(s)."

> > > > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > > > ...
> > > > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > > > +};
> > > > > 
> > > > > OMG!  Vile, vile vile!
> > > > 
> > > > I really feel this isn't the driver you are objecting to as such
> > > > but the way regmap operates and also we seem to always have the same
> > > > discussions around regmap every time we push a driver.
> > 
> > Absolutely.  I didn't like it before.  I like it even less now.
> > 
> > > > Is there
> > > > any way me, you and Mark could hash this out and find out a way to
> > > > handle regmaps that is acceptable to you? I don't suppose you are
> > > > in Edinburgh at the moment for ELCE?
> > 
> > I'm not at ELCE I'm afraid.
> > 
> > > I suppose if Mark was willing to promote the regmap drivers to be a
> > > top-level subsystem that could contain the regmap definitions of devices
> > > then we could dump our regmap definitions in there, where Mark can review
> > > it as he's familiar with regmap and the chips and the reasons why things
> > > are done the way they are, rather than Lee having to stress about it every
> > > time we need to create an MFD device that uses regmap. Though that would
> > > make the initialization of an MFD rather awkward with the code required
> > > to init the MFD it not actually being in the MFD tree.
> > 
> > My issue isn't where all this bumph lives.
> > 
> > It's the fact that it's required (at least at this level) at all.
> > 
> 
> As above, if one subsystem owner doesn't like another subsystem then those
> subsystem owners should talk to each other and sort something out. It shouldn't
> block patches that are just trying to use the subsystem as it currently exists
> in the kernel.

Again, no one is blocking this patch.

This driver was submitted for review/discussion.  We are discussing.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists