Re: [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support

From: Olivier MOYSAN <olivier.moysan@foss.st.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: <robh+dt@kernel.org>, <mark.rutland@arm.com>, <knaack.h@gmx.de>,
	<lars@metafoo.de>, <devicetree@vger.kernel.org>,
	<linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<pmeerw@pmeerw.net>, <linux-stm32@st-md-mailman.stormreply.com>,
	<linux-arm-kernel@lists.infradead.org>,
	Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>,
	Fabrice GASNIER <fabrice.gasnier@foss.st.com>
Subject: Re: [PATCH 4/4] iio: adc: stm32-dfsdm: add scale and offset support
Date: Wed, 29 Sep 2021 18:44:30 +0200	[thread overview]
Message-ID: <a7467649-e949-9d1d-eed6-93830bf83bb4@foss.st.com> (raw)
In-Reply-To: <20210926155607.3a7fae81@jic23-huawei>

Hi Jonathan,

>>>>
>>>> If 'backend' option turns out to be the most appropriated to match DFSDM
>>>> constraints, I can prepare some patches to support it.
>>>> Would you have some guidelines or requirements for the implementation of
>>>> such feature, in this case ?
>>>
>>> Closest example is that rcar-gyroadc but in this case we'd want to define
>>> something standard to support the modulators so that if we have other filters
>>> in future we can reuse them.
>>>
>>> That means implementing them as child devices of the filter - probably put
>>> the on the IIO bus, but as different device type.  Take a look at how
>>> triggers are done in industrialio-trigger.c
>>> You need struct device_type sd_modulator
>>> and a suitable device struct (burred in an iio_sd_modulator struct probably).
>>>
>>> Also needed would be a bunch of standard callbacks to allow you to query things
>>> like scaling.   Keep that interface simple. Until we have a lot of modulator
>>> drivers it will be hard to know exactly what is needed.  Also whilst we don't
>>> have many it is easy to modify the interface.
>>>
>>> Then have your filter driver walk it's own dt children and instantiate
>>> appropriate new elements and register them on the iio_bus.  They will have
>>> the filter as their parent.
>>>
>>> There are various examples of this sort of thing in tree.
>>> If you want a good one, drivers/cxl does a lot of this sort magic to manage
>>> a fairly complex graph of devices including some nice registration stuff to
>>> cause the correct device drivers to load automatically.
>>>
>>> Hmm.  Thinking more on this, there is an ordering issue for driver load.
>>> Instead of making the modulator nodes children of the modulator, you may need
>>> to give them their own existence and use a phandle to reference them.
>>> That will let you defer probe in the filter driver until those
>>> modulator drivers are ready.
>>>
>>> This isn't going to be particularly simple, so you may want to have a look
>>> at how various other subsystems do similar things and mock up the dependencies
>>> to make sure you have something that doesn't end up with a loop of dependencies.
>>> In some ways the modulators are on a bus below the filter, but the filter driver
>>> needs them to be in place to do the rest.
>>> You may end up with some sort of delayed load.
>>> 1. Initial filter driver load + parsing of the modulator dt children (if done that way).
>>> 2. Filter driver goes to sleep until...
>>> 3. Modulator drivers call something on the filter driver to say they are ready.
>>> 4. Filter driver finishes loading and create the IIO device etc.
>>> You'll need some reference counting etc in there to make removal safe etc but it
>>> shouldn't be 'too bad'.
>>>
>>> Good luck!
>>>
>>> Jonathan
>>>
I'am on the way to prototype this proposal for DFSDM.
Looking at your advices, I see that the current topolgy based on 
hardware consumer, already meets most of the requirements.

- SD modulators are described in DT with their own nodes and are 
referred in DFSDM nodes through their phandle.
- Dependencies at probe are managed (defer probe through 
devm_iio_hw_consumer_alloc())
- SD modulator scaling is retrieved through iio_read_channel_scale() ABI.

So, it seems that the current implementation is not so far from this 
solution.
It remains the unwanted sysfs interface for SD modulator. Or more than 
that, if I missed something ?
Instead of introducing a new device type for SD modulator, could the 
mode field be used to identify devices not requesting an IIO sysfs ?
(A dedicated mode may be used to skip sysfs register in device registration)
Otherwise let's go for a new type.

Regards
Olivier