Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <786adb30-1b5e-96d7-358b-0b4d3e89964c@linaro.org>
Date:   Mon, 24 Apr 2023 16:11:30 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v6 3/7] mfd: Add support for the Lantiq PEF2256 framer
Content-Language: en-US
To:     Lee Jones <lee@kernel.org>
Cc:     Herve Codina <herve.codina@bootlin.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
References: <20230417171601.74656-1-herve.codina@bootlin.com>
 <20230417171601.74656-4-herve.codina@bootlin.com>
 <20230420123946.GB996918@google.com> <20230420151551.78c1288b@bootlin.com>
 <20230420134703.GC996918@google.com> <20230421092645.56127f11@bootlin.com>
 <20230421074558.GQ996918@google.com>
 <054a384b-b0ef-2697-4a70-09868694ec6d@linaro.org>
 <20230424095200.GC8035@google.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230424095200.GC8035@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@kernel.org/

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