Re: [PATCH v4 1/6] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration

[Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>]

On 06/19, Mark Brown wrote:
> On Wed, Jun 19, 2024 at 03:58:00PM -0300, Marcelo Schmitt wrote:
> > On 06/19, David Lechner wrote:
> > > On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
> 
> > > > +In this extension to the usual SPI protocol, the MOSI line state is specified to
> > > > +be kept high when CS is active but the controller is not clocking out data to
> 
> > > I think it would be less ambiguous to say "asserted" instead of "active".

ack, replaced "active" by "asserted" when describing CS state for v5.

> 
> > I'm not sure. IMHO, it looks less ambiguous to say a CS is active.
> > I think the most common for CS lines is to have a CS that is active low (i.e.
> > the line is at a low voltage level when the controller is selecting the device).
> > To me, "assert" sounds closer to the idea o setting something (like a bit to 1),
> > which is the opposite of active low CS.
> > Though, no strong opinion about it.
> > I go with what the maintainers prefer.
> 
> I think they're synonyms but asserted is the more common term for chip
> selects.
> 
> 
> > > > +#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
> > > > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */
> 
> > > I don't see where these are used anywhere else in the series. They
> > > seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH.
> 
> > Good point.
> > They are currently not being used.
> > Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be
> > handy to have dt properties to indicate controller MOSI idle capabilities.
> > Does that sound reasonable?
> 
> We shouldn't need DT properties, we should just know if the controller
> supports this based on knowing what controller is, and I'd not expect it
> to depend on board wiring.

Okay, though, I fail to see the need for 
#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */

It looks like SPI_CONTROLLER bits are used to tweak controller operation in
various ways.
Right now, I'm not aware of any additional tweak needed to support the MOSI idle
feature. I have tested that on Raspberry Pi with bitbang/gpio controller and on
CoraZ7 with spi-engine and it did work fine in those setups.
Anyway, I'm prone to implement any additional changes to make this set better.

Thanks,
Marcelo