Re: [PATCH 2/2] w1: fix the resume command API

[Jean-Francois Dagenais]

Hi Mariusz,

I appreciate your work on this.

> On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <manio@xxxxxxxxxx> 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@xxxxxxxxxx>
> Cc: Jean-Francois Dagenais <jeff.dagenais@xxxxxxxxx>

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

> ---
> 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
>