[PATCH 1/2 v2] spi: add SPI_MOSI_IDLE_LOW support from device tree

[PATCH 1/2 v2] spi: add SPI_MOSI_IDLE_LOW support from device tree

[charles-antoine.couret]

From: Charles-Antoine Couret <charles-antoine.couret@mind.be>

This flag was introduced but was not added as device tree property which is
limiting the possibility to use this flag on real devices.

This flag can be configured as done for other flags.

Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@mind.be>
---
 drivers/spi/spi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9b1125556d29..489a64e20305 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2363,6 +2363,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 		spi->mode |= SPI_LSB_FIRST;
 	if (of_property_read_bool(nc, "spi-cs-high"))
 		spi->mode |= SPI_CS_HIGH;
+	if (of_property_read_bool(nc, "spi-mosi-idle-low"))
+		spi->mode |= SPI_MOSI_IDLE_LOW;
 
 	/* Device DUAL/QUAD mode */
 
-- 
2.53.0

Re: [PATCH 1/2 v2] spi: add SPI_MOSI_IDLE_LOW support from device tree

[Marcelo Schmitt]

Hello Charles-Antoine,

On 03/29, charles-antoine.couret@mind.be wrote:
> From: Charles-Antoine Couret <charles-antoine.couret@mind.be>
> 
> This flag was introduced but was not added as device tree property which is
> limiting the possibility to use this flag on real devices.

I'm not seeing why a device tree property is needed for SPI idle modes. For
idling high, the configuration is requested through spi_setup(). It should
work in similar way for idling low. See spi-summary.rst. If believe a dt
property is needed despite the spi_setup() interface, can you elaborate on why?

> 
> This flag can be configured as done for other flags.
> 
> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@mind.be>
> ---
>  drivers/spi/spi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 9b1125556d29..489a64e20305 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2363,6 +2363,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  		spi->mode |= SPI_LSB_FIRST;
>  	if (of_property_read_bool(nc, "spi-cs-high"))
>  		spi->mode |= SPI_CS_HIGH;
> +	if (of_property_read_bool(nc, "spi-mosi-idle-low"))
> +		spi->mode |= SPI_MOSI_IDLE_LOW;
>  
>  	/* Device DUAL/QUAD mode */
>  
> -- 
> 2.53.0
> 
> 

With best regards,
Marcelo

Re: [PATCH 1/2 v2] spi: add SPI_MOSI_IDLE_LOW support from device tree

[Couret Charles-Antoine]

Le 29/03/26 à 16:25, Marcelo Schmitt a écrit :
> Hello Charles-Antoine,
>
> On 03/29, charles-antoine.couret@mind.be wrote:
>> From: Charles-Antoine Couret <charles-antoine.couret@mind.be>
>>
>> This flag was introduced but was not added as device tree property which is
>> limiting the possibility to use this flag on real devices.
> I'm not seeing why a device tree property is needed for SPI idle modes. For
> idling high, the configuration is requested through spi_setup(). It should
> work in similar way for idling low. See spi-summary.rst. If believe a dt
> property is needed despite the spi_setup() interface, can you elaborate on why?

Hi Marcelo,

You're right that for a compliant SPI device, this devicetree option is 
not really relevant and this must be in the driver itself. However, I 
think the purpose of this mode is itself not designed for compliant SPI 
devices.

It's not unusual to use Linux SPI subsystem for devices which are not 
fully compliant with SPI in embedded context and where both options 
(idle low or idle high) can make sense based on hardware design around 
the device or the feature that you want. So having this property in 
device tree is documenting the hardware then giving more flexibility.

For example we used that to communicate with TI DAC161P997 device, where 
"IDLE low" setting can be used to detect when the device is really 
powered or not. But this is an optional setting, this option does not 
affect the rest of the driver.

I can understand this is a corner case and you don't want to support it 
at all, I thought this can be interesting to provide it anyway. If you 
want to reject it, I understand.


