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

[jeffy]

Hi brian,

thanx for your comments.

On 06/27/2017 06:17 AM, Brian Norris wrote:
> 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.
right, i need to rewrite this.
> > 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
> 	}
ok, will do.
> > -	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/
done
> > +	 * 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."?
done
> > +	 */
> > +	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?
done
> > +
> > +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.
right, i'll mention it in commit message too.
> Brian
>
> >   static int rockchip_spi_prepare_message(struct spi_master *master,
> > --
> > 2.1.4