Hi Mariusz, I appreciate your work on this.
> On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <[email protected]> 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 <[email protected]> > Cc: Jean-Francois Dagenais <[email protected]> Reviewed-by: Jean-Francois Dagenais <[email protected]> > --- > 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 >
