| Message ID | 20200423174543.17161-6-michael@walle.cc (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: > This patch adds 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 ... > obj-$(CONFIG_MFD_STMFX) += stmfx.o > > obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o > + > +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o Perhaps keep an order? ... > + return devm_mfd_add_devices(dev, -1, sl28cpld_devs, -1 has its own definition. > + ARRAY_SIZE(sl28cpld_devs), NULL, > + i2c->irq, NULL); > +}
Am 2020-04-28 14:50, schrieb Andy Shevchenko: > On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: >> This patch adds 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 > > ... > >> obj-$(CONFIG_MFD_STMFX) += stmfx.o >> >> obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o >> + >> +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o > > Perhaps keep an order? I don't see any order in that makefile. Looked to me like every new file was added at the end. > > ... > >> + return devm_mfd_add_devices(dev, -1, sl28cpld_devs, > > -1 has its own definition. ok, I'll replace that by PLATFORM_DEVID_NONE. Thanks, -michael > >> + ARRAY_SIZE(sl28cpld_devs), NULL, >> + i2c->irq, NULL); >> +}
On Tue, Apr 28, 2020 at 04:43:24PM +0200, Michael Walle wrote: > Am 2020-04-28 14:50, schrieb Andy Shevchenko: > > On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: > > > This patch adds 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 > > > > ... > > > > > obj-$(CONFIG_MFD_STMFX) += stmfx.o > > > > > > obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o > > > + > > > +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o > > > > Perhaps keep an order? > > I don't see any order in that makefile. Looked to me like every new > file was added at the end. Okay, just didn't note from above context.
On Tue, 28 Apr 2020, Andy Shevchenko wrote: > On Tue, Apr 28, 2020 at 04:43:24PM +0200, Michael Walle wrote: > > Am 2020-04-28 14:50, schrieb Andy Shevchenko: > > > On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: > > > > This patch adds 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 > > > > > > ... > > > > > > > obj-$(CONFIG_MFD_STMFX) += stmfx.o > > > > > > > > obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o > > > > + > > > > +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o > > > > > > Perhaps keep an order? > > > > I don't see any order in that makefile. Looked to me like every new > > file was added at the end. > > Okay, just didn't note from above context. Yes, this is historical. I've been meaning to visit this for ~7 years!
On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: > This patch adds 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 | 153 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 174 insertions(+) > create mode 100644 drivers/mfd/sl28cpld.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 0a59249198d3..be0c8d93c526 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -2060,5 +2060,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 f935d10cbf0f..9bc38863b9c7 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -259,3 +259,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..1e5860cc7ffc > --- /dev/null > +++ b/drivers/mfd/sl28cpld.c > @@ -0,0 +1,153 @@ > +// 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_WATCHDOG_BASE 0x04 > +#define SL28CPLD_HWMON_FAN_BASE 0x0b > +#define SL28CPLD_PWM0_BASE 0x0c > +#define SL28CPLD_PWM1_BASE 0x0e > +#define SL28CPLD_GPIO0_BASE 0x10 > +#define SL28CPLD_GPIO1_BASE 0x15 > +#define SL28CPLD_GPO_BASE 0x1a > +#define SL28CPLD_GPI_BASE 0x1b > +#define SL28CPLD_INTC_BASE 0x1c If you want to use 'reg' in the binding, these are the numbers you should be using rather than making up numbering! However, I still don't think you need any child nodes. All the data in the DT binding is right here in the driver already. There's no advantage to putting child nodes in DT, because this driver still has to be updated if you add more nodes. Rob
Am 2020-05-11 23:13, schrieb Rob Herring: > On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: >> This patch adds 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 | 153 >> +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 174 insertions(+) >> create mode 100644 drivers/mfd/sl28cpld.c >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index 0a59249198d3..be0c8d93c526 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -2060,5 +2060,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 f935d10cbf0f..9bc38863b9c7 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -259,3 +259,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..1e5860cc7ffc >> --- /dev/null >> +++ b/drivers/mfd/sl28cpld.c >> @@ -0,0 +1,153 @@ >> +// 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_WATCHDOG_BASE 0x04 >> +#define SL28CPLD_HWMON_FAN_BASE 0x0b >> +#define SL28CPLD_PWM0_BASE 0x0c >> +#define SL28CPLD_PWM1_BASE 0x0e >> +#define SL28CPLD_GPIO0_BASE 0x10 >> +#define SL28CPLD_GPIO1_BASE 0x15 >> +#define SL28CPLD_GPO_BASE 0x1a >> +#define SL28CPLD_GPI_BASE 0x1b >> +#define SL28CPLD_INTC_BASE 0x1c > > If you want to use 'reg' in the binding, these are the numbers you > should be using rather than making up numbering! My motivation is that I don't want to hardcode the internal addresses of the management controller in the device tree. For example if they will move around with a later update of the controller, so a driver can be compatible with both the old and the new version. If they are in the device tree, only one register layout is possible. > However, I still don't think you need any child nodes. All the data in > the DT binding is right here in the driver already. There's no > advantage > to putting child nodes in DT, because this driver still has to be > updated if you add more nodes. But then any phandle will reference the mfd device. And for example there are two different interrupt controllers, that is the INTC and the GPIO[01], which will then be combined into one device tree node, right? So the mfd node would be cpld: sl28cpld@4a { interrupt-controller; #interrupt-cells = <2>; gpio-controller; #gpio-cells = <2>; [..] }; and then depending on the mapping one could use: interrupts-extended = <&cpld 0 FLAGS>; /* gpio0 line 0 */ interrupts-extended = <&cpld 8 FLAGS>; /* gpio1 line 0 */ interrupts-extended = <&cpld 12 FLAGS>; /* irq0 */ gpios = <&cpld 0> /* gpio0 line 0 */ But there is also offset 12, but then it is the GPI controller: gpios = <&cpld 12> /* gpi line 0, nothing to do with irq0 */ I don't know if this is good practice, I guess you have to tell me. And is it possible to combine any sub device into the mfd node in that way? -michael
Am 2020-05-11 23:44, schrieb Michael Walle: > Am 2020-05-11 23:13, schrieb Rob Herring: >> On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: >>> This patch adds 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 | 153 >>> +++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 174 insertions(+) >>> create mode 100644 drivers/mfd/sl28cpld.c >>> >>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>> index 0a59249198d3..be0c8d93c526 100644 >>> --- a/drivers/mfd/Kconfig >>> +++ b/drivers/mfd/Kconfig >>> @@ -2060,5 +2060,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 f935d10cbf0f..9bc38863b9c7 100644 >>> --- a/drivers/mfd/Makefile >>> +++ b/drivers/mfd/Makefile >>> @@ -259,3 +259,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..1e5860cc7ffc >>> --- /dev/null >>> +++ b/drivers/mfd/sl28cpld.c >>> @@ -0,0 +1,153 @@ >>> +// 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_WATCHDOG_BASE 0x04 >>> +#define SL28CPLD_HWMON_FAN_BASE 0x0b >>> +#define SL28CPLD_PWM0_BASE 0x0c >>> +#define SL28CPLD_PWM1_BASE 0x0e >>> +#define SL28CPLD_GPIO0_BASE 0x10 >>> +#define SL28CPLD_GPIO1_BASE 0x15 >>> +#define SL28CPLD_GPO_BASE 0x1a >>> +#define SL28CPLD_GPI_BASE 0x1b >>> +#define SL28CPLD_INTC_BASE 0x1c >> >> If you want to use 'reg' in the binding, these are the numbers you >> should be using rather than making up numbering! > > My motivation is that I don't want to hardcode the internal addresses > of the management controller in the device tree. For example if they > will move around with a later update of the controller, so a driver can > be compatible with both the old and the new version. If they are in the > device tree, only one register layout is possible. > >> However, I still don't think you need any child nodes. All the data in >> the DT binding is right here in the driver already. There's no >> advantage >> to putting child nodes in DT, because this driver still has to be >> updated if you add more nodes. > > But then any phandle will reference the mfd device. And for example > there > are two different interrupt controllers, that is the INTC and the > GPIO[01], > which will then be combined into one device tree node, right? > > So the mfd node would be > > cpld: sl28cpld@4a { > interrupt-controller; > #interrupt-cells = <2>; > gpio-controller; > #gpio-cells = <2>; > [..] > }; > > and then depending on the mapping one could use: > > interrupts-extended = <&cpld 0 FLAGS>; /* gpio0 line 0 */ > interrupts-extended = <&cpld 8 FLAGS>; /* gpio1 line 0 */ > interrupts-extended = <&cpld 12 FLAGS>; /* irq0 */ > > gpios = <&cpld 0> /* gpio0 line 0 */ > > But there is also offset 12, but then it is the GPI controller: > > gpios = <&cpld 12> /* gpi line 0, nothing to do with irq0 */ > > I don't know if this is good practice, I guess you have to tell me. And > is it possible to combine any sub device into the mfd node in that way? Oh I don't think that will work for the watchdog. If you just have one watchdog it just looks odd. cpld: sl28cpld@4a { interrupt-controller; #interrupt-cells = <2>; gpio-controller; #gpio-cells = <2>; timeout-sec = <10>; /* watchdog property */ }; And won't pass the dtbindings check because the nodename is not "watchdog(@[0-9]+)?". But it really falls short if you want to have two watchdogs with different properties. -michael
On Mon, May 11, 2020 at 4:45 PM Michael Walle <michael@walle.cc> wrote: > > Am 2020-05-11 23:13, schrieb Rob Herring: > > On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: > >> This patch adds 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 | 153 > >> +++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 174 insertions(+) > >> create mode 100644 drivers/mfd/sl28cpld.c > >> > >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >> index 0a59249198d3..be0c8d93c526 100644 > >> --- a/drivers/mfd/Kconfig > >> +++ b/drivers/mfd/Kconfig > >> @@ -2060,5 +2060,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 f935d10cbf0f..9bc38863b9c7 100644 > >> --- a/drivers/mfd/Makefile > >> +++ b/drivers/mfd/Makefile > >> @@ -259,3 +259,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..1e5860cc7ffc > >> --- /dev/null > >> +++ b/drivers/mfd/sl28cpld.c > >> @@ -0,0 +1,153 @@ > >> +// 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_WATCHDOG_BASE 0x04 > >> +#define SL28CPLD_HWMON_FAN_BASE 0x0b > >> +#define SL28CPLD_PWM0_BASE 0x0c > >> +#define SL28CPLD_PWM1_BASE 0x0e > >> +#define SL28CPLD_GPIO0_BASE 0x10 > >> +#define SL28CPLD_GPIO1_BASE 0x15 > >> +#define SL28CPLD_GPO_BASE 0x1a > >> +#define SL28CPLD_GPI_BASE 0x1b > >> +#define SL28CPLD_INTC_BASE 0x1c > > > > If you want to use 'reg' in the binding, these are the numbers you > > should be using rather than making up numbering! > > My motivation is that I don't want to hardcode the internal addresses > of the management controller in the device tree. For example if they > will move around with a later update of the controller, so a driver can > be compatible with both the old and the new version. If they are in the > device tree, only one register layout is possible. I don't understand, if the addresses change, then the above defines have to change. So your driver is only compatible with 1 version. If you change the CPLD, then that's a h/w change and your h/w description (DT) should change. That can either be the compatible string changing and updating the driver with new match data such as register offsets or all the differences are in DT and there's no kernel change. > > However, I still don't think you need any child nodes. All the data in > > the DT binding is right here in the driver already. There's no > > advantage > > to putting child nodes in DT, because this driver still has to be > > updated if you add more nodes. > > But then any phandle will reference the mfd device. And for example > there > are two different interrupt controllers, that is the INTC and the > GPIO[01], > which will then be combined into one device tree node, right? You either have to add a cell for 'bank' or divide the 1st cell into a bank and index. Both have been done before. To go the other direction, AIUI you shouldn't need OF_MFD_CELL_REG entries if you have the child devices in DT. Pick one way or the other. It's ultimately a judgement call. For a one-off device, sub devices in DT doesn't really buy you anything. If you have sub-blocks showing up multiple devices, then sub devices makes sense. If there's only 2-3 combinations, then it's a toss up. Rob
Am 2020-05-12 23:59, schrieb Rob Herring: > On Mon, May 11, 2020 at 4:45 PM Michael Walle <michael@walle.cc> wrote: >> >> Am 2020-05-11 23:13, schrieb Rob Herring: >> > On Thu, Apr 23, 2020 at 07:45:32PM +0200, Michael Walle wrote: >> >> +#define SL28CPLD_VERSION 0x03 >> >> +#define SL28CPLD_WATCHDOG_BASE 0x04 >> >> +#define SL28CPLD_HWMON_FAN_BASE 0x0b >> >> +#define SL28CPLD_PWM0_BASE 0x0c >> >> +#define SL28CPLD_PWM1_BASE 0x0e >> >> +#define SL28CPLD_GPIO0_BASE 0x10 >> >> +#define SL28CPLD_GPIO1_BASE 0x15 >> >> +#define SL28CPLD_GPO_BASE 0x1a >> >> +#define SL28CPLD_GPI_BASE 0x1b >> >> +#define SL28CPLD_INTC_BASE 0x1c >> > >> > If you want to use 'reg' in the binding, these are the numbers you >> > should be using rather than making up numbering! >> >> My motivation is that I don't want to hardcode the internal addresses >> of the management controller in the device tree. For example if they >> will move around with a later update of the controller, so a driver >> can >> be compatible with both the old and the new version. If they are in >> the >> device tree, only one register layout is possible. > > I don't understand, if the addresses change, then the above defines > have to change. So your driver is only compatible with 1 version. If > you change the CPLD, then that's a h/w change and your h/w description > (DT) should change. That can either be the compatible string changing > and updating the driver with new match data such as register offsets > or all the differences are in DT and there's no kernel change. The CPLD and the board is designed in a way that it is possible to update and/or change its function (or parts of it). It must not be a hardware change, although I admit thats a bit of a grey area wether you treat it as hardware or "firmware". Anyway, yes you'd have to change the register offsets, but as this is code it might support different register offsets. For example you could dynamically add functionality, if there is a newer controller version while still being compatible with older versions. >> > However, I still don't think you need any child nodes. All the data in >> > the DT binding is right here in the driver already. There's no >> > advantage >> > to putting child nodes in DT, because this driver still has to be >> > updated if you add more nodes. >> >> But then any phandle will reference the mfd device. And for example >> there >> are two different interrupt controllers, that is the INTC and the >> GPIO[01], >> which will then be combined into one device tree node, right? > > You either have to add a cell for 'bank' or divide the 1st cell into a > bank and index. Both have been done before. But this won't work with watchdogs, correct? See https://lore.kernel.org/linux-devicetree/7acbb6d9b2240b1856136fa35c1318bf@walle.cc/ > To go the other direction, AIUI you shouldn't need OF_MFD_CELL_REG > entries if you have the child devices in DT. This is a general problem IMHO. There are mfd drivers which have mfd cells and a device tree node associated with each cell. But it just works as long as there is only one sub device per unique compatible string. So you cannot have multiple mfd cells with the same compatible string. That being said, I can try to reimplement it using of_platform_populate() and its internal offset as its unit address. > Pick one way or the > other. It's ultimately a judgement call. For a one-off device, sub > devices in DT doesn't really buy you anything. If you have sub-blocks > showing up multiple devices, then sub devices makes sense. If there's > only 2-3 combinations, then it's a toss up. -michael
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 0a59249198d3..be0c8d93c526 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -2060,5 +2060,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 f935d10cbf0f..9bc38863b9c7 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -259,3 +259,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..1e5860cc7ffc --- /dev/null +++ b/drivers/mfd/sl28cpld.c @@ -0,0 +1,153 @@ +// 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_WATCHDOG_BASE 0x04 +#define SL28CPLD_HWMON_FAN_BASE 0x0b +#define SL28CPLD_PWM0_BASE 0x0c +#define SL28CPLD_PWM1_BASE 0x0e +#define SL28CPLD_GPIO0_BASE 0x10 +#define SL28CPLD_GPIO1_BASE 0x15 +#define SL28CPLD_GPO_BASE 0x1a +#define SL28CPLD_GPI_BASE 0x1b +#define SL28CPLD_INTC_BASE 0x1c + +/* all subdevices share the same IRQ */ +#define SL28CPLD_IRQ 0 + +#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 struct resource sl28cpld_watchdog_resources[] = { + DEFINE_RES_REG(SL28CPLD_WATCHDOG_BASE, 1), +}; + +static struct resource sl28cpld_hwmon_fan_resources[] = { + DEFINE_RES_REG(SL28CPLD_HWMON_FAN_BASE, 1), +}; + +static struct resource sl28cpld_pwm0_resources[] = { + DEFINE_RES_REG(SL28CPLD_PWM0_BASE, 1), +}; + +static struct resource sl28cpld_pwm1_resources[] = { + DEFINE_RES_REG(SL28CPLD_PWM1_BASE, 1), +}; + +static struct resource sl28cpld_gpio0_resources[] = { + DEFINE_RES_REG(SL28CPLD_GPIO0_BASE, 1), + DEFINE_RES_IRQ(SL28CPLD_IRQ), +}; + +static struct resource sl28cpld_gpio1_resources[] = { + DEFINE_RES_REG(SL28CPLD_GPIO1_BASE, 1), + DEFINE_RES_IRQ(SL28CPLD_IRQ), +}; + +static struct resource sl28cpld_gpo_resources[] = { + DEFINE_RES_REG(SL28CPLD_GPO_BASE, 1), +}; + +static struct resource sl28cpld_gpi_resources[] = { + DEFINE_RES_REG(SL28CPLD_GPI_BASE, 1), +}; + +static struct resource sl28cpld_intc_resources[] = { + DEFINE_RES_REG(SL28CPLD_INTC_BASE, 1), + DEFINE_RES_IRQ(SL28CPLD_IRQ), +}; + +static const struct mfd_cell sl28cpld_devs[] = { + OF_MFD_CELL_REG("sl28cpld-wdt", sl28cpld_watchdog_resources, + NULL, 0, 0, "kontron,sl28cpld-wdt", 0), + OF_MFD_CELL_REG("sl28cpld-fan", sl28cpld_hwmon_fan_resources, + NULL, 0, 0, "kontron,sl28cpld-fan", 1), + OF_MFD_CELL_REG("sl28cpld-pwm", sl28cpld_pwm0_resources, + NULL, 0, 0, "kontron,sl28cpld-pwm", 2), + OF_MFD_CELL_REG("sl28cpld-pwm", sl28cpld_pwm1_resources, + NULL, 0, 1, "kontron,sl28cpld-pwm", 3), + OF_MFD_CELL_REG("sl28cpld-gpio", sl28cpld_gpio0_resources, + NULL, 0, 0, "kontron,sl28cpld-gpio", 4), + OF_MFD_CELL_REG("sl28cpld-gpio", sl28cpld_gpio1_resources, + NULL, 0, 1, "kontron,sl28cpld-gpio", 5), + OF_MFD_CELL_REG("sl28cpld-gpo", sl28cpld_gpo_resources, + NULL, 0, 0, "kontron,sl28cpld-gpo", 6), + OF_MFD_CELL_REG("sl28cpld-gpi", sl28cpld_gpi_resources, + NULL, 0, 0, "kontron,sl28cpld-gpi", 7), + MFD_CELL_RES("sl28cpld-intc", sl28cpld_intc_resources), +}; + +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_mfd_add_devices(dev, -1, sl28cpld_devs, + ARRAY_SIZE(sl28cpld_devs), NULL, + i2c->irq, NULL); +} + +static const struct of_device_id sl28cpld_of_match[] = { + { .compatible = "kontron,sl28cpld", }, + {} +}; +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");
This patch adds 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 | 153 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+) create mode 100644 drivers/mfd/sl28cpld.c