Messages in this thread | | | Subject | Re: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas OneWire (W1) bus | From | "Alex A. Mihaylov" <> | Date | Fri, 26 May 2017 18:50:25 +0300 |
| |
> 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.
| |