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

From: Andy Shevchenko
Date: Sun Jan 28 2018 - 12:32:15 EST


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.

> + 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.

> + 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.

> + 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.

> + 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;

--
With Best Regards,
Andy Shevchenko