Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner <dlechner@baylibre.com>]

On 6/3/25 2:29 AM, Jorge Marques wrote:
> On Mon, Jun 02, 2025 at 12:23:40PM -0500, David Lechner wrote:
>> On 6/2/25 11:32 AM, Jorge Marques wrote:
>>> Hi David,
>>>
>>> On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote:
>>>> On 6/2/25 4:17 AM, Jorge Marques wrote:
>>>>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
>>>>>> On 4/29/25 8:48 AM, Jorge Marques wrote:
>>>>>>> Hi David, 
>>>>>>>
>>>>>>> I didn't went through your's and Jonathan's ad4052.c review yet,
>>>>>>> but for the trigger-source-cells I need to dig deeper and make
>>>>>>> considerable changes to the driver, as well as hardware tests.
>>>>>>> My idea was to have a less customizable driver, but I get that it is
>>>>>>> more interesting to make it user-definable.
>>>>>>
>>>>>> We don't need to make the driver support all possibilities, but the devicetree
>>>>>> needs to be as complete as possible since it can't be as easily changed in the
>>>>>> future.
>>>>>>
>>>>>
>>>>> Ack.
>>>>>
>>>>> I see that the node goes in the spi controller (the parent). To use the
>>>>> same information in the driver I need to look-up the parent node, then
>>>>> the node. I don't plan to do that in the version of the driver, just an
>>>>> observation.
>>>>>
>>>>> There is something else I want to discuss on the dt-bindings actually.
>>>>> According to the schema, the spi-max-frequency is:
>>>>>
>>>>>   > Maximum SPI clocking speed of the device in Hz.
>>>>>
>>>>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
>>>>> (higher, depends on VIO). The solution I came up, to not require a
>>>>> custom regmap spi bus, is to have spi-max-frequency bound the
>>>>> Configuration mode speed,
>>>>
>>>> The purpose of spi-max-frequency in the devicetree is that sometimes
>>>> the wiring of a complete system makes the effective max frequency
>>>> lower than what is allowed by the datasheet. So this really needs
>>>> to be the absolute highest frequency allowed.
>>>>
>>>>> and have ADC Mode set by VIO regulator
>>>>> voltage, through spi_transfer.speed_hz. At the end of the day, both are
>>>>> bounded by the spi controller maximum speed.
>>>>
>>>> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code
>>>> uses spi-max-frequency. So I don't think this would actually work.
>>>>
>>> Ok, so that's something that may be worth some attention.
>>>
>>> At spi/spi.c#2472
>>> 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
>>> 		spi->max_speed_hz = value;
>>>
>>> At spi/spi.c#4090
>>> 	if (!xfer->speed_hz)
>>> 		xfer->speed_hz = spi->max_speed_hz;
>>>
>>> So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and
>>> not bounded by it.
>>
>> Ah, OK, my memory was wrong. It is only bound by the controller max
>> speed, not the device max speed.
>>
>> 	if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz)
>> 		xfer->speed_hz = ctlr->max_speed_hz;
>>
>> It does seem odd that it would allow setting an individual xfer
>> speed higher than than the given device max speed. I suppose we
>> could submit a patch adding that check to the SPI core code and
>> see what Mark has to say.
>>
> 
> Agreed, the patch itself would be simple:
> 
>  	if (!xfer->speed_hz || xfer->speed_hz > spi->max_speed_hz)
>  		xfer->speed_hz = spi->max_speed_hz;
> 
> But I wonder how many drivers rely on this behaviour

Only one way to find out. Try it. :-)

>>>
>>> Then at spi-axi-spi-engine.c:
>>>
>>> 	static int spi_engine_precompile_message(struct spi_message *msg)
>>> 	{
>>>   		clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz);
>>> 		xfer->effective_speed_hz = max_hz / min(clk_div, 256U);
>>> 	}
>>>
>>> Where max_hz is set only by the IP spi_clk. If at the driver I set
>>> xfer.speed_hz, it won't be bounded by max-spi-frequency.
>>>
>>> The only that seems to bound as described is the layer for flash memory
>>> at spi-mem.c@spi_mem_adjust_op_freq.
>>>
>>> For the adc driver, I will then consider your behavioral description and
>>> create a custom regmap bus to limit set the reg access speed (fixed),
>>> and keep adc mode speed set by VIO. And consider spi-max-frequency can
>>> further reduce both speeds.
>>> (or should instead be handled at the driver like spi-mem.c ?)
>>
>> It would be more work, but if it is common enough, we could generalize this
>> in the core code. For example add a spi-register-max-frequency binding (or
>> even a more general spi-max-freqency-map to map operations to max frequencies).
>> Then we could bake it into the regmap_spi code to handle this property
>> and not have to make a separate bus.
>>
>> FWIW, there are also some SPI TFT displays that use a different frequency
>> for register access compared to framebuffer data that could potentially
>> use this too. Right now, these just have a hard-coded register access
>> frequency of e.g. 10 MHz.
>>
> 
> I implemented the custom regmap bus for this series.

Good plan.

> With a `spi-max-frequency-map`, the regmap bus can be removed.
> I don't want to include this regmap spi patch to this series.
> As I see it, struct regmap_but first need to be extended to add
> a max_speed, e.g.
>   
>    @max_speed: Max transfer speed that can be used on the bus.
> 
> regmap_spi.c would then look for the devicetree node to fill the value
> and on regmap_write/read fill speed_hz.
> In this case, it could be called "register-frequency" or
> "regmap-frequency"
> If instead it is up to spi.c to read the devicetree node, then a way to
> differentiate "regular" transfers from "regmap" transfers would be
> necessary.
> 
> About submitting v3, should I submit only up-to the base driver, or can
> I submit also the add offload support and add event support commits?

I wouldn't add anything new at this point. Being able to spread out
the review a bit will lead to better reviews.