Re: [PATCH v3 4/5] spi: imx: rename config callback and add useful parameters

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

On Mon, Dec 03, 2018 at 08:09:59AM +0000, Robin Gong wrote:
> 
> 
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > Sent: 2018年11月30日 14:47
> > To: Mark Brown <broonie@xxxxxxxxxx>; Robin Gong <yibin.gong@xxxxxxx>
> > Cc: Marek Vasut <marex@xxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>;
> > kernel@xxxxxxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx;
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Subject: [PATCH v3 4/5] spi: imx: rename config callback and add useful
> > parameters
> > 
> > The config callback is called once per transfer while some things can (and
> > should) be done on a per message manner. To have unambiguous naming in the
> > end include "transfer" in the callback's name and rename the implementations
> > accordingly. Also pass the driver struct and transfer which allows further
> > simplifications in the following patch.
> > 
> > There is no change in behavior intended here.
> > 
> > Reviewed-by: Marek Vasut <marex@xxxxxxx>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > ---
> >  drivers/spi/spi-imx.c | 39 ++++++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > d954b6d958c2..72c879226abd 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -60,7 +60,8 @@ struct spi_imx_data;
> >  struct spi_imx_devtype_data {
> >  	void (*intctrl)(struct spi_imx_data *, int);
> >  	int (*prepare_message)(struct spi_imx_data *, struct spi_message *);
> > -	int (*config)(struct spi_device *);
> > +	int (*prepare_transfer)(struct spi_imx_data *, struct spi_device *,
> > +				struct spi_transfer *);
> >  	void (*trigger)(struct spi_imx_data *);
> >  	int (*rx_available)(struct spi_imx_data *);
> >  	void (*reset)(struct spi_imx_data *);
> > @@ -556,9 +557,10 @@ static int mx51_ecspi_prepare_message(struct
> > spi_imx_data *spi_imx,
> >  	return 0;
> >  }
> > 
> > -static int mx51_ecspi_config(struct spi_device *spi)
> > +static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
> > +				       struct spi_device *spi,
> > +				       struct spi_transfer *t)
> >  {
> > -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
> Since spi_imx could be get from spi as before, why not remove the parameter *spi_imx for
> prepare_transfer() function?

My rationale to add spi_imx was that the caller already has it and this
function needs it. So it seems natural for me to pass it as argument.
Also the prepare_message callback gets the driver data so it also looks
consistent to me. Also it might save a few cycles to follow the pointer
chain (spi->master->dev.driver_data) if the compiler doesn't notice it
already has the requested value. But I don't feel like arguing and
didn't check if it makes a difference here in the binary.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |