Thread (9 messages) 9 messages, 3 authors, 2017-05-26

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

From: Mark Brown <broonie@kernel.org>
Date: 2017-05-26 10:22:05
Also in: lkml

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.

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help