Re: [PATCH v3 1/1] leds: simatic-ipc-leds-gpio: add support for module BX-59A

[Andy Shevchenko]

On Wed, Mar 13, 2024 at 10:15 AM Xing Tong Wu <xingtong_wu@xxxxxxx> wrote:
>
> From: Xing Tong Wu <xingtong.wu@xxxxxxxxxxx>
>
> This is used for the Siemens Simatic IPC BX-59A, which has its LEDs
> connected to GPIOs provided by the Nuvoton NCT6126D.

FWIW, LGTM
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

Although a couple of minor remarks below.

> Signed-off-by: Xing Tong Wu <xingtong.wu@xxxxxxxxxxx>
> ---
>  .../leds/simple/simatic-ipc-leds-gpio-core.c  |  1 +
>  .../simple/simatic-ipc-leds-gpio-f7188x.c     | 53 ++++++++++++++++---
>  2 files changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-core.c b/drivers/leds/simple/simatic-ipc-leds-gpio-core.c
> index 667ba1bc3a30..85003fd7f1aa 100644
> --- a/drivers/leds/simple/simatic-ipc-leds-gpio-core.c
> +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-core.c
> @@ -56,6 +56,7 @@ int simatic_ipc_leds_gpio_probe(struct platform_device *pdev,
>         case SIMATIC_IPC_DEVICE_127E:
>         case SIMATIC_IPC_DEVICE_227G:
>         case SIMATIC_IPC_DEVICE_BX_21A:
> +       case SIMATIC_IPC_DEVICE_BX_59A:
>                 break;
>         default:
>                 return -ENODEV;
> diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
> index c7c3a1f986e6..08d8e580b4f1 100644
> --- a/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
> +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
> @@ -17,7 +17,12 @@
>
>  #include "simatic-ipc-leds-gpio.h"
>
> -static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> +struct simatic_ipc_led_tables {
> +       struct gpiod_lookup_table *led_lookup_table;
> +       struct gpiod_lookup_table *led_lookup_table_extra;
> +};
> +
> +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),
> @@ -30,7 +35,7 @@ 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_extra_227g = {
>         .dev_id = NULL, /* Filled during initialization */
>         .table = {
>                 GPIO_LOOKUP_IDX("gpio-f7188x-3", 6, NULL, 6, GPIO_ACTIVE_HIGH),
> @@ -39,16 +44,52 @@ static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra = {
>         },
>  };
>
> +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_bx_59a = {
> +       .dev_id = "leds-gpio",
> +       .table = {
> +               GPIO_LOOKUP_IDX("gpio-f7188x-2", 0, NULL, 0, GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP_IDX("gpio-f7188x-2", 3, NULL, 1, GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP_IDX("gpio-f7188x-5", 3, NULL, 2, GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP_IDX("gpio-f7188x-5", 2, NULL, 3, GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP_IDX("gpio-f7188x-7", 7, NULL, 4, GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP_IDX("gpio-f7188x-7", 4, NULL, 5, GPIO_ACTIVE_LOW),
> +               {} /* Terminating entry */
> +       }
> +};
> +
>  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);
> +       const struct simatic_ipc_platform *plat = dev_get_platdata(&pdev->dev);
> +       struct simatic_ipc_led_tables *led_tables;
> +
> +       led_tables = devm_kzalloc(&pdev->dev, sizeof(*led_tables), GFP_KERNEL);
> +       if (!led_tables)
> +               return -ENOMEM;

You can make this smarter, i.e. only allocate memory after the switch
case. But it might require additional code and variables which will be
more churn. So, I'm fine with this approach.

> +       switch (plat->devmode) {
> +       case SIMATIC_IPC_DEVICE_227G:
> +               led_tables->led_lookup_table = &simatic_ipc_led_gpio_table_227g;
> +               led_tables->led_lookup_table_extra = &simatic_ipc_led_gpio_table_extra_227g;
> +               break;
> +       case SIMATIC_IPC_DEVICE_BX_59A:
> +               led_tables->led_lookup_table = &simatic_ipc_led_gpio_table_bx_59a;
> +               break;
> +       default:
> +               return -ENODEV;
> +       }
> +
> +       platform_set_drvdata(pdev, led_tables);
> +       return simatic_ipc_leds_gpio_probe(pdev, led_tables->led_lookup_table,
> +                                          led_tables->led_lookup_table_extra);
>  }
>
>  static void simatic_ipc_leds_gpio_f7188x_remove(struct platform_device *pdev)
>  {
> -       simatic_ipc_leds_gpio_remove(pdev, &simatic_ipc_led_gpio_table,
> -                                    &simatic_ipc_led_gpio_table_extra);

> +       struct simatic_ipc_led_tables *led_tables;
> +       led_tables = platform_get_drvdata(pdev);

This can be done on a single line.

> +       simatic_ipc_leds_gpio_remove(pdev, led_tables->led_lookup_table,
> +                                    led_tables->led_lookup_table_extra);
>  }
>
>  static struct platform_driver simatic_ipc_led_gpio_driver = {


-- 
With Best Regards,
Andy Shevchenko