On Sun, Jun 19, 2022 at 1:20 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Sat, Jun 18, 2022 at 11:40 PM Aidan MacDonald
> <aidanmacdonald.0x0@xxxxxxxxx> wrote:
Hit 'Send' accidentally, here is the rest of the review, including
previous comments.
...
> > +config PINCTRL_AXP192
> > + tristate "X-Powers AXP192 PMIC pinctrl and GPIO Support"
> > + depends on MFD_AXP20X
>
>
> > + depends on OF
>
> Why?
>
> > + select PINMUX
> > + select GENERIC_PINCONF
> > + select GPIOLIB
>
> ...
>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
>
> Why?
Perhaps you missed mod_devicetable.h.
> ...
>
> > +struct axp192_pctl_function {
> > + const char *name;
> > + /* Mux value written to the control register to select the function (-1 if unsupported) */
>
> Comment is misleading. -1 can't be a value of unsigned type.
>
> > + const u8 *muxvals;
> > + const char * const *groups;
> > + unsigned int ngroups;
> > +};
>
> ...
>
> > +struct axp192_pctl_desc {
> > + unsigned int npins;
> > + const struct pinctrl_pin_desc *pins;
> > + /* Description of the function control register for each pin */
> > + const struct axp192_pctl_reg_info *ctrl_regs;
> > + /* Description of the output signal register for each pin */
> > + const struct axp192_pctl_reg_info *out_regs;
> > + /* Description of the input signal register for each pin */
> > + const struct axp192_pctl_reg_info *in_regs;
> > + /* Description of the pull down resistor config register for each pin */
>
> Can you just convert these comments to a kernel-doc?
>
> > + const struct axp192_pctl_reg_info *pull_down_regs;
> > +
> > + unsigned int nfunctions;
> > + const struct axp192_pctl_function *functions;
> > +};
>
> ...
>
> > +
> > +
>
> One blank line is enough.
>
> ...
>
> > + switch (param) {
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + ret = axp192_pinconf_get_pull_down(pctldev, pin);
> > + if (ret < 0)
> > + return ret;
>
> > + else if (ret != 0)
>
> 1. Redundant 'else'
> 2. if (ret > 0)
>
> > + return -EINVAL;
> > + break;
> > +
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + ret = axp192_pinconf_get_pull_down(pctldev, pin);
> > + if (ret < 0)
> > + return ret;
> > + else if (ret == 0)
>
> Ditto.
>
> Looking at this I would rather expect the function to return something
> defined, than 0, non-0.
>
> > + return -EINVAL;
> > + break;
>
> > + default:
> > + return -ENOTSUPP;
> > + }
>
> ...
>
> > + for (cfg = 0; cfg < num_configs; ++cfg) {
>
> cfg++ will work the same way and easier to read.
...
You may make some lines shorter by introducing here
struct device *dev = &pdev->dev;
> > + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
dev->parent
and so on...
...
> > + pctl->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, pctl);
> > + if (IS_ERR(pctl->pctl_dev))
> > + dev_err_probe(&pdev->dev, PTR_ERR(pctl->pctl_dev),
> > + "couldn't register pinctrl driver\n");
With the above it probably fits one line.
--
With Best Regards,
Andy Shevchenko
