diff mbox

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

Message ID 20170525174639.3191-2-minimumlaw@rambler.ru (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

minimumlaw@rambler.ru May 25, 2017, 5:46 p.m. UTC
Signed-off-by: Alex A. Mihaylov <minimumlaw@rambler.ru>
---
 drivers/base/regmap/Kconfig     |   6 +-
 drivers/base/regmap/Makefile    |   1 +
 drivers/base/regmap/regmap-w1.c | 241 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h          |  34 ++++++
 4 files changed, 281 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-w1.c

Comments

Greg KH May 25, 2017, 6:51 p.m. UTC | #1
On Thu, May 25, 2017 at 08:46:36PM +0300, Alex A. Mihaylov wrote:
> Signed-off-by: Alex A. Mihaylov <minimumlaw@rambler.ru>

I just said I can ont take patches without any changelog text.  Why did
you resend these without fixing that up?

greg k-h
Mark Brown May 26, 2017, 9:56 a.m. UTC | #2
On Thu, May 25, 2017 at 08:51:36PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 25, 2017 at 08:46:36PM +0300, Alex A. Mihaylov wrote:

> > Signed-off-by: Alex A. Mihaylov <minimumlaw@rambler.ru>

> I just said I can ont take patches without any changelog text.  Why did
> you resend these without fixing that up?

Please don't take regmap patches without my review!  Though that's going
to be a lot faster if the subject line matches the style for the
subsystem...
Mark Brown May 26, 2017, 10:21 a.m. UTC | #3
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.
Mark Brown May 26, 2017, 11:06 a.m. UTC | #4
On Thu, May 25, 2017 at 11:14:39PM +0300, Evgeniy Polyakov wrote:

> Frankly saying, your non-regmap version was so much simpler, smaller and cleaner.
> I wonder why did people force you to use regmap.

> Guys, please speak up, if you want driver authors to use THIS, it is really flawed.

> Sebastien, iirc it was your idea to use regmaps, what was the reason for this?

So, this is the first I've seen of this series but in general the main
reason for using regmap is that it allows best practice to be factored
out into common code both in terms of the I/O itself and also in terms
of framework code being able to provide regmap based helpers for common
operations so drivers need less code.  The cache code is also really
useful for a lot of devices, it's a performance boost for reads and is
well optimized for restoring device state if you don't care about
sequencing (this is where some of the MMIO users come from).  You also
get a bunch of debug support for free, things like the tracepoints and
debugfs files (those do work better if you describe the register map).

Looking at the code in the regmap patch I have to say I'm very surprised
if this is adding much complexity, usually moving to regmap is basically
a sed.
minimumlaw@rambler.ru May 26, 2017, 3:50 p.m. UTC | #5
> 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.
minimumlaw@rambler.ru May 26, 2017, 4:17 p.m. UTC | #6
> Alex, it is up to you to decide whether you want to push your regmap version or not,
> I will not object against, but in my personal opinion your first version version was much cleaner.

I don't know. First edition (without regmap) was very simple. I think 
anyone could understand how this code works.

On the other hand, there is already a lot of duplicate code in the 
kernel, which is responsible for accessing device registers on the bus. 
In this sense, using the regmap infrastructure should help. As for the 
complex description of registers of a simple device, I consciously went 
into this complication by describing all the holes in the register map. 
Since this driver is a pioneer, he must use the maximum of 
infrastructure capabilities.

But I somehow do not really believe in the correctness of the 
realisation of regmap for W1. I have too few devices working with this 
bus. Theory, they all can work with my implementation of regmap. But not 
the fact that there will not be those who can not.
Mark Brown May 26, 2017, 5:18 p.m. UTC | #7
On Fri, May 26, 2017 at 04:45:12PM +0300, Evgeniy Polyakov wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> I wonder what is 'cache' argument about? If I know that some read can be cached,
> since underlying hardware never changes these values, I can store it into local variable
> and use it. If I do not know in advance whether given register can be cached,
> how can regmap determine it for me? How to debug cases when register
> was erroneously considered to be cached?

regmap will only cache registers if it was explicitly told to cache, if
the driver gets that wrong it's just your bog standard bug that can be
debugged in the usual ways one might debug things.  By default nothing
will be cached.

> What is the reason to use regmap besides caching?

There's the other reasons I mentioned in the message you're replying
to for a start...
Mark Brown June 28, 2017, 7:37 p.m. UTC | #8
On Fri, May 26, 2017 at 11:26:31PM +0300, Evgeniy Polyakov wrote:

> 26.05.2017, 20:18, "Mark Brown" <broonie@kernel.org>:
> > On Fri, May 26, 2017 at 04:45:12PM +0300, Evgeniy Polyakov wrote:

> > Please fix your mail client to word wrap within paragraphs at something
> > substantially less than 80 columns. Doing this makes your messages much
> > easier to read and reply to.

> What part of the message spans behind this limit? Did you handwrap paragraph below
> or it looked like a single line?

Essentially every line of your mail is overly long, including the first
line of the above paragraph.  I've not reflowed anything.  I'd guess
your client is doing the wrapping (this is fairly standard for GUI
clients).

> >>  What is the reason to use regmap besides caching?

> > There's the other reasons I mentioned in the message you're replying
> > to for a start...

> That didn't show how it helps w1, so was the question.

I'm not clear why any of these benefits would be less applicable to w1
than to other buses.
Mark Brown July 7, 2017, 5:11 p.m. UTC | #9
On Fri, Jul 07, 2017 at 06:26:28PM +0300, Evgeniy Polyakov wrote:

> In fact, regmap-w1 brings virtually nothing to w1 from regmap point fo view -
> code is more complex and even a bit more error prone, but we found a nice feature,
> that we do not have to export IO functions anymore, they will be hidden behind
> regmap wrappers.

It's not for the benefit of the bus, it's for the benefit of the devices
on the bus and their subsystems.  From the point of view of the
subsystem it shouldn't have any impact at all, it's a helper library for
the layers above.
diff mbox

Patch

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index db9d00c..413af5f 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@ 
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ)
 	select LZO_COMPRESS
 	select LZO_DECOMPRESS
 	select IRQ_DOMAIN if REGMAP_IRQ
