lkml.org 
[lkml]   [2017]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/4] Introduce regmap infrastructure over Maxim/Dalas OneWire (W1) bus
On Thu, May 25, 2017 at 08:46:36PM +0300, Alex A. Mihaylov wrote:

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.

> +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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2017-05-26 12:24    [W:1.991 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site