lkml.org 
[lkml]   [2019]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] w1: fix the resume command API
From
Date
Hi Mariusz,

I appreciate your work on this.

> On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <manio@skyboo.net> wrote:
>
> From the DS2408 datasheet [1]:
> "Resume Command function checks the status of the RC flag and, if it is set,
> directly transfers control to the control functions, similar to a Skip ROM
> command. The only way to set the RC flag is through successfully executing
> the Match ROM, Search ROM, Conditional Search ROM, or Overdrive-Match ROM
> command"

Indeed, figure 12-2 flow chart shows that SKIP_ROM resets RC to 0, then RESUME
looks for RC==1 to enter the control function "mode". Nice find!

I don't know however if other slaves are like that. Since the true impact of
your suggested change is indeed null on the bus (bit count wise). I guess even
this specific slave case is enough to warrant the change in the subsys.

>
> The function currently works perfectly fine in a multidrop bus, but when we
> have only a single slave connected, then only a Skip ROM is used and Match
> ROM is not called at all. This is leading to problems e.g. with single one
> DS2408 connected, as the Resume Command is not working properly and the
> device is responding with failing results after the Resume Command.
>
> This commit is fixing this by using a Skip ROM instead in those cases.
> The bandwidth / performance advantage is exactly the same.
>
> Refs:
> [1] https://datasheets.maximintegrated.com/en/ds/DS2408.pdf
>
> Signed-off-by: Mariusz Bialonczyk <manio@skyboo.net>
> Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>

Reviewed-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>

> ---
> drivers/w1/w1_io.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c
> index 0364d3329c52..4697136b9027 100644
> --- a/drivers/w1/w1_io.c
> +++ b/drivers/w1/w1_io.c
> @@ -432,8 +432,15 @@ int w1_reset_resume_command(struct w1_master *dev)
> if (w1_reset_bus(dev))
> return -1;
>
> - /* This will make only the last matched slave perform a skip ROM. */
> - w1_write_8(dev, W1_RESUME_CMD);
> + if (dev->slave_count == 1) {
> + /* Resume Command has to be preceeded with e.g. Match ROM which is
> + * not happening on single-slave buses, just do a Skip ROM instead
> + */
> + w1_write_8(dev, W1_SKIP_ROM);
> + } else {
> + /* This will make only the last matched slave perform a skip ROM. */
> + w1_write_8(dev, W1_RESUME_CMD);
> + }

This may be a subsys maintainer's style preference, but perhaps the verbose comments
might be better suited for the git commit message. Could this then be shorted to

if (dev->slave_count == 1)
w1_write_8(dev, W1_SKIP_ROM);
else
w1_write_8(dev, W1_RESUME_CMD);

Or maybe:

w1_write_8(dev, dev->slave_count > 1 ? W1_RESUME_CMD : W1_SKIP_ROM);


I am also ok with this proposed version, hence the "reviewed-by".

> return 0;
> }
> EXPORT_SYMBOL_GPL(w1_reset_resume_command);
> --
> 2.19.0.rc1
>

\
 
 \ /
  Last update: 2019-03-19 14:21    [W:0.136 / U:3.736 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site