Message-ID: <65m4itn5xp3ytc7hvpskuk4kmu54wznk4m2odt7d5a5k35vy26@ekjxegpjy5wq>
Date: Tue, 3 Jun 2025 09:29:29 +0200
From: Jorge Marques <gastmaier@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Uwe Kleine-König <ukleinek@...nel.org>, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

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
> > 
> > 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@..._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.
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?

Regards,
Jorge

Powered by blists - more mailing lists