Thank you for the feedback and have a nice day.

Regards,

Re: [PATCH 1/2 v2] spi: add SPI_MOSI_IDLE_LOW support from device tree

[Marcelo Schmitt]

On 03/29, Couret Charles-Antoine wrote:
> Le 29/03/26 à 16:25, Marcelo Schmitt a écrit :
> > Hello Charles-Antoine,
> > 
> > On 03/29, charles-antoine.couret@mind.be wrote:
> > > From: Charles-Antoine Couret <charles-antoine.couret@mind.be>
> > > 
> > > This flag was introduced but was not added as device tree property which is
> > > limiting the possibility to use this flag on real devices.
> > I'm not seeing why a device tree property is needed for SPI idle modes. For
> > idling high, the configuration is requested through spi_setup(). It should
> > work in similar way for idling low. See spi-summary.rst. If believe a dt
> > property is needed despite the spi_setup() interface, can you elaborate on why?
> 
> Hi Marcelo,
> 
> You're right that for a compliant SPI device, this devicetree option is not
> really relevant and this must be in the driver itself. However, I think the
> purpose of this mode is itself not designed for compliant SPI devices.
> 
> It's not unusual to use Linux SPI subsystem for devices which are not fully
> compliant with SPI in embedded context and where both options (idle low or
> idle high) can make sense based on hardware design around the device or the
> feature that you want. So having this property in device tree is documenting
> the hardware then giving more flexibility.

I agree that having an spi-mosi-idle property in dt can make the hw description
more complete. I'm not seeing how that would provide more flexibility to device
configuration. Do the controller or anything else needs to check whether a
peripheral needs a particular MOSI idle mode before the peripheral driver probes
the device itself?

> For example we used that to communicate with TI DAC161P997 device, where
> "IDLE low" setting can be used to detect when the device is really powered
> or not. But this is an optional setting, this option does not affect the
> rest of the driver.

Can't that be done with the existing support for idle modes? E.g.
		spi->mode |= SPI_MOSI_IDLE_LOW;
		ret = spi_setup(spi);
		if (ret < 0) {
			/* No controller MOSI idle low support. */
			/* Can't verify device is powered on. Return or do something else. */
		}
		/* MOSI idle low support. Verify the device is powered on. */

> I can understand this is a corner case and you don't want to support it at
> all, I thought this can be interesting to provide it anyway. If you want to
> reject it, I understand.
> 
Not rejecting neither accepting. I just don't see benefit of having an
spi-idle-mode prop from the mentioned use case. 

Re: [PATCH 1/2 v2] spi: add SPI_MOSI_IDLE_LOW support from device tree

[Couret Charles-Antoine]

Hi Marcelo,
Le 29/03/26 à 21:29, Marcelo Schmitt a écrit :
>> For example we used that to communicate with TI DAC161P997 device, where
>> "IDLE low" setting can be used to detect when the device is really powered
>> or not. But this is an optional setting, this option does not affect the
>> rest of the driver.
> Can't that be done with the existing support for idle modes? E.g.
> 		spi->mode |= SPI_MOSI_IDLE_LOW;
> 		ret = spi_setup(spi);
> 		if (ret < 0) {
> 			/* No controller MOSI idle low support. */
> 			/* Can't verify device is powered on. Return or do something else. */
> 		}
> 		/* MOSI idle low support. Verify the device is powered on. */
>
This means we need to hardcode the behaviour in the driver or to add an 
extra setting if we don't especially want to enable this.

I agree with you: there is always a way to deal with it without this 
property in the device tree. However, I don't think this means the 
property in the device tree is irrelevant.

Regards,

-- 

Charles-Antoine Couret

Embedded Software Developer

+32 488 27 56 40
Website <https://mind.be> • LinkedIn 
<https://www.linkedin.com/company/mind_essensium/> • Newslette 
<http://eepurl.com/h-gy7v>r