| Message ID | 20230301170215.23382-3-henning.schild@siemens.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: simatic-ipc-leds-gpio: split up | expand |
On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote: > In order to clearly describe the dependencies between the GPIO > controller drivers and the users the driver is split up into a core, > two drivers and a common header. ... > .../simple/simatic-ipc-leds-gpio-apollolake.c | 64 +++++++ > .../leds/simple/simatic-ipc-leds-gpio-core.c | 103 ++++++++++++ > .../simple/simatic-ipc-leds-gpio-f7188x.c | 64 +++++++ > drivers/leds/simple/simatic-ipc-leds-gpio.c | 159 ------------------ I'm wondering if you have used -M -C when creating this patch. ... > +#include <linux/gpio/machine.h> > +#include <linux/gpio/consumer.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/platform_data/x86/simatic-ipc-base.h> + Blank line? > +#include "simatic-ipc-leds-gpio.h" ... > + .table = { > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 0, GPIO_ACTIVE_LOW), > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 1, GPIO_ACTIVE_LOW), > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 2, GPIO_ACTIVE_LOW), > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 3, GPIO_ACTIVE_LOW), > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 4, GPIO_ACTIVE_LOW), > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 5, GPIO_ACTIVE_LOW), > + }, Shouldn't this have the terminator entry? ... > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra = { > + .dev_id = NULL, As per previous patch. > + .table = { > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW), > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH), > + }, As per above. > +}; ... > + .driver = { > + .name = KBUILD_MODNAME, Strictly speaking this is an ABI (as something may instantiate the driver from the user space or elsewhere by this name. At the same time this may change with the file name change. Personally I prefer explicit string literal here. > + }, ... > + Redundant blank line. > +module_platform_driver(simatic_ipc_led_gpio_apollolake_driver); ... > +MODULE_ALIAS("platform:" KBUILD_MODNAME); Why? HAve you missed MODULE_DEVICE_TABLE()? ... > +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c Similar comments as per above.
Am Wed, 1 Mar 2023 19:28:12 +0200 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote: > > In order to clearly describe the dependencies between the GPIO > > controller drivers and the users the driver is split up into a core, > > two drivers and a common header. > > ... > > > .../simple/simatic-ipc-leds-gpio-apollolake.c | 64 +++++++ > > .../leds/simple/simatic-ipc-leds-gpio-core.c | 103 ++++++++++++ > > .../simple/simatic-ipc-leds-gpio-f7188x.c | 64 +++++++ > > drivers/leds/simple/simatic-ipc-leds-gpio.c | 159 > > ------------------ > > I'm wondering if you have used -M -C when creating this patch. > > ... > > > +#include <linux/gpio/machine.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/leds.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/platform_data/x86/simatic-ipc-base.h> > > + Blank line? > > > +#include "simatic-ipc-leds-gpio.h" > > ... > > > + .table = { > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, > > 0, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, > > 1, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, > > 2, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, > > 3, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, > > 4, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, > > 5, GPIO_ACTIVE_LOW), > > + }, > > Shouldn't this have the terminator entry? > > ... > > > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra > > = { > > + .dev_id = NULL, > > As per previous patch. > > > + .table = { > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, > > 6, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, > > 7, GPIO_ACTIVE_HIGH), > > + }, > > As per above. > > > +}; > > ... > > > + .driver = { > > + .name = KBUILD_MODNAME, > > Strictly speaking this is an ABI (as something may instantiate the > driver from the user space or elsewhere by this name. At the same > time this may change with the file name change. > > Personally I prefer explicit string literal here. Switching from one module to three the names have to change. People who explicitly loaded the old module which supported multiple machines, will now how to load either both oŕ know which one to load. I personally think the ABI change is acceptable, the assumption would be that the drivers load automatically anyhow. And since there are no params i doubt users will have /etc/modprobe.d/ or /sys/module/ stuff around. And with the split i guess an ABI change can not be fully avoided. Whether the names is explicit or implicit is another discussion and just a matter of style. I prefer to stay with the currently used pattern, it is not un-common in the kernel. > > + }, > > ... > > > + > > Redundant blank line. > > > +module_platform_driver(simatic_ipc_led_gpio_apollolake_driver); > > ... > > > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > Why? HAve you missed MODULE_DEVICE_TABLE()? I do not know what that is, but i will have a look. Henning > ... > > > +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c > > Similar comments as per above. >
On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote: > Am Wed, 1 Mar 2023 19:28:12 +0200 > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote: ... > > > + .driver = { > > > + .name = KBUILD_MODNAME, > > > > Strictly speaking this is an ABI (as something may instantiate the > > driver from the user space or elsewhere by this name. At the same > > time this may change with the file name change. > > > > Personally I prefer explicit string literal here. > > Switching from one module to three the names have to change. People who > explicitly loaded the old module which supported multiple machines, > will now how to load either both oŕ know which one to load. Wait, are you telling that now users load modules _manually_?! > I personally think the ABI change is acceptable, the assumption would > be that the drivers load automatically anyhow. And since there are no > params i doubt users will have /etc/modprobe.d/ or /sys/module/ stuff > around. > > And with the split i guess an ABI change can not be fully avoided. > Whether the names is explicit or implicit is another discussion and > just a matter of style. I prefer to stay with the currently used > pattern, it is not un-common in the kernel. The problem with that pattern is possible, while unlikely, renaming of the file which triggers this to be updated. Under sysfs the folder will change its name. If user has a script relying on this, it will be broken. So, I prefer mine. > > > + },
Am Thu, 2 Mar 2023 17:46:54 +0200 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote: > > Am Wed, 1 Mar 2023 19:28:12 +0200 > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote: > > ... > > > > > + .driver = { > > > > + .name = KBUILD_MODNAME, > > > > > > Strictly speaking this is an ABI (as something may instantiate the > > > driver from the user space or elsewhere by this name. At the same > > > time this may change with the file name change. > > > > > > Personally I prefer explicit string literal here. > > > > Switching from one module to three the names have to change. People > > who explicitly loaded the old module which supported multiple > > machines, will now how to load either both oŕ know which one to > > load. > > Wait, are you telling that now users load modules _manually_?! No i am not, the modules all load automatically. I was trying to construct a hypothetical case where the name change could affect users doing unexpected things. > > I personally think the ABI change is acceptable, the assumption > > would be that the drivers load automatically anyhow. And since > > there are no params i doubt users will have /etc/modprobe.d/ or > > /sys/module/ stuff around. > > > > And with the split i guess an ABI change can not be fully avoided. > > Whether the names is explicit or implicit is another discussion and > > just a matter of style. I prefer to stay with the currently used > > pattern, it is not un-common in the kernel. > > The problem with that pattern is possible, while unlikely, renaming > of the file which triggers this to be updated. > > Under sysfs the folder will change its name. If user has a script > relying on this, it will be broken. So, I prefer mine. Yes, the name of the module will change ... and the location of module metadata and params in sysfs, both not a big deal here. Because there are no params, and there is not need to modprobe manually. Henning > > > > + }, >
On Thu, Mar 02, 2023 at 04:58:24PM +0100, Henning Schild wrote: > Am Thu, 2 Mar 2023 17:46:54 +0200 > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote: > > > Am Wed, 1 Mar 2023 19:28:12 +0200 > > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > > > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote: ... > > > > > + .driver = { > > > > > + .name = KBUILD_MODNAME, > > > > > > > > Strictly speaking this is an ABI (as something may instantiate the > > > > driver from the user space or elsewhere by this name. At the same > > > > time this may change with the file name change. > > > > > > > > Personally I prefer explicit string literal here. > > > > > > Switching from one module to three the names have to change. People > > > who explicitly loaded the old module which supported multiple > > > machines, will now how to load either both oŕ know which one to > > > load. > > > > Wait, are you telling that now users load modules _manually_?! > > No i am not, the modules all load automatically. I was trying to > construct a hypothetical case where the name change could affect users > doing unexpected things. > > > > I personally think the ABI change is acceptable, the assumption > > > would be that the drivers load automatically anyhow. And since > > > there are no params i doubt users will have /etc/modprobe.d/ or > > > /sys/module/ stuff around. > > > > > > And with the split i guess an ABI change can not be fully avoided. > > > Whether the names is explicit or implicit is another discussion and > > > just a matter of style. I prefer to stay with the currently used > > > pattern, it is not un-common in the kernel. > > > > The problem with that pattern is possible, while unlikely, renaming > > of the file which triggers this to be updated. > > > > Under sysfs the folder will change its name. If user has a script > > relying on this, it will be broken. So, I prefer mine. > > Yes, the name of the module will change ... and the location of module > metadata and params in sysfs, both not a big deal here. Because there > are no params, and there is not need to modprobe manually. It's not only one folder AFAIU. Also folder in the drivers will change its name. Parameters is one thing, the folder presence is another. Yes, the case is quite unlikely to happen (to break anyone's setup) that's why I started this with 'Strictly speaking...'. So, seems you are staying on your side, I will leave this to maintainers. If they are fine, I will have no objections. > > > > > + },
Am Thu, 2 Mar 2023 18:21:21 +0200 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Thu, Mar 02, 2023 at 04:58:24PM +0100, Henning Schild wrote: > > Am Thu, 2 Mar 2023 17:46:54 +0200 > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > > On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote: > > > > Am Wed, 1 Mar 2023 19:28:12 +0200 > > > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > > > > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild > > > > > wrote: > > ... > > > > > > > + .driver = { > > > > > > + .name = KBUILD_MODNAME, > > > > > > > > > > Strictly speaking this is an ABI (as something may > > > > > instantiate the driver from the user space or elsewhere by > > > > > this name. At the same time this may change with the file > > > > > name change. > > > > > > > > > > Personally I prefer explicit string literal here. > > > > > > > > Switching from one module to three the names have to change. > > > > People who explicitly loaded the old module which supported > > > > multiple machines, will now how to load either both oŕ know > > > > which one to load. > > > > > > Wait, are you telling that now users load modules _manually_?! > > > > No i am not, the modules all load automatically. I was trying to > > construct a hypothetical case where the name change could affect > > users doing unexpected things. > > > > > > I personally think the ABI change is acceptable, the assumption > > > > would be that the drivers load automatically anyhow. And since > > > > there are no params i doubt users will have /etc/modprobe.d/ or > > > > /sys/module/ stuff around. > > > > > > > > And with the split i guess an ABI change can not be fully > > > > avoided. Whether the names is explicit or implicit is another > > > > discussion and just a matter of style. I prefer to stay with > > > > the currently used pattern, it is not un-common in the kernel. > > > > > > > > > > The problem with that pattern is possible, while unlikely, > > > renaming of the file which triggers this to be updated. > > > > > > Under sysfs the folder will change its name. If user has a script > > > relying on this, it will be broken. So, I prefer mine. > > > > Yes, the name of the module will change ... and the location of > > module metadata and params in sysfs, both not a big deal here. > > Because there are no params, and there is not need to modprobe > > manually. > > It's not only one folder AFAIU. Also folder in the drivers will change > its name. Parameters is one thing, the folder presence is another. > > Yes, the case is quite unlikely to happen (to break anyone's setup) > that's why I started this with 'Strictly speaking...'. So, seems you > are staying on your side, I will leave this to maintainers. If they > are fine, I will have no objections. We are splitting one module into three, so we will end up with three names. Or i miss something here. The only thing one could talk about is whether that string should be hardcoded or derived from the name of the c-file. Anyhow, thanks and we should probably just wait what others have to say. Henning > > > > > > > + }, >
Am Wed, 1 Mar 2023 19:28:12 +0200 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote: > > In order to clearly describe the dependencies between the GPIO > > controller drivers and the users the driver is split up into a core, > > two drivers and a common header. > > ... > > > .../simple/simatic-ipc-leds-gpio-apollolake.c | 64 +++++++ > > .../leds/simple/simatic-ipc-leds-gpio-core.c | 103 ++++++++++++ > > .../simple/simatic-ipc-leds-gpio-f7188x.c | 64 +++++++ > > drivers/leds/simple/simatic-ipc-leds-gpio.c | 159 > > ------------------ > > I'm wondering if you have used -M -C when creating this patch. > > ... > > > +#include <linux/gpio/machine.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/leds.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/platform_data/x86/simatic-ipc-base.h> > > + Blank line? Will be added in 3 files in v3. > > +#include "simatic-ipc-leds-gpio.h" > > ... > > > + .table = { > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, > > 0, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, > > 1, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, > > 2, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, > > 3, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, > > 4, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, > > 5, GPIO_ACTIVE_LOW), > > + }, > > Shouldn't this have the terminator entry? > > ... > > > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra > > = { > > + .dev_id = NULL, > > As per previous patch. comment will be added in p1 and p2 and be in v3. > > + .table = { > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, > > 6, GPIO_ACTIVE_LOW), > > + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, > > 7, GPIO_ACTIVE_HIGH), > > + }, > > As per above. > > > +}; > > ... > > > + .driver = { > > + .name = KBUILD_MODNAME, > > Strictly speaking this is an ABI (as something may instantiate the > driver from the user space or elsewhere by this name. At the same > time this may change with the file name change. > > Personally I prefer explicit string literal here. > > > + }, > > ... > > > + > > Redundant blank line. Will be removed in v3 in 3 files. Henning > > +module_platform_driver(simatic_ipc_led_gpio_apollolake_driver); > > ... > > > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > Why? HAve you missed MODULE_DEVICE_TABLE()? > > ... > > > +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c > > Similar comments as per above. >
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile index 1c7ef5e1324b..ed9057f7b6da 100644 --- a/drivers/leds/simple/Makefile +++ b/drivers/leds/simple/Makefile @@ -1,3 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o -obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio-core.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio-apollolake.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio-f7188x.o diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c b/drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c new file mode 100644 index 000000000000..8b874cdb38a1 --- /dev/null +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Siemens SIMATIC IPC driver for GPIO based LEDs + * + * Copyright (c) Siemens AG, 2023 + * + * Author: + * Henning Schild <henning.schild@siemens.com> + */ + +#include <linux/gpio/machine.h> +#include <linux/gpio/consumer.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/platform_data/x86/simatic-ipc-base.h> +#include "simatic-ipc-leds-gpio.h" + +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = { + .dev_id = "leds-gpio", + .table = { + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 0, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 1, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 2, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 3, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 4, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 5, GPIO_ACTIVE_LOW), + }, +}; + +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra = { + .dev_id = NULL, + .table = { + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH), + }, +}; + +static int simatic_ipc_leds_gpio_apollolake_probe(struct platform_device *pdev) +{ + return simatic_ipc_leds_gpio_probe(pdev, &simatic_ipc_led_gpio_table, + &simatic_ipc_led_gpio_table_extra); +} + +static int simatic_ipc_leds_gpio_apollolake_remove(struct platform_device *pdev) +{ + return simatic_ipc_leds_gpio_remove(pdev, &simatic_ipc_led_gpio_table, + &simatic_ipc_led_gpio_table_extra); +} + +static struct platform_driver simatic_ipc_led_gpio_apollolake_driver = { + .probe = simatic_ipc_leds_gpio_apollolake_probe, + .remove = simatic_ipc_leds_gpio_apollolake_remove, + .driver = { + .name = KBUILD_MODNAME, + }, +}; + +module_platform_driver(simatic_ipc_led_gpio_apollolake_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" KBUILD_MODNAME); +MODULE_SOFTDEP("pre: simatic-ipc-leds-gpio-core platform:apollolake-pinctrl"); +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>"); diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-core.c b/drivers/leds/simple/simatic-ipc-leds-gpio-core.c new file mode 100644 index 000000000000..ad5c9f8986d4 --- /dev/null +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-core.c @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Siemens SIMATIC IPC driver for GPIO based LEDs + * + * Copyright (c) Siemens AG, 2023 + * + * Author: + * Henning Schild <henning.schild@siemens.com> + */ + +#include <linux/gpio/machine.h> +#include <linux/gpio/consumer.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/platform_data/x86/simatic-ipc-base.h> +#include "simatic-ipc-leds-gpio.h" + +static struct platform_device *simatic_leds_pdev; + +static const struct gpio_led simatic_ipc_gpio_leds[] = { + { .name = "red:" LED_FUNCTION_STATUS "-1" }, + { .name = "green:" LED_FUNCTION_STATUS "-1" }, + { .name = "red:" LED_FUNCTION_STATUS "-2" }, + { .name = "green:" LED_FUNCTION_STATUS "-2" }, + { .name = "red:" LED_FUNCTION_STATUS "-3" }, + { .name = "green:" LED_FUNCTION_STATUS "-3" }, +}; + +static const struct gpio_led_platform_data simatic_ipc_gpio_leds_pdata = { + .num_leds = ARRAY_SIZE(simatic_ipc_gpio_leds), + .leds = simatic_ipc_gpio_leds, +}; + +int simatic_ipc_leds_gpio_remove(struct platform_device *pdev, + struct gpiod_lookup_table *table, + struct gpiod_lookup_table *table_extra) +{ + gpiod_remove_lookup_table(table); + gpiod_remove_lookup_table(table_extra); + platform_device_unregister(simatic_leds_pdev); + + return 0; +} +EXPORT_SYMBOL_GPL(simatic_ipc_leds_gpio_remove); + +int simatic_ipc_leds_gpio_probe(struct platform_device *pdev, + struct gpiod_lookup_table *table, + struct gpiod_lookup_table *table_extra) +{ + const struct simatic_ipc_platform *plat = pdev->dev.platform_data; + struct device *dev = &pdev->dev; + struct gpio_desc *gpiod; + int err; + + switch (plat->devmode) { + case SIMATIC_IPC_DEVICE_127E: + case SIMATIC_IPC_DEVICE_227G: + break; + default: + return -ENODEV; + } + + gpiod_add_lookup_table(table); + simatic_leds_pdev = platform_device_register_resndata(NULL, + "leds-gpio", PLATFORM_DEVID_NONE, NULL, 0, + &simatic_ipc_gpio_leds_pdata, + sizeof(simatic_ipc_gpio_leds_pdata)); + if (IS_ERR(simatic_leds_pdev)) { + err = PTR_ERR(simatic_leds_pdev); + goto out; + } + + table_extra->dev_id = dev_name(dev); + gpiod_add_lookup_table(table_extra); + + /* PM_BIOS_BOOT_N */ + gpiod = gpiod_get_index(dev, NULL, 6, GPIOD_OUT_LOW); + if (IS_ERR(gpiod)) { + err = PTR_ERR(gpiod); + goto out; + } + gpiod_put(gpiod); + + /* PM_WDT_OUT */ + gpiod = gpiod_get_index(dev, NULL, 7, GPIOD_OUT_LOW); + if (IS_ERR(gpiod)) { + err = PTR_ERR(gpiod); + goto out; + } + gpiod_put(gpiod); + + return 0; +out: + simatic_ipc_leds_gpio_remove(pdev, table, table_extra); + + return err; +} +EXPORT_SYMBOL_GPL(simatic_ipc_leds_gpio_probe); + +MODULE_LICENSE("GPL v2"); +MODULE_SOFTDEP("pre: platform:leds-gpio"); +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>"); diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c new file mode 100644 index 000000000000..d3dff1bffae3 --- /dev/null +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Siemens SIMATIC IPC driver for GPIO based LEDs + * + * Copyright (c) Siemens AG, 2023 + * + * Author: + * Henning Schild <henning.schild@siemens.com> + */ + +#include <linux/gpio/machine.h> +#include <linux/gpio/consumer.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/platform_data/x86/simatic-ipc-base.h> +#include "simatic-ipc-leds-gpio.h" + +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = { + .dev_id = "leds-gpio", + .table = { + GPIO_LOOKUP_IDX("gpio-f7188x-2", 0, NULL, 0, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 1, NULL, 1, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 2, NULL, 2, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 3, NULL, 3, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 4, NULL, 4, GPIO_ACTIVE_LOW), + GPIO_LOOKUP_IDX("gpio-f7188x-2", 5, NULL, 5, GPIO_ACTIVE_LOW), + }, +}; + +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra = { + .dev_id = NULL, + .table = { + GPIO_LOOKUP_IDX("gpio-f7188x-3", 6, NULL, 6, GPIO_ACTIVE_HIGH), + GPIO_LOOKUP_IDX("gpio-f7188x-3", 7, NULL, 7, GPIO_ACTIVE_HIGH), + }, +}; + +static int simatic_ipc_leds_gpio_f7188x_probe(struct platform_device *pdev) +{ + return simatic_ipc_leds_gpio_probe(pdev, &simatic_ipc_led_gpio_table, + &simatic_ipc_led_gpio_table_extra); +} + +static int simatic_ipc_leds_gpio_f7188x_remove(struct platform_device *pdev) +{ + return simatic_ipc_leds_gpio_remove(pdev, &simatic_ipc_led_gpio_table, + &simatic_ipc_led_gpio_table_extra); +} + +static struct platform_driver simatic_ipc_led_gpio_driver = { + .probe = simatic_ipc_leds_gpio_f7188x_probe, + .remove = simatic_ipc_leds_gpio_f7188x_remove, + .driver = { + .name = KBUILD_MODNAME, + }, +}; + +module_platform_driver(simatic_ipc_led_gpio_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" KBUILD_MODNAME); +MODULE_SOFTDEP("pre: simatic-ipc-leds-gpio-core gpio_f7188x"); +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>"); diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c deleted file mode 100644 index ba0f24638af5..000000000000 --- a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++ /dev/null @@ -1,159 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -/* - * Siemens SIMATIC IPC driver for GPIO based LEDs - * - * Copyright (c) Siemens AG, 2022 - * - * Authors: - * Henning Schild <henning.schild@siemens.com> - */ - -#include <linux/gpio/machine.h> -#include <linux/gpio/consumer.h> -#include <linux/leds.h> -#include <linux/module.h> -#include <linux/platform_device.h> -#include <linux/platform_data/x86/simatic-ipc-base.h> - -static struct gpiod_lookup_table *simatic_ipc_led_gpio_table; -static struct gpiod_lookup_table *simatic_ipc_led_gpio_table_extra; - -static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e = { - .dev_id = "leds-gpio", - .table = { - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 0, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 1, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 2, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 3, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 4, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 5, GPIO_ACTIVE_LOW), - }, -}; - -static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e_extra = { - .dev_id = NULL, - .table = { - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH), - }, -}; - -static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g = { - .dev_id = "leds-gpio", - .table = { - GPIO_LOOKUP_IDX("gpio-f7188x-2", 0, NULL, 0, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("gpio-f7188x-2", 1, NULL, 1, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("gpio-f7188x-2", 2, NULL, 2, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("gpio-f7188x-2", 3, NULL, 3, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("gpio-f7188x-2", 4, NULL, 4, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("gpio-f7188x-2", 5, NULL, 5, GPIO_ACTIVE_LOW), - }, -}; - -static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g_extra = { - .dev_id = NULL, - .table = { - GPIO_LOOKUP_IDX("gpio-f7188x-3", 6, NULL, 6, GPIO_ACTIVE_HIGH), - GPIO_LOOKUP_IDX("gpio-f7188x-3", 7, NULL, 7, GPIO_ACTIVE_HIGH), - }, -}; - -static const struct gpio_led simatic_ipc_gpio_leds[] = { - { .name = "red:" LED_FUNCTION_STATUS "-1" }, - { .name = "green:" LED_FUNCTION_STATUS "-1" }, - { .name = "red:" LED_FUNCTION_STATUS "-2" }, - { .name = "green:" LED_FUNCTION_STATUS "-2" }, - { .name = "red:" LED_FUNCTION_STATUS "-3" }, - { .name = "green:" LED_FUNCTION_STATUS "-3" }, -}; - -static const struct gpio_led_platform_data simatic_ipc_gpio_leds_pdata = { - .num_leds = ARRAY_SIZE(simatic_ipc_gpio_leds), - .leds = simatic_ipc_gpio_leds, -}; - -static struct platform_device *simatic_leds_pdev; - -static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev) -{ - gpiod_remove_lookup_table(simatic_ipc_led_gpio_table); - gpiod_remove_lookup_table(simatic_ipc_led_gpio_table_extra); - platform_device_unregister(simatic_leds_pdev); - - return 0; -} - -static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev) -{ - const struct simatic_ipc_platform *plat = pdev->dev.platform_data; - struct device *dev = &pdev->dev; - struct gpio_desc *gpiod; - int err; - - switch (plat->devmode) { - case SIMATIC_IPC_DEVICE_127E: - if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON)) - return -ENODEV; - simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_127e; - simatic_ipc_led_gpio_table_extra = &simatic_ipc_led_gpio_table_127e_extra; - break; - case SIMATIC_IPC_DEVICE_227G: - if (!IS_ENABLED(CONFIG_GPIO_F7188X)) - return -ENODEV; - request_module("gpio-f7188x"); - simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_227g; - simatic_ipc_led_gpio_table_extra = &simatic_ipc_led_gpio_table_227g_extra; - break; - default: - return -ENODEV; - } - - gpiod_add_lookup_table(simatic_ipc_led_gpio_table); - simatic_leds_pdev = platform_device_register_resndata(NULL, - "leds-gpio", PLATFORM_DEVID_NONE, NULL, 0, - &simatic_ipc_gpio_leds_pdata, - sizeof(simatic_ipc_gpio_leds_pdata)); - if (IS_ERR(simatic_leds_pdev)) { - err = PTR_ERR(simatic_leds_pdev); - goto out; - } - - simatic_ipc_led_gpio_table_extra->dev_id = dev_name(dev); - gpiod_add_lookup_table(simatic_ipc_led_gpio_table_extra); - - /* PM_BIOS_BOOT_N */ - gpiod = gpiod_get_index(dev, NULL, 6, GPIOD_OUT_LOW); - if (IS_ERR(gpiod)) { - err = PTR_ERR(gpiod); - goto out; - } - gpiod_put(gpiod); - - /* PM_WDT_OUT */ - gpiod = gpiod_get_index(dev, NULL, 7, GPIOD_OUT_LOW); - if (IS_ERR(gpiod)) { - err = PTR_ERR(gpiod); - goto out; - } - gpiod_put(gpiod); - - return 0; -out: - simatic_ipc_leds_gpio_remove(pdev); - - return err; -} - -static struct platform_driver simatic_ipc_led_gpio_driver = { - .probe = simatic_ipc_leds_gpio_probe, - .remove = simatic_ipc_leds_gpio_remove, - .driver = { - .name = KBUILD_MODNAME, - } -}; -module_platform_driver(simatic_ipc_led_gpio_driver); - -MODULE_LICENSE("GPL v2"); -MODULE_ALIAS("platform:" KBUILD_MODNAME); -MODULE_SOFTDEP("pre: platform:leds-gpio"); -MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>"); diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.h b/drivers/leds/simple/simatic-ipc-leds-gpio.h new file mode 100644 index 000000000000..bf258c32f83d --- /dev/null +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.h @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Siemens SIMATIC IPC driver for GPIO based LEDs + * + * Copyright (c) Siemens AG, 2023 + * + * Author: + * Henning Schild <henning.schild@siemens.com> + */ + +#ifndef _SIMATIC_IPC_LEDS_GPIO_H +#define _SIMATIC_IPC_LEDS_GPIO_H + +int simatic_ipc_leds_gpio_probe(struct platform_device *pdev, + struct gpiod_lookup_table *table, + struct gpiod_lookup_table *table_extra); + +int simatic_ipc_leds_gpio_remove(struct platform_device *pdev, + struct gpiod_lookup_table *table, + struct gpiod_lookup_table *table_extra); + +#endif /* _SIMATIC_IPC_LEDS_GPIO_H */ diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c index b3622419cd1a..c773995b230d 100644 --- a/drivers/platform/x86/simatic-ipc.c +++ b/drivers/platform/x86/simatic-ipc.c @@ -68,9 +68,10 @@ static int register_platform_devices(u32 station_id) } if (ledmode != SIMATIC_IPC_DEVICE_NONE) { - if (ledmode == SIMATIC_IPC_DEVICE_127E || - ledmode == SIMATIC_IPC_DEVICE_227G) - pdevname = KBUILD_MODNAME "_leds_gpio"; + if (ledmode == SIMATIC_IPC_DEVICE_127E) + pdevname = KBUILD_MODNAME "_leds_gpio_apollolake"; + if (ledmode == SIMATIC_IPC_DEVICE_227G) + pdevname = KBUILD_MODNAME "_leds_gpio_f7188x"; platform_data.devmode = ledmode; ipc_led_platform_device = platform_device_register_data(NULL,
In order to clearly describe the dependencies between the GPIO controller drivers and the users the driver is split up into a core, two drivers and a common header. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/leds/simple/Makefile | 4 +- .../simple/simatic-ipc-leds-gpio-apollolake.c | 64 +++++++ .../leds/simple/simatic-ipc-leds-gpio-core.c | 103 ++++++++++++ .../simple/simatic-ipc-leds-gpio-f7188x.c | 64 +++++++ drivers/leds/simple/simatic-ipc-leds-gpio.c | 159 ------------------ drivers/leds/simple/simatic-ipc-leds-gpio.h | 22 +++ drivers/platform/x86/simatic-ipc.c | 7 +- 7 files changed, 260 insertions(+), 163 deletions(-) create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-core.c create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c delete mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.h