Messages in this thread |  | | From | Andy Shevchenko <> | Date | Wed, 15 Jun 2022 16:51:52 +0200 | Subject | Re: [PATCH 10/10] pinctrl: Add AXP192 pin control driver |
| |
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
|  |