Re: [PATCH v2 2/5] mfd: lochnagar: Add support for the Cirrus Logic Lochnagar

On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:

On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

quoted

On 25/10/18 09:26, Charles Keepax wrote:

quoted

On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:

quoted

On Mon, 08 Oct 2018, Charles Keepax wrote:

quoted

From: Charles Keepax <redacted>
+static const struct reg_default lochnagar1_reg_defaults[] = {
+	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
+	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },

...

quoted

quoted

+	{ 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.

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.

quoted

quoted

quoted

quoted

+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:

...

quoted

quoted

+	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:

...

quoted

quoted

+	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.

quoted

quoted

quoted

quoted

+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 },

...

quoted

quoted

+	{ 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.

quoted

quoted

quoted

quoted

+	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,
Charles