Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220603135714.12007-1-aidanmacdonald.0x0@gmail.com> <20220603135714.12007-11-aidanmacdonald.0x0@gmail.com>
In-Reply-To: <20220603135714.12007-11-aidanmacdonald.0x0@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 15 Jun 2022 16:51:52 +0200
Message-ID: <CAHp75VevetU0p+BTcQ6HcAn=2xgVGAL34ZuAi53rK3SDt=O-cw@mail.gmail.com>
Subject: Re: [PATCH 10/10] pinctrl: Add AXP192 pin control driver
To:     Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Chen-Yu Tsai <wens@csie.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Lee Jones <lee.jones@linaro.org>,
        Sebastian Reichel <sre@kernel.org>,
        Mark Brown <broonie@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jun 3, 2022 at 6:29 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:
>
> The AXP192 PMIC's GPIO registers are much different from the GPIO
> registers of the AXP20x and AXP813 PMICs supported by the existing
> pinctrl-axp209 driver. It makes more sense to add a new driver for
> the AXP192, rather than add support in the existing axp20x driver.
>
> The pinctrl-axp192 driver is considerably more flexible in terms of
> register layout and should be able to support other X-Powers PMICs.
> Interrupts and pull down resistor configuration are supported too.

Thank you for contribution, overall looks good, below some not very
critical comments.

...

> +static const struct axp192_pctl_reg_info axp192_pin_ctrl_regs[] = {
> +       { .reg = AXP192_GPIO0_CTRL,   .mask = 0x07 },
> +       { .reg = AXP192_GPIO1_CTRL,   .mask = 0x07 },
> +       { .reg = AXP192_GPIO2_CTRL,   .mask = 0x07 },
> +       { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x03 },
> +       { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x0c },
> +       { .reg = AXP192_N_RSTO_CTRL,  .mask = 0xc0 },
> +};

GENMASK()

...

> +       if ((val & reginfo->mask) == (input_muxvals[offset] << (ffs(reginfo->mask) - 1)))
> +               return GPIO_LINE_DIRECTION_IN;

> +       else

Redundant.
Also applies for the other similar cases in your code. Note, this is
also redundant for 'continue' and 'break' in case of loops.

> +               return GPIO_LINE_DIRECTION_OUT;

...

> +       if (!reginfo->mask)
> +               return -EOPNOTSUPP;

Please, double check that this is used by the pin control subsystem
and not ENOTSUP in your case here.

...

> +       default:
> +               return -EOPNOTSUPP;

Ditto.

...

> +               default:
> +                       return -EOPNOTSUPP;

Ditto.

...

> +               default:
> +                       /* unreachable */
> +                       break;

return 0?! Perhaps you need to return an error?

> +               }
> +       }
> +
> +       return 0;

...

> +       if (muxvals[group] == (u8)-1)

limits.h and U8_MAX? Or GENMASK()? Choose one which suits you.

> +               return -EINVAL;

...

> +       if (!of_device_is_available(pdev->dev.of_node))
> +               return -ENODEV;

Dead code.

> +       if (!axp20x) {
> +               dev_err(&pdev->dev, "Parent drvdata not set\n");
> +               return -EINVAL;
> +       }

Another useless piece of code.

...

> +       pctl->desc = of_device_get_match_data(&pdev->dev);

device_get_match_data()

...

> +       pctl->chip.to_irq               = axp192_gpio_to_irq;

Why a custom method?

...

> +       pctl->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, pctl);
> +       if (IS_ERR(pctl->pctl_dev)) {
> +               dev_err(&pdev->dev, "couldn't register pinctrl driver\n");
> +               return PTR_ERR(pctl->pctl_dev);

Here and everywhere else in ->probe() and Co, use

  return dev_err_probe(...);

pattern.

> +       }

...

> +       ret = gpiochip_add_pin_range(&pctl->chip, dev_name(&pdev->dev),
> +                                    pctl->desc->pins->number,
> +                                    pctl->desc->pins->number,
> +                                    pctl->desc->npins);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to add pin range\n");
> +               return ret;
> +       }

We have a specific callback where you may put this, otherwise on some
systems it may not work as expected.

...

> +       dev_info(&pdev->dev, "AXP192 pinctrl and GPIO driver loaded\n");

Useless.

...

> +static struct platform_driver axp192_pctl_driver = {
> +       .probe          = axp192_pctl_probe,
> +       .driver = {
> +               .name           = "axp192-gpio",
> +               .of_match_table = axp192_pctl_match,
> +       },
> +};

> +

Redundant blank line.

> +module_platform_driver(axp192_pctl_driver);

...

Globally two comments:
1) I also believe that you may utilize gpio-regmap API;
2) try to get rid of OFisms, make it property provider agnostic.

-- 
With Best Regards,
Andy Shevchenko