Re: [PATCH 10/10] pinctrl: Add AXP192 pin control driver

[Aidan MacDonald]

Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes:

> On Fri, Jun 3, 2022 at 6:29 PM Aidan MacDonald
> <aidanmacdonald.0x0@xxxxxxxxx> 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.
>
> ...
>

Thanks very much for the review. I'll fix up the issues you spotted
in v3. (v2 doesn't make any changes to the pinctrl driver.)

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

Sorry, I'm not sure what you're referring to here. The "else"?
I'm missing the generalization.

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

Whoops. You're right, it should be ENOTSUPP.

>
> ...
>
>> +       default:
>> +               return -EOPNOTSUPP;
>
> Ditto.
>
> ...
>
>> +               default:
>> +                       return -EOPNOTSUPP;
>
> Ditto.
>
> ...
>
>> +               default:
>> +                       /* unreachable */
>> +                       break;
>
> return 0?! Perhaps you need to return an error?
>

Yeah, that sounds like a good idea for maintainability. I think
there's no need to check that the requested configs are supported
beforehand since the caller must deal with errors in the middle of
the sequence anyway, so I'll drop that check and add ENOTSUPP here.

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

OK. Did some digging, and this is useless because the parent mfd
device is checking availability.

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

The irq chip is part of the mfd device, not the gpio chip. There does
not seem to be any default implementation for this case so I have to
provide one. A similar example is gpio-wm8994.

I did notice I'm doing something wrong by calling regmap_irq_get_virq()
in the probe function, which creates an irq mapping; I think I should be
doing that in the to_irq() callback like the other drivers do.

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

Ah, sorry, I see that function is deprecated. The documentation points
to doing this in the device tree instead. So if I understand correctly
I should follow the example of pinctrl-thunderbay and add gpio-ranges:

    pinctrl0: gpio@0 {
        compatible = "x-powers,axp192-gpio";
        gpio-controller;
        #gpio-cells = <2>;
        gpio-ranges = <&pinctrl0 0 0 6>;
    };

which means I'll have to update the gpio DT bindings. I'm guessing the
callback you mentioned is add_pin_ranges() or of_gpio_ranges_fallback()
but neither of those seem appropriate in this case. The DT node should
be good enough.

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

I wasn't aware of gpio-regmap, will check it out.

Regards,
Aidan