by Krzysztof Kozlowski

[permalink] [raw]

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

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/[email protected]/

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