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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 21/04/2023 09:45, Lee Jones wrote:
> On Fri, 21 Apr 2023, Herve Codina wrote:
> 
>> Hi Lee, Krzysztof,
>>
>> On Thu, 20 Apr 2023 14:47:03 +0100
>> Lee Jones <lee@kernel.org> wrote:
>>
>>> On Thu, 20 Apr 2023, Herve Codina wrote:
>>>
>>>> On Thu, 20 Apr 2023 13:39:46 +0100
>>>> Lee Jones <lee@kernel.org> wrote:
>>>>   
>>>>> On Mon, 17 Apr 2023, Herve Codina wrote:
>>>>>   
>>>>>> The Lantiq PEF2256 is a framer and line interface component designed to
>>>>>> fulfill all required interfacing between an analog E1/T1/J1 line and the
>>>>>> digital PCM system highway/H.100 bus.
>>>>>>
>>>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
>>>>>> ---
>>>>>>  drivers/mfd/Kconfig         |  16 +
>>>>>>  drivers/mfd/Makefile        |   1 +
>>>>>>  drivers/mfd/pef2256-regs.h  | 250 ++++++++++
>>>>>>  drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++    
>>>>>
>>>>> 95% of this driver needs to be moved somewhere else.
>>>>>
>>>>> What is a Framer?  Perhaps sound/ is a good candidate?  
>>>>
>>>> The pef2256 framer is a device that transfers data to/from a TDM (time-slots
>>>> data) from/to quite old telecommunication lines (E1 in my case).
>>>> Several subsystem can set/get data to/from the TDM. Each device using their
>>>> own time-slots set.
>>>>
>>>> On my use-case, I have some audio consumer and a not yet upstreamed HDLC
>>>> consumer. Both of them uses the framer to know the E1 link state.
>>>> The framer needs to be initialized 'globally' and not by a specific consumer
>>>> as several consumers can use the framer.  
>>>
>>> I can't think of a good place for this.
>>>
>>> If all else fails, it's drivers/misc
>>>
>>>>>>  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.

Anyway this discussions should not be about DT. How Linux drivers are
implementing DT is not really a guide how to write DT. Since these
series were brought there were some DT decisions made based how you want
to write the driver. No, please don't. I also do not see any problems in
handling more-or-less complex driver structures without poking the DT.
We have already many such device families.

Best regards,
Krzysztof