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

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

On 06/17/2019 12:30 PM, Lee Jones wrote:

>>>> Add the MFD driver for Renesas RPC-IF which registers either the SPI or
>>>> HyperFLash  device depending on the contents of the device tree subnode.
>>>> It also provides the absract "back end" device APIs that can be used by
>>>> the "front end" SPI/MTD drivers to talk to the real hardware.
>>>>
>>>> Based on the original patch by Mason Yang <masonccyang@xxxxxxxxxxx>.
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
>>
>> [...]
>>>> Index: mfd/drivers/mfd/rpc-if.c
>>>> ===================================================================
>>>> --- /dev/null
>>>> +++ mfd/drivers/mfd/rpc-if.c
>>>> @@ -0,0 +1,535 @@
[...]
>>>> +#define RPCIF_DIRMAP_SIZE	0x4000000
>>>
>>> Can you shift this lot out to a header file please.
>>
>>    You mean all register #defne's? Why? I'm not intending to use them outside
>> this file.
> 
> Because its 10's of lines of cruft.

   Thank you! :-)

> People won't want to wade through that to get to real functional C
> code every time they open up this file.

   This is how the most drivers are written.

> You already have a header file, please use it.

   Headers are for public things. I've encapsulated the h/w assess into
the MFD driver, the client code doesn't have to see all the gory hardware
details... IOW, I don't agree to this request.

>>>> +void rpcif_enable_rpm(struct rpcif *rpc)
>>>> +{
>>>> +	pm_runtime_enable(rpc->dev);
>>>> +}
>>>> +EXPORT_SYMBOL(rpcif_enable_rpm);
>>>> +
>>>> +void rpcif_disable_rpm(struct rpcif *rpc)
>>>> +{
>>>> +	pm_runtime_disable(rpc->dev);
>>>> +}
>>>> +EXPORT_SYMBOL(rpcif_disable_rpm);
>>>
>>> Where are you exporting these out to?
>>
>>    The "sub-drivers" (SPI/HyperFlash-to-RPC translation drivers).
>>
>>> Why are you seemingly unnecessarily abstracting out the pm_* API?
>>
>>    With RPM being otherwise controlled by this driver, I thought that should be
>> consistent.
> 
> Just use the pm_runtime_*() API directly.

   This would go against the driver encapsulation as well, I would leave it
as is...

>>>> +void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
>>>> +{
>>>> +	pm_runtime_get_sync(rpc->dev);
>>>> +
>>>> +	/*
>>>> +	 * NOTE: The 0x260 are undocumented bits, but they must be set.
>>>> +	 *	 RPCIF_PHYCNT_STRTIM is strobe timing adjustment bit,
>>>> +	 *	 0x0 : the delay is biggest,
>>>> +	 *	 0x1 : the delay is 2nd biggest,
>>>> +	 *	 On H3 ES1.x, the value should be 0, while on others,
>>>> +	 *	 the value should be 6.
>>>> +	 */
>>>> +	regmap_write(rpc->regmap, RPCIF_PHYCNT,
>>>> +		     RPCIF_PHYCNT_CAL | RPCIF_PHYCNT_STRTIM(6) |
>>>> +		     RPCIF_PHYCNT_PHYMEM(hyperflash ? 3 : 0) | 0x260);
>>>
>>> At least #define it, even it it's not documented.
>>
>>    Do you have an idea how to name such #define?
> 
> How did you find out that they must be set?

   Mason lifted all this "magic" from the bootloader's driver.

> How did you find out the value?

> What happens if they are not set?

   Don't know, maybe Mason does. :-)

[...]
>>>> +static int wait_msg_xfer_end(struct rpcif *rpc)
>>>> +{
>>>> +	u32 sts;
>>>> +
>>>> +	return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
>>>> +					sts & RPCIF_CMNSR_TEND, 0,
>>>
>>> Aren't you using sts undefined here?
>>
>>    No, the macro reads 'sts' from the register first.
> 
> That's confusing and ugly.
> 
> Please re-write it to the code is clear and easy to read/maintain.

   OK, I will look into this...

[...]

>>> Looking at all this code from here ...

[...]

>>> ... to here.
>>>
>>> Not sure what all this is, but it looks like it doesn't have anything
>>
>>    This is an abstracted "back end" API for the "front end" MTD/SPI drivers.
>> Only this code talks to real RPC-IF hardware. Probably needs some kernel-doc...
> 
> I suggest this needs moving out to a suitable subsystem.
> 
> Possibly MTD.
> 
>>> to do with MFD.  I suggest you move it to somewhere more appropriate.
>>
>>    Like where?
> 
> MTD?

   Well, the new idea is /drivers/memory/, right?

>>>> +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.
> 
> Lots of drivers have multiple ways to control them - they are not all
> MFDs.

   OK, sounds like drivers/mfd/ are for a single device having multiple
distinct subdevices, right?

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

   Well, both are done in software. :-)

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

   That doesn't go well with the DT spec, I'm afraid. And much code would be
duplicated in case of 2 independent drivers...

MBR, Sergei