Re: [PATCH RFC 2/2] mfd: add Renesas RPC-IF driver

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Lee,

Thanks for your comments!

On Mon, Jun 17, 2019 at 11:31 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Fri, 14 Jun 2019, Sergei Shtylyov wrote:
> > On 06/12/2019 12:05 PM, Lee Jones wrote:
> > >> +static const struct mfd_cell rpcif_hf_ctlr = {
> > >> +  .name = "rpcif-hyperflash",
> > >> +};
> > >> +
> > >> +static const struct mfd_cell rpcif_spi_ctlr = {
> > >> +  .name = "rpcif-spi",
> > >> +};
> > >
> > > This looks like a very tenuous use of the MFD API.
> > >
> > > I suggest that this isn't actually an MFD at all.
> >
> >    The same hardware supports 2 different physical interfaces, hence
> > the drivers have to comply to 2 different driver frameworks... sounded
> > like MFD to me. :-)
>
> Not to me.
>
> This appears to be some kind of 'mode selector' for an MTD device.

... for either an SPI or MTD device.

> Lots of drivers have multiple ways to control them - they are not all
> MFDs.

So where to reside the common part? drivers/platform/renesas/?

> > [...]
> > >> +static int rpcif_probe(struct platform_device *pdev)
> > >> +{
> > >> +  struct device_node *flash;
> > >> +  const struct mfd_cell *cell;
> > >> +  struct resource *res;
> > >> +  void __iomem *base;
> > >> +  struct rpcif *rpc;
> > >> +
> > >> +  flash = of_get_next_child(pdev->dev.of_node, NULL);
> > >> +  if (!flash) {
> > >> +          dev_warn(&pdev->dev, "no flash node found\n");
> > >> +          return -ENODEV;
> > >> +  }
> > >> +
> > >> +  if (of_device_is_compatible(flash, "jedec,spi-nor")) {
> > >> +          cell = &rpcif_spi_ctlr;
> > >> +  } else if (of_device_is_compatible(flash, "cfi-flash")) {
> > >> +          cell = &rpcif_hf_ctlr;
> > >> +  } else {
> > >> +          dev_warn(&pdev->dev, "unknown flash type\n");
> > >> +          return -ENODEV;
> > >> +  }
> > >
> > > Why not let DT choose which device to probe?
> >
> >    It's the DT that decides here. How else would you imagine that?
> > It's the same hardware, just the different physical busses that it
> > commands...
>
> DT is not deciding here.  This driver is deciding based on the
> information contained in the tables - very different thing.
>
> Why not just let the OF framework probe the correct device i.e. let it
> parse the 2 compatible strings and let it match the correct driver for
> the device?

The OF framework matches against the RPC-IF node, which is a single
hardware type, hence has a fixed compatible value.
The mode depends on the subnode in DT, which is something the OF
framework doesn't match against, so the driver itself has to check the
subnode's compatible value.
DT describes hardware, not software (Linux subsystem boundary) policy.

I think you could have two drivers (SPI and MFD) each matching against
the same compatible value, with .probe() functions returning -ENODEV
if the subnode doesn't have the appropriate compatible value.
However, (1) I don't know how well that would play with module
autoloading based on of_device_id, and (2) that still leaves the question
where to put the shared code.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds