linux-kernel - Re: [PATCH 1/2] x86: gpio: AMD G-Series pch gpio platform 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:   Fri, 8 Feb 2019 15:25:44 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Enrico Weigelt, metux IT consult" <info@...ux.net>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        platform-driver-x86 <platform-driver-x86@...r.kernel.org>
Subject: Re: [PATCH 1/2] x86: gpio: AMD G-Series pch gpio platform driver

Hi Enrico!

Thanks for your patch! I would really like Andy to have a look at it
too, he's more on top of things in the x86 world.

On Fri, Feb 8, 2019 at 2:16 AM Enrico Weigelt, metux IT consult
<lkml@...ux.net> wrote:

> From: "Enrico Weigelt, metux IT consult" <info@...ux.net>
>
> GPIO platform driver for the AMD G-series PCH (eg. on GX-412TC)
>
> This driver doesn't registers itself automatically, as it needs to
> be provided with platform specific configuration, provided by some
> board driver setup code.
>
> Didn't implement oftree probing yet, as it's rarely found on x86.
>
> Cc: linux-gpio@...r.kernel.org
> Cc: linus.walleij@...aro.org
> Cc: bgolaszewski@...libre.com
> Cc: dvhart@...radead.org
> Cc: andy@...radead.org
> Cc: platform-driver-x86@...r.kernel.org
>
> Signed-off-by: Enrico Weigelt, metux IT consult <info@...ux.net>

(...)

> +config GPIO_AMD_FCH
> +       tristate "GPIO support for AMD Fusion Controller Hub (G-series SOCs)"
> +       select GPIO_GENERIC

You are selecting GPIO_GENERIC, is this necessary? I thought
X86 was already selecting this.

> +/*
> + * GPIO driver for the AMD G series FCH (eg. GX-412TC)
> + *
> + * Copyright (C) 2018 metux IT consult
> + * Author: Enrico Weigelt <info@...ux.net>
> + *
> + * SPDX-License-Identifier: GPL+
> + */

I think checkpatch will complain on that SPDX thing.
Copy something from one of the other drivers, it should be
on the first line of the file.

> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_data/x86/amd-fch-gpio-pdata.h>
> +
> +
> +#define GPIO_BIT_DIR           23
> +#define GPIO_BIT_WRITE         22
> +#define GPIO_BIT_READ          16
> +
> +

Cut down the excessive newlines.

> +static void amd_fch_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
> +{
> +       struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc);
> +       (void)priv;
> +
> +       seq_printf(s, "debug info not implemented yet\n");
> +}

I think you can just skip implementing this then.

> +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin)
> +{
> +       if (gpio_pin < chip->ngpio)
> +               return 0;
> +
> +       return -EINVAL;
> +}

You can probably skip this too. The core already does this check.

> +       priv->gc.owner                  = THIS_MODULE;
> +       priv->gc.parent                 = &pdev->dev;
> +       priv->gc.label                  = dev_name(&pdev->dev);
> +       priv->gc.base                   = priv->pdata->gpio_base;

No please, use priv->gc.base = -1;

> +       dev_info(&pdev->dev, "initializing on my own II\n");

Drop this.

> +       if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> +               dev_info(&pdev->dev, "enabling debugfs\n");
> +               priv->gc.dbg_show = amd_fch_gpio_dbg_show;
> +       }

I think you can drop this too.

> +       platform_set_drvdata(pdev, priv);
> +
> +       err = devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
> +       dev_info(&pdev->dev, "probe finished\n");

If you keep this info, write something more helpful.

> +/*
> + * struct amd_fch_gpio_reg - GPIO register definition
> + * @reg: register index
> + * @name: signal name
> + */
> +struct amd_fch_gpio_reg {
> +    int         reg;
> +    const char* name;
> +};

Can't you put this in the driver file?

> +struct amd_fch_gpio_pdata {
> +    struct resource          res;
> +    int                      gpio_num;
> +    struct amd_fch_gpio_reg *gpio_reg;
> +    int                      gpio_base;
> +};

Drop gpio_base. We don't hardcode the GPIO base anymore.
Use the character device instead if you want it because of
userspace thingies. (See tools/gpio/*)

Yours,
Linus Walleij