Received-SPF: pass (google.com: domain of linux-kernel+bounces-205084-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 6 Jun 2024 18:31:49 -0300
From: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
To: David Lechner <dlechner@baylibre.com>
Cc: Nuno =?iso-8859-1?Q?S=E1?= <noname.nuno@gmail.com>,
	Marcelo Schmitt <marcelo.schmitt@analog.com>, broonie@kernel.org,
	lars@metafoo.de, Michael.Hennerich@analog.com, jic23@kernel.org,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	conor+dt@kernel.org, nuno.sa@analog.com, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-spi@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 4/6] spi: spi-axi-spi-engine: Add support for MOSI
 idle configuration
Message-ID: <ZmIqxS-xUVMNH_lJ@debian-BULLSEYE-live-builder-AMD64>
References: <cover.1717539384.git.marcelo.schmitt@analog.com>
 <a6b00e84325bbe44919cc49509e837f2555367d0.1717539384.git.marcelo.schmitt@analog.com>
 <ed4fe3de-726b-4eba-a12a-d2f7b1da26d1@baylibre.com>
 <0e18b3aa83a62103b0f06ee516193c03f80abae9.camel@gmail.com>
 <f8ce5dc8-ed68-4f04-af3a-187bf0e4a3b3@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f8ce5dc8-ed68-4f04-af3a-187bf0e4a3b3@baylibre.com>

On 06/06, David Lechner wrote:
> On 6/6/24 1:51 AM, Nuno S? wrote:
> > On Wed, 2024-06-05 at 12:03 -0500, David Lechner wrote:
> >> On 6/4/24 5:43 PM, Marcelo Schmitt wrote:
> >>> Implement MOSI idle low and MOSI idle high to better support peripherals
> >>> that request specific MOSI behavior.
> >>>
> >>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> >>> ---
> >>> ?drivers/spi/spi-axi-spi-engine.c | 8 +++++++-
> >>> ?1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-
> >>> engine.c
> >>> index 0aa31d745734..549f03069d0e 100644
> >>> --- a/drivers/spi/spi-axi-spi-engine.c
> >>> +++ b/drivers/spi/spi-axi-spi-engine.c
> >>> @@ -41,6 +41,7 @@
> >>> ?#define SPI_ENGINE_CONFIG_CPHA			BIT(0)
> >>> ?#define SPI_ENGINE_CONFIG_CPOL			BIT(1)
> >>> ?#define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
> >>> +#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)
> >>> ?
> >>> ?#define SPI_ENGINE_INST_TRANSFER		0x0
> >>> ?#define SPI_ENGINE_INST_ASSERT			0x1
> >>> @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct
> >>> spi_device *spi)
> >>> ?		config |= SPI_ENGINE_CONFIG_CPHA;
> >>> ?	if (spi->mode & SPI_3WIRE)
> >>> ?		config |= SPI_ENGINE_CONFIG_3WIRE;
> >>> +	if (spi->mode & SPI_MOSI_IDLE_HIGH)
> >>> +		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
> >>> +	if (spi->mode & SPI_MOSI_IDLE_LOW)
> >>> +		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
> >>> ?
> >>> ?	return config;
> >>> ?}
> >>> @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device
> >>> *pdev)
> >>> ?		return ret;
> >>> ?
> >>> ?	host->dev.of_node = pdev->dev.of_node;
> >>> -	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
> >>> +	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE |
> >>> SPI_MOSI_IDLE_LOW
> >>> +			? | SPI_MOSI_IDLE_HIGH;
> >>> ?	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> >>> ?	host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
> >>> ?	host->transfer_one_message = spi_engine_transfer_one_message;
> >>
> >> I think we need a version check instead of setting the flags unconditionally
> >> here since older versions of the AXI SPI Engine won't support this feature.
> > 
> > Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only
> > add my r-b tag with the version change in place.
> > 
> > - Nuno S?

Nuno,

I think there will be more disscussion about this series.
Maybe better I not add the tag at all so you may check to agree with the next
patch version.

> 
> Actually, looking at [1], it looks like this could be a compile-time
> flag when the HDL is built. If it stays that way, then we would need
> a way to read that flag from a register instead of using the version.
> 
> 
> [1]: https://github.com/analogdevicesinc/hdl/pull/1320#issuecomment-2145744521

When is a driver version check needed?
Yes, older versions of SPI-Engine won't support this, but the patch set should
cause no regression. Even if loading the current ad4000 driver with
older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?)
and do what's possible without MOSI idle feature (probably only be able to do
reg access) or fail probing.

We decided to have the MOSI idle state feature for SPI-Engine configured by
writing to a dedicated bit [1] in the SPI Configuration Register [2].
Does this looks good?

[1]: https://github.com/analogdevicesinc/hdl/pull/1320/commits/941937eedae6701d253b4930d8f279c21ef3f807#diff-dc9213744b55493ca9430cd02cd62212436c2379ca121d1a2681356e6a37e22dR257
[2]: https://analogdevicesinc.github.io/hdl/library/spi_engine/instruction-format.html#spi-configuration-register

Thanks,
Marcelo