Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220511153905.13980-1-henning.schild@siemens.com> <20220511153905.13980-5-henning.schild@siemens.com>
In-Reply-To: <20220511153905.13980-5-henning.schild@siemens.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 11 May 2022 19:53:15 +0200
Message-ID: <CAHp75VdSYgAj-8scpZK1EmiNywuji6cpsCWnCo+6vqqdEvg+5w@mail.gmail.com>
Subject: Re: [PATCH v2 4/4] leds: simatic-ipc-leds-gpio: add GPIO version of
 Siemens driver
To:     Henning Schild <henning.schild@siemens.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Mark Gross <markgross@kernel.org>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux LED Subsystem <linux-leds@vger.kernel.org>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        linux-watchdog@vger.kernel.org, Enrico Weigelt <lkml@metux.net>,
        Gerd Haeussler <gerd.haeussler.ext@siemens.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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