Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH 2/2] w1: fix the resume command API
From:   Jean-Francois Dagenais <jeff.dagenais@gmail.com>
In-Reply-To: <20190318092737.8170-3-manio@skyboo.net>
Date:   Tue, 19 Mar 2019 09:21:16 -0400
Cc:     linux-kernel@vger.kernel.org, Evgeniy Polyakov <zbr@ioremap.net>,
        Greg Kroah-Hartman <greg@kroah.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <6836A9A3-AF86-4E2F-9215-647CB36E22DA@gmail.com>
References: <20190318092737.8170-1-manio@skyboo.net>
 <20190318092737.8170-3-manio@skyboo.net>
To:     Mariusz Bialonczyk <manio@skyboo.net>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mariusz,

I appreciate your work on this.

> On Mar 18, 2019, at 05:27, Mariusz Bialonczyk <manio@skyboo.net> =
wrote:
>=20
> =46rom 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=3D=3D1 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.

>=20
> 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.
>=20
> This commit is fixing this by using a Skip ROM instead in those cases.
> The bandwidth / performance advantage is exactly the same.
>=20
> Refs:
> [1] https://datasheets.maximintegrated.com/en/ds/DS2408.pdf
>=20
> 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(-)
>=20
> 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;
>=20
> -	/* This will make only the last matched slave perform a skip =
ROM. */
> -	w1_write_8(dev, W1_RESUME_CMD);
> +	if (dev->slave_count =3D=3D 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 =3D=3D 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);
> --=20
> 2.19.0.rc1
>=20