Re: [PATCH v6 3/7] mfd: Add support for the Lantiq PEF2256 framer

[Krzysztof Kozlowski]

On 24/04/2023 11:52, Lee Jones wrote:
>>>>>>>>  include/linux/mfd/pef2256.h |  52 ++
>>>>>>>>  5 files changed, 1269 insertions(+)
>>>>>>>>  create mode 100644 drivers/mfd/pef2256-regs.h
>>>>>>>>  create mode 100644 drivers/mfd/pef2256.c
>>>>>>>>  create mode 100644 include/linux/mfd/pef2256.h    
>>>>>>>
>>>>>>> [...]
>>>>>>>   
>>>>>>>> +static int pef2256_add_audio_devices(struct pef2256 *pef2256)
>>>>>>>> +{
>>>>>>>> +	const char *compatible = "lantiq,pef2256-codec";
>>>>>>>> +	struct mfd_cell *audio_devs;
>>>>>>>> +	struct device_node *np;
>>>>>>>> +	unsigned int count = 0;
>>>>>>>> +	unsigned int i;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	for_each_available_child_of_node(pef2256->dev->of_node, np) {
>>>>>>>> +		if (of_device_is_compatible(np, compatible))
>>>>>>>> +			count++;
>>>>>>>> +	}    
>>>>>>>
>>>>>>> Converting Device Tree nodes into MFD cells to register with the
>>>>>>> Platform Device API is not a reasonable use-case of MFD.
>>>>>>>
>>>>>>> Have the CODEC driver match on "lantiq,pef2256-codec" and let it
>>>>>>> instantiate itself.  
>>>>>>
>>>>>> As the framer is going to used by several subsystem, I cannot instantiate
>>>>>> it in the specific ASoC subsystem.
>>>>>>   
>>>>>>>
>>>>>>> Your first version using of_platform_populate() was closer to the mark.  
>>>>>>
>>>>>> The issue was that I need MFD cells for the pinctrl part.  
>>>>>
>>>>> Why can't it be represented in DT?
>>>>
>>>> The pinctrl part has no specific compatible string.
>>>> Not sure that a compatible string for pinctrl can be accepted
>>>> as there is only one pinctrl subnode and no specific reg for this
>>>> subnode.
>>>>
>>>> The DT looks like this:
>>>>     framer@2000000 {
>>>>       compatible = "lantiq,pef2256";
>>>>       reg = <0x2000000 0x100>;
>>>>       ...
>>>>       pinctrl {
>>>>         pef2256_rpa_sypr: rpa-pins {
>>>>           pins = "RPA";
>>>>           function = "SYPR";
>>>>         };
>>>>       };
>>>>
>>>>       pef2256_codec0: codec-0 {
>>>>         compatible = "lantiq,pef2256-codec";
>>>>         #sound-dai-cells = <0>;
>>>>         sound-name-prefix = "PEF2256_0";
>>>>       };
>>>>     };
>>>>
>>>> Krzysztof, is it acceptable to have a compatible string in the pinctrl node ?
>>>
>>> Why wouldn't it be?
>>>
>>> $ git grep ".compatible" -- drivers/pinctrl/
>>>
>>>> In this case, it will looks like this:
>>>>     framer@2000000 {
>>>>       compatible = "lantiq,pef2256";
>>>>       reg = <0x2000000 0x100>;
>>>>       ...
>>>>       pinctrl {
>>>>         compatible = "lantiq,pef2256-pinctrl";
>>
>> If you do not have any resources, there is no point in having separate
>> compatible for separate device node.
> 
> That's a new rule.  Is that documented somewhere?  I'm sure we already
> have device nodes for devices whom only operate on shared resources. 

Let me clarify - no need for separate node for such case, when this is
in general one device and it's sub-block does not look re-usable. For
SoC blocks it is a bit different. For PMICs which pretty often re-use
pieces between different devices, as well.

But here there is not much benefit of separate device node for pinctrl.

Whether rule is new? Dunno, depends, I saw it from reviews from Rob
since long time, e.g.:
https://lore.kernel.org/all/20220902172808.GB52527-robh@xxxxxxxxxx/

Maybe this is a bit different because of children - pinconf settings?
But I would still look at this as:
1. For a re-usable sub-block: separate device node and compatible is useful,
2. Non-reusable but having a child node only to group children like pin
configuration nodes: no need for compatible.

Best regards,
Krzysztof