@@ -24,6 +24,10 @@  config REGMAP_SPMI
 	tristate
 	depends on SPMI
 
+config REGMAP_W1
+	tristate
+	depends on W1
+
 config REGMAP_MMIO
 	tristate
 
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 609e4c8..17741ae 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -10,3 +10,4 @@  obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
 obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
 obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
 obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
+obj-$(CONFIG_REGMAP_W1) += regmap-w1.o
diff --git a/drivers/base/regmap/regmap-w1.c b/drivers/base/regmap/regmap-w1.c
new file mode 100644
index 0000000..6d042cb
--- /dev/null
+++ b/drivers/base/regmap/regmap-w1.c
@@ -0,0 +1,241 @@ 
+/*
+ * Register map access API - W1 (OneWire) support
+ *
+ * Copyright (C) 2017 OAO Radioavionica
+ * Author: Alex A. Mihaylov <minimumlaw@rambler.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation
+ */
+
+#include <linux/regmap.h>
+#include <linux/module.h>
+#include "../../w1/w1.h"
+
+#include "internal.h"
+
+#define W1_CMD_READ_DATA	0x69
+#define W1_CMD_WRITE_DATA	0x6C
+
+/*
+ * OneWire slaves registers with addess 8 bit and data 8 bit
+ */
+
+static int w1_reg_a8_v8_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct device *dev = context;
+	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+	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);
+
+	return ret;
+}
+
+static int w1_reg_a8_v8_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct device *dev = context;
+	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+	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_WRITE_DATA);
+		w1_write_8(sl->master, reg);
+		w1_write_8(sl->master, val);
+		ret = 0;
+	}
+	mutex_unlock(&sl->master->bus_mutex);
+
+	return ret;
+}
+
+static struct regmap_bus regmap_w1_bus_a8_v8 = {
+	.reg_read = w1_reg_a8_v8_read,
+	.reg_write = w1_reg_a8_v8_write,
+};
+
+/*
+ * OneWire slaves registers with addess 8 bit and data 16 bit
+ */
+
+static int w1_reg_a8_v16_read(void *context, unsigned int reg,
+				unsigned int *val)
+{
+	struct device *dev = context;
+	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+	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);
+		*val |= w1_read_8(sl->master)<<8;
+		ret = 0;
+	}
+	mutex_unlock(&sl->master->bus_mutex);
+
+	return ret;
+}
+
+static int w1_reg_a8_v16_write(void *context, unsigned int reg,
+				unsigned int val)
+{
+	struct device *dev = context;
+	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+	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_WRITE_DATA);
+		w1_write_8(sl->master, reg);
+		w1_write_8(sl->master, val & 0x00FF);
+		w1_write_8(sl->master, val>>8 & 0x00FF);
+		ret = 0;
+	}
+	mutex_unlock(&sl->master->bus_mutex);
+
+	return ret;
+}
+
+static struct regmap_bus regmap_w1_bus_a8_v16 = {
+	.reg_read = w1_reg_a8_v16_read,
+	.reg_write = w1_reg_a8_v16_write,
+};
+
+/*
+ * OneWire slaves registers with addess 16 bit and data 16 bit
+ */
+
+static int w1_reg_a16_v16_read(void *context, unsigned int reg,
+				unsigned int *val)
+{
+	struct device *dev = context;
+	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+	int ret = -ENODEV;
+
+
+	if (reg > 65535)
+		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 & 0x00FF);
+		w1_write_8(sl->master, reg>>8 & 0x00FF);
+		*val = w1_read_8(sl->master);
+		*val |= w1_read_8(sl->master)<<8;
+		ret = 0;
+	}
+	mutex_unlock(&sl->master->bus_mutex);
+
+	return ret;
+}
+
+static int w1_reg_a16_v16_write(void *context, unsigned int reg,
+				unsigned int val)
+{
+	struct device *dev = context;
+	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+	int ret = -ENODEV;
+
+
+	if (reg > 65535)
+		return -EINVAL;
+
+	mutex_lock(&sl->master->bus_mutex);
+	if (!w1_reset_select_slave(sl)) {
+		w1_write_8(sl->master, W1_CMD_WRITE_DATA);
+		w1_write_8(sl->master, reg & 0x00FF);
+		w1_write_8(sl->master, reg>>8 & 0x00FF);
+		w1_write_8(sl->master, val & 0x00FF);
+		w1_write_8(sl->master, val>>8 & 0x00FF);
+		ret = 0;
+	}
+	mutex_unlock(&sl->master->bus_mutex);
+
+	return ret;
+}
+
+static struct regmap_bus regmap_w1_bus_a16_v16 = {
+	.reg_read = w1_reg_a16_v16_read,
+	.reg_write = w1_reg_a16_v16_write,
+};
+
+static const struct regmap_bus *regmap_get_w1_bus(struct device *w1_dev,
+					const struct regmap_config *config)
+{
+	if (config->reg_bits == 8 && config->val_bits == 8)
+		return &regmap_w1_bus_a8_v8;
+
+	if (config->reg_bits == 8 && config->val_bits == 16)
+		return &regmap_w1_bus_a8_v16;
+
+	if (config->reg_bits == 16 && config->val_bits == 16)
+		return &regmap_w1_bus_a16_v16;
+
+	return ERR_PTR(-ENOTSUPP);
+}
+
+struct regmap *__regmap_init_w1(struct device *w1_dev,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name)
+{
+
+	const struct regmap_bus *bus = regmap_get_w1_bus(w1_dev, config);
+
+	if (IS_ERR(bus))
+		return ERR_CAST(bus);
+
+	return __regmap_init(w1_dev, bus, w1_dev, config,
+			 lock_key, lock_name);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__regmap_init_w1);
+
+struct regmap *__devm_regmap_init_w1(struct device *w1_dev,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name)
+{
+
+	const struct regmap_bus *bus = regmap_get_w1_bus(w1_dev, config);
+
+	if (IS_ERR(bus))
+		return ERR_CAST(bus);
+
+	return __devm_regmap_init(w1_dev, bus, w1_dev, config,
+				 lock_key, lock_name);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_w1);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index e886492..86eeacc 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -461,6 +461,10 @@  struct regmap *__regmap_init_spmi_ext(struct spmi_device *dev,
 				      const struct regmap_config *config,
 				      struct lock_class_key *lock_key,
 				      const char *lock_name);
+struct regmap *__regmap_init_w1(struct device *w1_dev,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name);
 struct regmap *__regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 				      void __iomem *regs,
 				      const struct regmap_config *config,
@@ -493,6 +497,10 @@  struct regmap *__devm_regmap_init_spmi_ext(struct spmi_device *dev,
 					   const struct regmap_config *config,
 					   struct lock_class_key *lock_key,
 					   const char *lock_name);
+struct regmap *__devm_regmap_init_w1(struct device *w1_dev,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name);
 struct regmap *__devm_regmap_init_mmio_clk(struct device *dev,
 					   const char *clk_id,
 					   void __iomem *regs,
@@ -597,6 +605,19 @@  int regmap_attach_dev(struct device *dev, struct regmap *map,
 				dev, config)
 
 /**
+ * regmap_init_w1() - Initialise register map
+ *
+ * @w1_dev: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+#define regmap_init_w1(w1_dev, config)					\
+	__regmap_lockdep_wrapper(__regmap_init_w1, #config,		\
+				w1_dev, config)
+
+/**
  * regmap_init_mmio_clk() - Initialise register map with register clock
  *
  * @dev: Device that will be interacted with
@@ -712,6 +733,19 @@  bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 				dev, config)
 
 /**
+ * devm_regmap_init_w1() - Initialise managed register map
+ *
+ * @w1_dev: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_w1(w1_dev, config)				\
+	__regmap_lockdep_wrapper(__devm_regmap_init_w1, #config,	\
+				w1_dev, config)
+/**
  * devm_regmap_init_mmio_clk() - Initialise managed register map with clock
  *
  * @dev: Device that will be interacted with