RE: [Patch V3 2/2] spi: imx: add lpspi bus driver

[Pandy Gao <pandy.gao@xxxxxxx>]

 From: Mark Brown <mailto:broonie@xxxxxxxxxx>  Sent: Wednesday, November 23, 2016 3:13 AM
> To: Pandy Gao <pandy.gao@xxxxxxx>
> Cc: robh@xxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx; Frank Li
> <frank.li@xxxxxxx>; Andy Duan <fugang.duan@xxxxxxx>
> Subject: Re: [Patch V3 2/2] spi: imx: add lpspi bus driver
> 
> On Tue, Nov 22, 2016 at 09:52:17PM +0800, Gao Pan wrote:
> > This patch adds lpspi driver to support new i.MX products which use
> > lpspi instead of ecspi.
> 
> I'll apply this but there's some small issues, can you please fix them up
> with incremental patches:
>
> > +static int fsl_lpspi_prepare_message(struct spi_master *master,
> > +				     struct spi_message *msg)
> > +{
> > +	struct fsl_lpspi_data *fsl_lpspi =
> spi_master_get_devdata(master);
> > +
> > +	return clk_prepare_enable(fsl_lpspi->clk);
> > +}
> 
> Rather than doing this per message it's a bit better to do it in
> prepare_transfer_hardware(), that way if there's a sequence of messages
> queued one after another we don't turn the clock on and off all the time.
> In practice it's likely to make a negligable difference to performance but
> it's just better practice.
> 
> > +static int fsl_lpspi_txfifo_empty(struct fsl_lpspi_data *fsl_lpspi) {
> > +	u32 txcnt;
> > +	unsigned long orig_jiffies = jiffies;
> > +
> > +	do {
> > +		txcnt = readl(fsl_lpspi->base + IMX7ULP_FSR) & 0xff;
> > +
> > +		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500)))
> {
> > +			dev_dbg(fsl_lpspi->dev, "txfifo empty
> timeout\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +		schedule();
> 
> Call cond_resched() instead, it has the effect you're looking for in a more
> modern way.
 
Thanks, will submit  incremental patches to fix them up.

Best Regards
Gao Pan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html