On Wed, Feb 13, 2019 at 10:57 PM Enrico Weigelt, metux IT consult
<info@xxxxxxxxx> wrote:
Thanks for this version, my comments below.
>
> 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.
Since I don't see neither changelog nor version of the series, it's
hard to say what had been changed and addressed.
> +static void *amd_fch_gpio_addr(struct amd_fch_gpio_priv *priv,
> + unsigned int gpio)
> +{
> + if (unlikely(gpio > priv->pdata->gpio_num)) {
> + dev_err(&priv->pdev->dev,
> + "gpio number %d out of range\n", gpio);
> + return NULL;
> + }
I'm pretty sure this wouldn't be the case.
> +
> + return priv->base + priv->pdata->gpio_reg[gpio]*sizeof(u32);
> +}
> + if (unlikely(!ptr))
> + return -EINVAL;
Neither those ones.
> + spin_lock_irqsave(&priv->gc.bgpio_lock, flags);
> +
> + if (value)
> + writel_relaxed(readl_relaxed(ptr) | AMD_FCH_GPIO_FLAG_WRITE,
> + ptr);
> + else
> + writel_relaxed(readl_relaxed(ptr) & ~AMD_FCH_GPIO_FLAG_WRITE,
> + ptr);
This will look better in usual pattern, i.e.
u32 value;
...
value = readl...
if (...)
...
else
...
writel...
> +static int amd_fch_gpio_request(struct gpio_chip *chip,
> + unsigned int gpio_pin)
> +{
> + if (likely(gpio_pin < chip->ngpio))
> + return 0;
> +
> + return -EINVAL;
> +}
Can't be generic request routine used?
> +
> +static int amd_fch_gpio_probe(struct platform_device *pdev)
> +{
> + struct amd_fch_gpio_priv *priv;
> + struct amd_fch_gpio_pdata *pdata;
> + struct resource res = DEFINE_RES_MEM_NAMED(
> + AMD_FCH_MMIO_BASE + AMD_FCH_GPIO_BANK0_BASE,
> + AMD_FCH_GPIO_SIZE,
> + "amd-fch-gpio-iomem");
Define this outside the function as static variable.
> +
> + pdata = dev_get_platdata(&pdev->dev);
> +
Redundant blank line.
> + if (unlikely(pdata == NULL)) {
Simple if (!pdata)
> + dev_err(&pdev->dev, "no platform_data\n");
> + return -ENOENT;
> + }
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +
Redundant blank line.
> + if (unlikely(priv == NULL))
This check is simple
if (!priv)
> + return -ENOMEM;
> +
> + spin_lock_init(&priv->gc.bgpio_lock);
> +
> + priv->base = devm_ioremap_resource(&pdev->dev, &res);
> + if (IS_ERR(priv->base))
> + return -ENXIO;
Do not shadow error code.
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
> +}
--
With Best Regards,
Andy Shevchenko
