Re: [PATCH 1/2] x86: gpio: AMD G-Series pch gpio platform driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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@xxxxxxxxx> wrote:

> From: "Enrico Weigelt, metux IT consult" <info@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Cc: linus.walleij@xxxxxxxxxx
> Cc: bgolaszewski@xxxxxxxxxxxx
> Cc: dvhart@xxxxxxxxxxxxx
> Cc: andy@xxxxxxxxxxxxx
> Cc: platform-driver-x86@xxxxxxxxxxxxxxx
>
> Signed-off-by: Enrico Weigelt, metux IT consult <info@xxxxxxxxx>

(...)

> +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@xxxxxxxxx>
> + *
> + * 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