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@xxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
