Re: [PATCH v3 05/16] mfd: Add support for Kontron sl28cpld management controller

Am 2020-05-12 23:59, schrieb Rob Herring:

On Mon, May 11, 2020 at 4:45 PM Michael Walle [off-list ref] wrote:

quoted

Am 2020-05-11 23:13, schrieb Rob Herring:

quoted

On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote:

quoted

+#define SL28CPLD_VERSION    0x03
+#define SL28CPLD_WATCHDOG_BASE      0x04
+#define SL28CPLD_HWMON_FAN_BASE     0x0b
+#define SL28CPLD_PWM0_BASE  0x0c
+#define SL28CPLD_PWM1_BASE  0x0e
+#define SL28CPLD_GPIO0_BASE 0x10
+#define SL28CPLD_GPIO1_BASE 0x15
+#define SL28CPLD_GPO_BASE   0x1a
+#define SL28CPLD_GPI_BASE   0x1b
+#define SL28CPLD_INTC_BASE  0x1c

If you want to use 'reg' in the binding, these are the numbers you
should be using rather than making up numbering!

My motivation is that I don't want to hardcode the internal addresses
of the management controller in the device tree. For example if they
will move around with a later update of the controller, so a driver 
can
be compatible with both the old and the new version. If they are in 
the
device tree, only one register layout is possible.

I don't understand, if the addresses change, then the above defines
have to change. So your driver is only compatible with 1 version. If
you change the CPLD, then that's a h/w change and your h/w description
(DT) should change. That can either be the compatible string changing
and updating the driver with new match data such as register offsets
or all the differences are in DT and there's no kernel change.

The CPLD and the board is designed in a way that it is possible to
update and/or change its function (or parts of it). It must not be
a hardware change, although I admit thats a bit of a grey area wether
you treat it as hardware or "firmware". Anyway, yes you'd have to
change the register offsets, but as this is code it might support
different register offsets. For example you could dynamically add
functionality, if there is a newer controller version while still
being compatible with older versions.

quoted

quoted

However, I still don't think you need any child nodes. All the data in
the DT binding is right here in the driver already. There's no
advantage
to putting child nodes in DT, because this driver still has to be
updated if you add more nodes.

But then any phandle will reference the mfd device. And for example
there
are two different interrupt controllers, that is the INTC and the
GPIO[01],
which will then be combined into one device tree node, right?

You either have to add a cell for 'bank' or divide the 1st cell into a
bank and index. Both have been done before.

But this won't work with watchdogs, correct? See
https://lore.kernel.org/linux-devicetree/7acbb6d9b2240b1856136fa35c1318bf@walle.cc/ (local)

To go the other direction, AIUI you shouldn't need OF_MFD_CELL_REG
entries if you have the child devices in DT.

This is a general problem IMHO. There are mfd drivers which have mfd
cells and a device tree node associated with each cell. But it just
works as long as there is only one sub device per unique compatible
string. So you cannot have multiple mfd cells with the same
compatible string.

That being said, I can try to reimplement it using
of_platform_populate() and its internal offset as its unit address.

Pick one way or the
other. It's ultimately a judgement call. For a one-off device, sub
devices in DT doesn't really buy you anything. If you have sub-blocks
showing up multiple devices, then sub devices makes sense. If there's
only 2-3 combinations, then it's a toss up.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel