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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 May 2022 19:53:15 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Henning Schild <henning.schild@...mens.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mark Gross <markgross@...nel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        linux-watchdog@...r.kernel.org, Enrico Weigelt <lkml@...ux.net>,
        Gerd Haeussler <gerd.haeussler.ext@...mens.com>
Subject: Re: [PATCH v2 4/4] leds: simatic-ipc-leds-gpio: add GPIO version of
 Siemens driver

On Wed, May 11, 2022 at 6:40 PM Henning Schild
<henning.schild@...mens.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