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

[Jorge Marques]

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.

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 ?)

Thanks for the quick reply!
Jorge