| Message ID | 20200604211039.12689-3-michael@walle.cc |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
Mark, what do you think? On Thu, 04 Jun 2020, Michael Walle wrote: > Add the core support for the board management controller found on the > SMARC-sAL28 board. It consists of the following functions: > - watchdog > - GPIO controller > - PWM controller > - fan sensor > - interrupt controller > > At the moment, this controller is used on the Kontron SMARC-sAL28 board. > > Please note that the MFD driver is defined as bool in the Kconfig > because the next patch will add interrupt support. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/mfd/Kconfig | 19 ++++++++++ > drivers/mfd/Makefile | 2 ++ > drivers/mfd/sl28cpld.c | 79 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 100 insertions(+) > create mode 100644 drivers/mfd/sl28cpld.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 4f8b73d92df3..5c0cd514d197 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -2109,5 +2109,24 @@ config SGI_MFD_IOC3 > If you have an SGI Origin, Octane, or a PCI IOC3 card, > then say Y. Otherwise say N. > > +config MFD_SL28CPLD > + bool "Kontron sl28 core driver" "Kontron SL28 Core Driver" > + depends on I2C=y > + depends on OF > + select REGMAP_I2C > + select MFD_CORE > + help > + This option enables support for the board management controller > + found on the Kontron sl28 CPLD. You have to select individual I can't find any reference to the "Kontron sl28 CPLD" online. Is there a datasheet? > + functions, such as watchdog, GPIO, etc, under the corresponding menus > + in order to enable them. > + > + Currently supported boards are: > + > + Kontron SMARC-sAL28 > + > + To compile this driver as a module, choose M here: the module will be > + called sl28cpld. > + > endmenu > endif > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 9367a92f795a..be59fb40aa28 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > obj-$(CONFIG_MFD_STMFX) += stmfx.o > > obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o > + > +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o > diff --git a/drivers/mfd/sl28cpld.c b/drivers/mfd/sl28cpld.c > new file mode 100644 > index 000000000000..a23194bb6efa > --- /dev/null > +++ b/drivers/mfd/sl28cpld.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * MFD core for the sl28cpld. Ideally this would match the Kconfig subject line. > + * Copyright 2019 Kontron Europe GmbH This is out of date. > + */ > + > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/regmap.h> > + > +#define SL28CPLD_VERSION 0x03 > +#define SL28CPLD_MIN_REQ_VERSION 14 > + > +struct sl28cpld { > + struct device *dev; > + struct regmap *regmap; > +}; Why do you need this structure? > +static const struct regmap_config sl28cpld_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .reg_stride = 1, > +}; > + > +static int sl28cpld_probe(struct i2c_client *i2c) > +{ > + struct sl28cpld *sl28cpld; > + struct device *dev = &i2c->dev; > + unsigned int cpld_version; > + int ret; > + > + sl28cpld = devm_kzalloc(dev, sizeof(*sl28cpld), GFP_KERNEL); > + if (!sl28cpld) > + return -ENOMEM; > + > + sl28cpld->regmap = devm_regmap_init_i2c(i2c, &sl28cpld_regmap_config); > + if (IS_ERR(sl28cpld->regmap)) > + return PTR_ERR(sl28cpld->regmap); This is now a shared memory allocator and not an MFD at all. I'm clamping down on these type of drivers! Please find a better way to accomplish this. Potentially using "simple-mfd" and "simple-regmap". The former already exists and does what you want. The latter doesn't yet exist, but could solve your and lots of other contributor's issues. Heck maybe I'll implement it myself if this keeps happening. > + ret = regmap_read(sl28cpld->regmap, SL28CPLD_VERSION, &cpld_version); > + if (ret) > + return ret; > + > + if (cpld_version < SL28CPLD_MIN_REQ_VERSION) { > + dev_err(dev, "unsupported CPLD version %d\n", cpld_version); > + return -ENODEV; > + } > + > + sl28cpld->dev = dev; > + i2c_set_clientdata(i2c, sl28cpld); If the struct definition is in here, how do you use it elsewhere? > + dev_info(dev, "successfully probed. CPLD version %d\n", cpld_version); > + > + return devm_of_platform_populate(&i2c->dev); > +} > + > +static const struct of_device_id sl28cpld_of_match[] = { > + { .compatible = "kontron,sl28cpld-r1", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, sl28cpld_of_match); > + > +static struct i2c_driver sl28cpld_driver = { > + .probe_new = sl28cpld_probe, > + .driver = { > + .name = "sl28cpld", > + .of_match_table = of_match_ptr(sl28cpld_of_match), > + }, > +}; > +module_i2c_driver(sl28cpld_driver); > + > +MODULE_DESCRIPTION("sl28cpld MFD Core Driver"); > +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); > +MODULE_LICENSE("GPL");
On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote: > > Add the core support for the board management controller found on the > SMARC-sAL28 board. It consists of the following functions: > - watchdog > - GPIO controller > - PWM controller > - fan sensor > - interrupt controller > > At the moment, this controller is used on the Kontron SMARC-sAL28 board. > > Please note that the MFD driver is defined as bool in the Kconfig > because the next patch will add interrupt support. ... > +config MFD_SL28CPLD > + bool "Kontron sl28 core driver" > + depends on I2C=y Why not module? > + depends on OF I didn't find an evidence this is needed. No Compile Test? > + select REGMAP_I2C > + select MFD_CORE ... > +#include <linux/of_platform.h> No evidence of user of this. I think you meant mod_devicetable.h. ... > +static struct i2c_driver sl28cpld_driver = { > + .probe_new = sl28cpld_probe, > + .driver = { > + .name = "sl28cpld", > + .of_match_table = of_match_ptr(sl28cpld_of_match), Drop of_match_ptr(). It has a little sense in this context (depends OF). It will have a little sense even if you drop depends OF b/c you will introduce a compiler warning. > + }, > +};
On Fri, Jun 5, 2020 at 11:01 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote: ... > > Please note that the MFD driver is defined as bool in the Kconfig > > because the next patch will add interrupt support. > > + bool "Kontron sl28 core driver" > > + depends on I2C=y > > Why not module? To be clear, I have read above, but it didn't shed a light.
Hi Lee, thanks for the review. Am 2020-06-05 08:57, schrieb Lee Jones: > Mark, what do you think? > > On Thu, 04 Jun 2020, Michael Walle wrote: > >> Add the core support for the board management controller found on the >> SMARC-sAL28 board. It consists of the following functions: >> - watchdog >> - GPIO controller >> - PWM controller >> - fan sensor >> - interrupt controller >> >> At the moment, this controller is used on the Kontron SMARC-sAL28 >> board. >> >> Please note that the MFD driver is defined as bool in the Kconfig >> because the next patch will add interrupt support. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/mfd/Kconfig | 19 ++++++++++ >> drivers/mfd/Makefile | 2 ++ >> drivers/mfd/sl28cpld.c | 79 >> ++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 100 insertions(+) >> create mode 100644 drivers/mfd/sl28cpld.c >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index 4f8b73d92df3..5c0cd514d197 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -2109,5 +2109,24 @@ config SGI_MFD_IOC3 >> If you have an SGI Origin, Octane, or a PCI IOC3 card, >> then say Y. Otherwise say N. >> >> +config MFD_SL28CPLD >> + bool "Kontron sl28 core driver" > > "Kontron SL28 Core Driver" ok > >> + depends on I2C=y >> + depends on OF >> + select REGMAP_I2C >> + select MFD_CORE >> + help >> + This option enables support for the board management controller >> + found on the Kontron sl28 CPLD. You have to select individual > > I can't find any reference to the "Kontron sl28 CPLD" online. > > Is there a datasheet? Unfortunately not. >> + functions, such as watchdog, GPIO, etc, under the corresponding >> menus >> + in order to enable them. >> + >> + Currently supported boards are: >> + >> + Kontron SMARC-sAL28 >> + >> + To compile this driver as a module, choose M here: the module will >> be >> + called sl28cpld. >> + >> endmenu >> endif >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 9367a92f795a..be59fb40aa28 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o >> obj-$(CONFIG_MFD_STMFX) += stmfx.o >> >> obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o >> + >> +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o >> diff --git a/drivers/mfd/sl28cpld.c b/drivers/mfd/sl28cpld.c >> new file mode 100644 >> index 000000000000..a23194bb6efa >> --- /dev/null >> +++ b/drivers/mfd/sl28cpld.c >> @@ -0,0 +1,79 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * MFD core for the sl28cpld. > > Ideally this would match the Kconfig subject line. ok > >> + * Copyright 2019 Kontron Europe GmbH > > This is out of date. ok >> + */ >> + >> +#include <linux/i2c.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/mfd/core.h> >> +#include <linux/module.h> >> +#include <linux/of_platform.h> >> +#include <linux/regmap.h> >> + >> +#define SL28CPLD_VERSION 0x03 >> +#define SL28CPLD_MIN_REQ_VERSION 14 >> + >> +struct sl28cpld { >> + struct device *dev; >> + struct regmap *regmap; >> +}; > > Why do you need this structure? see below >> +static const struct regmap_config sl28cpld_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + .reg_stride = 1, >> +}; >> + >> +static int sl28cpld_probe(struct i2c_client *i2c) >> +{ >> + struct sl28cpld *sl28cpld; >> + struct device *dev = &i2c->dev; >> + unsigned int cpld_version; >> + int ret; >> + >> + sl28cpld = devm_kzalloc(dev, sizeof(*sl28cpld), GFP_KERNEL); >> + if (!sl28cpld) >> + return -ENOMEM; >> + >> + sl28cpld->regmap = devm_regmap_init_i2c(i2c, >> &sl28cpld_regmap_config); >> + if (IS_ERR(sl28cpld->regmap)) >> + return PTR_ERR(sl28cpld->regmap); > > This is now a shared memory allocator and not an MFD at all. > > I'm clamping down on these type of drivers! > > Please find a better way to accomplish this. > > Potentially using "simple-mfd" and "simple-regmap". > > The former already exists and does what you want. The latter doesn't > yet exist, but could solve your and lots of other contributor's > issues. Mh, the former doesn't provide a regmap, correct? Most MMIO drivers won't need it, but this is an I2C device. So yes, I could come up with a "simple-regmap" proposal. I guess that should go into drivers/mfd/ because it is more than just adding one line, i.e. you would have to configure the regmap and its different interfaces. But how would you model that version check below for example? (Not that I'm very keen of it.) > > Heck maybe I'll implement it myself if this keeps happening. > >> + ret = regmap_read(sl28cpld->regmap, SL28CPLD_VERSION, >> &cpld_version); >> + if (ret) >> + return ret; >> + >> + if (cpld_version < SL28CPLD_MIN_REQ_VERSION) { >> + dev_err(dev, "unsupported CPLD version %d\n", cpld_version); >> + return -ENODEV; >> + } >> + >> + sl28cpld->dev = dev; >> + i2c_set_clientdata(i2c, sl28cpld); > > If the struct definition is in here, how do you use it elsewhere? I wanted to store the regmap somewhere, but because there are no users, I'll drop that. >> + dev_info(dev, "successfully probed. CPLD version %d\n", >> cpld_version); >> + >> + return devm_of_platform_populate(&i2c->dev); >> +} >> + >> +static const struct of_device_id sl28cpld_of_match[] = { >> + { .compatible = "kontron,sl28cpld-r1", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, sl28cpld_of_match); >> + >> +static struct i2c_driver sl28cpld_driver = { >> + .probe_new = sl28cpld_probe, >> + .driver = { >> + .name = "sl28cpld", >> + .of_match_table = of_match_ptr(sl28cpld_of_match), >> + }, >> +}; >> +module_i2c_driver(sl28cpld_driver); >> + >> +MODULE_DESCRIPTION("sl28cpld MFD Core Driver"); >> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); >> +MODULE_LICENSE("GPL");
Hi Andy, Am 2020-06-05 10:01, schrieb Andy Shevchenko: > On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote: >> >> Add the core support for the board management controller found on the >> SMARC-sAL28 board. It consists of the following functions: >> - watchdog >> - GPIO controller >> - PWM controller >> - fan sensor >> - interrupt controller >> >> At the moment, this controller is used on the Kontron SMARC-sAL28 >> board. >> >> Please note that the MFD driver is defined as bool in the Kconfig >> because the next patch will add interrupt support. > > ... > >> +config MFD_SL28CPLD >> + bool "Kontron sl28 core driver" >> + depends on I2C=y > > Why not module? There are users of the interupt lines provided by the interrupt controller. For example, the gpio-button driver. If this is compiled into the kernel (which it is by default in the arm64 defconfig), probing will fail because the interrupt is not found. Is there a better way for that? I guess the same is true for the GPIO driver. > >> + depends on OF > > I didn't find an evidence this is needed. see below. > > No Compile Test? ok >> + select REGMAP_I2C >> + select MFD_CORE > > ... > >> +#include <linux/of_platform.h> > > No evidence of user of this. > I think you meant mod_devicetable.h. devm_of_platform_populate(), so I need CONFIG_OF, too right? >> +static struct i2c_driver sl28cpld_driver = { >> + .probe_new = sl28cpld_probe, >> + .driver = { >> + .name = "sl28cpld", >> + .of_match_table = of_match_ptr(sl28cpld_of_match), > > Drop of_match_ptr(). It has a little sense in this context (depends > OF). > It will have a little sense even if you drop depends OF b/c you will > introduce a compiler warning. ok > >> + }, >> +};
On Fri, Jun 5, 2020 at 1:09 PM Michael Walle <michael@walle.cc> wrote: > Am 2020-06-05 10:01, schrieb Andy Shevchenko: > > On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote: ... > >> + bool "Kontron sl28 core driver" > >> + depends on I2C=y > > > > Why not module? > > There are users of the interupt lines provided by the interrupt > controller. > For example, the gpio-button driver. If this is compiled into the kernel > (which it is by default in the arm64 defconfig), probing will fail > because > the interrupt is not found. Is there a better way for that? I guess the > same > is true for the GPIO driver. And GPIO nicely handles this via deferred probe mechanism. Why it can't be used here? So, we really need to have a strong argument to limit module nowadays to be only builtin. ... > >> + depends on OF > > > > I didn't find an evidence this is needed. > >> +#include <linux/of_platform.h> > > > > No evidence of user of this. > > I think you meant mod_devicetable.h. > > devm_of_platform_populate(), so I need CONFIG_OF, too right? Ah, this explains header, thanks! But it doesn't explain depends OF. So, perhaps, depends OF || COMPILE_TEST will be more informative, i.e. tells "okay, this driver can be compiled w/o OF, but won't be functional".
On Fri, Jun 05, 2020 at 07:57:09AM +0100, Lee Jones wrote: > On Thu, 04 Jun 2020, Michael Walle wrote: > > + sl28cpld->regmap = devm_regmap_init_i2c(i2c, &sl28cpld_regmap_config); > > + if (IS_ERR(sl28cpld->regmap)) > > + return PTR_ERR(sl28cpld->regmap); > This is now a shared memory allocator and not an MFD at all. > I'm clamping down on these type of drivers! > Please find a better way to accomplish this. What is the concern with this? Looking at the patch I'm guessing the concern would be that the driver isn't instantiating any MFD children and instead requiring them to be put in the DT? > Potentially using "simple-mfd" and "simple-regmap". > The former already exists and does what you want. The latter doesn't > yet exist, but could solve your and lots of other contributor's > issues. I have no idea what you are thinking of when you say "simple-regmap" so it is difficult to comment.
Am 2020-06-05 12:48, schrieb Andy Shevchenko: > On Fri, Jun 5, 2020 at 1:09 PM Michael Walle <michael@walle.cc> wrote: >> Am 2020-06-05 10:01, schrieb Andy Shevchenko: >> > On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote: > > ... > >> >> + bool "Kontron sl28 core driver" >> >> + depends on I2C=y >> > >> > Why not module? >> >> There are users of the interupt lines provided by the interrupt >> controller. >> For example, the gpio-button driver. If this is compiled into the >> kernel >> (which it is by default in the arm64 defconfig), probing will fail >> because >> the interrupt is not found. Is there a better way for that? I guess >> the >> same >> is true for the GPIO driver. > > And GPIO nicely handles this via deferred probe mechanism. Why it > can't be used here? > So, we really need to have a strong argument to limit module nowadays > to be only builtin. Was that a question for me? TBH thats how other interrupt drivers doing it for now. And it would be the users who need to be fixed, right? Or even the platform code? Because it will complain with [ 2.962990] irq: no irq domain found for interrupt-controller@1c ! [ 2.975762] gpio-keys buttons0: Found button without gpio or irq [ 2.981872] gpio-keys: probe of buttons0 failed with error -22 >> >> + depends on OF >> > >> > I didn't find an evidence this is needed. > >> >> +#include <linux/of_platform.h> >> > >> > No evidence of user of this. >> > I think you meant mod_devicetable.h. >> >> devm_of_platform_populate(), so I need CONFIG_OF, too right? > > Ah, this explains header, thanks! > But it doesn't explain depends OF. > > So, perhaps, > > depends OF || COMPILE_TEST will be more informative, i.e. > tells "okay, this driver can be compiled w/o OF, but won't be > functional". ok
Hi, Am 2020-06-05 12:50, schrieb Mark Brown: > On Fri, Jun 05, 2020 at 07:57:09AM +0100, Lee Jones wrote: >> On Thu, 04 Jun 2020, Michael Walle wrote: > >> > + sl28cpld->regmap = devm_regmap_init_i2c(i2c, &sl28cpld_regmap_config); >> > + if (IS_ERR(sl28cpld->regmap)) >> > + return PTR_ERR(sl28cpld->regmap); > >> This is now a shared memory allocator and not an MFD at all. > >> I'm clamping down on these type of drivers! > >> Please find a better way to accomplish this. > > What is the concern with this? Looking at the patch I'm guessing the > concern would be that the driver isn't instantiating any MFD children > and instead requiring them to be put in the DT? > >> Potentially using "simple-mfd" and "simple-regmap". > >> The former already exists and does what you want. The latter doesn't >> yet exist, but could solve your and lots of other contributor's >> issues. > > I have no idea what you are thinking of when you say "simple-regmap" so > it is difficult to comment. I guess, Lee is suggesting to be able to create a regmap instance via device tree (and populate its child nodes?). Like compatible = "syscon", "simple-mfd"; but for any regmap, not just MMIO. But, there is more in my driver: (1) there is a version check (2) there is another function for which there is no suitable linux subsystem I'm aware of and thus which I'd like to us sysfs attributes for: This controller supports 16 non-volatile configuration bits. (this is still TBD) I don't see what is different between this driver and for example the gateworks-gsc.c. Just that mine doesn't use a global register set, but local offsets and a base for each component. From a hardware perspective its one device behind an I2C address providing different functions across multiple driver subsystems. Actually, I've tried to remove the devm_of_platform_populate() and instead added the "simple-mfd" to my mfd node: compatible = "kontron,sl28cpld-r1", "simple-mfd"; I guess that doesn't work because the device is below the i2c bus?
On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > Am 2020-06-05 12:50, schrieb Mark Brown: > > I have no idea what you are thinking of when you say "simple-regmap" so > > it is difficult to comment. > I guess, Lee is suggesting to be able to create a regmap instance via > device tree (and populate its child nodes?). Like > compatible = "syscon", "simple-mfd"; > but for any regmap, not just MMIO. I don't understand why this would be anything separate to simple-mfd. > But, there is more in my driver: > (1) there is a version check > (2) there is another function for which there is no suitable linux > subsystem I'm aware of and thus which I'd like to us sysfs > attributes for: This controller supports 16 non-volatile > configuration bits. (this is still TBD) TBH I'd also say that the enumeration of the subdevices for this device should be in the device rather than the DT, they don't seem to be things that exist outside of this one device.
Am 2020-06-06 13:46, schrieb Mark Brown: > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: >> Am 2020-06-05 12:50, schrieb Mark Brown: > >> > I have no idea what you are thinking of when you say "simple-regmap" so >> > it is difficult to comment. > >> I guess, Lee is suggesting to be able to create a regmap instance via >> device tree (and populate its child nodes?). Like >> compatible = "syscon", "simple-mfd"; >> but for any regmap, not just MMIO. > > I don't understand why this would be anything separate to > simple-mfd. Don't just simple-mfd tells the of core, to probe the children this node? Where does the regmap then come from? > >> But, there is more in my driver: >> (1) there is a version check >> (2) there is another function for which there is no suitable linux >> subsystem I'm aware of and thus which I'd like to us sysfs >> attributes for: This controller supports 16 non-volatile >> configuration bits. (this is still TBD) > > TBH I'd also say that the enumeration of the subdevices for this > device should be in the device rather than the DT, they don't > seem to be things that exist outside of this one device. We're going circles here, formerly they were enumerated in the MFD. Yes, they are devices which aren't likely be used outside a "sl28cpld", but there might there might be other versions of the sl28cpld with other components on different base addresses. I don't care if they are enumerated in DT or MFD, actually, I'd prefer the latter. _But_ I would like to have the device tree properties for its subdevices, e.g. the ones for the watchdog or whatever components there might be in the future. MFD core can match a device tree node today; but only one per unique compatible string. So what should I use to differentiate the different subdevices? Rob suggested the internal offset, which I did here. But then, there is less use in duplicating the offsets in the MFD just to have the MFD enumerate the subdevices and then match the device tree nodes against it. I can just use of_platform_populate() to enumerate the children and I won't have to duplicate the base addresses. So here we are, any ideas appreciated. -michael
Rob, something for you below. On Sat, 06 Jun 2020, Michael Walle wrote: > Am 2020-06-06 13:46, schrieb Mark Brown: > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > > > > > I have no idea what you are thinking of when you say "simple-regmap" so > > > > it is difficult to comment. > > > > > I guess, Lee is suggesting to be able to create a regmap instance via > > > device tree (and populate its child nodes?). Like > > > compatible = "syscon", "simple-mfd"; > > > but for any regmap, not just MMIO. Bingo! > > I don't understand why this would be anything separate to > > simple-mfd. > > Don't just simple-mfd tells the of core, to probe the children this > node? Where does the regmap then come from? Right. I'm suggesting a means to extrapolate complex shared and sometimes intertwined batches of register sets to be consumed by multiple (sub-)devices spanning different subsystems. Actually scrap that. The most common case I see is a single Regmap covering all child-devices. It would be great if there was a way in which we could make an assumption that the entire register address space for a 'tagged' (MFD) device is to be shared (via Regmap) between each of the devices described by its child-nodes. Probably by picking up on the 'simple-mfd' compatible string in the first instance. Rob, is the above something you would contemplate? Michael, do your register addresses overlap i.e. are they intermingled with one another? Do multiple child devices need access to the same registers i.e. are they shared? > > > But, there is more in my driver: > > > (1) there is a version check If we can rid the Regmap dependency, then creating an entire driver to conduct a version check is unjustifiable. This could become an inline function which is called by each of the sub-devices instead, for example. > > > (2) there is another function for which there is no suitable linux > > > subsystem I'm aware of and thus which I'd like to us sysfs > > > attributes for: This controller supports 16 non-volatile > > > configuration bits. (this is still TBD) There is a place for everything in Linux. What do these bits configure? > > TBH I'd also say that the enumeration of the subdevices for this > > device should be in the device rather than the DT, they don't > > seem to be things that exist outside of this one device. > > We're going circles here, formerly they were enumerated in the MFD. > Yes, they are devices which aren't likely be used outside a > "sl28cpld", but there might there might be other versions of the > sl28cpld with other components on different base addresses. I > don't care if they are enumerated in DT or MFD, actually, I'd > prefer the latter. _But_ I would like to have the device tree > properties for its subdevices, e.g. the ones for the watchdog or > whatever components there might be in the future. [...] > MFD core can > match a device tree node today; but only one per unique compatible > string. So what should I use to differentiate the different > subdevices? Right. I have been aware of this issue. The only suitable solution to this would be to match on 'reg'. FYI: I plan to fix this. If your register map needs to change, then I suggest that this is either a new device or at least a different version of the device and would also have to be represented as different (sub-)mfd_cell. > Rob suggested the internal offset, which I did here. FWIW, I don't like this idea. DTs should not have to be modified (either in the first instance or subsequently) or specifically designed to patch inadequacies in any given OS. > But then, there is less use in duplicating the offsets in the MFD > just to have the MFD enumerate the subdevices and then match > the device tree nodes against it. I can just use > of_platform_populate() to enumerate the children and I won't > have to duplicate the base addresses. Which is fine. However this causes a different issue for you. By stripping out the MFD code you render the MFD portion seemingly superfluous. Another issue driver authors commonly contend with. > So here we are, any ideas appreciated. Working on it!
+Cc: some Intel people WRT our internal discussion about similar problem and solutions. On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: > On Sat, 06 Jun 2020, Michael Walle wrote: > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > Am 2020-06-05 12:50, schrieb Mark Brown: ... > Right. I'm suggesting a means to extrapolate complex shared and > sometimes intertwined batches of register sets to be consumed by > multiple (sub-)devices spanning different subsystems. > > Actually scrap that. The most common case I see is a single Regmap > covering all child-devices. Yes, because often we need a synchronization across the entire address space of the (parent) device in question. > It would be great if there was a way in > which we could make an assumption that the entire register address > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > each of the devices described by its child-nodes. Probably by picking > up on the 'simple-mfd' compatible string in the first instance. > > Rob, is the above something you would contemplate? > > Michael, do your register addresses overlap i.e. are they intermingled > with one another? Do multiple child devices need access to the same > registers i.e. are they shared? > > > > > But, there is more in my driver: > > > > (1) there is a version check > > If we can rid the Regmap dependency, then creating an entire driver to > conduct a version check is unjustifiable. This could become an inline > function which is called by each of the sub-devices instead, for > example. > > > > > (2) there is another function for which there is no suitable linux > > > > subsystem I'm aware of and thus which I'd like to us sysfs > > > > attributes for: This controller supports 16 non-volatile > > > > configuration bits. (this is still TBD) > > There is a place for everything in Linux. > > What do these bits configure? > > > > TBH I'd also say that the enumeration of the subdevices for this > > > device should be in the device rather than the DT, they don't > > > seem to be things that exist outside of this one device. > > > > We're going circles here, formerly they were enumerated in the MFD. > > Yes, they are devices which aren't likely be used outside a > > "sl28cpld", but there might there might be other versions of the > > sl28cpld with other components on different base addresses. I > > don't care if they are enumerated in DT or MFD, actually, I'd > > prefer the latter. _But_ I would like to have the device tree > > properties for its subdevices, e.g. the ones for the watchdog or > > whatever components there might be in the future. > > [...] > > > MFD core can > > match a device tree node today; but only one per unique compatible > > string. So what should I use to differentiate the different > > subdevices? > > Right. I have been aware of this issue. The only suitable solution > to this would be to match on 'reg'. > > FYI: I plan to fix this. > > If your register map needs to change, then I suggest that this is > either a new device or at least a different version of the device and > would also have to be represented as different (sub-)mfd_cell. > > > Rob suggested the internal offset, which I did here. > > FWIW, I don't like this idea. DTs should not have to be modified > (either in the first instance or subsequently) or specifically > designed to patch inadequacies in any given OS. > > > But then, there is less use in duplicating the offsets in the MFD > > just to have the MFD enumerate the subdevices and then match > > the device tree nodes against it. I can just use > > of_platform_populate() to enumerate the children and I won't > > have to duplicate the base addresses. > > Which is fine. However this causes a different issue for you. By > stripping out the MFD code you render the MFD portion seemingly > superfluous. Another issue driver authors commonly contend with.
Am 2020-06-08 12:02, schrieb Andy Shevchenko: > +Cc: some Intel people WRT our internal discussion about similar > problem and solutions. > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: >> On Sat, 06 Jun 2020, Michael Walle wrote: >> > Am 2020-06-06 13:46, schrieb Mark Brown: >> > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: >> > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > ... > >> Right. I'm suggesting a means to extrapolate complex shared and >> sometimes intertwined batches of register sets to be consumed by >> multiple (sub-)devices spanning different subsystems. >> >> Actually scrap that. The most common case I see is a single Regmap >> covering all child-devices. > > Yes, because often we need a synchronization across the entire address > space of the (parent) device in question. > >> It would be great if there was a way in >> which we could make an assumption that the entire register address >> space for a 'tagged' (MFD) device is to be shared (via Regmap) between >> each of the devices described by its child-nodes. Probably by picking >> up on the 'simple-mfd' compatible string in the first instance. >> >> Rob, is the above something you would contemplate? >> >> Michael, do your register addresses overlap i.e. are they intermingled >> with one another? Do multiple child devices need access to the same >> registers i.e. are they shared? No they don't overlap, expect for maybe the version register, which is just there once and not per function block. >> >> > > > But, there is more in my driver: >> > > > (1) there is a version check >> >> If we can rid the Regmap dependency, then creating an entire driver to >> conduct a version check is unjustifiable. This could become an inline >> function which is called by each of the sub-devices instead, for >> example. sounds good to me. (although there would then be a probe fail per sub-device if the version is not supported) >> > > > (2) there is another function for which there is no suitable linux >> > > > subsystem I'm aware of and thus which I'd like to us sysfs >> > > > attributes for: This controller supports 16 non-volatile >> > > > configuration bits. (this is still TBD) >> >> There is a place for everything in Linux. >> >> What do these bits configure? - hardware strappings which have to be there before the board powers up, like clocking mode for different SerDes settings - "keep-in-reset" bits for onboard peripherals if you want to save power - disable watchdog bits (there is a watchdog which is active right from the start and supervises the bootloader start and switches to failsafe mode if it wasn't successfully started) - special boot modes, like eMMC, etc. Think of it as a 16bit configuration word. >> > > TBH I'd also say that the enumeration of the subdevices for this >> > > device should be in the device rather than the DT, they don't >> > > seem to be things that exist outside of this one device. >> > >> > We're going circles here, formerly they were enumerated in the MFD. >> > Yes, they are devices which aren't likely be used outside a >> > "sl28cpld", but there might there might be other versions of the >> > sl28cpld with other components on different base addresses. I >> > don't care if they are enumerated in DT or MFD, actually, I'd >> > prefer the latter. _But_ I would like to have the device tree >> > properties for its subdevices, e.g. the ones for the watchdog or >> > whatever components there might be in the future. >> >> [...] >> >> > MFD core can >> > match a device tree node today; but only one per unique compatible >> > string. So what should I use to differentiate the different >> > subdevices? >> >> Right. I have been aware of this issue. The only suitable solution >> to this would be to match on 'reg'. see below (1) >> >> FYI: I plan to fix this. >> >> If your register map needs to change, then I suggest that this is >> either a new device or at least a different version of the device and >> would also have to be represented as different (sub-)mfd_cell. >> >> > Rob suggested the internal offset, which I did here. >> >> FWIW, I don't like this idea. DTs should not have to be modified >> (either in the first instance or subsequently) or specifically >> designed to patch inadequacies in any given OS. How does (1) play together with this? What do you propose the "reg" property should contain? >> >> > But then, there is less use in duplicating the offsets in the MFD >> > just to have the MFD enumerate the subdevices and then match >> > the device tree nodes against it. I can just use >> > of_platform_populate() to enumerate the children and I won't >> > have to duplicate the base addresses. >> >> Which is fine. However this causes a different issue for you. By >> stripping out the MFD code you render the MFD portion seemingly >> superfluous. Another issue driver authors commonly contend with. Yeah, I agree. -michael
On Mon, 08 Jun 2020, Andy Shevchenko wrote: > +Cc: some Intel people WRT our internal discussion about similar > problem and solutions. > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Sat, 06 Jun 2020, Michael Walle wrote: > > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > ... > > > Right. I'm suggesting a means to extrapolate complex shared and > > sometimes intertwined batches of register sets to be consumed by > > multiple (sub-)devices spanning different subsystems. > > > > Actually scrap that. The most common case I see is a single Regmap > > covering all child-devices. > > Yes, because often we need a synchronization across the entire address > space of the (parent) device in question. Exactly. Because of the reasons in the paragraph above: "complex shared and sometimes intertwined batches of register sets to be consumed by multiple (sub-)devices spanning different subsystems" > > It would be great if there was a way in > > which we could make an assumption that the entire register address > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > > each of the devices described by its child-nodes. Probably by picking > > up on the 'simple-mfd' compatible string in the first instance. > > > > Rob, is the above something you would contemplate? > > > > Michael, do your register addresses overlap i.e. are they intermingled > > with one another? Do multiple child devices need access to the same > > registers i.e. are they shared? > > > > > > > But, there is more in my driver: > > > > > (1) there is a version check > > > > If we can rid the Regmap dependency, then creating an entire driver to > > conduct a version check is unjustifiable. This could become an inline > > function which is called by each of the sub-devices instead, for > > example. > > > > > > > (2) there is another function for which there is no suitable linux > > > > > subsystem I'm aware of and thus which I'd like to us sysfs > > > > > attributes for: This controller supports 16 non-volatile > > > > > configuration bits. (this is still TBD) > > > > There is a place for everything in Linux. > > > > What do these bits configure? > > > > > > TBH I'd also say that the enumeration of the subdevices for this > > > > device should be in the device rather than the DT, they don't > > > > seem to be things that exist outside of this one device. > > > > > > We're going circles here, formerly they were enumerated in the MFD. > > > Yes, they are devices which aren't likely be used outside a > > > "sl28cpld", but there might there might be other versions of the > > > sl28cpld with other components on different base addresses. I > > > don't care if they are enumerated in DT or MFD, actually, I'd > > > prefer the latter. _But_ I would like to have the device tree > > > properties for its subdevices, e.g. the ones for the watchdog or > > > whatever components there might be in the future. > > > > [...] > > > > > MFD core can > > > match a device tree node today; but only one per unique compatible > > > string. So what should I use to differentiate the different > > > subdevices? > > > > Right. I have been aware of this issue. The only suitable solution > > to this would be to match on 'reg'. > > > > FYI: I plan to fix this. > > > > If your register map needs to change, then I suggest that this is > > either a new device or at least a different version of the device and > > would also have to be represented as different (sub-)mfd_cell. > > > > > Rob suggested the internal offset, which I did here. > > > > FWIW, I don't like this idea. DTs should not have to be modified > > (either in the first instance or subsequently) or specifically > > designed to patch inadequacies in any given OS. > > > > > But then, there is less use in duplicating the offsets in the MFD > > > just to have the MFD enumerate the subdevices and then match > > > the device tree nodes against it. I can just use > > > of_platform_populate() to enumerate the children and I won't > > > have to duplicate the base addresses. > > > > Which is fine. However this causes a different issue for you. By > > stripping out the MFD code you render the MFD portion seemingly > > superfluous. Another issue driver authors commonly contend with. >
On Mon, 08 Jun 2020, Michael Walle wrote: > Am 2020-06-08 12:02, schrieb Andy Shevchenko: > > +Cc: some Intel people WRT our internal discussion about similar > > problem and solutions. > > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: > > > On Sat, 06 Jun 2020, Michael Walle wrote: > > > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > > > ... > > > > > Right. I'm suggesting a means to extrapolate complex shared and > > > sometimes intertwined batches of register sets to be consumed by > > > multiple (sub-)devices spanning different subsystems. > > > > > > Actually scrap that. The most common case I see is a single Regmap > > > covering all child-devices. > > > > Yes, because often we need a synchronization across the entire address > > space of the (parent) device in question. > > > > > It would be great if there was a way in > > > which we could make an assumption that the entire register address > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > > > each of the devices described by its child-nodes. Probably by picking > > > up on the 'simple-mfd' compatible string in the first instance. > > > > > > Rob, is the above something you would contemplate? > > > > > > Michael, do your register addresses overlap i.e. are they intermingled > > > with one another? Do multiple child devices need access to the same > > > registers i.e. are they shared? > > No they don't overlap, expect for maybe the version register, which is > just there once and not per function block. Then what's stopping you having each device Regmap their own space? The issues I wish to resolve using 'simple-mfd' are when sub-devices register maps overlap and intertwine. > > > > > > But, there is more in my driver: > > > > > > (1) there is a version check > > > > > > If we can rid the Regmap dependency, then creating an entire driver to > > > conduct a version check is unjustifiable. This could become an inline > > > function which is called by each of the sub-devices instead, for > > > example. > > sounds good to me. (although there would then be a probe fail per sub-device > if the version is not supported) I don't see an issue with that. I would put that check inside a shared call though, complete with support for locking. > > > > > > (2) there is another function for which there is no suitable linux > > > > > > subsystem I'm aware of and thus which I'd like to us sysfs > > > > > > attributes for: This controller supports 16 non-volatile > > > > > > configuration bits. (this is still TBD) > > > > > > There is a place for everything in Linux. > > > > > > What do these bits configure? > > - hardware strappings which have to be there before the board powers up, > like clocking mode for different SerDes settings > - "keep-in-reset" bits for onboard peripherals if you want to save power > - disable watchdog bits (there is a watchdog which is active right from > the start and supervises the bootloader start and switches to failsafe > mode if it wasn't successfully started) > - special boot modes, like eMMC, etc. > > Think of it as a 16bit configuration word. And you wish for users to be able to view these at run-time? Can they adapt any of them on-the-fly or will the be RO? > > > > > TBH I'd also say that the enumeration of the subdevices for this > > > > > device should be in the device rather than the DT, they don't > > > > > seem to be things that exist outside of this one device. > > > > > > > > We're going circles here, formerly they were enumerated in the MFD. > > > > Yes, they are devices which aren't likely be used outside a > > > > "sl28cpld", but there might there might be other versions of the > > > > sl28cpld with other components on different base addresses. I > > > > don't care if they are enumerated in DT or MFD, actually, I'd > > > > prefer the latter. _But_ I would like to have the device tree > > > > properties for its subdevices, e.g. the ones for the watchdog or > > > > whatever components there might be in the future. > > > > > > [...] > > > > > > > MFD core can > > > > match a device tree node today; but only one per unique compatible > > > > string. So what should I use to differentiate the different > > > > subdevices? > > > > > > Right. I have been aware of this issue. The only suitable solution > > > to this would be to match on 'reg'. > > see below (1) > > > > > > > FYI: I plan to fix this. > > > > > > If your register map needs to change, then I suggest that this is > > > either a new device or at least a different version of the device and > > > would also have to be represented as different (sub-)mfd_cell. > > > > > > > Rob suggested the internal offset, which I did here. > > > > > > FWIW, I don't like this idea. DTs should not have to be modified > > > (either in the first instance or subsequently) or specifically > > > designed to patch inadequacies in any given OS. > > How does (1) play together with this? What do you propose the "reg" > property should contain? Whatever is in the 'reg' property contained in the Device Tree node. Either the full address or an offset would be suitable. Caveat: All this thinking has been done on-the-fly. I would need to look at some examples of existing devices and start coding before I could really think the solution through. Happy to discuss and/or take recommendations though.
Am 2020-06-08 20:56, schrieb Lee Jones: > On Mon, 08 Jun 2020, Michael Walle wrote: > >> Am 2020-06-08 12:02, schrieb Andy Shevchenko: >> > +Cc: some Intel people WRT our internal discussion about similar >> > problem and solutions. >> > >> > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: >> > > On Sat, 06 Jun 2020, Michael Walle wrote: >> > > > Am 2020-06-06 13:46, schrieb Mark Brown: >> > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: >> > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: >> > >> > ... >> > >> > > Right. I'm suggesting a means to extrapolate complex shared and >> > > sometimes intertwined batches of register sets to be consumed by >> > > multiple (sub-)devices spanning different subsystems. >> > > >> > > Actually scrap that. The most common case I see is a single Regmap >> > > covering all child-devices. >> > >> > Yes, because often we need a synchronization across the entire address >> > space of the (parent) device in question. >> > >> > > It would be great if there was a way in >> > > which we could make an assumption that the entire register address >> > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between >> > > each of the devices described by its child-nodes. Probably by picking >> > > up on the 'simple-mfd' compatible string in the first instance. >> > > >> > > Rob, is the above something you would contemplate? >> > > >> > > Michael, do your register addresses overlap i.e. are they intermingled >> > > with one another? Do multiple child devices need access to the same >> > > registers i.e. are they shared? >> >> No they don't overlap, expect for maybe the version register, which is >> just there once and not per function block. > > Then what's stopping you having each device Regmap their own space? Because its just one I2C device, AFAIK thats not possible, right? > The issues I wish to resolve using 'simple-mfd' are when sub-devices > register maps overlap and intertwine. > >> > > > > > But, there is more in my driver: >> > > > > > (1) there is a version check >> > > >> > > If we can rid the Regmap dependency, then creating an entire driver to >> > > conduct a version check is unjustifiable. This could become an inline >> > > function which is called by each of the sub-devices instead, for >> > > example. >> >> sounds good to me. (although there would then be a probe fail per >> sub-device >> if the version is not supported) > > I don't see an issue with that. I would put that check inside a > shared call though, complete with support for locking. > >> > > > > > (2) there is another function for which there is no suitable linux >> > > > > > subsystem I'm aware of and thus which I'd like to us sysfs >> > > > > > attributes for: This controller supports 16 non-volatile >> > > > > > configuration bits. (this is still TBD) >> > > >> > > There is a place for everything in Linux. >> > > >> > > What do these bits configure? >> >> - hardware strappings which have to be there before the board powers >> up, >> like clocking mode for different SerDes settings >> - "keep-in-reset" bits for onboard peripherals if you want to save >> power >> - disable watchdog bits (there is a watchdog which is active right >> from >> the start and supervises the bootloader start and switches to >> failsafe >> mode if it wasn't successfully started) >> - special boot modes, like eMMC, etc. >> >> Think of it as a 16bit configuration word. > > And you wish for users to be able to view these at run-time? And esp. change them. > Can they adapt any of them on-the-fly or will the be RO? They are R/W but only will only affect the board behavior after a reset. -michael > >> > > > > TBH I'd also say that the enumeration of the subdevices for this >> > > > > device should be in the device rather than the DT, they don't >> > > > > seem to be things that exist outside of this one device. >> > > > >> > > > We're going circles here, formerly they were enumerated in the MFD. >> > > > Yes, they are devices which aren't likely be used outside a >> > > > "sl28cpld", but there might there might be other versions of the >> > > > sl28cpld with other components on different base addresses. I >> > > > don't care if they are enumerated in DT or MFD, actually, I'd >> > > > prefer the latter. _But_ I would like to have the device tree >> > > > properties for its subdevices, e.g. the ones for the watchdog or >> > > > whatever components there might be in the future. >> > > >> > > [...] >> > > >> > > > MFD core can >> > > > match a device tree node today; but only one per unique compatible >> > > > string. So what should I use to differentiate the different >> > > > subdevices? >> > > >> > > Right. I have been aware of this issue. The only suitable solution >> > > to this would be to match on 'reg'. >> >> see below (1) >> >> > > >> > > FYI: I plan to fix this. >> > > >> > > If your register map needs to change, then I suggest that this is >> > > either a new device or at least a different version of the device and >> > > would also have to be represented as different (sub-)mfd_cell. >> > > >> > > > Rob suggested the internal offset, which I did here. >> > > >> > > FWIW, I don't like this idea. DTs should not have to be modified >> > > (either in the first instance or subsequently) or specifically >> > > designed to patch inadequacies in any given OS. >> >> How does (1) play together with this? What do you propose the "reg" >> property should contain? > > Whatever is in the 'reg' property contained in the Device Tree node. > Either the full address or an offset would be suitable. > > Caveat: All this thinking has been done on-the-fly. I would need to > look at some examples of existing devices and start coding before I > could really think the solution through. > > Happy to discuss and/or take recommendations though.
On Mon, 08 Jun 2020, Michael Walle wrote: > Am 2020-06-08 20:56, schrieb Lee Jones: > > On Mon, 08 Jun 2020, Michael Walle wrote: > > > > > Am 2020-06-08 12:02, schrieb Andy Shevchenko: > > > > +Cc: some Intel people WRT our internal discussion about similar > > > > problem and solutions. > > > > > > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > > On Sat, 06 Jun 2020, Michael Walle wrote: > > > > > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > > > > > > > ... > > > > > > > > > Right. I'm suggesting a means to extrapolate complex shared and > > > > > sometimes intertwined batches of register sets to be consumed by > > > > > multiple (sub-)devices spanning different subsystems. > > > > > > > > > > Actually scrap that. The most common case I see is a single Regmap > > > > > covering all child-devices. > > > > > > > > Yes, because often we need a synchronization across the entire address > > > > space of the (parent) device in question. > > > > > > > > > It would be great if there was a way in > > > > > which we could make an assumption that the entire register address > > > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > > > > > each of the devices described by its child-nodes. Probably by picking > > > > > up on the 'simple-mfd' compatible string in the first instance. > > > > > > > > > > Rob, is the above something you would contemplate? > > > > > > > > > > Michael, do your register addresses overlap i.e. are they intermingled > > > > > with one another? Do multiple child devices need access to the same > > > > > registers i.e. are they shared? > > > > > > No they don't overlap, expect for maybe the version register, which is > > > just there once and not per function block. > > > > Then what's stopping you having each device Regmap their own space? > > Because its just one I2C device, AFAIK thats not possible, right? Not sure what (if any) the restrictions are. I can't think of any reasons why not, off the top of my head. Does Regmap only deal with shared accesses from multiple devices accessing a single register map, or can it also handle multiple devices communicating over a single I2C channel? One for Mark perhaps. > > The issues I wish to resolve using 'simple-mfd' are when sub-devices > > register maps overlap and intertwine. [...] > > > > > What do these bits configure? > > > > > > - hardware strappings which have to be there before the board powers > > > up, > > > like clocking mode for different SerDes settings > > > - "keep-in-reset" bits for onboard peripherals if you want to save > > > power > > > - disable watchdog bits (there is a watchdog which is active right > > > from > > > the start and supervises the bootloader start and switches to > > > failsafe > > > mode if it wasn't successfully started) > > > - special boot modes, like eMMC, etc. > > > > > > Think of it as a 16bit configuration word. > > > > And you wish for users to be able to view these at run-time? > > And esp. change them. > > > Can they adapt any of them on-the-fly or will the be RO? > > They are R/W but only will only affect the board behavior after a reset. I see. Makes sense. This is board controller territory. Perhaps suitable for inclusion into drivers/soc or drivers/platform.
Am 2020-06-09 08:47, schrieb Lee Jones: > On Mon, 08 Jun 2020, Michael Walle wrote: > >> Am 2020-06-08 20:56, schrieb Lee Jones: >> > On Mon, 08 Jun 2020, Michael Walle wrote: >> > >> > > Am 2020-06-08 12:02, schrieb Andy Shevchenko: >> > > > +Cc: some Intel people WRT our internal discussion about similar >> > > > problem and solutions. >> > > > >> > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: >> > > > > On Sat, 06 Jun 2020, Michael Walle wrote: >> > > > > > Am 2020-06-06 13:46, schrieb Mark Brown: >> > > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: >> > > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: >> > > > >> > > > ... >> > > > >> > > > > Right. I'm suggesting a means to extrapolate complex shared and >> > > > > sometimes intertwined batches of register sets to be consumed by >> > > > > multiple (sub-)devices spanning different subsystems. >> > > > > >> > > > > Actually scrap that. The most common case I see is a single Regmap >> > > > > covering all child-devices. >> > > > >> > > > Yes, because often we need a synchronization across the entire address >> > > > space of the (parent) device in question. >> > > > >> > > > > It would be great if there was a way in >> > > > > which we could make an assumption that the entire register address >> > > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between >> > > > > each of the devices described by its child-nodes. Probably by picking >> > > > > up on the 'simple-mfd' compatible string in the first instance. >> > > > > >> > > > > Rob, is the above something you would contemplate? >> > > > > >> > > > > Michael, do your register addresses overlap i.e. are they intermingled >> > > > > with one another? Do multiple child devices need access to the same >> > > > > registers i.e. are they shared? >> > > >> > > No they don't overlap, expect for maybe the version register, which is >> > > just there once and not per function block. >> > >> > Then what's stopping you having each device Regmap their own space? >> >> Because its just one I2C device, AFAIK thats not possible, right? > > Not sure what (if any) the restrictions are. You can only have one device per I2C address. Therefore, I need one device which is enumerated by the I2C bus, which then enumerates its sub-devices. I thought this was one of the use cases for MFD. (Regardless of how a sub-device access its registers). So even in the "simple-regmap" case this would need to be an i2c device. E.g. &i2cbus { mfd-device@10 { compatible = "simple-regmap", "simple-mfd"; reg = <10>; regmap,reg-bits = <8>; regmap,val-bits = <8>; sub-device@0 { compatible = "vendor,sub-device0"; reg = <0>; }; ... }; Or if you just want the regmap: &soc { regmap: regmap@fff0000 { compatible = "simple-regmap"; reg = <0xfff0000>; regmap,reg-bits = <16>; regmap,val-bits = <32>; }; enet-which-needs-syscon-too@1000000 { vendor,ctrl-regmap = <®map>; }; }; Similar to the current syscon (which is MMIO only..). -michael > > I can't think of any reasons why not, off the top of my head. > > Does Regmap only deal with shared accesses from multiple devices > accessing a single register map, or can it also handle multiple > devices communicating over a single I2C channel? > > One for Mark perhaps. > >> > The issues I wish to resolve using 'simple-mfd' are when sub-devices >> > register maps overlap and intertwine. > > [...] > >> > > > > What do these bits configure? >> > > >> > > - hardware strappings which have to be there before the board powers >> > > up, >> > > like clocking mode for different SerDes settings >> > > - "keep-in-reset" bits for onboard peripherals if you want to save >> > > power >> > > - disable watchdog bits (there is a watchdog which is active right >> > > from >> > > the start and supervises the bootloader start and switches to >> > > failsafe >> > > mode if it wasn't successfully started) >> > > - special boot modes, like eMMC, etc. >> > > >> > > Think of it as a 16bit configuration word. >> > >> > And you wish for users to be able to view these at run-time? >> >> And esp. change them. >> >> > Can they adapt any of them on-the-fly or will the be RO? >> >> They are R/W but only will only affect the board behavior after a >> reset. > > I see. Makes sense. This is board controller territory. Perhaps > suitable for inclusion into drivers/soc or drivers/platform.
On Tue, Jun 09, 2020 at 04:38:31PM +0200, Michael Walle wrote: > mfd-device@10 { > compatible = "simple-regmap", "simple-mfd"; > reg = <10>; > regmap,reg-bits = <8>; > regmap,val-bits = <8>; > sub-device@0 { > compatible = "vendor,sub-device0"; > reg = <0>; > }; A DT binding like this is not a good idea, encoding the details of the register map into the DT binding makes it an ABI which is begging for trouble. I'd also suggest that any device using a generic driver like this should have a specific compatible string for the device so we can go back and add quirks later if we need them. > ... > }; > > Or if you just want the regmap: > > &soc { > regmap: regmap@fff0000 { > compatible = "simple-regmap"; > reg = <0xfff0000>; > regmap,reg-bits = <16>; > regmap,val-bits = <32>; > }; > > enet-which-needs-syscon-too@1000000 { > vendor,ctrl-regmap = <®map>; > }; > }; > > Similar to the current syscon (which is MMIO only..). > > -michael > > > > > I can't think of any reasons why not, off the top of my head. > > > > Does Regmap only deal with shared accesses from multiple devices > > accessing a single register map, or can it also handle multiple > > devices communicating over a single I2C channel? > > > > One for Mark perhaps. > > > > > > The issues I wish to resolve using 'simple-mfd' are when sub-devices > > > > register maps overlap and intertwine. > > > > [...] > > > > > > > > > What do these bits configure? > > > > > > > > > > - hardware strappings which have to be there before the board powers > > > > > up, > > > > > like clocking mode for different SerDes settings > > > > > - "keep-in-reset" bits for onboard peripherals if you want to save > > > > > power > > > > > - disable watchdog bits (there is a watchdog which is active right > > > > > from > > > > > the start and supervises the bootloader start and switches to > > > > > failsafe > > > > > mode if it wasn't successfully started) > > > > > - special boot modes, like eMMC, etc. > > > > > > > > > > Think of it as a 16bit configuration word. > > > > > > > > And you wish for users to be able to view these at run-time? > > > > > > And esp. change them. > > > > > > > Can they adapt any of them on-the-fly or will the be RO? > > > > > > They are R/W but only will only affect the board behavior after a > > > reset. > > > > I see. Makes sense. This is board controller territory. Perhaps > > suitable for inclusion into drivers/soc or drivers/platform.
Am 2020-06-09 16:42, schrieb Mark Brown: > On Tue, Jun 09, 2020 at 04:38:31PM +0200, Michael Walle wrote: > >> mfd-device@10 { >> compatible = "simple-regmap", "simple-mfd"; >> reg = <10>; >> regmap,reg-bits = <8>; >> regmap,val-bits = <8>; >> sub-device@0 { >> compatible = "vendor,sub-device0"; >> reg = <0>; >> }; > > A DT binding like this is not a good idea, encoding the details of the > register map into the DT binding makes it an ABI which is begging for > trouble. I'd also suggest that any device using a generic driver like > this should have a specific compatible string for the device so we can > go back and add quirks later if we need them. Like in the spidev case, yes. But OTOH if I _just_ encode the parameters for the regmap a MFD, Lee don't agree because its just a shim. So either way I seem to be stuck here. Where should I put the code to create an i2c driver, init a regmap and populate its childen? -michael > >> ... >> }; >> >> Or if you just want the regmap: >> >> &soc { >> regmap: regmap@fff0000 { >> compatible = "simple-regmap"; >> reg = <0xfff0000>; >> regmap,reg-bits = <16>; >> regmap,val-bits = <32>; >> }; >> >> enet-which-needs-syscon-too@1000000 { >> vendor,ctrl-regmap = <®map>; >> }; >> }; >> >> Similar to the current syscon (which is MMIO only..). >> >> -michael >> >> > >> > I can't think of any reasons why not, off the top of my head. >> > >> > Does Regmap only deal with shared accesses from multiple devices >> > accessing a single register map, or can it also handle multiple >> > devices communicating over a single I2C channel? >> > >> > One for Mark perhaps. >> > >> > > > The issues I wish to resolve using 'simple-mfd' are when sub-devices >> > > > register maps overlap and intertwine. >> > >> > [...] >> > >> > > > > > > What do these bits configure? >> > > > > >> > > > > - hardware strappings which have to be there before the board powers >> > > > > up, >> > > > > like clocking mode for different SerDes settings >> > > > > - "keep-in-reset" bits for onboard peripherals if you want to save >> > > > > power >> > > > > - disable watchdog bits (there is a watchdog which is active right >> > > > > from >> > > > > the start and supervises the bootloader start and switches to >> > > > > failsafe >> > > > > mode if it wasn't successfully started) >> > > > > - special boot modes, like eMMC, etc. >> > > > > >> > > > > Think of it as a 16bit configuration word. >> > > > >> > > > And you wish for users to be able to view these at run-time? >> > > >> > > And esp. change them. >> > > >> > > > Can they adapt any of them on-the-fly or will the be RO? >> > > >> > > They are R/W but only will only affect the board behavior after a >> > > reset. >> > >> > I see. Makes sense. This is board controller territory. Perhaps >> > suitable for inclusion into drivers/soc or drivers/platform.
On Tue, 09 Jun 2020, Michael Walle wrote: > Am 2020-06-09 08:47, schrieb Lee Jones: > > On Mon, 08 Jun 2020, Michael Walle wrote: > > > > > Am 2020-06-08 20:56, schrieb Lee Jones: > > > > On Mon, 08 Jun 2020, Michael Walle wrote: > > > > > > > > > Am 2020-06-08 12:02, schrieb Andy Shevchenko: > > > > > > +Cc: some Intel people WRT our internal discussion about similar > > > > > > problem and solutions. > > > > > > > > > > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > On Sat, 06 Jun 2020, Michael Walle wrote: > > > > > > > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > > > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > > > > > > > > > > > ... > > > > > > > > > > > > > Right. I'm suggesting a means to extrapolate complex shared and > > > > > > > sometimes intertwined batches of register sets to be consumed by > > > > > > > multiple (sub-)devices spanning different subsystems. > > > > > > > > > > > > > > Actually scrap that. The most common case I see is a single Regmap > > > > > > > covering all child-devices. > > > > > > > > > > > > Yes, because often we need a synchronization across the entire address > > > > > > space of the (parent) device in question. > > > > > > > > > > > > > It would be great if there was a way in > > > > > > > which we could make an assumption that the entire register address > > > > > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > > > > > > > each of the devices described by its child-nodes. Probably by picking > > > > > > > up on the 'simple-mfd' compatible string in the first instance. > > > > > > > > > > > > > > Rob, is the above something you would contemplate? > > > > > > > > > > > > > > Michael, do your register addresses overlap i.e. are they intermingled > > > > > > > with one another? Do multiple child devices need access to the same > > > > > > > registers i.e. are they shared? > > > > > > > > > > No they don't overlap, expect for maybe the version register, which is > > > > > just there once and not per function block. > > > > > > > > Then what's stopping you having each device Regmap their own space? > > > > > > Because its just one I2C device, AFAIK thats not possible, right? > > > > Not sure what (if any) the restrictions are. > > You can only have one device per I2C address. Therefore, I need one device > which is enumerated by the I2C bus, which then enumerates its sub-devices. > I thought this was one of the use cases for MFD. (Regardless of how a > sub-device access its registers). So even in the "simple-regmap" case this > would need to be an i2c device. > > E.g. > > &i2cbus { > mfd-device@10 { > compatible = "simple-regmap", "simple-mfd"; > reg = <10>; > regmap,reg-bits = <8>; > regmap,val-bits = <8>; > sub-device@0 { > compatible = "vendor,sub-device0"; > reg = <0>; > }; > ... > }; > > Or if you just want the regmap: > > &soc { > regmap: regmap@fff0000 { > compatible = "simple-regmap"; > reg = <0xfff0000>; > regmap,reg-bits = <16>; > regmap,val-bits = <32>; > }; > > enet-which-needs-syscon-too@1000000 { > vendor,ctrl-regmap = <®map>; > }; > }; > > Similar to the current syscon (which is MMIO only..). We do not need a 'simple-regmap' solution for your use-case. Since your device's registers are segregated, just split up the register map and allocate each sub-device with it's own slice. > > I can't think of any reasons why not, off the top of my head. > > > > Does Regmap only deal with shared accesses from multiple devices > > accessing a single register map, or can it also handle multiple > > devices communicating over a single I2C channel? > > > > One for Mark perhaps.
Am 2020-06-09 17:19, schrieb Lee Jones: > On Tue, 09 Jun 2020, Michael Walle wrote: > >> Am 2020-06-09 08:47, schrieb Lee Jones: >> > On Mon, 08 Jun 2020, Michael Walle wrote: >> > >> > > Am 2020-06-08 20:56, schrieb Lee Jones: >> > > > On Mon, 08 Jun 2020, Michael Walle wrote: >> > > > >> > > > > Am 2020-06-08 12:02, schrieb Andy Shevchenko: >> > > > > > +Cc: some Intel people WRT our internal discussion about similar >> > > > > > problem and solutions. >> > > > > > >> > > > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: >> > > > > > > On Sat, 06 Jun 2020, Michael Walle wrote: >> > > > > > > > Am 2020-06-06 13:46, schrieb Mark Brown: >> > > > > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: >> > > > > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: >> > > > > > >> > > > > > ... >> > > > > > >> > > > > > > Right. I'm suggesting a means to extrapolate complex shared and >> > > > > > > sometimes intertwined batches of register sets to be consumed by >> > > > > > > multiple (sub-)devices spanning different subsystems. >> > > > > > > >> > > > > > > Actually scrap that. The most common case I see is a single Regmap >> > > > > > > covering all child-devices. >> > > > > > >> > > > > > Yes, because often we need a synchronization across the entire address >> > > > > > space of the (parent) device in question. >> > > > > > >> > > > > > > It would be great if there was a way in >> > > > > > > which we could make an assumption that the entire register address >> > > > > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between >> > > > > > > each of the devices described by its child-nodes. Probably by picking >> > > > > > > up on the 'simple-mfd' compatible string in the first instance. >> > > > > > > >> > > > > > > Rob, is the above something you would contemplate? >> > > > > > > >> > > > > > > Michael, do your register addresses overlap i.e. are they intermingled >> > > > > > > with one another? Do multiple child devices need access to the same >> > > > > > > registers i.e. are they shared? >> > > > > >> > > > > No they don't overlap, expect for maybe the version register, which is >> > > > > just there once and not per function block. >> > > > >> > > > Then what's stopping you having each device Regmap their own space? >> > > >> > > Because its just one I2C device, AFAIK thats not possible, right? >> > >> > Not sure what (if any) the restrictions are. >> >> You can only have one device per I2C address. Therefore, I need one >> device >> which is enumerated by the I2C bus, which then enumerates its >> sub-devices. >> I thought this was one of the use cases for MFD. (Regardless of how a >> sub-device access its registers). So even in the "simple-regmap" case >> this >> would need to be an i2c device. Here (see below) >> >> E.g. >> >> &i2cbus { >> mfd-device@10 { >> compatible = "simple-regmap", "simple-mfd"; >> reg = <10>; >> regmap,reg-bits = <8>; >> regmap,val-bits = <8>; >> sub-device@0 { >> compatible = "vendor,sub-device0"; >> reg = <0>; >> }; >> ... >> }; >> >> Or if you just want the regmap: >> >> &soc { >> regmap: regmap@fff0000 { >> compatible = "simple-regmap"; >> reg = <0xfff0000>; >> regmap,reg-bits = <16>; >> regmap,val-bits = <32>; >> }; >> >> enet-which-needs-syscon-too@1000000 { >> vendor,ctrl-regmap = <®map>; >> }; >> }; >> >> Similar to the current syscon (which is MMIO only..). > > We do not need a 'simple-regmap' solution for your use-case. > > Since your device's registers are segregated, just split up the > register map and allocate each sub-device with it's own slice. I don't get it, could you make a device tree example for my use-case? (see also above) -michael > >> > I can't think of any reasons why not, off the top of my head. >> > >> > Does Regmap only deal with shared accesses from multiple devices >> > accessing a single register map, or can it also handle multiple >> > devices communicating over a single I2C channel? >> > >> > One for Mark perhaps.
On Mon, Jun 08, 2020 at 09:28:27AM +0100, Lee Jones wrote: > Rob, something for you below. > > On Sat, 06 Jun 2020, Michael Walle wrote: > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > > > > > > > I have no idea what you are thinking of when you say "simple-regmap" so > > > > > it is difficult to comment. > > > > > > > I guess, Lee is suggesting to be able to create a regmap instance via > > > > device tree (and populate its child nodes?). Like > > > > compatible = "syscon", "simple-mfd"; > > > > but for any regmap, not just MMIO. > > Bingo! > > > > I don't understand why this would be anything separate to > > > simple-mfd. > > > > Don't just simple-mfd tells the of core, to probe the children this > > node? Where does the regmap then come from? > > Right. I'm suggesting a means to extrapolate complex shared and > sometimes intertwined batches of register sets to be consumed by > multiple (sub-)devices spanning different subsystems. > > Actually scrap that. The most common case I see is a single Regmap > covering all child-devices. It would be great if there was a way in > which we could make an assumption that the entire register address > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > each of the devices described by its child-nodes. Probably by picking > up on the 'simple-mfd' compatible string in the first instance. > > Rob, is the above something you would contemplate? No. I'd like to just kill off syscon and simple-mfd really. Those are just hints meaning a specific compatible is still needed, but I see them all the time alone (or combined like above). 'syscon' just serves to create a regmap. This could be accomplished just with a list of compatibles to register a regmap for. That might be a longish list, but wanting a regmap is really a kernel implementation detail and decision. > Michael, do your register addresses overlap i.e. are they intermingled > with one another? Do multiple child devices need access to the same > registers i.e. are they shared? > > > > > But, there is more in my driver: > > > > (1) there is a version check > > If we can rid the Regmap dependency, then creating an entire driver to > conduct a version check is unjustifiable. This could become an inline > function which is called by each of the sub-devices instead, for > example. > > > > > (2) there is another function for which there is no suitable linux > > > > subsystem I'm aware of and thus which I'd like to us sysfs > > > > attributes for: This controller supports 16 non-volatile > > > > configuration bits. (this is still TBD) > > There is a place for everything in Linux. > > What do these bits configure? > > > > TBH I'd also say that the enumeration of the subdevices for this > > > device should be in the device rather than the DT, they don't > > > seem to be things that exist outside of this one device. > > > > We're going circles here, formerly they were enumerated in the MFD. > > Yes, they are devices which aren't likely be used outside a > > "sl28cpld", but there might there might be other versions of the > > sl28cpld with other components on different base addresses. I > > don't care if they are enumerated in DT or MFD, actually, I'd > > prefer the latter. _But_ I would like to have the device tree > > properties for its subdevices, e.g. the ones for the watchdog or > > whatever components there might be in the future. > > [...] > > > MFD core can > > match a device tree node today; but only one per unique compatible > > string. So what should I use to differentiate the different > > subdevices? > > Right. I have been aware of this issue. The only suitable solution > to this would be to match on 'reg'. > > FYI: I plan to fix this. > > If your register map needs to change, then I suggest that this is > either a new device or at least a different version of the device and > would also have to be represented as different (sub-)mfd_cell. The same register set at a different offset is the same (sub)device. > > > Rob suggested the internal offset, which I did here. > > FWIW, I don't like this idea. DTs should not have to be modified > (either in the first instance or subsequently) or specifically > designed to patch inadequacies in any given OS. My understanding is there can be differing combinations or number of instances of sub devices for this device. That's when having DT sub devices makes sense. If the h/w changes, then the DT should change. Multiple instances of devices require an address to identify them and we don't make up numbering if we can avoid it. The earlier revisions just had made up indices for addresses. Rob
On Tue, Jun 09, 2020 at 05:01:17PM +0200, Michael Walle wrote: > Am 2020-06-09 16:42, schrieb Mark Brown: > > On Tue, Jun 09, 2020 at 04:38:31PM +0200, Michael Walle wrote: > > > > > mfd-device@10 { > > > compatible = "simple-regmap", "simple-mfd"; > > > reg = <10>; > > > regmap,reg-bits = <8>; > > > regmap,val-bits = <8>; > > > sub-device@0 { > > > compatible = "vendor,sub-device0"; > > > reg = <0>; > > > }; > > > > A DT binding like this is not a good idea, encoding the details of the > > register map into the DT binding makes it an ABI which is begging for > > trouble. I'd also suggest that any device using a generic driver like > > this should have a specific compatible string for the device so we can > > go back and add quirks later if we need them. > > Like in the spidev case, yes. But OTOH if I _just_ encode the parameters > for the regmap a MFD, Lee don't agree because its just a shim. So either > way I seem to be stuck here. > > Where should I put the code to create an i2c driver, init a regmap and > populate its childen? Find another driver doing this already and rename it 'simple-mfd' (no relation to the DT binding) and add your compatible string to it. 'Generic' or 'simple' drivers don't require generic/simple DT bindings. Or extend the existing syscon driver to look up the bus_type and create the regmap based on the bus type? Rob
On Tue, Jun 09, 2020 at 11:15:20AM -0600, Rob Herring wrote: > Find another driver doing this already and rename it 'simple-mfd' (no > relation to the DT binding) and add your compatible string to it. > 'Generic' or 'simple' drivers don't require generic/simple DT bindings. > Or extend the existing syscon driver to look up the bus_type and create > the regmap based on the bus type? You'd need a particular bus driver to instantiate for a given bus (or I'm misunderstanding your proposal) so it wouldn't even need a lookup, just per-bus ID tables (and ideally also data tables with the regmap and child descriptions).
On Tue, 09 Jun 2020, Rob Herring wrote: > On Tue, Jun 09, 2020 at 05:01:17PM +0200, Michael Walle wrote: > > Am 2020-06-09 16:42, schrieb Mark Brown: > > > On Tue, Jun 09, 2020 at 04:38:31PM +0200, Michael Walle wrote: > > > > > > > mfd-device@10 { > > > > compatible = "simple-regmap", "simple-mfd"; > > > > reg = <10>; > > > > regmap,reg-bits = <8>; > > > > regmap,val-bits = <8>; > > > > sub-device@0 { > > > > compatible = "vendor,sub-device0"; > > > > reg = <0>; > > > > }; > > > > > > A DT binding like this is not a good idea, encoding the details of the > > > register map into the DT binding makes it an ABI which is begging for > > > trouble. I'd also suggest that any device using a generic driver like > > > this should have a specific compatible string for the device so we can > > > go back and add quirks later if we need them. > > > > Like in the spidev case, yes. But OTOH if I _just_ encode the parameters > > for the regmap a MFD, Lee don't agree because its just a shim. So either > > way I seem to be stuck here. > > > > Where should I put the code to create an i2c driver, init a regmap and > > populate its childen? > > Find another driver doing this already and rename it 'simple-mfd' (no > relation to the DT binding) and add your compatible string to it. > 'Generic' or 'simple' drivers don't require generic/simple DT bindings. Creating a generic driver is one of the options spinning around in my head. If nothing better comes of these discussions, I'll turn my hand to it soon. > Or extend the existing syscon driver to look up the bus_type and create > the regmap based on the bus type?
On Tue, 09 Jun 2020, Rob Herring wrote: > On Mon, Jun 08, 2020 at 09:28:27AM +0100, Lee Jones wrote: > > Rob, something for you below. > > > > On Sat, 06 Jun 2020, Michael Walle wrote: > > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > > > > > > > > > I have no idea what you are thinking of when you say "simple-regmap" so > > > > > > it is difficult to comment. > > > > > > > > > I guess, Lee is suggesting to be able to create a regmap instance via > > > > > device tree (and populate its child nodes?). Like > > > > > compatible = "syscon", "simple-mfd"; > > > > > but for any regmap, not just MMIO. > > > > Bingo! > > > > > > I don't understand why this would be anything separate to > > > > simple-mfd. > > > > > > Don't just simple-mfd tells the of core, to probe the children this > > > node? Where does the regmap then come from? > > > > Right. I'm suggesting a means to extrapolate complex shared and > > sometimes intertwined batches of register sets to be consumed by > > multiple (sub-)devices spanning different subsystems. > > > > Actually scrap that. The most common case I see is a single Regmap > > covering all child-devices. It would be great if there was a way in > > which we could make an assumption that the entire register address > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > > each of the devices described by its child-nodes. Probably by picking > > up on the 'simple-mfd' compatible string in the first instance. > > > > Rob, is the above something you would contemplate? > > No. I'd like to just kill off syscon and simple-mfd really. Those are > just hints meaning a specific compatible is still needed, but I see them > all the time alone (or combined like above). 'syscon' just serves to > create a regmap. This could be accomplished just with a list of > compatibles to register a regmap for. That might be a longish list, but > wanting a regmap is really a kernel implementation detail and decision. Exactly. Syscon is a real tangible thing and Regmap is a Linux subsystem. So swapping out the former for the latter sounds like the opposite of what you'd want to do. > > > MFD core can > > > match a device tree node today; but only one per unique compatible > > > string. So what should I use to differentiate the different > > > subdevices? > > > > Right. I have been aware of this issue. The only suitable solution > > to this would be to match on 'reg'. > > > > FYI: I plan to fix this. > > > > If your register map needs to change, then I suggest that this is > > either a new device or at least a different version of the device and > > would also have to be represented as different (sub-)mfd_cell. > > The same register set at a different offset is the same (sub)device. See below. > > > Rob suggested the internal offset, which I did here. > > > > FWIW, I don't like this idea. DTs should not have to be modified > > (either in the first instance or subsequently) or specifically > > designed to patch inadequacies in any given OS. > > My understanding is there can be differing combinations or number of > instances of sub devices for this device. That's when having DT sub > devices makes sense. If the h/w changes, then the DT should change. This is the same point I was making above. > Multiple instances of devices require an address to identify them and we > don't make up numbering if we can avoid it. The earlier revisions just > had made up indices for addresses. Right. Which I'm against. Placing "internal offsets" into the 'reg' property is a hack. The issue is, if we need to configure the devices differently, then how do we identify them in order to ensure the correct OF node pointer is allocated to the correct platform device?
On Tue, 09 Jun 2020, Michael Walle wrote: > Am 2020-06-09 17:19, schrieb Lee Jones: > > On Tue, 09 Jun 2020, Michael Walle wrote: > > > > > Am 2020-06-09 08:47, schrieb Lee Jones: > > > > On Mon, 08 Jun 2020, Michael Walle wrote: > > > > > > > > > Am 2020-06-08 20:56, schrieb Lee Jones: > > > > > > On Mon, 08 Jun 2020, Michael Walle wrote: > > > > > > > > > > > > > Am 2020-06-08 12:02, schrieb Andy Shevchenko: > > > > > > > > +Cc: some Intel people WRT our internal discussion about similar > > > > > > > > problem and solutions. > > > > > > > > > > > > > > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > > > On Sat, 06 Jun 2020, Michael Walle wrote: > > > > > > > > > > Am 2020-06-06 13:46, schrieb Mark Brown: > > > > > > > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: > > > > > > > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > Right. I'm suggesting a means to extrapolate complex shared and > > > > > > > > > sometimes intertwined batches of register sets to be consumed by > > > > > > > > > multiple (sub-)devices spanning different subsystems. > > > > > > > > > > > > > > > > > > Actually scrap that. The most common case I see is a single Regmap > > > > > > > > > covering all child-devices. > > > > > > > > > > > > > > > > Yes, because often we need a synchronization across the entire address > > > > > > > > space of the (parent) device in question. > > > > > > > > > > > > > > > > > It would be great if there was a way in > > > > > > > > > which we could make an assumption that the entire register address > > > > > > > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between > > > > > > > > > each of the devices described by its child-nodes. Probably by picking > > > > > > > > > up on the 'simple-mfd' compatible string in the first instance. > > > > > > > > > > > > > > > > > > Rob, is the above something you would contemplate? > > > > > > > > > > > > > > > > > > Michael, do your register addresses overlap i.e. are they intermingled > > > > > > > > > with one another? Do multiple child devices need access to the same > > > > > > > > > registers i.e. are they shared? > > > > > > > > > > > > > > No they don't overlap, expect for maybe the version register, which is > > > > > > > just there once and not per function block. > > > > > > > > > > > > Then what's stopping you having each device Regmap their own space? > > > > > > > > > > Because its just one I2C device, AFAIK thats not possible, right? > > > > > > > > Not sure what (if any) the restrictions are. > > > > > > You can only have one device per I2C address. Therefore, I need one > > > device > > > which is enumerated by the I2C bus, which then enumerates its > > > sub-devices. > > > I thought this was one of the use cases for MFD. (Regardless of how a > > > sub-device access its registers). So even in the "simple-regmap" > > > case this > > > would need to be an i2c device. > > Here (see below) Yes, it should still be an I2C device. > > > > > > E.g. > > > > > > &i2cbus { > > > mfd-device@10 { > > > compatible = "simple-regmap", "simple-mfd"; > > > reg = <10>; > > > regmap,reg-bits = <8>; > > > regmap,val-bits = <8>; > > > sub-device@0 { > > > compatible = "vendor,sub-device0"; > > > reg = <0>; > > > }; > > > ... > > > }; > > > > > > Or if you just want the regmap: > > > > > > &soc { > > > regmap: regmap@fff0000 { > > > compatible = "simple-regmap"; > > > reg = <0xfff0000>; > > > regmap,reg-bits = <16>; > > > regmap,val-bits = <32>; > > > }; > > > > > > enet-which-needs-syscon-too@1000000 { > > > vendor,ctrl-regmap = <®map>; > > > }; > > > }; > > > > > > Similar to the current syscon (which is MMIO only..). > > > > We do not need a 'simple-regmap' solution for your use-case. > > > > Since your device's registers are segregated, just split up the > > register map and allocate each sub-device with it's own slice. > > I don't get it, could you make a device tree example for my > use-case? (see also above) &i2cbus { mfd-device@10 { compatible = "simple-mfd"; reg = <10>; sub-device@10 { compatible = "vendor,sub-device"; reg = <10>; }; }; The Regmap config would be present in each of the child devices. Each child device would call devm_regmap_init_i2c() in .probe(). > > > > I can't think of any reasons why not, off the top of my head. > > > > > > > > Does Regmap only deal with shared accesses from multiple devices > > > > accessing a single register map, or can it also handle multiple > > > > devices communicating over a single I2C channel? > > > > > > > > One for Mark perhaps.
Am 2020-06-09 21:45, schrieb Lee Jones: > On Tue, 09 Jun 2020, Michael Walle wrote: > >> Am 2020-06-09 17:19, schrieb Lee Jones: >> > On Tue, 09 Jun 2020, Michael Walle wrote: >> > >> > > Am 2020-06-09 08:47, schrieb Lee Jones: >> > > > On Mon, 08 Jun 2020, Michael Walle wrote: >> > > > >> > > > > Am 2020-06-08 20:56, schrieb Lee Jones: >> > > > > > On Mon, 08 Jun 2020, Michael Walle wrote: >> > > > > > >> > > > > > > Am 2020-06-08 12:02, schrieb Andy Shevchenko: >> > > > > > > > +Cc: some Intel people WRT our internal discussion about similar >> > > > > > > > problem and solutions. >> > > > > > > > >> > > > > > > > On Mon, Jun 8, 2020 at 11:30 AM Lee Jones <lee.jones@linaro.org> wrote: >> > > > > > > > > On Sat, 06 Jun 2020, Michael Walle wrote: >> > > > > > > > > > Am 2020-06-06 13:46, schrieb Mark Brown: >> > > > > > > > > > > On Fri, Jun 05, 2020 at 10:07:36PM +0200, Michael Walle wrote: >> > > > > > > > > > > > Am 2020-06-05 12:50, schrieb Mark Brown: >> > > > > > > > >> > > > > > > > ... >> > > > > > > > >> > > > > > > > > Right. I'm suggesting a means to extrapolate complex shared and >> > > > > > > > > sometimes intertwined batches of register sets to be consumed by >> > > > > > > > > multiple (sub-)devices spanning different subsystems. >> > > > > > > > > >> > > > > > > > > Actually scrap that. The most common case I see is a single Regmap >> > > > > > > > > covering all child-devices. >> > > > > > > > >> > > > > > > > Yes, because often we need a synchronization across the entire address >> > > > > > > > space of the (parent) device in question. >> > > > > > > > >> > > > > > > > > It would be great if there was a way in >> > > > > > > > > which we could make an assumption that the entire register address >> > > > > > > > > space for a 'tagged' (MFD) device is to be shared (via Regmap) between >> > > > > > > > > each of the devices described by its child-nodes. Probably by picking >> > > > > > > > > up on the 'simple-mfd' compatible string in the first instance. >> > > > > > > > > >> > > > > > > > > Rob, is the above something you would contemplate? >> > > > > > > > > >> > > > > > > > > Michael, do your register addresses overlap i.e. are they intermingled >> > > > > > > > > with one another? Do multiple child devices need access to the same >> > > > > > > > > registers i.e. are they shared? >> > > > > > > >> > > > > > > No they don't overlap, expect for maybe the version register, which is >> > > > > > > just there once and not per function block. >> > > > > > >> > > > > > Then what's stopping you having each device Regmap their own space? >> > > > > >> > > > > Because its just one I2C device, AFAIK thats not possible, right? >> > > > >> > > > Not sure what (if any) the restrictions are. >> > > >> > > You can only have one device per I2C address. Therefore, I need one >> > > device >> > > which is enumerated by the I2C bus, which then enumerates its >> > > sub-devices. >> > > I thought this was one of the use cases for MFD. (Regardless of how a >> > > sub-device access its registers). So even in the "simple-regmap" >> > > case this >> > > would need to be an i2c device. >> >> Here (see below) > > Yes, it should still be an I2C device. > >> > > >> > > E.g. >> > > >> > > &i2cbus { >> > > mfd-device@10 { >> > > compatible = "simple-regmap", "simple-mfd"; >> > > reg = <10>; >> > > regmap,reg-bits = <8>; >> > > regmap,val-bits = <8>; >> > > sub-device@0 { >> > > compatible = "vendor,sub-device0"; >> > > reg = <0>; >> > > }; >> > > ... >> > > }; >> > > >> > > Or if you just want the regmap: >> > > >> > > &soc { >> > > regmap: regmap@fff0000 { >> > > compatible = "simple-regmap"; >> > > reg = <0xfff0000>; >> > > regmap,reg-bits = <16>; >> > > regmap,val-bits = <32>; >> > > }; >> > > >> > > enet-which-needs-syscon-too@1000000 { >> > > vendor,ctrl-regmap = <®map>; >> > > }; >> > > }; >> > > >> > > Similar to the current syscon (which is MMIO only..). >> > >> > We do not need a 'simple-regmap' solution for your use-case. >> > >> > Since your device's registers are segregated, just split up the >> > register map and allocate each sub-device with it's own slice. >> >> I don't get it, could you make a device tree example for my >> use-case? (see also above) > > &i2cbus { > mfd-device@10 { > compatible = "simple-mfd"; > reg = <10>; > > sub-device@10 { > compatible = "vendor,sub-device"; > reg = <10>; > }; > }; > > The Regmap config would be present in each of the child devices. > > Each child device would call devm_regmap_init_i2c() in .probe(). Ah, I see. If I'm not wrong, this still means to create an i2c device driver with the name "simple-mfd". Besides that, I don't like this, because: - Rob already expressed its concerns with "simple-mfd" and so on. - you need to duplicate the config in each sub device - which also means you are restricting the sub devices to be i2c only (unless you implement and duplicate other regmap configs, too). For this driver, SPI and MMIO may be viable options. Thus, I'd rather implement a simple-mfd.c which implement a common I2C driver for now and populate its children using devm_of_platform_populate(). This could be extended to support other type of regmaps like SPI in the future. Also some MFD drivers could be moved to this, a likely candidate is the smsc-ece1099.c. Although I don't really understand its purpose, if don't have CONFIG_OF. Judging from the existing code, this simple-mfd.c wouldn't just be "a list of compatible" strings but also additional quirks and tweaks for particular devices in this list. -michael
On Wed, 10 Jun 2020, Michael Walle wrote: > Am 2020-06-09 21:45, schrieb Lee Jones: > > On Tue, 09 Jun 2020, Michael Walle wrote: > > > > We do not need a 'simple-regmap' solution for your use-case. > > > > > > > > Since your device's registers are segregated, just split up the > > > > register map and allocate each sub-device with it's own slice. > > > > > > I don't get it, could you make a device tree example for my > > > use-case? (see also above) > > > > &i2cbus { > > mfd-device@10 { > > compatible = "simple-mfd"; > > reg = <10>; > > > > sub-device@10 { > > compatible = "vendor,sub-device"; > > reg = <10>; > > }; > > }; > > > > The Regmap config would be present in each of the child devices. > > > > Each child device would call devm_regmap_init_i2c() in .probe(). > > Ah, I see. If I'm not wrong, this still means to create an i2c > device driver with the name "simple-mfd". Yes, it does. > Besides that, I don't like this, because: > - Rob already expressed its concerns with "simple-mfd" and so on. Where did this take place? I'd like to read up on this. > - you need to duplicate the config in each sub device You can have a share a single config. > - which also means you are restricting the sub devices to be > i2c only (unless you implement and duplicate other regmap configs, > too). For this driver, SPI and MMIO may be viable options. You could also have a shared implementation to choose between different busses. > Thus, I'd rather implement a simple-mfd.c which implement a common > I2C driver for now and populate its children using > devm_of_platform_populate(). This could be extended to support other > type of regmaps like SPI in the future. > > Also some MFD drivers could be moved to this, a likely candidate is > the smsc-ece1099.c. Although I don't really understand its purpose, > if don't have CONFIG_OF. > > Judging from the existing code, this simple-mfd.c wouldn't just be > "a list of compatible" strings but also additional quirks and tweaks > for particular devices in this list. Hold off on the simple-mfd.c idea, as I'm not taken by it yet and wouldn't want you to waste your time. I have another idea which would help. Give me a few days to put something together.
Am 2020-06-10 09:19, schrieb Lee Jones: > On Wed, 10 Jun 2020, Michael Walle wrote: >> Am 2020-06-09 21:45, schrieb Lee Jones: >> > On Tue, 09 Jun 2020, Michael Walle wrote: >> > > > We do not need a 'simple-regmap' solution for your use-case. >> > > > >> > > > Since your device's registers are segregated, just split up the >> > > > register map and allocate each sub-device with it's own slice. >> > > >> > > I don't get it, could you make a device tree example for my >> > > use-case? (see also above) >> > >> > &i2cbus { >> > mfd-device@10 { >> > compatible = "simple-mfd"; >> > reg = <10>; >> > >> > sub-device@10 { >> > compatible = "vendor,sub-device"; >> > reg = <10>; >> > }; >> > }; >> > >> > The Regmap config would be present in each of the child devices. >> > >> > Each child device would call devm_regmap_init_i2c() in .probe(). >> >> Ah, I see. If I'm not wrong, this still means to create an i2c >> device driver with the name "simple-mfd". > > Yes, it does. > >> Besides that, I don't like this, because: >> - Rob already expressed its concerns with "simple-mfd" and so on. > > Where did this take place? I'd like to read up on this. In this thread: https://lore.kernel.org/linux-devicetree/20200604211039.12689-1-michael@walle.cc/T/#m16fdba5962069e7cd4aa817582ee358c9fe2ecbf > >> - you need to duplicate the config in each sub device > > You can have a share a single config. > >> - which also means you are restricting the sub devices to be >> i2c only (unless you implement and duplicate other regmap configs, >> too). For this driver, SPI and MMIO may be viable options. > > You could also have a shared implementation to choose between different > busses. Then what is the difference between to have this shared config in the parent driver only and use the functions which are already there, i.e. dev_get_regmap(parent). But see, below, I'll wait with what you're coming up. > >> Thus, I'd rather implement a simple-mfd.c which implement a common >> I2C driver for now and populate its children using >> devm_of_platform_populate(). This could be extended to support other >> type of regmaps like SPI in the future. >> >> Also some MFD drivers could be moved to this, a likely candidate is >> the smsc-ece1099.c. Although I don't really understand its purpose, >> if don't have CONFIG_OF. >> >> Judging from the existing code, this simple-mfd.c wouldn't just be >> "a list of compatible" strings but also additional quirks and tweaks >> for particular devices in this list. > > Hold off on the simple-mfd.c idea, as I'm not taken by it yet and > wouldn't want you to waste your time. I have another idea which would > help. Give me a few days to put something together. Sure. I'm just glad there is now a discussion about this issue. -michael
On Wed, 10 Jun 2020, Michael Walle wrote: > Am 2020-06-10 09:19, schrieb Lee Jones: > > On Wed, 10 Jun 2020, Michael Walle wrote: > > > Am 2020-06-09 21:45, schrieb Lee Jones: > > > > On Tue, 09 Jun 2020, Michael Walle wrote: > > > > > > We do not need a 'simple-regmap' solution for your use-case. > > > > > > > > > > > > Since your device's registers are segregated, just split up the > > > > > > register map and allocate each sub-device with it's own slice. > > > > > > > > > > I don't get it, could you make a device tree example for my > > > > > use-case? (see also above) > > > > > > > > &i2cbus { > > > > mfd-device@10 { > > > > compatible = "simple-mfd"; > > > > reg = <10>; > > > > > > > > sub-device@10 { > > > > compatible = "vendor,sub-device"; > > > > reg = <10>; > > > > }; > > > > }; > > > > > > > > The Regmap config would be present in each of the child devices. > > > > > > > > Each child device would call devm_regmap_init_i2c() in .probe(). > > > > > > Ah, I see. If I'm not wrong, this still means to create an i2c > > > device driver with the name "simple-mfd". > > > > Yes, it does. > > > > > Besides that, I don't like this, because: > > > - Rob already expressed its concerns with "simple-mfd" and so on. > > > > Where did this take place? I'd like to read up on this. > > In this thread: > https://lore.kernel.org/linux-devicetree/20200604211039.12689-1-michael@walle.cc/T/#m16fdba5962069e7cd4aa817582ee358c9fe2ecbf > > > > > > - you need to duplicate the config in each sub device > > > > You can have a share a single config. > > > > > - which also means you are restricting the sub devices to be > > > i2c only (unless you implement and duplicate other regmap configs, > > > too). For this driver, SPI and MMIO may be viable options. > > > > You could also have a shared implementation to choose between different > > busses. > > Then what is the difference between to have this shared config in the > parent driver only and use the functions which are already there, i.e. > dev_get_regmap(parent). But see, below, I'll wait with what you're > coming up. The difference is the omission of an otherwise pointless/superfluous driver. Actually, it's the difference between the omission of 10 pointless drivers! > > > Thus, I'd rather implement a simple-mfd.c which implement a common > > > I2C driver for now and populate its children using > > > devm_of_platform_populate(). This could be extended to support other > > > type of regmaps like SPI in the future. > > > > > > Also some MFD drivers could be moved to this, a likely candidate is > > > the smsc-ece1099.c. Although I don't really understand its purpose, > > > if don't have CONFIG_OF. > > > > > > Judging from the existing code, this simple-mfd.c wouldn't just be > > > "a list of compatible" strings but also additional quirks and tweaks > > > for particular devices in this list. > > > > Hold off on the simple-mfd.c idea, as I'm not taken by it yet and > > wouldn't want you to waste your time. I have another idea which would > > help. Give me a few days to put something together. > > Sure. I'm just glad there is now a discussion about this issue. It's very much in my mind. I've been meaning to do something about it for quite some time.
Am 2020-06-10 09:56, schrieb Lee Jones: > On Wed, 10 Jun 2020, Michael Walle wrote: > >> Am 2020-06-10 09:19, schrieb Lee Jones: >> > On Wed, 10 Jun 2020, Michael Walle wrote: >> > > Am 2020-06-09 21:45, schrieb Lee Jones: >> > > > On Tue, 09 Jun 2020, Michael Walle wrote: >> > > > > > We do not need a 'simple-regmap' solution for your use-case. >> > > > > > >> > > > > > Since your device's registers are segregated, just split up the >> > > > > > register map and allocate each sub-device with it's own slice. >> > > > > >> > > > > I don't get it, could you make a device tree example for my >> > > > > use-case? (see also above) >> > > > >> > > > &i2cbus { >> > > > mfd-device@10 { >> > > > compatible = "simple-mfd"; >> > > > reg = <10>; >> > > > >> > > > sub-device@10 { >> > > > compatible = "vendor,sub-device"; >> > > > reg = <10>; >> > > > }; >> > > > }; >> > > > >> > > > The Regmap config would be present in each of the child devices. >> > > > >> > > > Each child device would call devm_regmap_init_i2c() in .probe(). >> > > >> > > Ah, I see. If I'm not wrong, this still means to create an i2c >> > > device driver with the name "simple-mfd". >> > >> > Yes, it does. >> > >> > > Besides that, I don't like this, because: >> > > - Rob already expressed its concerns with "simple-mfd" and so on. >> > >> > Where did this take place? I'd like to read up on this. >> >> In this thread: >> https://lore.kernel.org/linux-devicetree/20200604211039.12689-1-michael@walle.cc/T/#m16fdba5962069e7cd4aa817582ee358c9fe2ecbf >> >> > >> > > - you need to duplicate the config in each sub device >> > >> > You can have a share a single config. >> > >> > > - which also means you are restricting the sub devices to be >> > > i2c only (unless you implement and duplicate other regmap configs, >> > > too). For this driver, SPI and MMIO may be viable options. >> > >> > You could also have a shared implementation to choose between different >> > busses. >> >> Then what is the difference between to have this shared config in the >> parent driver only and use the functions which are already there, i.e. >> dev_get_regmap(parent). But see, below, I'll wait with what you're >> coming up. > > The difference is the omission of an otherwise pointless/superfluous > driver. Actually, it's the difference between the omission of 10 > pointless drivers! If you want to omit anything generic in the device tree - and as far as I understand it - that should be the way to go, the specific compatible string of the parent device has to go somewhere. Thus I'd appreciate a consolidated (MFD) driver which holds all these, as you say it pointless drivers. Because IMHO they are not pointless, rather they are the actual drivers for the MFD. Its sub nodes are just an implementation detail to be able to use the OF bindings (like your clock example or a phandle to a PWM controller). Just because it is almost nothing there except the regmap instantiation doesn't mean it is not a valid MFD driver. And there is also additional stuff, like clock enable, version checks, etc. -michael >> > > Thus, I'd rather implement a simple-mfd.c which implement a common >> > > I2C driver for now and populate its children using >> > > devm_of_platform_populate(). This could be extended to support other >> > > type of regmaps like SPI in the future. >> > > >> > > Also some MFD drivers could be moved to this, a likely candidate is >> > > the smsc-ece1099.c. Although I don't really understand its purpose, >> > > if don't have CONFIG_OF. >> > > >> > > Judging from the existing code, this simple-mfd.c wouldn't just be >> > > "a list of compatible" strings but also additional quirks and tweaks >> > > for particular devices in this list. >> > >> > Hold off on the simple-mfd.c idea, as I'm not taken by it yet and >> > wouldn't want you to waste your time. I have another idea which would >> > help. Give me a few days to put something together. >> >> Sure. I'm just glad there is now a discussion about this issue. > > It's very much in my mind. > > I've been meaning to do something about it for quite some time.
On Wed, Jun 10, 2020 at 1:19 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Wed, 10 Jun 2020, Michael Walle wrote: > > Am 2020-06-09 21:45, schrieb Lee Jones: > > > On Tue, 09 Jun 2020, Michael Walle wrote: > > > > > We do not need a 'simple-regmap' solution for your use-case. > > > > > > > > > > Since your device's registers are segregated, just split up the > > > > > register map and allocate each sub-device with it's own slice. > > > > > > > > I don't get it, could you make a device tree example for my > > > > use-case? (see also above) > > > > > > &i2cbus { > > > mfd-device@10 { > > > compatible = "simple-mfd"; > > > reg = <10>; > > > > > > sub-device@10 { > > > compatible = "vendor,sub-device"; > > > reg = <10>; > > > }; > > > }; > > > > > > The Regmap config would be present in each of the child devices. > > > > > > Each child device would call devm_regmap_init_i2c() in .probe(). > > > > Ah, I see. If I'm not wrong, this still means to create an i2c > > device driver with the name "simple-mfd". > > Yes, it does. TBC, while fine for a driver to bind on 'simple-mfd', a DT compatible with that alone is not fine. > > Besides that, I don't like this, because: > > - Rob already expressed its concerns with "simple-mfd" and so on. > > Where did this take place? I'd like to read up on this. > > > - you need to duplicate the config in each sub device > > You can have a share a single config. > > > - which also means you are restricting the sub devices to be > > i2c only (unless you implement and duplicate other regmap configs, > > too). For this driver, SPI and MMIO may be viable options. > > You could also have a shared implementation to choose between different > busses. I think it is really the syscon mfd driver you want to generalize to other buses. Though with a quick look at it, there's not really a whole lot to share. The regmap lookup would be the main thing. You are going to need a driver instance for each bus type. > > Thus, I'd rather implement a simple-mfd.c which implement a common > > I2C driver for now and populate its children using > > devm_of_platform_populate(). This could be extended to support other > > type of regmaps like SPI in the future. > > > > Also some MFD drivers could be moved to this, a likely candidate is > > the smsc-ece1099.c. Although I don't really understand its purpose, > > if don't have CONFIG_OF. > > > > Judging from the existing code, this simple-mfd.c wouldn't just be > > "a list of compatible" strings but also additional quirks and tweaks > > for particular devices in this list. Yes, this is why specific compatible strings are required. > Hold off on the simple-mfd.c idea, as I'm not taken by it yet and > wouldn't want you to waste your time. I have another idea which would > help. Give me a few days to put something together. > > -- > Lee Jones [李琼斯] > Senior Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog
On Wed, 10 Jun 2020, Rob Herring wrote: > On Wed, Jun 10, 2020 at 1:19 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Wed, 10 Jun 2020, Michael Walle wrote: > > > Am 2020-06-09 21:45, schrieb Lee Jones: > > > > On Tue, 09 Jun 2020, Michael Walle wrote: > > > > > > We do not need a 'simple-regmap' solution for your use-case. > > > > > > > > > > > > Since your device's registers are segregated, just split up the > > > > > > register map and allocate each sub-device with it's own slice. > > > > > > > > > > I don't get it, could you make a device tree example for my > > > > > use-case? (see also above) > > > > > > > > &i2cbus { > > > > mfd-device@10 { > > > > compatible = "simple-mfd"; > > > > reg = <10>; > > > > > > > > sub-device@10 { > > > > compatible = "vendor,sub-device"; > > > > reg = <10>; > > > > }; > > > > }; > > > > > > > > The Regmap config would be present in each of the child devices. > > > > > > > > Each child device would call devm_regmap_init_i2c() in .probe(). > > > > > > Ah, I see. If I'm not wrong, this still means to create an i2c > > > device driver with the name "simple-mfd". > > > > Yes, it does. > > TBC, while fine for a driver to bind on 'simple-mfd', a DT compatible > with that alone is not fine. 'simple-mfd' essentially means: "This device doesn't do anything useful, but the children do." When used with 'syscon' it means: "Memory for this device is shared between all children" Adding more specific/descriptive compatible strings is conceptually fine, but they should not be forced to bind to a real driver using them. Else we're creating drivers for the sake of creating drivers. This is especially true with 'simple-mfd' is used without 'syscon'. > > > Besides that, I don't like this, because: > > > - Rob already expressed its concerns with "simple-mfd" and so on. > > > > Where did this take place? I'd like to read up on this. > > > > > - you need to duplicate the config in each sub device > > > > You can have a share a single config. > > > > > - which also means you are restricting the sub devices to be > > > i2c only (unless you implement and duplicate other regmap configs, > > > too). For this driver, SPI and MMIO may be viable options. > > > > You could also have a shared implementation to choose between different > > busses. > > I think it is really the syscon mfd driver you want to generalize to > other buses. Though with a quick look at it, there's not really a > whole lot to share. The regmap lookup would be the main thing. You are > going to need a driver instance for each bus type. On it.
On Wed, 10 Jun 2020, Michael Walle wrote: > Am 2020-06-10 09:56, schrieb Lee Jones: > > On Wed, 10 Jun 2020, Michael Walle wrote: > > > > > Am 2020-06-10 09:19, schrieb Lee Jones: > > > > On Wed, 10 Jun 2020, Michael Walle wrote: > > > > > Am 2020-06-09 21:45, schrieb Lee Jones: > > > > > > On Tue, 09 Jun 2020, Michael Walle wrote: > > > > > > > > We do not need a 'simple-regmap' solution for your use-case. > > > > > > > > > > > > > > > > Since your device's registers are segregated, just split up the > > > > > > > > register map and allocate each sub-device with it's own slice. > > > > > > > > > > > > > > I don't get it, could you make a device tree example for my > > > > > > > use-case? (see also above) > > > > > > > > > > > > &i2cbus { > > > > > > mfd-device@10 { > > > > > > compatible = "simple-mfd"; > > > > > > reg = <10>; > > > > > > > > > > > > sub-device@10 { > > > > > > compatible = "vendor,sub-device"; > > > > > > reg = <10>; > > > > > > }; > > > > > > }; > > > > > > > > > > > > The Regmap config would be present in each of the child devices. > > > > > > > > > > > > Each child device would call devm_regmap_init_i2c() in .probe(). > > > > > > > > > > Ah, I see. If I'm not wrong, this still means to create an i2c > > > > > device driver with the name "simple-mfd". > > > > > > > > Yes, it does. > > > > > > > > > Besides that, I don't like this, because: > > > > > - Rob already expressed its concerns with "simple-mfd" and so on. > > > > > > > > Where did this take place? I'd like to read up on this. > > > > > > In this thread: > > > https://lore.kernel.org/linux-devicetree/20200604211039.12689-1-michael@walle.cc/T/#m16fdba5962069e7cd4aa817582ee358c9fe2ecbf > > > > > > > > > > > > - you need to duplicate the config in each sub device > > > > > > > > You can have a share a single config. > > > > > > > > > - which also means you are restricting the sub devices to be > > > > > i2c only (unless you implement and duplicate other regmap configs, > > > > > too). For this driver, SPI and MMIO may be viable options. > > > > > > > > You could also have a shared implementation to choose between different > > > > busses. > > > > > > Then what is the difference between to have this shared config in the > > > parent driver only and use the functions which are already there, i.e. > > > dev_get_regmap(parent). But see, below, I'll wait with what you're > > > coming up. > > > > The difference is the omission of an otherwise pointless/superfluous > > driver. Actually, it's the difference between the omission of 10 > > pointless drivers! > > If you want to omit anything generic in the device tree - and as far as > I understand it - that should be the way to go, the specific compatible > string of the parent device has to go somewhere. Thus I'd appreciate > a consolidated (MFD) driver which holds all these, as you say it > pointless drivers. > Because IMHO they are not pointless, rather they are > the actual drivers for the MFD. Its sub nodes are just an implementation > detail to be able to use the OF bindings > (like your clock example or > a phandle to a PWM controller). Just because it is almost nothing there > except the regmap instantiation doesn't mean it is not a valid MFD driver. A valid MFD driver is whatever we (the Linux community at large) define it to be. An MFD is not a real thing. We made it up. It's MFD which is the implementation detail, not the child devices. If a driver a) does very little, and b) the very little it does do can be resolved in a different way, is not a valid driver. It's a waste of disk space. > And there is also additional stuff, like clock enable, version checks, etc. As more functionality is added *then* we can justify a driver.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 4f8b73d92df3..5c0cd514d197 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -2109,5 +2109,24 @@ config SGI_MFD_IOC3 If you have an SGI Origin, Octane, or a PCI IOC3 card, then say Y. Otherwise say N. +config MFD_SL28CPLD + bool "Kontron sl28 core driver" + depends on I2C=y + depends on OF + select REGMAP_I2C + select MFD_CORE + help + This option enables support for the board management controller + found on the Kontron sl28 CPLD. You have to select individual + functions, such as watchdog, GPIO, etc, under the corresponding menus + in order to enable them. + + Currently supported boards are: + + Kontron SMARC-sAL28 + + To compile this driver as a module, choose M here: the module will be + called sl28cpld. + endmenu endif diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 9367a92f795a..be59fb40aa28 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o obj-$(CONFIG_MFD_STMFX) += stmfx.o obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o + +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o diff --git a/drivers/mfd/sl28cpld.c b/drivers/mfd/sl28cpld.c new file mode 100644 index 000000000000..a23194bb6efa --- /dev/null +++ b/drivers/mfd/sl28cpld.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * MFD core for the sl28cpld. + * + * Copyright 2019 Kontron Europe GmbH + */ + +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/mfd/core.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/regmap.h> + +#define SL28CPLD_VERSION 0x03 +#define SL28CPLD_MIN_REQ_VERSION 14 + +struct sl28cpld { + struct device *dev; + struct regmap *regmap; +}; + +static const struct regmap_config sl28cpld_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .reg_stride = 1, +}; + +static int sl28cpld_probe(struct i2c_client *i2c) +{ + struct sl28cpld *sl28cpld; + struct device *dev = &i2c->dev; + unsigned int cpld_version; + int ret; + + sl28cpld = devm_kzalloc(dev, sizeof(*sl28cpld), GFP_KERNEL); + if (!sl28cpld) + return -ENOMEM; + + sl28cpld->regmap = devm_regmap_init_i2c(i2c, &sl28cpld_regmap_config); + if (IS_ERR(sl28cpld->regmap)) + return PTR_ERR(sl28cpld->regmap); + + ret = regmap_read(sl28cpld->regmap, SL28CPLD_VERSION, &cpld_version); + if (ret) + return ret; + + if (cpld_version < SL28CPLD_MIN_REQ_VERSION) { + dev_err(dev, "unsupported CPLD version %d\n", cpld_version); + return -ENODEV; + } + + sl28cpld->dev = dev; + i2c_set_clientdata(i2c, sl28cpld); + + dev_info(dev, "successfully probed. CPLD version %d\n", cpld_version); + + return devm_of_platform_populate(&i2c->dev); +} + +static const struct of_device_id sl28cpld_of_match[] = { + { .compatible = "kontron,sl28cpld-r1", }, + {} +}; +MODULE_DEVICE_TABLE(of, sl28cpld_of_match); + +static struct i2c_driver sl28cpld_driver = { + .probe_new = sl28cpld_probe, + .driver = { + .name = "sl28cpld", + .of_match_table = of_match_ptr(sl28cpld_of_match), + }, +}; +module_i2c_driver(sl28cpld_driver); + +MODULE_DESCRIPTION("sl28cpld MFD Core Driver"); +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); +MODULE_LICENSE("GPL");
Add the core support for the board management controller found on the SMARC-sAL28 board. It consists of the following functions: - watchdog - GPIO controller - PWM controller - fan sensor - interrupt controller At the moment, this controller is used on the Kontron SMARC-sAL28 board. Please note that the MFD driver is defined as bool in the Kconfig because the next patch will add interrupt support. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/mfd/Kconfig | 19 ++++++++++ drivers/mfd/Makefile | 2 ++ drivers/mfd/sl28cpld.c | 79 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+) create mode 100644 drivers/mfd/sl28cpld.c