diff mbox

[3/8] regmap: Add support for using regmap over ssbi

Message ID 1386718523-2587-4-git-send-email-sboyd@codeaurora.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Stephen Boyd Dec. 10, 2013, 11:35 p.m. UTC 
From: Srinivas Ramana <sramana@codeaurora.org>

This patch will add support for using regmap API over SSBI.
Many of the Qualcomm PMIC chips(ex: PM8038, 8921) uses SSBI interface.
This patch adds only regmap-ssbi suport. Individual PMIC drivers will
need to register with their config to use the regmap API.
regmap-ssbi uses the SSBI driver interface.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Srinivas Ramana <sramana@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/regmap/Kconfig       |   5 +-
 drivers/base/regmap/Makefile      |   1 +
 drivers/base/regmap/regmap-ssbi.c | 103 ++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h            |   4 ++
 4 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-ssbi.c

Comments

Mark Brown Dec. 10, 2013, 11:50 p.m. UTC | #1 
On Tue, Dec 10, 2013 at 03:35:18PM -0800, Stephen Boyd wrote:

> +	while (val_size) {
> +		ret = ssbi_read(context, *(u16 *)reg, val, 1);
> +		if (ret)
> +			return ret;
> +		reg += sizeof(u16);
> +		val += sizeof(u8);
> +		val_size -= sizeof(u8);

I'd expect this to generate out of bounds accesses and bad interactions
on the bus if we go through the loop more than once since it appears to
incrementing the address of reg for every register.  I'm also having a
hard time understanding why this is doing a read per byte, ssbi_read()
seems to map fairly directly onto the interface of the operation so
there doesn't seem to be any reason for this loop to exist in the first
place.

Has this been tested?  

It'd be helpful to CC the entire series, or at least the patches this
builds on...
Stephen Boyd Dec. 11, 2013, 12:13 a.m. UTC | #2 
On 12/10/13 15:50, Mark Brown wrote:
> On Tue, Dec 10, 2013 at 03:35:18PM -0800, Stephen Boyd wrote:
>
>> +	while (val_size) {
>> +		ret = ssbi_read(context, *(u16 *)reg, val, 1);
>> +		if (ret)
>> +			return ret;
>> +		reg += sizeof(u16);
>> +		val += sizeof(u8);
>> +		val_size -= sizeof(u8);
> I'd expect this to generate out of bounds accesses and bad interactions
> on the bus if we go through the loop more than once since it appears to
> incrementing the address of reg for every register.  I'm also having a
> hard time understanding why this is doing a read per byte, ssbi_read()
> seems to map fairly directly onto the interface of the operation so
> there doesn't seem to be any reason for this loop to exist in the first
> place.

ssbi_read() just reads the same register x number of times and doesn't
do any sort of incrementing of address. My understanding was that
regmap_bulk_read() will read incrementing addresses and then call down
into this code with the sequential addresses formatted into the reg
buffer. That was the flaw. Instead we need to take reg and then
increment reg by 1 every time through this loop. Or should we just have
use_single_rw == true?

>
> Has this been tested?  

Yes. But so far they've all been single register reads.

>
> It'd be helpful to CC the entire series, or at least the patches this
> builds on...

Sure, will do next time.
Mark Brown Dec. 11, 2013, 12:51 a.m. UTC | #3 
On Tue, Dec 10, 2013 at 04:13:15PM -0800, Stephen Boyd wrote:
> On 12/10/13 15:50, Mark Brown wrote:
> > On Tue, Dec 10, 2013 at 03:35:18PM -0800, Stephen Boyd wrote:

> >> +		reg += sizeof(u16);

> > I'd expect this to generate out of bounds accesses and bad interactions
> > on the bus if we go through the loop more than once since it appears to
> > incrementing the address of reg for every register.  I'm also having a

> ssbi_read() just reads the same register x number of times and doesn't
> do any sort of incrementing of address. My understanding was that
> regmap_bulk_read() will read incrementing addresses and then call down
> into this code with the sequential addresses formatted into the reg
> buffer. That was the flaw. Instead we need to take reg and then

That's possibly not the ideal decision for the underlying API, if
nothing else it's confusing naming given what a read function would
normally do.

> increment reg by 1 every time through this loop. Or should we just have
> use_single_rw == true?

No, it doesn't - it increments the address of reg by the size of a
register value each time.  Using use_single_rw might make sense, or if
you can't do bulk I/O at all then hooking in via reg_read() and
reg_write() in the config rather than trying to parse out the buffers
might be even better (you can still make helpers to set that up).
Stephen Boyd Dec. 11, 2013, 1:32 a.m. UTC | #4 
On 12/10/13 16:51, Mark Brown wrote:
> On Tue, Dec 10, 2013 at 04:13:15PM -0800, Stephen Boyd wrote:
>> increment reg by 1 every time through this loop. Or should we just have
>> use_single_rw == true?
> No, it doesn't - it increments the address of reg by the size of a
> register value each time.  Using use_single_rw might make sense, or if
> you can't do bulk I/O at all then hooking in via reg_read() and
> reg_write() in the config rather than trying to parse out the buffers
> might be even better (you can still make helpers to set that up).


Are you suggesting we implement the reg_read/reg_write as global helpers
that the config points to and then call regmap_init()? At a quick glance
it looks like we lose out on regmap_bulk_read() if we do that. There is
one driver that will use regmap_bulk_read(), but I suppose we can just
loop on regmap_read() and do our own increment? If we use use_single_rw
everything works and we can simplify this code to just pass the reg and
val buffers directly to ssbi_read/write.

This is what I have now.

static int regmap_ssbi_read(void *context,
                            const void *regp, size_t reg_size,
                            void *val, size_t val_size)
{
        int ret;
        u16 reg;

        BUG_ON(reg_size != 2);

        reg = *(u16 *)regp;
        while (val_size) {
                ret = ssbi_read(context, reg, val, 1);
                if (ret)
                        return ret;
                reg++;
                val += sizeof(u8);
                val_size -= sizeof(u8);
        }

        return 0;
}

With use_single_rw I think it can be this.

static int regmap_ssbi_read(void *context,
                            const void *reg, size_t reg_size,
                            void *val, size_t val_size)
{
        BUG_ON(reg_size != 2);  
        return ssbi_read(context, *(u16 *)reg, val, 1);
}
Mark Brown Dec. 11, 2013, 1:27 p.m. UTC | #5 
On Tue, Dec 10, 2013 at 05:32:51PM -0800, Stephen Boyd wrote:

> Are you suggesting we implement the reg_read/reg_write as global helpers
> that the config points to and then call regmap_init()? At a quick glance

Yes, assuming your bus really is limited in the way it seems from the
code.

> it looks like we lose out on regmap_bulk_read() if we do that. There is
> one driver that will use regmap_bulk_read(), but I suppose we can just

bulk_read() should decay to individual reads if there isn't a block
operaton and it's not like the hardware actually supports bulk reads
anyway.

> With use_single_rw I think it can be this.

> static int regmap_ssbi_read(void *context,
>                             const void *reg, size_t reg_size,
>                             void *val, size_t val_size)
> {
>         BUG_ON(reg_size != 2);  
>         return ssbi_read(context, *(u16 *)reg, val, 1);
> }

Right, which is a lot simpler.  Though I'm still not sure the
ssbi_read() interface is the best.
diff mbox

Patch

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 4251570..fd0e264 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_MMIO || REGMAP_IRQ)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SSBI)
 	select LZO_COMPRESS
 	select LZO_DECOMPRESS
 	select IRQ_DOMAIN if REGMAP_IRQ
