On Mon, Nov 25, 2013 at 09:45:03AM +0000, Lee Jones wrote: > On Fri, 22 Nov 2013, Thierry Reding wrote: > > > From: Rhyland Klein <[email protected]> > > > > 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 <[email protected]> > > Reviewed-by: Bernie Thompson <[email protected]> > > Reviewed-by: Andrew Bresticker <[email protected]> > > Cc: Rob Herring <[email protected]> > > Cc: Pawel Moll <[email protected]> > > Cc: Mark Rutland <[email protected]> > > Cc: Ian Campbell <[email protected]> > > Signed-off-by: Thierry Reding <[email protected]> > > --- > > 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
pgpLozUrbW0o7.pgp
Description: PGP signature
