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:4.386 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site