From: Lee Jones <lee.jones@linaro.org>
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: Richard Fitzgerald <rf@opensource.cirrus.com>,
mturquette@baylibre.com, sboyd@kernel.org, broonie@kernel.org,
linus.walleij@linaro.org, robh+dt@kernel.org,
mark.rutland@arm.com, lgirdwood@gmail.com,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
patches@opensource.cirrus.com, linux-clk@vger.kernel.org,
linux-gpio@vger.kernel.org
Subject: Re: [PATCH v2 2/5] mfd: lochnagar: Add support for the Cirrus Logic Lochnagar
Date: Fri, 26 Oct 2018 08:33:01 +0100 [thread overview]
Message-ID: <20181026073301.GJ4870@dell> (raw)
In-Reply-To: <20181025124905.GF16508@imbe.wolfsonmicro.main>
On Thu, 25 Oct 2018, Charles Keepax wrote:
> On Thu, Oct 25, 2018 at 12:42:05PM +0100, 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@opensource.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.
> > 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. Then default values can be changed at the leisure of the
> > s/w.
> >
> > 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?
>
> I don't think you can sensibly get away with not supplying
> default values. You say most sane register layouts have zero
> values, alas again you may not be the biggest fan of our hardware
> guys. The Lochnagar actually does have mostly zero defaults but
> that is very much not generally the case with our hardware.
>
> One of the main aims of regmap is to avoid bus accesses when
> possible but I guess on the first write/read to any register you
> could insert a read to pull the default, although I do worry
> there is some corner case/flexibility that is going to cause
> headaches later.
This is basically what I am suggesting.
> I am not sure that dynamically constructing the defaults would be
> possible from a table of non-zero ones, or at least doing so would
> probably require a fairly major change to the way registers are
> specified since with the current callback based approach and a
> sparse regmap you could need to iterate over billions of
> potential registers to build the table.
What if a valid register range was provided with only the non-zero
values?
> > > > > > +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.
>
> Removing code is one thing but I would argue that data tables are
> somewhat less of a concern. I guess kernel size for very small
> systems but then is someone going to be using Lochnagar on one of
> those.
>
> > 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
>
> It might be possible to combine all these into one thing,
> although again its a fairly major core change.
I'm not suggesting that this will be solved overnight.
> > > > > > +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.
>
> I guess the question from my side becomes do you want to block
> this driver pending on major refactoring to regmap? I will have a
> think about what I can do but its going to affect a LOT of drivers.
No one is NACKing this driver.
We're using it as a vehicle for discussion.
> > > > > > + ret = devm_of_platform_populate(dev);
> > > > > > + if (ret < 0) {
> > > > > > + dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > > > > > + return ret;
> > > > > > + }
> > > > >
> > > > > Please do not mix OF and MFD device registration strategies.
> > > > >
> > > > > Pick one and register all devices through your chosen method.
> > > >
> > > > Hmmm we use this to do things like register some fixed regulators
> > > > and clocks that don't need any control but do need to be associated
> > > > with the device. I could do that through the MFD although it is in
> > > > direct conflict with the feedback on the clock patches I received
> > > > to move the fixed clocks into devicetree rather than registering
> > > > them manually (see v2 of the patch chain).
> >
> > The I suggest moving everything to DT.
>
> I will have a look and see what that would look like.
Thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
next prev parent reply other threads:[~2018-10-26 7:33 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-08 13:25 [PATCH v2 1/5] mfd: lochnagar: Add initial binding documentation Charles Keepax
2018-10-08 13:25 ` [PATCH v2 2/5] mfd: lochnagar: Add support for the Cirrus Logic Lochnagar Charles Keepax
2018-10-25 7:44 ` Lee Jones
2018-10-25 8:26 ` Charles Keepax
2018-10-25 9:28 ` Richard Fitzgerald
2018-10-25 10:12 ` Mark Brown
2018-10-25 10:56 ` Charles Keepax
2018-10-25 11:42 ` Lee Jones
2018-10-25 12:49 ` Charles Keepax
2018-10-25 13:20 ` Charles Keepax
2018-10-25 13:47 ` Richard Fitzgerald
2018-10-26 15:49 ` Mark Brown
2018-10-26 7:33 ` Lee Jones [this message]
2018-10-26 15:47 ` Mark Brown
2018-10-25 13:40 ` Richard Fitzgerald
2018-10-26 8:00 ` Lee Jones
2018-10-26 20:32 ` Mark Brown
2018-10-29 11:04 ` Lee Jones
2018-10-29 11:52 ` Richard Fitzgerald
2018-10-29 12:36 ` Richard Fitzgerald
2018-10-29 12:57 ` Mark Brown
2018-11-01 10:28 ` Charles Keepax
2018-11-01 11:40 ` Richard Fitzgerald
2018-11-01 12:04 ` Mark Brown
2018-11-01 12:01 ` Mark Brown
2018-11-01 14:17 ` Richard Fitzgerald
2018-10-08 13:25 ` [PATCH v2 3/5] clk: " Charles Keepax
2018-10-11 7:00 ` Stephen Boyd
2018-10-11 13:26 ` Charles Keepax
2018-10-12 15:59 ` Stephen Boyd
2018-10-15 10:49 ` Charles Keepax
2018-10-15 16:39 ` Stephen Boyd
2018-10-15 16:55 ` Mark Brown
2018-10-15 21:53 ` Stephen Boyd
2018-10-11 14:54 ` Mark Brown
2018-10-11 19:36 ` Stephen Boyd
2018-10-12 16:52 ` Mark Brown
2018-10-08 13:25 ` [PATCH v2 4/5] regulator: " Charles Keepax
2018-10-08 13:25 ` [PATCH v2 5/5] pinctrl: " Charles Keepax
2018-10-12 22:08 ` [PATCH v2 1/5] mfd: lochnagar: Add initial binding documentation Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181026073301.GJ4870@dell \
--to=lee.jones@linaro.org \
--cc=broonie@kernel.org \
--cc=ckeepax@opensource.cirrus.com \
--cc=devicetree@vger.kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mturquette@baylibre.com \
--cc=patches@opensource.cirrus.com \
--cc=rf@opensource.cirrus.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).