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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Feb 7, 2019 at 7:14 PM Enrico Weigelt, metux IT consult
<info@xxxxxxxxx> wrote:
>
> 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.

Thanks for the patch, see my comments below.

Overall I have a feeling that this driver can be replaced with
existing generic one where one register per pin is allocated.
Unfortunately, I didn't look deep into this and hope Linus will help
to figure this out.

> @@ -0,0 +1,171 @@
> +/*
> + * 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+

SPDX should go as a separate first line in a proper format.

> + */

> +// FIXME: add spinlocks

Then fix them and come again.

> +#include <linux/init.h>
> +#include <linux/module.h>

One of them should be present, another one dropped.

> +#define GPIO_BIT_DIR           23
> +#define GPIO_BIT_WRITE         22
> +#define GPIO_BIT_READ          16

Oh, namespace issues.
What about using BIT() macro?

> +
> +

Why two blank lines?

> +static uint32_t *amd_fch_gpio_addr(struct gpio_chip *gc, unsigned gpio)
> +{
> +       struct amd_fch_gpio_priv *priv = gpiochip_get_data(gc);
> +

> +       if (gpio > priv->pdata->gpio_num) {
> +               dev_err(&priv->pdev->dev, "gpio number %d out of range\n", gpio);
> +               return NULL;
> +       }

On which circumstances it may happen?

> +
> +       return priv->base + priv->pdata->gpio_reg[gpio].reg*sizeof(u32);
> +}
> +
> +static int amd_fch_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
> +{

> +       volatile uint32_t *ptr = amd_fch_gpio_addr(gc, offset);

volatile?!

I think you need to use readl()/writel() (or their _relaxed variants) instead.
Same applies for entire code.

> +       if (!ptr) return -EINVAL;

This code has style issues.
Check your entire file.

> +
> +       *ptr &= ~(1 << GPIO_BIT_DIR);
> +       return 0;
> +}

> +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");
> +}

Remove whatever is not implemented and not required to have a stub.

> +static int amd_fch_gpio_request(struct gpio_chip *chip, unsigned gpio_pin)
> +{

> +       if (gpio_pin < chip->ngpio)
> +               return 0;

Is it even possible?

> +
> +       return -EINVAL;
> +}


> +
> +static int amd_fch_gpio_probe(struct platform_device *pdev)
> +{
> +       struct amd_fch_gpio_priv *priv;

> +       struct amd_fch_gpio_pdata *pdata = pdev->dev.platform_data;

We have a helper to get this. platform_get_data() IIRC.

> +       int err;
> +
> +       if (!pdata) {
> +               dev_err(&pdev->dev, "no platform_data\n");
> +               return -ENOENT;
> +       }
> +

> +       if (!(priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL))) {

Should be two lines.

> +               dev_err(&pdev->dev, "failed to allocate priv struct\n");

Noise.

> +               return -ENOMEM;
> +       }
> +

> +       if (IS_ERR(priv->base = devm_ioremap_resource(&pdev->dev, &priv->pdata->res))) {

> +               dev_err(&pdev->dev, "failed to map iomem\n");

Noise (that function will print a message)

> +               return -ENXIO;

Shadowed error code.

> +       }
> +

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

Noise.

> +

> +       if (IS_ENABLED(CONFIG_DEBUG_FS)) {

Do you really care?

> +               dev_info(&pdev->dev, "enabling debugfs\n");

Noise.

> +               priv->gc.dbg_show = amd_fch_gpio_dbg_show;
> +       }
> +
> +       platform_set_drvdata(pdev, priv);
> +

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

return devm_gpiochip_add_data(...);

> +}

> +MODULE_LICENSE("GPL");

License mismatch. I really don't look what 'GPL+' means. OTOH I know
this one corresponds to GPL-2.0+.

> +++ b/include/linux/platform_data/x86/amd-fch-gpio-pdata.h
> @@ -0,0 +1,41 @@
> +/*
> + * AMD FCH gpio driver platform-data
> + *
> + * Copyright (C) 2018 metux IT consult
> + * Author: Enrico Weigelt <info@xxxxxxxxx>
> + *

> + * SPDX-License-Identifier: GPL

Same comments.

> + */

> +/*

It's not marked as kernel doc.

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

Isn't this provided by GPIO library? We have so called labels.

> +/*
> + * struct amd_fch_gpio_pdata - GPIO chip platform data
> + * @resource: iomem range
> + * @gpio_reg: array of gpio registers
> + * @gpio_num: number of entries
> + */
> +struct amd_fch_gpio_pdata {
> +    struct resource          res;
> +    int                      gpio_num;
> +    struct amd_fch_gpio_reg *gpio_reg;
> +    int                      gpio_base;
> +};

-- 
With Best Regards,
Andy Shevchenko