[RFC,v1] spi: fsl_spi: Convert to transfer_one

Let the core handle all the chipselect bakery and replace
transfer_one_message() by transfer_one() and prepare_message().

At the time being, there is fsl_spi_cs_control() to handle
chipselects. That function handles both GPIO and non-GPIO
chipselects. The GPIO chipselects will now be handled by
the core directly, so only handle non-GPIO chipselects and
hook it to ->set_cs

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
Sending as an RFC as I'm not 100% sure of the correctness.
I successfully tested it on the hardware I have though.
Not sure about the change from m->is_dma_mapped to !!t->tx_dma || !!t->rx_dma
---
 drivers/spi/spi-fsl-spi.c | 157 +++++++++++---------------------------
 1 file changed, 43 insertions(+), 114 deletions(-)

Le 18/08/2022 à 15:38, Christophe Leroy a écrit :
> Let the core handle all the chipselect bakery and replace
> transfer_one_message() by transfer_one() and prepare_message().
> 
> At the time being, there is fsl_spi_cs_control() to handle
> chipselects. That function handles both GPIO and non-GPIO
> chipselects. The GPIO chipselects will now be handled by
> the core directly, so only handle non-GPIO chipselects and
> hook it to ->set_cs

Any comment for/about this conversion ?
Did I do it the right way ? Any recommendation ?

Thanks
Christophe


> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> Sending as an RFC as I'm not 100% sure of the correctness.
> I successfully tested it on the hardware I have though.
> Not sure about the change from m->is_dma_mapped to !!t->tx_dma || !!t->rx_dma
> ---
>   drivers/spi/spi-fsl-spi.c | 157 +++++++++++---------------------------
>   1 file changed, 43 insertions(+), 114 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
> index bdf94cc7be1a..731624f157fc 100644
> --- a/drivers/spi/spi-fsl-spi.c
> +++ b/drivers/spi/spi-fsl-spi.c
> @@ -111,32 +111,6 @@ static void fsl_spi_change_mode(struct spi_device *spi)
>   	local_irq_restore(flags);
>   }
>   
> -static void fsl_spi_chipselect(struct spi_device *spi, int value)
> -{
> -	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
> -	struct fsl_spi_platform_data *pdata;
> -	struct spi_mpc8xxx_cs	*cs = spi->controller_state;
> -
> -	pdata = spi->dev.parent->parent->platform_data;
> -
> -	if (value == BITBANG_CS_INACTIVE) {
> -		if (pdata->cs_control)
> -			pdata->cs_control(spi, false);
> -	}
> -
> -	if (value == BITBANG_CS_ACTIVE) {
> -		mpc8xxx_spi->rx_shift = cs->rx_shift;
> -		mpc8xxx_spi->tx_shift = cs->tx_shift;
> -		mpc8xxx_spi->get_rx = cs->get_rx;
> -		mpc8xxx_spi->get_tx = cs->get_tx;
> -
> -		fsl_spi_change_mode(spi);
> -
> -		if (pdata->cs_control)
> -			pdata->cs_control(spi, true);
> -	}
> -}
> -
>   static void fsl_spi_qe_cpu_set_shifts(u32 *rx_shift, u32 *tx_shift,
>   				      int bits_per_word, int msb_first)
>   {
> @@ -354,15 +328,11 @@ static int fsl_spi_bufs(struct spi_device *spi, struct spi_transfer *t,
>   	return mpc8xxx_spi->count;
>   }
>   
> -static int fsl_spi_do_one_msg(struct spi_master *master,
> -			      struct spi_message *m)
> +static int fsl_spi_prepare_message(struct spi_controller *ctlr,
> +				   struct spi_message *m)
>   {
> -	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
> -	struct spi_device *spi = m->spi;
> -	struct spi_transfer *t, *first;
> -	unsigned int cs_change;
> -	const int nsecs = 50;
> -	int status, last_bpw;
> +	struct mpc8xxx_spi *mpc8xxx_spi = spi_controller_get_devdata(ctlr);
> +	struct spi_transfer *t;
>   
>   	/*
>   	 * In CPU mode, optimize large byte transfers to use larger
> @@ -378,62 +348,30 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
>   				t->bits_per_word = 16;
>   		}
>   	}
> +	return 0;
> +}
>   
> -	/* Don't allow changes if CS is active */
> -	cs_change = 1;
> -	list_for_each_entry(t, &m->transfers, transfer_list) {
> -		if (cs_change)
> -			first = t;
> -		cs_change = t->cs_change;
> -		if (first->speed_hz != t->speed_hz) {
> -			dev_err(&spi->dev,
> -				"speed_hz cannot change while CS is active\n");
> -			return -EINVAL;
> -		}
> -	}
> -
> -	last_bpw = -1;
> -	cs_change = 1;
> -	status = -EINVAL;
> -	list_for_each_entry(t, &m->transfers, transfer_list) {
> -		if (cs_change || last_bpw != t->bits_per_word)
> -			status = fsl_spi_setup_transfer(spi, t);
> -		if (status < 0)
> -			break;
> -		last_bpw = t->bits_per_word;
> -
> -		if (cs_change) {
> -			fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
> -			ndelay(nsecs);
> -		}
> -		cs_change = t->cs_change;
> -		if (t->len)
> -			status = fsl_spi_bufs(spi, t, m->is_dma_mapped);
> -		if (status) {
> -			status = -EMSGSIZE;
> -			break;
> -		}
> -		m->actual_length += t->len;
> -
> -		spi_transfer_delay_exec(t);
> -
> -		if (cs_change) {
> -			ndelay(nsecs);
> -			fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
> -			ndelay(nsecs);
> -		}
> -	}
> +static int fsl_spi_transfer_one(struct spi_controller *controller,
> +				struct spi_device *spi,
> +				struct spi_transfer *t)
> +{
> +	int status;
>   
> -	m->status = status;
> +	status = fsl_spi_setup_transfer(spi, t);
> +	if (status < 0)
> +		return status;
> +	if (t->len)
> +		status = fsl_spi_bufs(spi, t, !!t->tx_dma || !!t->rx_dma);
> +	if (status > 0)
> +		return -EMSGSIZE;
>   
> -	if (status || !cs_change) {
> -		ndelay(nsecs);
> -		fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
> -	}
> +	return status;
> +}
>   
> -	fsl_spi_setup_transfer(spi, NULL);
> -	spi_finalize_current_message(master);
> -	return 0;
> +static int fsl_spi_unprepare_message(struct spi_controller *controller,
> +				     struct spi_message *msg)
> +{
> +	return fsl_spi_setup_transfer(msg->spi, NULL);
>   }
>   
>   static int fsl_spi_setup(struct spi_device *spi)
> @@ -482,9 +420,6 @@ static int fsl_spi_setup(struct spi_device *spi)
>   		return retval;
>   	}
>   
> -	/* Initialize chipselect - might be active for SPI_CS_HIGH mode */
> -	fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
> -
>   	return 0;
>   }
>   
> @@ -557,9 +492,7 @@ static void fsl_spi_grlib_cs_control(struct spi_device *spi, bool on)
>   	u32 slvsel;
>   	u16 cs = spi->chip_select;
>   
> -	if (spi->cs_gpiod) {
> -		gpiod_set_value(spi->cs_gpiod, on);
> -	} else if (cs < mpc8xxx_spi->native_chipselects) {
> +	if (cs < mpc8xxx_spi->native_chipselects) {
>   		slvsel = mpc8xxx_spi_read_reg(&reg_base->slvsel);
>   		slvsel = on ? (slvsel | (1 << cs)) : (slvsel & ~(1 << cs));
>   		mpc8xxx_spi_write_reg(&reg_base->slvsel, slvsel);
> @@ -568,7 +501,6 @@ static void fsl_spi_grlib_cs_control(struct spi_device *spi, bool on)
>   
>   static void fsl_spi_grlib_probe(struct device *dev)
>   {
> -	struct fsl_spi_platform_data *pdata = dev_get_platdata(dev);
>   	struct spi_master *master = dev_get_drvdata(dev);
>   	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
>   	struct fsl_spi_reg __iomem *reg_base = mpc8xxx_spi->reg_base;
> @@ -588,7 +520,18 @@ static void fsl_spi_grlib_probe(struct device *dev)
>   		mpc8xxx_spi_write_reg(&reg_base->slvsel, 0xffffffff);
>   	}
>   	master->num_chipselect = mpc8xxx_spi->native_chipselects;
> -	pdata->cs_control = fsl_spi_grlib_cs_control;
> +	master->set_cs = fsl_spi_grlib_cs_control;
> +}
> +
> +static void fsl_spi_cs_control(struct spi_device *spi, bool on)
> +{
> +	struct device *dev = spi->dev.parent->parent;
> +	struct fsl_spi_platform_data *pdata = dev_get_platdata(dev);
> +	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(pdata);
> +
> +	if (WARN_ON_ONCE(!pinfo->immr_spi_cs))
> +		return;
> +	iowrite32be(on ? 0 : SPI_BOOT_SEL_BIT, pinfo->immr_spi_cs);
>   }
>   
>   static struct spi_master *fsl_spi_probe(struct device *dev,
> @@ -613,8 +556,11 @@ static struct spi_master *fsl_spi_probe(struct device *dev,
>   
>   	master->setup = fsl_spi_setup;
>   	master->cleanup = fsl_spi_cleanup;
> -	master->transfer_one_message = fsl_spi_do_one_msg;
> +	master->prepare_message = fsl_spi_prepare_message;
> +	master->transfer_one = fsl_spi_transfer_one;
> +	master->unprepare_message = fsl_spi_unprepare_message;
>   	master->use_gpio_descriptors = true;
> +	master->set_cs = fsl_spi_cs_control;
>   
>   	mpc8xxx_spi = spi_master_get_devdata(master);
>   	mpc8xxx_spi->max_bits_per_word = 32;
> @@ -688,21 +634,6 @@ static struct spi_master *fsl_spi_probe(struct device *dev,
>   	return ERR_PTR(ret);
>   }
>   
> -static void fsl_spi_cs_control(struct spi_device *spi, bool on)
> -{
> -	if (spi->cs_gpiod) {
> -		gpiod_set_value(spi->cs_gpiod, on);
> -	} else {
> -		struct device *dev = spi->dev.parent->parent;
> -		struct fsl_spi_platform_data *pdata = dev_get_platdata(dev);
> -		struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(pdata);
> -
> -		if (WARN_ON_ONCE(!pinfo->immr_spi_cs))
> -			return;
> -		iowrite32be(on ? 0 : SPI_BOOT_SEL_BIT, pinfo->immr_spi_cs);
> -	}
> -}
> -
>   static int of_fsl_spi_probe(struct platform_device *ofdev)
>   {
>   	struct device *dev = &ofdev->dev;
> @@ -744,12 +675,10 @@ static int of_fsl_spi_probe(struct platform_device *ofdev)
>   		ret = gpiod_count(dev, "cs");
>   		if (ret < 0)
>   			ret = 0;
> -		if (ret == 0 && !spisel_boot) {
> +		if (ret == 0 && !spisel_boot)
>   			pdata->max_chipselect = 1;
> -		} else {
> +		else
>   			pdata->max_chipselect = ret + spisel_boot;
> -			pdata->cs_control = fsl_spi_cs_control;
> -		}
>   	}
>   
>   	ret = of_address_to_resource(np, 0, &mem);

On Thu, 18 Aug 2022 15:38:37 +0200, Christophe Leroy wrote:
> Let the core handle all the chipselect bakery and replace
> transfer_one_message() by transfer_one() and prepare_message().
> 
> At the time being, there is fsl_spi_cs_control() to handle
> chipselects. That function handles both GPIO and non-GPIO
> chipselects. The GPIO chipselects will now be handled by
> the core directly, so only handle non-GPIO chipselects and
> hook it to ->set_cs
> 
> [...]

Applied to

   broonie/spi.git for-next

Thanks!

[1/1] spi: fsl_spi: Convert to transfer_one
      commit: 64ca1a034f00bf6366701df0af9194a6425d5406

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark