Re: [PATCH v4 03/14] pinctrl-ingenic: add a pinctrl driver for the Ingenic jz47xx SoCs

[Lee Jones]

On Fri, 07 Apr 2017, Linus Walleij wrote:

> On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> 
> > This driver handles pin configuration and pin muxing for the
> > JZ4740 and JZ4780 SoCs from Ingenic.
> >
> > Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> (...)
> > +       select MFD_CORE
> (...)
> > +#include <linux/mfd/core.h>
> 
> That's unorthodox. Still quite pretty!
> I would nee Lee Jones to say something about this, as it is
> essentially hijacking MFD into the pinctrl subsystem.
> 
> > +static struct mfd_cell ingenic_pinctrl_mfd_cells[] = {
> > +       {
> > +               .id = 0,
> > +               .name = "GPIOA",
> > +               .of_compatible = "ingenic,gpio-bank-a",
> > +       },
> > +       {
> > +               .id = 1,
> > +               .name = "GPIOB",
> > +               .of_compatible = "ingenic,gpio-bank-b",
> > +       },
> > +       {
> > +               .id = 2,
> > +               .name = "GPIOC",
> > +               .of_compatible = "ingenic,gpio-bank-c",
> > +       },
> > +       {
> > +               .id = 3,
> > +               .name = "GPIOD",
> > +               .of_compatible = "ingenic,gpio-bank-d",
> > +       },
> > +       {
> > +               .id = 4,
> > +               .name = "GPIOE",
> > +               .of_compatible = "ingenic,gpio-bank-e",
> > +       },
> > +       {
> > +               .id = 5,
> > +               .name = "GPIOF",
> > +               .of_compatible = "ingenic,gpio-bank-f",
> > +       },
> > +};
> (...)
> > +       err = devm_mfd_add_devices(dev, 0, ingenic_pinctrl_mfd_cells,
> > +                       ARRAY_SIZE(ingenic_pinctrl_mfd_cells), NULL, 0, NULL);
> > +       if (err) {
> > +               dev_err(dev, "Failed to add MFD devices\n");
> > +               return err;
> > +       }
> 
> I guess the alternative would be to reimplement the MFD structure.
> 
> Did you check the approach to use "simple-mfd" and just let the subnodes
> spawn as devices that way? I guess you did and this adds something
> necessary.

I'd like to hear what the OP has to say about why this is necessary.
However, as a first glimpse, I'm dead against exporting MFD
functionality to outside of the subsystem.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog