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

Groups > linux.kernel > #1650703 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [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

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

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

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
-- 
2.8.4 (Apple Git-73)

[toc] | [next] | [standalone]

#1650749 — Re: [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:
> 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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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...

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

> 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.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

> 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.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web