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

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Tue, 18 Jun 2019, Geert Uytterhoeven wrote:
> 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.

Okay, so I think I misunderstood the device.  I was under the
impression that it was a flash memory device where the only difference
was the interface by which it is controlled?

> > Lots of drivers have multiple ways to control them - they are not all
> > MFDs.
> 
> So where to reside the common part? drivers/platform/renesas/?

That does not make sense, since this is not a platform controller.

> > > [...]
> > > >> +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.

I can see how it has been implemented.

It is that which I was questioning.

> DT describes hardware, not software (Linux subsystem boundary) policy.

So is an RPC-IF a real hardware device.  Can you share the datasheet?

> I think you could have two drivers (SPI and MFD) each matching against
> the same compatible value, with .probe() functions returning -ENODEV

No, don't do that.

> 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.

Other than the SPI driver in this set (which I have now looked at),
what else uses the MFD "back-end"?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog