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;
From:   Evgeniy Polyakov <zbr@ioremap.net>
Envelope-From: drustafa@yandex.ru
To:     Jean-Francois Dagenais <jeff.dagenais@gmail.com>,
        Mariusz Bialonczyk <manio@skyboo.net>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <greg@kroah.com>
In-Reply-To: <6836A9A3-AF86-4E2F-9215-647CB36E22DA@gmail.com>
References: <20190318092737.8170-1-manio@skyboo.net>
         <20190318092737.8170-3-manio@skyboo.net> <6836A9A3-AF86-4E2F-9215-647CB36E22DA@gmail.com>
Subject: Re: [PATCH 2/2] w1: fix the resume command API
MIME-Version: 1.0
Date:   Tue, 19 Mar 2019 17:21:09 +0300
Message-Id: <9351961553005269@iva4-406defa25fee.qloud-c.yandex.net>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=utf-8
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi everyone

Mariusz, thank you for the patch, a small comment on it logic

19.03.2019, 16:21, "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);

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