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

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

Hello!

On 06/18/2019 12:19 PM, Lee Jones wrote:

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

   How do you imagine that? We typically declare SoC devices in the <soc>.dtsi
files so we'd have to override the "compatible" prop in the <board>.dts files?
Or we'd just skip that prop in the <soc>.dtsi and specify it only in a <board>.dts.
Seems quite ugly... 

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

   s/MFD/MTD/?

>> 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"?

   drivers/mtd/hyperbus/. See the (still in-flight) patch set at:

http://lists.infradead.org/pipermail/linux-mtd/2019-June/089873.html

MBR, Sergei