Re: [PATCH] spi: rockchip: Keep master alive when CS asserted

[Brian Norris]

Hi Jeffy,

On Mon, Jun 26, 2017 at 11:10:06AM +0800, Jeffy Chen wrote:
> The cros_ec requires CS line to be active after last message. But the CS
> would be toggled when powering off/on rockchip spi, which breaks ec xfer.

I believe Doug's point was larger than just the CS line. He was
guessing that you're also stopping driving any of the other pins (e.g.,
CLK), which could cause more problems. Seems like it'd be good to note
that in the second sentence here.

> Keep spi alive after CS asserted to prevent that.
> 
> Suggested-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
> 
> ---
> 
>  drivers/spi/spi-rockchip.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index acf31f3..df016a1 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -264,7 +264,7 @@ static inline u32 rx_max(struct rockchip_spi *rs)
>  
>  static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
>  {
> -	u32 ser;
> +	u32 ser, new_ser;
>  	struct spi_master *master = spi->master;
>  	struct rockchip_spi *rs = spi_master_get_devdata(master);
>  
> @@ -288,13 +288,25 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
>  	 * Note: enable(rockchip_spi_set_cs) = !enable(spi_set_cs)
>  	 */
>  	if (!enable)
> -		ser |= 1 << spi->chip_select;
> +		new_ser = ser | BIT(spi->chip_select);
>  	else
> -		ser &= ~(1 << spi->chip_select);
> +		new_ser = ser & ~BIT(spi->chip_select);
>  
> -	writel_relaxed(ser, rs->regs + ROCKCHIP_SPI_SER);
> +	if (new_ser == ser)
> +		goto out;

Hmm, this doens't exactly feel like a situation for 'goto'. Personally,
an indented 'if' block would make this clearer:

	if (new_ser != ser) {
		... // all the stuff you do only on CS edges
	}

>  
> -	pm_runtime_put_sync(rs->dev);
> +	writel_relaxed(new_ser, rs->regs + ROCKCHIP_SPI_SER);
> +
> +	/*
> +	 * The rockchip spi would stop driving CS when power down.

Replace "CS" with "all pins"? And:
s/power/powered/

> +	 * So we need to keep it alive after CS asserted

To make this clearer, note that we're keeping an unbalanced runtime PM
reference on purpose. Like instead of "we need to keep it alive after CS
asserted", maybe "we hold a runtime PM reference as long as CS is
asserted."?

> +	 */
> +	if (!enable)
> +		return;
> +	pm_runtime_put(rs->dev);

I still had to read this through a few times to be sure it was right (I
think it's correct?). Maybe to make this extra clear, a comment like
"Drop reference from when we first asserted CS" could go above this?

> +
> +out:
> +	pm_runtime_put(rs->dev);

You've dropped the "_sync()" that used to be here. I think that's
probably fine, but I wanted to call it out.

Brian

>  
>  static int rockchip_spi_prepare_message(struct spi_master *master,
> -- 
> 2.1.4
> 
>