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

[Robin Gong <yibin.gong@xxxxxxx>]

> -----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?
>  	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