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
