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

[David Lechner <dlechner@baylibre.com>]

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.

> 
> 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.

> 
> Thanks for the quick reply!
> Jorge