From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752142AbeA1RcC (ORCPT <rfc822;w@1wt.eu>);
        Sun, 28 Jan 2018 12:32:02 -0500
Received: from mail-qk0-f193.google.com ([209.85.220.193]:35080 "EHLO
        mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751901AbeA1RcA (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sun, 28 Jan 2018 12:32:00 -0500
X-Google-Smtp-Source: AH8x224l1I8qkfxqANNQ/3MdL6+WTCuYSwTRvKJFv0m/+dTkpcRGre8bR9kC5yZ5TphqJ228Jg9JcUxIZGJKb37CN1o=
MIME-Version: 1.0
In-Reply-To: <20180122050411.32460-4-j.neuschaefer@gmx.net>
References: <20180122050411.32460-1-j.neuschaefer@gmx.net> <20180122050411.32460-4-j.neuschaefer@gmx.net>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Sun, 28 Jan 2018 19:31:58 +0200
Message-ID: <CAHp75VcWshngZFuUp4hzYdD43HF7TZ1sRE3Dh+HckB5do1=gGw@mail.gmail.com>
Subject: Re: [PATCH v2 3/6] gpio: Add GPIO driver for Nintendo Wii
To: =?UTF-8?Q?Jonathan_Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:LINUX FOR POWERPC PA SEMI PWRFICIENT"
        <linuxppc-dev@lists.ozlabs.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Albert Herranz <albert_herranz@yahoo.es>,
        Segher Boessenkool <segher@kernel.crashing.org>,
        Linus Walleij <linus.walleij@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id w0SHW9wW008673

On Mon, Jan 22, 2018 at 7:04 AM, Jonathan Neuschäfer
<j.neuschaefer@gmx.net> 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