Re: [PATCH] mfd: cros ec: spi: Add delay for raising CS

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3370 bytes --]

On Mon, Nov 25, 2013 at 09:45:03AM +0000, Lee Jones wrote:
> On Fri, 22 Nov 2013, Thierry Reding wrote:
> 
> > From: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > The EC has specific timing it requires. Add support for an optional delay
> > after raising CS to fix timing issues. This is configurable based on
> > a DT property "google,cros-ec-spi-msg-delay".
> > 
> > If this property isn't set, then no delay will be added. However, if set
> > it will cause a delay equal to the value passed to it to be inserted at
> > the end of a transaction.
> > 
> > Signed-off-by: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Bernie Thompson <bhthompson-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > Changes in v2:
> > - make property description more verbose
> > 
> >  Documentation/devicetree/bindings/mfd/cros-ec.txt |  9 +++++++
> 
> We need a DT dude to look over this.

I think Mark Rutland looked at this last week and I think I've addressed
all his comments. Hopefully he'll find the time to review this.

> >  drivers/mfd/cros_ec_spi.c                         | 30 +++++++++++++++++++++++
> >  2 files changed, 39 insertions(+)
> 
> <snip>
> 
> >  static void debug_packet(struct device *dev, const char *name, u8 *ptr,
> > @@ -238,6 +242,17 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
> >  
> >  	/* turn off CS */
> >  	spi_message_init(&msg);
> > +
> > +	if (ec_spi->end_of_msg_delay) {
> > +		/*
> > +		 * Add delay for last transaction, to ensure the rising edge
> > +		 * doesn't come too soon after the end of the data.
> > +		 */
> > +		memset(&trans, '\0', sizeof(trans));
> 
> Just use the usual 0 for the third parameter.

Will fix.

> > +static void cros_ec_probe_spi_dt(struct cros_ec_spi *ec_spi, struct device *dev)
> 
> Traditionally we have 'probe' as the last word in the function name.

Okay.

> > +{
> > +	struct device_node *np = dev->of_node;
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> > +	if (!ret)
> > +		ec_spi->end_of_msg_delay = val;
> > +}
> > +
> >  static int cros_ec_probe_spi(struct spi_device *spi)
> 
> Can you send a pre-patch to fix this too please:
>   static int cros_ec_spi_probe(struct spi_device *spi)

Yes, I will.

> <snip>
> 
> > +	/* Check for any DT properties */
> > +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> 
> No need for the first check.

Why not? While it is true that dev->of_node would be enough to determine
that the device was instantiated from a device tree, the IS_ENABLED()
will allow the compiler to throw away cros_ec_spi_dt_probe() if OF isn't
enabled. At the same time it's nicer than #ifdeffery sprinkled across
the file and it actually compile-tests all the code. Win-win-win, isn't
it?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]