Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1651447

Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas OneWire (W1) bus

Show all headers | View raw

> This looks mostly fine, a couple of small things and like I said in
> reply to Greg please use subject lines matching the style for the
> subsystem - this makes it a lot easier for people to identify relevant
> patches.
>
>> +	int ret = -ENODEV;
>> +
>> +
>> +	if (reg > 255)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&sl->master->bus_mutex);
>> +	if (!w1_reset_select_slave(sl)) {
>> +		w1_write_8(sl->master, W1_CMD_READ_DATA);
>> +		w1_write_8(sl->master, reg);
>> +		*val = w1_read_8(sl->master);
>> +		ret = 0;
>> +	}
>> +	mutex_unlock(&sl->master->bus_mutex);
> This is a bit confusing with how -ENODEV is generated - move the
> assignment into the if statement so it doesn't look like we're silently
> ignoring errors unless you look back to the top of the function.
Ok. I set default return value to -ENODEV. W1 (OneWire) Bus periodically 
scan for connected devices. Typical time between scans about 60 sec. 
This period W1 slave device can present in kernel device list, but will 
physically disconnected.

Only w1_reset_select_slave(sl) can say me about device still physically 
accessible. Only on success w1_reset_select_slave return zero. I think 
code, like

if (!w1_reset_select_slave(sl)) {
   [...]
   ret =0;
} else
   ret = -ENODEV;

not good.

>> +static struct regmap_bus regmap_w1_bus_a8_v16 = {
>> +	.reg_read = w1_reg_a8_v16_read,
>> +	.reg_write = w1_reg_a8_v16_write,
>> +};
> It'd be clearer to just have all all these structs at the end of the set
> of functions rather than scattered about randomly.
Ok. I move this structs down in next version of patches.

I hope that the next edition will not contain such a large number of 
registration errors. Sorry for all.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas OneWire (W1) bus "Alex A. Mihaylov" <minimumlaw@rambler.ru> - 2017-05-25 19:50 +0200
  Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas  OneWire (W1) bus Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-05-25 21:00 +0200
    Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas  OneWire (W1) bus Mark Brown <broonie@kernel.org> - 2017-05-26 12:00 +0200
  Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas  OneWire (W1) bus Mark Brown <broonie@kernel.org> - 2017-05-26 12:30 +0200
    Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas  OneWire (W1) bus "Alex A. Mihaylov" <minimumlaw@rambler.ru> - 2017-05-26 18:00 +0200
  Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas  OneWire (W1) bus Mark Brown <broonie@kernel.org> - 2017-05-26 13:10 +0200
    Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas  OneWire (W1) bus "Alex A. Mihaylov" <minimumlaw@rambler.ru> - 2017-05-26 18:20 +0200
    Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas  OneWire (W1) bus Mark Brown <broonie@kernel.org> - 2017-05-26 19:20 +0200

csiph-web