[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@denx.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2018年11月30日 14:47
> To: Mark Brown <broonie@kernel.org>; Robin Gong <yibin.gong@nxp.com>
> Cc: Marek Vasut <marex@denx.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-spi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> 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@denx.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  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?
>  	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
>  	u32 clk = spi_imx->speed_hz, delay;
> 
> @@ -685,9 +687,10 @@ static int mx31_prepare_message(struct
> spi_imx_data *spi_imx,
>  	return 0;
>  }
> 
> -static int mx31_config(struct spi_device *spi)
> +static int mx31_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);
>  	unsigned int reg = MX31_CSPICTRL_ENABLE | MX31_CSPICTRL_MASTER;
>  	unsigned int clk;
> 
> @@ -789,9 +792,10 @@ static int mx21_prepare_message(struct
> spi_imx_data *spi_imx,
>  	return 0;
>  }
> 
> -static int mx21_config(struct spi_device *spi)
> +static int mx21_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);
>  	unsigned int reg = MX21_CSPICTRL_ENABLE | MX21_CSPICTRL_MASTER;
>  	unsigned int max = is_imx27_cspi(spi_imx) ? 16 : 18;
>  	unsigned int clk;
> @@ -864,9 +868,10 @@ static int mx1_prepare_message(struct
> spi_imx_data *spi_imx,
>  	return 0;
>  }
> 
> -static int mx1_config(struct spi_device *spi)
> +static int mx1_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);
>  	unsigned int reg = MX1_CSPICTRL_ENABLE | MX1_CSPICTRL_MASTER;
>  	unsigned int clk;
> 
> @@ -899,7 +904,7 @@ static void mx1_reset(struct spi_imx_data *spi_imx)
> static struct spi_imx_devtype_data imx1_cspi_devtype_data = {
>  	.intctrl = mx1_intctrl,
>  	.prepare_message = mx1_prepare_message,
> -	.config = mx1_config,
> +	.prepare_transfer = mx1_prepare_transfer,
>  	.trigger = mx1_trigger,
>  	.rx_available = mx1_rx_available,
>  	.reset = mx1_reset,
> @@ -913,7 +918,7 @@ static struct spi_imx_devtype_data
> imx1_cspi_devtype_data = {  static struct spi_imx_devtype_data
> imx21_cspi_devtype_data = {
>  	.intctrl = mx21_intctrl,
>  	.prepare_message = mx21_prepare_message,
> -	.config = mx21_config,
> +	.prepare_transfer = mx21_prepare_transfer,
>  	.trigger = mx21_trigger,
>  	.rx_available = mx21_rx_available,
>  	.reset = mx21_reset,
> @@ -928,7 +933,7 @@ static struct spi_imx_devtype_data
> imx27_cspi_devtype_data = {
>  	/* i.mx27 cspi shares the functions with i.mx21 one */
>  	.intctrl = mx21_intctrl,
>  	.prepare_message = mx21_prepare_message,
> -	.config = mx21_config,
> +	.prepare_transfer = mx21_prepare_transfer,
>  	.trigger = mx21_trigger,
>  	.rx_available = mx21_rx_available,
>  	.reset = mx21_reset,
> @@ -942,7 +947,7 @@ static struct spi_imx_devtype_data
> imx27_cspi_devtype_data = {  static struct spi_imx_devtype_data
> imx31_cspi_devtype_data = {
>  	.intctrl = mx31_intctrl,
>  	.prepare_message = mx31_prepare_message,
> -	.config = mx31_config,
> +	.prepare_transfer = mx31_prepare_transfer,
>  	.trigger = mx31_trigger,
>  	.rx_available = mx31_rx_available,
>  	.reset = mx31_reset,
> @@ -957,7 +962,7 @@ static struct spi_imx_devtype_data
> imx35_cspi_devtype_data = {
>  	/* i.mx35 and later cspi shares the functions with i.mx31 one */
>  	.intctrl = mx31_intctrl,
>  	.prepare_message = mx31_prepare_message,
> -	.config = mx31_config,
> +	.prepare_transfer = mx31_prepare_transfer,
>  	.trigger = mx31_trigger,
>  	.rx_available = mx31_rx_available,
>  	.reset = mx31_reset,
> @@ -971,7 +976,7 @@ static struct spi_imx_devtype_data
> imx35_cspi_devtype_data = {  static struct spi_imx_devtype_data
> imx51_ecspi_devtype_data = {
>  	.intctrl = mx51_ecspi_intctrl,
>  	.prepare_message = mx51_ecspi_prepare_message,
> -	.config = mx51_ecspi_config,
> +	.prepare_transfer = mx51_ecspi_prepare_transfer,
>  	.trigger = mx51_ecspi_trigger,
>  	.rx_available = mx51_ecspi_rx_available,
>  	.reset = mx51_ecspi_reset,
> @@ -987,7 +992,7 @@ static struct spi_imx_devtype_data
> imx51_ecspi_devtype_data = {  static struct spi_imx_devtype_data
> imx53_ecspi_devtype_data = {
>  	.intctrl = mx51_ecspi_intctrl,
>  	.prepare_message = mx51_ecspi_prepare_message,
> -	.config = mx51_ecspi_config,
> +	.prepare_transfer = mx51_ecspi_prepare_transfer,
>  	.trigger = mx51_ecspi_trigger,
>  	.rx_available = mx51_ecspi_rx_available,
>  	.reset = mx51_ecspi_reset,
> @@ -1230,7 +1235,7 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  		spi_imx->slave_burst = t->len;
>  	}
> 
> -	spi_imx->devtype_data->config(spi);
> +	spi_imx->devtype_data->prepare_transfer(spi_imx, spi, t);
> 
>  	return 0;
>  }
> --
> 2.19.2

On Mon, Dec 03, 2018 at 08:09:59AM +0000, Robin Gong wrote:
> 
> 
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: 2018年11月30日 14:47
> > To: Mark Brown <broonie@kernel.org>; Robin Gong <yibin.gong@nxp.com>
> > Cc: Marek Vasut <marex@denx.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; linux-spi@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > 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@denx.de>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  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