Re: [PATCH 10/12] spi: add support for spi_controller::set_cs_timing

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Nov 25, 2024 at 11:08:55AM +0100, Marco Felsch wrote:
> On 24-11-25, Sascha Hauer wrote:
> > On Fri, Nov 15, 2024 at 08:57:45PM +0100, Marco Felsch wrote:
> > > Import the spi_controller set_cs_timing() hook to make it easier to port
> > > code from the Linux.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  include/spi/spi.h | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/include/spi/spi.h b/include/spi/spi.h
> > > index 8354b7d8baa6..092eacd4a8e1 100644
> > > --- a/include/spi/spi.h
> > > +++ b/include/spi/spi.h
> > > @@ -179,6 +179,9 @@ static inline void spi_set_ctldata(struct spi_device *spi, void *state)
> > >   *	must fail if an unrecognized or unsupported mode is requested.
> > >   *	It's always safe to call this unless transfers are pending on
> > >   *	the device whose settings are being modified.
> > > + * @set_cs_timing: optional hook for SPI devices to request SPI master
> > > + * controller for configuring specific CS setup time, hold time and inactive
> > > + * delay interms of clock counts
> > >   * @transfer: adds a message to the controller's transfer queue.
> > >   * @cleanup: frees controller-specific state
> > >   * @cs_gpiods: Array of GPIO descriptors to use as chip select lines; one per CS
> > > @@ -246,6 +249,16 @@ struct spi_controller {
> > >  	/* setup mode and clock, etc (spi driver may call many times) */
> > >  	int			(*setup)(struct spi_device *spi);
> > >  
> > > +	/*
> > > +	 * set_cs_timing() method is for SPI controllers that supports
> > > +	 * configuring CS timing.
> > > +	 *
> > > +	 * This hook allows SPI client drivers to request SPI controllers
> > > +	 * to configure specific CS timing through spi_set_cs_timing() after
> > > +	 * spi_setup().
> > > +	 */
> > > +	int (*set_cs_timing)(struct spi_device *spi);
> > 
> > You add this hook here, but it's not called from the core code changes
> > in the next commit and it's not implemented in the new driver you
> > introduce with this series. Is there something missing in this series?
> 
> It's checked by spi_set_cs(). I wanted to be as close as possible to the
> Linux version but can drop this hook and adapt the check. Another option
> is to add the Linux spi_set_cs_timing() to the barebox
> spi_register_controller().

I think when the hook exists there should be a chance that it is
actually called. Having it without using it just creates dead code in
the drivers.

So either implement it and use it, or remove it, whatever you prefer.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |