| Message ID | 1385143550-31901-1-git-send-email-treding@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 22 Nov 2013, Thierry Reding wrote: > From: Rhyland Klein <rklein@nvidia.com> > > 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@nvidia.com> > Reviewed-by: Bernie Thompson <bhthompson@chromium.org> > Reviewed-by: Andrew Bresticker <abrestic@chromium.org> > Cc: Rob Herring <rob.herring@calxeda.com> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > 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. > 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. > +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. > +{ > + 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) <snip> > + /* Check for any DT properties */ > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) No need for the first check. > + cros_ec_probe_spi_dt(ec_spi, dev); > + > spi_set_drvdata(spi, ec_dev); > ec_dev->name = "SPI"; > ec_dev->dev = dev;
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@nvidia.com> > > > > 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@nvidia.com> > > Reviewed-by: Bernie Thompson <bhthompson@chromium.org> > > Reviewed-by: Andrew Bresticker <abrestic@chromium.org> > > Cc: Rob Herring <rob.herring@calxeda.com> > > Cc: Pawel Moll <pawel.moll@arm.com> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > 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
On Mon, Nov 25, 2013 at 09:45:03AM +0000, Lee Jones wrote: > On Fri, 22 Nov 2013, Thierry Reding wrote: [...] > > 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) I just noticed that we should probably do the same for .remove(), which uses the same convention. While at it, do you want me to make the same changes for cros_ec_i2c.c as well? Thierry
On Mon, 25 Nov 2013, Thierry Reding wrote: > On Mon, Nov 25, 2013 at 09:45:03AM +0000, Lee Jones wrote: > > On Fri, 22 Nov 2013, Thierry Reding wrote: > [...] > > > 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) > > I just noticed that we should probably do the same for .remove(), which > uses the same convention. > > While at it, do you want me to make the same changes for cros_ec_i2c.c > as well? If the same issue exists there, then yes please.
> > > From: Rhyland Klein <rklein@nvidia.com> > > > > > > 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@nvidia.com> > > > Reviewed-by: Bernie Thompson <bhthompson@chromium.org> > > > Reviewed-by: Andrew Bresticker <abrestic@chromium.org> > > > Cc: Rob Herring <rob.herring@calxeda.com> > > > Cc: Pawel Moll <pawel.moll@arm.com> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > 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. Right, I just need his (or one of the other guy's) Ack(s) before I can apply the patch. > > > + /* 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? I agree that it's better than #ifdeffery, but I didn't know that if this check tested negative that the subordinate method would be optimised out by the compiler. Are you sure that happens? Also, how often is this used without DT?
On Mon, Nov 25, 2013 at 10:45:32AM +0000, Lee Jones wrote: > > > > From: Rhyland Klein <rklein@nvidia.com> > > > > > > > > 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@nvidia.com> > > > > Reviewed-by: Bernie Thompson <bhthompson@chromium.org> > > > > Reviewed-by: Andrew Bresticker <abrestic@chromium.org> > > > > Cc: Rob Herring <rob.herring@calxeda.com> > > > > Cc: Pawel Moll <pawel.moll@arm.com> > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > > --- > > > > 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. > > Right, I just need his (or one of the other guy's) Ack(s) before I can > apply the patch. Sure, no problem. > > > > + /* 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? > > I agree that it's better than #ifdeffery, but I didn't know that if > this check tested negative that the subordinate method would be > optimised out by the compiler. Are you sure that happens? I'm pretty sure that's what happens. If you have code like this (which is pretty much what the above evaluates to if !OF): static void foo(void) { ... } void bar(void) { if (0) foo(); } Then the compiler would be actively stupid if it kept foo around. Note that this is only thrown away because foo() is static and therefore it cannot be used by anything else except that dead branch. While I have never checked this personally, I remember it being highlighted as a feature of the IS_ENABLED() macro back when it was introduced. It is also quite commonly used throughout the kernel. > Also, how often is this used without DT? Well, it doesn't have a dependency on OF, so it can be used without it. In-tree there seems to be no indication that it is used without DT, but given that there's no dependency it makes sense to keep it optional. Of course we could also add the dependency if it really isn't used outside of a DT context. Thierry
> > > > > + /* 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? > > > > I agree that it's better than #ifdeffery, but I didn't know that if > > this check tested negative that the subordinate method would be > > optimised out by the compiler. Are you sure that happens? > > I'm pretty sure that's what happens. If you have code like this (which > is pretty much what the above evaluates to if !OF): > > static void foo(void) > { > ... > } > > void bar(void) > { > if (0) > foo(); > } > > Then the compiler would be actively stupid if it kept foo around. +1 > > Also, how often is this used without DT? > > Well, it doesn't have a dependency on OF, so it can be used without it. > In-tree there seems to be no indication that it is used without DT, but > given that there's no dependency it makes sense to keep it optional. > > Of course we could also add the dependency if it really isn't used > outside of a DT context. This would obviously be my preference. I'd be surprised if anyone was using this with platform data, but as you say, you never know. Perhaps it would be worth obtaining for clarification from the Google guys?
On Mon, Nov 25, 2013 at 12:10:35PM +0000, Lee Jones wrote: > > > > > > + /* 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? > > > > > > I agree that it's better than #ifdeffery, but I didn't know that if > > > this check tested negative that the subordinate method would be > > > optimised out by the compiler. Are you sure that happens? > > > > I'm pretty sure that's what happens. If you have code like this (which > > is pretty much what the above evaluates to if !OF): > > > > static void foo(void) > > { > > ... > > } > > > > void bar(void) > > { > > if (0) > > foo(); > > } > > > > Then the compiler would be actively stupid if it kept foo around. > > +1 > > > > Also, how often is this used without DT? > > > > Well, it doesn't have a dependency on OF, so it can be used without it. > > In-tree there seems to be no indication that it is used without DT, but > > given that there's no dependency it makes sense to keep it optional. > > > > Of course we could also add the dependency if it really isn't used > > outside of a DT context. > > This would obviously be my preference. I'd be surprised if anyone was > using this with platform data, but as you say, you never know. Perhaps > it would be worth obtaining for clarification from the Google guys? Bernie, Andrew, Rhyland, can anyone of you comment. Are you aware of this driver being used with a non-DT setup? I suspect maybe on Intel Chromebooks it might be, but I couldn't find any evidence of that in the upstream kernel. In fact the only upstream user of cros-ec seems to be exynos5250-snow, which I think is one of the Chromebooks, but it uses DT as well. Are there any objections to removing non-DT support from the driver? Thierry
> Bernie, Andrew, Rhyland, can anyone of you comment. Are you aware of > this driver being used with a non-DT setup? I suspect maybe on Intel > Chromebooks it might be, but I couldn't find any evidence of that in > the upstream kernel. I'm not aware of any non-DT Chromebooks that use this driver. The Intel ones are non-DT, but they don't use SPI. > Are there any objections to removing non-DT support from the driver? Seems fine to me. Thanks, Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.txt b/Documentation/devicetree/bindings/mfd/cros-ec.txt index 5f229c5f6da9..8009c3d87f33 100644 --- a/Documentation/devicetree/bindings/mfd/cros-ec.txt +++ b/Documentation/devicetree/bindings/mfd/cros-ec.txt @@ -17,6 +17,15 @@ Required properties (SPI): - compatible: "google,cros-ec-spi" - reg: SPI chip select +Optional properties (SPI): +- google,cros-ec-spi-msg-delay: Some implementations of the EC require some + additional processing time in order to accept new transactions. If the delay + between transactions is not long enough the EC may not be able to respond + properly to subsequent transactions and cause them to hang. This property + specifies the delay, in usecs, introduced between transactions to account + for the time required by the EC to get back into a state in which new data + can be accepted. + Required properties (LPC): - compatible: "google,cros-ec-lpc" - reg: List of (IO address, size) pairs defining the interface uses diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c index 27be73523b9c..e3690fcb86ae 100644 --- a/drivers/mfd/cros_ec_spi.c +++ b/drivers/mfd/cros_ec_spi.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/mfd/cros_ec.h> #include <linux/mfd/cros_ec_commands.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/slab.h> #include <linux/spi/spi.h> @@ -62,10 +63,13 @@ * @spi: SPI device we are connected to * @last_transfer_ns: time that we last finished a transfer, or 0 if there * if no record + * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that + * is sent when we want to turn off CS at the end of a transaction. */ struct cros_ec_spi { struct spi_device *spi; s64 last_transfer_ns; + unsigned int end_of_msg_delay; }; 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)); + trans.delay_usecs = ec_spi->end_of_msg_delay; + spi_message_add_tail(&trans, &msg); + } + final_ret = spi_sync(ec_spi->spi, &msg); ktime_get_ts(&ts); ec_spi->last_transfer_ns = timespec_to_ns(&ts); @@ -284,6 +299,17 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev, return 0; } +static void cros_ec_probe_spi_dt(struct cros_ec_spi *ec_spi, struct device *dev) +{ + 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) { struct device *dev = &spi->dev; @@ -305,6 +331,10 @@ static int cros_ec_probe_spi(struct spi_device *spi) if (!ec_dev) return -ENOMEM; + /* Check for any DT properties */ + if (IS_ENABLED(CONFIG_OF) && dev->of_node) + cros_ec_probe_spi_dt(ec_spi, dev); + spi_set_drvdata(spi, ec_dev); ec_dev->name = "SPI"; ec_dev->dev = dev;