Re: [PATCH v2 4/4] leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver

Andy Shevchenko
2022-05-11T17:53:57Z

On Wed, May 11, 2022 at 6:40 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
> that instead of open coding it.
> Create a new driver for that which can later be filled with more GPIO
> based models, and which has different dependencies.

...

> +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> + .dev_id = "leds-gpio",
> + .table = {
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH),

> + }

Keeping a comma is good here.

> +};

...

> + /* PM_BIOS_BOOT_N */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6, 0);

This is basically GPIOD_ASIS...

> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }

> + gpiod_set_value(gpiod, 0);

...but you may apply GPIOD_OUTPUT_LOW there and drop this call.

> + gpiod_put(gpiod);

Ditto for the rest GPIO requests.

...

> +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,
> + }
> +};

> +

No need to have this blank line.

> +module_platform_driver(simatic_ipc_led_gpio_driver);

--
With Best Regards,
Andy Shevchenko