Re: [PATCH 2/2] w1: fix the resume command API
Hi Evgeniy, On Tue, 19 Mar 2019 17:21:09 +0300 Evgeniy Polyakov wrote: > Looks like this may break the search logic, can you check that with this > patch applied > some other single slave device will correctly 'tell' its id and it can be > addressed via match rom (like, basically, just reading temperature or > something like that)? Yes, I have tested it in a mixed environment (regarding single slave/multidrop): /sys/devices/w1_bus_master5 # ll total 0 drwxr-xr-x9 root 00 Mar 21 10:53 . drwxr-xr-x 17 root 00 Mar 2 16:49 .. drwxr-xr-x4 root 00 Mar 21 10:55 28-0921ff5e drwxr-xr-x3 root 00 Mar 21 11:04 29-001246a4 drwxr-xr-x3 root 00 Mar 21 11:04 29-001246b1 drwxr-xr-x3 root 00 Mar 21 11:04 29-001246b9 drwxr-xr-x3 root 00 Mar 21 10:54 29-001246bc drwxr-xr-x3 root 00 Mar 21 10:53 3a-000f354f /sys/devices/w1_bus_master6 # ll total 0 drwxr-xr-x4 root 00 Mar 21 10:54 . drwxr-xr-x 17 root 00 Mar 2 16:49 .. drwxr-xr-x3 root 00 Mar 21 11:07 29-001246a9 No problem with searching and using other slaves on the bus. regards, -- Mariusz Białończyk | xmpp/e-mail: ma...@skyboo.net https://skyboo.net | https://github.com/manio
Re: [PATCH 2/2] w1: fix the resume command API
Hi everyone
Mariusz, thank you for the patch, a small comment on it logic
19.03.2019, 16:21, "Jean-Francois Dagenais" :
>> ---
>> 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);
Looks like this may break the search logic, can you check that with this patch
applied
some other single slave device will correctly 'tell' its id and it can be
addressed via match rom (like, basically, just reading temperature or something
like that)?
>> + } 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
Re: [PATCH 2/2] w1: fix the resume command API
Hi Mariusz,
I appreciate your work on this.
> On Mar 18, 2019, at 05:27, Mariusz Bialonczyk 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
> Cc: Jean-Francois Dagenais
Reviewed-by: Jean-Francois Dagenais
> ---
> 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
>
[PATCH 2/2] w1: fix the resume command API
>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"
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
Cc: Jean-Francois Dagenais
---
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);
+ }
return 0;
}
EXPORT_SYMBOL_GPL(w1_reset_resume_command);
--
2.19.0.rc1
