diff mbox

regmap: i2c: fallback to SMBus if the adapter does not support standard I2C

Message ID 1397636170-664-1-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON April 16, 2014, 8:16 a.m. UTC
Some I2C adapters are only compatible with the SMBus protocol and do not
support standard I2C transfers.

Fallback to SMBus transfers if we encounter such kind of adapters.
The transfer type is chosen according to the val_bits field in the regmap
config.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark, Lars,

Would this patch be acceptable ?

I'm still not convinced that we can decide which kind of protocol should
be used based only on the adapter functionalities and the val_bytes field.

But doing this as a first step should work in my case (my adapter only
supports SMBus byte transfers :-)).

Note that this patch has not been tested yet. I'll test it and resend a
proper version, if you agree on the implementation.

Best Regards,

Boris

 drivers/base/regmap/regmap-i2c.c | 101 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 2 deletions(-)

Comments

Mark Brown April 16, 2014, 5:06 p.m. UTC | #1
On Wed, Apr 16, 2014 at 10:16:10AM +0200, Boris BREZILLON wrote:

> +	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
> +		return &regmap_i2c;
> +	} else if (config->val_bits == 16 &&
> +		   i2c_check_functionality(i2c->adapter,
> +					   I2C_FUNC_SMBUS_WORD_DATA)) {
> +                config->reg_read = regmap_smbus_word_reg_read;
> +                config->reg_write = regmap_smbus_word_reg_write;
> +		return NULL;

This all looks good to me except we shouldn't be modifying the config
struct (it's supposed to be const).  We should instead add the ability
for the bus to set these ops - that'll also be useful for things like
AC'97.
Boris BREZILLON April 16, 2014, 5:16 p.m. UTC | #2
Hello Mark,

On 16/04/2014 19:06, Mark Brown wrote:
> On Wed, Apr 16, 2014 at 10:16:10AM +0200, Boris BREZILLON wrote:
>
>> +	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
>> +		return &regmap_i2c;
>> +	} else if (config->val_bits == 16 &&
>> +		   i2c_check_functionality(i2c->adapter,
>> +					   I2C_FUNC_SMBUS_WORD_DATA)) {
>> +                config->reg_read = regmap_smbus_word_reg_read;
>> +                config->reg_write = regmap_smbus_word_reg_write;
>> +		return NULL;
> This all looks good to me except we shouldn't be modifying the config
> struct (it's supposed to be const).  We should instead add the ability
> for the bus to set these ops - that'll also be useful for things like
> AC'97.

Actually I do not modify the passed config struct, I copy the config
values into a local instance and then modify this local instance
appropriately.
But I agree that having a proper way to overload config parameters would
be better.
Any suggestion on how this should be done ?

Best Regards,

Boris
Mark Brown April 16, 2014, 9 p.m. UTC | #3
On Wed, Apr 16, 2014 at 07:16:14PM +0200, Boris BREZILLON wrote:

> Actually I do not modify the passed config struct, I copy the config
> values into a local instance and then modify this local instance
> appropriately.
> But I agree that having a proper way to overload config parameters would
> be better.
> Any suggestion on how this should be done ?

I think just adding them as operations in the bus struct should work,
though I only had a fairly quick scan through so I might have missed a
case where it's more than just the init that needs updating to take
account of there being a bus with these ops.
diff mbox

Patch

diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index ebd1895..80051c5 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -14,6 +14,69 @@ 
 #include <linux/i2c.h>
 #include <linux/module.h>
 
+
+static int regmap_smbus_byte_reg_read(void *context, unsigned int reg,
+				      unsigned int *val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	int ret;
+
+	if (reg > 0xff)
+		return -EINVAL;
+
+	ret = i2c_smbus_read_byte_data(i2c, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+}
+
+static int regmap_smbus_byte_reg_write(void *context, unsigned int reg,
+				       unsigned int val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+
+	if (val > 0xff || reg > 0xff)
+		return -EINVAL;
+
+	return i2c_smbus_write_byte_data(i2c, reg, val);
+}
+
+static int regmap_smbus_word_reg_read(void *context, unsigned int reg,
+				      unsigned int *val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	int ret;
+
+	if (reg > 0xffff)
+		return -EINVAL;
+
+	ret = i2c_smbus_read_word_data(i2c, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+}
+
+static int regmap_smbus_word_reg_write(void *context, unsigned int reg,
+				       unsigned int val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+
+	if (val > 0xffff || reg > 0xffff)
+		return -EINVAL;
+
+	return i2c_smbus_write_word_data(i2c, reg, val);
+}
+
 static int regmap_i2c_write(void *context, const void *data, size_t count)
 {
 	struct device *dev = context;
@@ -97,6 +160,28 @@  static struct regmap_bus regmap_i2c = {
 	.read = regmap_i2c_read,
 };
 
+const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
+					    struct regmap_config *config)
+{
+	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
+		return &regmap_i2c;
+	} else if (config->val_bits == 16 &&
+		   i2c_check_functionality(i2c->adapter,
+					   I2C_FUNC_SMBUS_WORD_DATA)) {
+                config->reg_read = regmap_smbus_word_reg_read;
+                config->reg_write = regmap_smbus_word_reg_write;
+		return NULL;
+	} else if (config->val_bits == 8 &&
+		   i2c_check_functionality(i2c->adapter,
+					   I2C_FUNC_SMBUS_BYTE_DATA)) {
+                config->reg_read = regmap_smbus_byte_reg_read;
+                config->reg_write = regmap_smbus_byte_reg_write;
+		return NULL;
+	}
+
+	return ERR_PTR(-ENOTSUPP);
+}
+
 /**
  * regmap_init_i2c(): Initialise register map
  *
@@ -109,7 +194,13 @@  static struct regmap_bus regmap_i2c = {
 struct regmap *regmap_init_i2c(struct i2c_client *i2c,
 			       const struct regmap_config *config)
 {
-	return regmap_init(&i2c->dev, &regmap_i2c, &i2c->dev, config);
+	struct regmap_config upd_conf = *config;
+	const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, &upd_conf);
+
+	if (IS_ERR(bus))
+		return ERR_PTR(PTR_ERR(bus));
+
+	return regmap_init(&i2c->dev, bus, &i2c->dev, &upd_conf);
 }
 EXPORT_SYMBOL_GPL(regmap_init_i2c);
 
@@ -126,7 +217,13 @@  EXPORT_SYMBOL_GPL(regmap_init_i2c);
 struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
 				    const struct regmap_config *config)
 {
-	return devm_regmap_init(&i2c->dev, &regmap_i2c, &i2c->dev, config);
+	struct regmap_config upd_conf = *config;
+	const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, &upd_conf);
+
+	if (IS_ERR(bus))
+		return ERR_PTR(PTR_ERR(bus));
+
+	return devm_regmap_init(&i2c->dev, bus, &i2c->dev, &upd_conf);
 }
 EXPORT_SYMBOL_GPL(devm_regmap_init_i2c);