Re: [PATCH v4 2/9] w1_therm: fix reset_select_slave at beginning of search process

[Greg KH <greg@kroah.com>]

On Thu, Apr 30, 2020 at 12:59:43AM +0200, Akira Shimahara wrote:
> Fix reset_select_slave issue during devices discovery by the master on
> bus. The w1_reset_select_slave() from w1_io.c, which was previously used,
> assume that if the slave count is 1 there is only one slave attached on
> the bus. This is not always true. For example when discovering devices,
> when the first device is discover by the bus master, its slave count is
> 1, but some other slaves may be on the bus.
> 
> In that case instead of adressing command to the attached slave the 
> master throw a SKIP ROM command so that all slaves attached on the bus
> will answer simultenaously causing data collision.
> 
> A dedicated reset_select_slave() function is implemented here,
> it always perform an adressing to each slave using the MATCH ROM
> command.
> 
> Signed-off-by: Akira Shimahara <akira215corp@gmail.com>
> ---
>  drivers/w1/slaves/w1_therm.c | 29 ++++++++++++++++++++++-------
>  drivers/w1/slaves/w1_therm.h | 13 +++++++++++++
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index f027360..6245950 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -235,7 +235,7 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
>  	while (max_trying--) {
>  		crc = 0;
>  
> -		if (!w1_reset_select_slave(sl)) {
> +		if (!reset_select_slave(sl)) {
>  			int count = 0;
>  
>  			/* read values to only alter precision bits */
> @@ -248,7 +248,7 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
>  			if (rom[8] == crc) {
>  				rom[4] = (rom[4] & ~mask) | (precision_bits & mask);
>  
> -				if (!w1_reset_select_slave(sl)) {
> +				if (!reset_select_slave(sl)) {
>  					w1_write_8(dev, W1_WRITE_SCRATCHPAD);
>  					w1_write_8(dev, rom[2]);
>  					w1_write_8(dev, rom[3]);
> @@ -319,6 +319,21 @@ static void w1_therm_remove_slave(struct w1_slave *sl)
>  
>  /*------------------------Hardware Functions--------------------------*/
>  
> +/* Safe version of reser_select_slave - avoid using the one in w_io.c */
> +static int reset_select_slave(struct w1_slave *sl)
> +{
> +	u8 match[9] = { W1_MATCH_ROM, };
> +	u64 rn = le64_to_cpu(*((u64 *)&sl->reg_num));
> +
> +	if (w1_reset_bus(sl->master))
> +		return -ENODEV;
> +
> +	memcpy(&match[1], &rn, 8);
> +	w1_write_block(sl->master, match, 9);
> +
> +	return 0;
> +}

If you put this higher up in the .c file, no function definition is
needed in the .h file at all, right?

thanks,

greg k-h