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

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Lee,

On Tue, Jun 18, 2019 at 11:20 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> 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?

Unfortunately the datasheet for the R-Car Gen3 and RZ/G2 SoCs is
not yet public.

However, a very similar hardware block is present in the RZ/A2M SoC.
Please see Chapter 20 ("SPI Multi I/O Bus Controller") of the "RZ/A2M Group
User’s Manual: Hardware", which you can download from
https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza2m.html#documents

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