@@ -23,3 +23,6 @@  config REGMAP_MMIO
 
 config REGMAP_IRQ
 	bool
+
+config REGMAP_SSBI
+	tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index a7c670b..3f53763 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -6,3 +6,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_SSBI) += regmap-ssbi.o
diff --git a/drivers/base/regmap/regmap-ssbi.c b/drivers/base/regmap/regmap-ssbi.c
new file mode 100644
index 0000000..71bce4d
--- /dev/null
+++ b/drivers/base/regmap/regmap-ssbi.c
@@ -0,0 +1,103 @@ 
+/*
+ * Register map access API - SSBI support
+ *
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ *
+ * 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/device.h>
+#include <linux/regmap.h>
+#include <linux/module.h>
+#include <linux/ssbi.h>
+
+static int regmap_ssbi_read(void *context,
+			    const void *reg, size_t reg_size,
+			    void *val, size_t val_size)
+{
+	int ret;
+
+	BUG_ON(reg_size != 2);
+
+	while (val_size) {
+		ret = ssbi_read(context, *(u16 *)reg, val, 1);
+		if (ret)
+			return ret;
+		reg += sizeof(u16);
+		val += sizeof(u8);
+		val_size -= sizeof(u8);
+	}
+
+	return 0;
+}
+
+static int regmap_ssbi_gather_write(void *context,
+				   const void *reg, size_t reg_size,
+				   const void *val, size_t val_size)
+{
+	int ret;
+
+	BUG_ON(reg_size != 2);
+
+	while (val_size) {
+		ret = ssbi_write(context, *(u16 *)reg, val, 1);
+		if (ret)
+			return ret;
+		reg += sizeof(u16);
+		val += sizeof(u8);
+		val_size -= sizeof(u8);
+	}
+
+	return 0;
+}
+
+static int regmap_ssbi_write(void *context, const void *data, size_t count)
+{
+	BUG_ON(count < 2);
+	return regmap_ssbi_gather_write(context, data, 2, data + 2, count - 2);
+}
+
+static const struct regmap_bus regmap_ssbi = {
+	.read				= regmap_ssbi_read,
+	.write				= regmap_ssbi_write,
+	.gather_write			= regmap_ssbi_gather_write,
+	.reg_format_endian_default	= REGMAP_ENDIAN_NATIVE,
+	.val_format_endian_default	= REGMAP_ENDIAN_NATIVE,
+};
+
+/**
+ * regmap_init_ssbi(): Initialise register map
+ *
+ * @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.
+ */
+struct regmap *regmap_init_ssbi(struct device *dev,
+			       const struct regmap_config *config)
+{
+	return regmap_init(dev, &regmap_ssbi, dev->parent, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_ssbi);
+
+/**
+ * devm_regmap_init_ssbi(): Initialise managed register map
+ *
+ * @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.
+ */
+struct regmap *devm_regmap_init_ssbi(struct device *dev,
+				    const struct regmap_config *config)
+{
+	return devm_regmap_init(dev, &regmap_ssbi, dev->parent, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_ssbi);
+
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index e559078..528ee99 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -326,6 +326,8 @@  struct regmap *regmap_init_spmi(struct spmi_device *dev,
 struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 				    void __iomem *regs,
 				    const struct regmap_config *config);
+struct regmap *regmap_init_ssbi(struct device *dev,
+				const struct regmap_config *config);
 
 struct regmap *devm_regmap_init(struct device *dev,
 				const struct regmap_bus *bus,
@@ -340,6 +342,8 @@  struct regmap *devm_regmap_init_spmi(struct spmi_device *dev,
 struct regmap *devm_regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 					 void __iomem *regs,
 					 const struct regmap_config *config);
+struct regmap *devm_regmap_init_ssbi(struct device *dev,
+					const struct regmap_config *config);
 
 /**
  * regmap_init_mmio(): Initialise register map