linux-kernel - Re: [PATCH v2 3/6] gpio: Add GPIO driver for Nintendo Wii

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]

Date:   Wed, 31 Jan 2018 09:37:17 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:LINUX FOR POWERPC PA SEMI PWRFICIENT" 
        <linuxppc-dev@...ts.ozlabs.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Albert Herranz <albert_herranz@...oo.es>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v2 3/6] gpio: Add GPIO driver for Nintendo Wii

Hi,

On Sun, Jan 28, 2018 at 07:31:58PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 22, 2018 at 7:04 AM, Jonathan Neuschäfer
> <j.neuschaefer@....net> wrote:
> 
> Style issues below.
> 
> > +#define HW_GPIO_OWNER          0x3c
> > +
> > +
> > +struct hlwd_gpio {
> 
> No need extra empty line in between.

Ok.

> > +       struct gpio_chip gpioc;
> > +       void __iomem *regs;
> > +       struct device *dev;
> > +};
> > +
> > +static int hlwd_gpio_probe(struct platform_device *pdev)
> > +{
> > +       struct hlwd_gpio *hlwd;
> > +       struct resource *regs_resource;
> > +       u32 ngpios;
> > +       int res;
> > +
> > +       hlwd = devm_kzalloc(&pdev->dev, sizeof(*hlwd), GFP_KERNEL);
> > +       if (!hlwd)
> > +               return -ENOMEM;
> > +
> 
> > +       /* Save the struct device pointer so dev_info, etc. can be used. */
> 
> Useless.

Ok

> > +       hlwd->dev = &pdev->dev;
> > +
> 
> > +       regs_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 
> > +       if (IS_ERR(regs_resource))
> > +               return PTR_ERR(regs_resource);
> > +
> 
> This is redundant. Below does it for ya.

devm_ioremap_resource does not check if the resource argument is a
negative error (which is what I'm trying to catch in the above code).
But it seems that platform_get_resource can't return a negative error,
so you're right. Thanks.

> 
> > +       hlwd->regs = devm_ioremap_resource(&pdev->dev, regs_resource);
> > +       if (IS_ERR(hlwd->regs))
> > +               return PTR_ERR(hlwd->regs);
> 
> 
> > +       res = bgpio_init(&hlwd->gpioc, &pdev->dev, 4,
> > +                       hlwd->regs + HW_GPIOB_IN, hlwd->regs + HW_GPIOB_OUT,
> > +                       NULL, hlwd->regs + HW_GPIOB_DIR, NULL,
> > +                       BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> 
> > +
> 
> Remove this extra line.

Ok.

> 
> > +       if (res < 0) {
> > +               dev_warn(hlwd->dev, "bgpio_init failed: %d\n", res);
> > +               return res;
> > +       }
> 
> > +       if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios))
> > +               ngpios = 32;
> 
> A nit: I would rather go with
> res = of_property_read(...);
> if (res)
>   ngpios = 32;

Ok, I have no strong opinion on this, so I'll do what you suggest.


Thanks,
Jonathan Neuschäfer

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)