linux-kernel - Re: [PATCH 1/2] gpio: AMD G-Series PCH gpio 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]

Message-ID: <CAHp75VeUEtRnRyJfpRC9Pdjy4ji=EMk=QQJ8gk7RpX6SsLg9Qg@mail.gmail.com>
Date:   Thu, 14 Feb 2019 04:08:44 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Enrico Weigelt, metux IT consult" <info@...ux.net>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Andy Shevchenko <andy@...radead.org>,
        Darren Hart <dvhart@...radead.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH 1/2] gpio: AMD G-Series PCH gpio driver

On Wed, Feb 13, 2019 at 10:57 PM Enrico Weigelt, metux IT consult
<info@...ux.net> 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