Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver

[Tomer Maimon <tmaimon77@gmail.com>]

Hi Krzysztof,

Thanks for your comments.

On Tue, 12 Jul 2022 at 12:50, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 10/07/2022 12:21, Tomer Maimon wrote:
> > Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
> >
> > Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the
> > pin mux mapping difference the NPCM8XX GPIO supports adjust debounce
> > period time.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>
> (...)
>
> > +
> > +static int npcm8xx_pinctrl_probe(struct platform_device *pdev)
> > +{
> > +     struct npcm8xx_pinctrl *pctrl;
> > +     int ret;
> > +
> > +     pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> > +     if (!pctrl)
> > +             return -ENOMEM;
> > +
> > +     pctrl->dev = &pdev->dev;
> > +     dev_set_drvdata(&pdev->dev, pctrl);
> > +
> > +     pctrl->gcr_regmap =
> > +             syscon_regmap_lookup_by_compatible("nuvoton,npcm845-gcr");
>
> No. Use property. By this patchset, I would expect that you learnt from
> previous mistakes around this. Why repeating the same trouble second time?
You suggest to use phandle property like nuvoton,sysgcr even that the
NPCM8XX pin controller driver is used only NPCM8XX SoC, so the only
GCR node in the NPCM8XX SoC is nuvoton,npcm845-gcr?
>
> > +     if (IS_ERR(pctrl->gcr_regmap)) {
> > +             dev_err(pctrl->dev, "didn't find nuvoton,npcm845-gcr\n");
> > +             return PTR_ERR(pctrl->gcr_regmap);
> > +     }
> > +
> > +     ret = npcm8xx_gpio_of(pctrl);
> > +     if (ret < 0) {
> > +             dev_err(pctrl->dev, "Failed to gpio dt-binding %u\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     pctrl->pctldev = devm_pinctrl_register(&pdev->dev,
> > +                                            &npcm8xx_pinctrl_desc, pctrl);
> > +     if (IS_ERR(pctrl->pctldev)) {
> > +             dev_err(&pdev->dev, "Failed to register pinctrl device\n");
> > +             return PTR_ERR(pctrl->pctldev);
> > +     }
> > +
> > +     ret = npcm8xx_gpio_register(pctrl);
> > +     if (ret < 0) {
> > +             dev_err(pctrl->dev, "Failed to register gpio %u\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     pr_info("npcm8xx Pinctrl driver probed\n");
>
>
> No pr_ in devices. No success debug messages.
>
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id npcm8xx_pinctrl_match[] = {
> > +     { .compatible = "nuvoton,npcm845-pinctrl" },
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(of, npcm8xx_pinctrl_match);
> > +
> > +static struct platform_driver npcm8xx_pinctrl_driver = {
> > +     .probe = npcm8xx_pinctrl_probe,
> > +     .driver = {
> > +             .name = "npcm8xx-pinctrl",
> > +             .of_match_table = npcm8xx_pinctrl_match,
> > +             .suppress_bind_attrs = true,
> > +     },
> > +};
> > +
> > +static int __init npcm8xx_pinctrl_register(void)
> > +{
> > +     return platform_driver_register(&npcm8xx_pinctrl_driver);
> > +}
> > +arch_initcall(npcm8xx_pinctrl_register);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("tomer.maimon@nuvoton.com");
> > +MODULE_DESCRIPTION("Nuvoton NPCM8XX Pinctrl and GPIO driver");
>
>
> Best regards,
> Krzysztof

Best regards,

Tomer