[PATCH 0/3] Add support for limited i2c tunnel for exynos5250-spring

[PATCH 0/3] Add support for limited i2c tunnel for exynos5250-spring

[Doug Anderson]

This patches series possibly adds support for getting to the battery
and tps65090 device on exynos5250-spring.  I have simulated things on
exynos5420-peach-pit and found that this seems to work OK and I can
talk to both the battery and tps65090.  I have simulated this on
exynos5250-snow and found that it properly detects that the full I2C
passhtrough isn't available.  I don't have a Spring (or Skate) setup
for upstream testing right now but I figured possibly Andreas would be
interested in testing this.

As discussed in patch #3 this code path on the EC is not actually
exercised on production kernels but I think it should be solid and get
us up and running quickly.  I think this is also a cleaner solution
than what's in production kernels but possibly folks will be able to
come up with benefits of and arguments for using the specialized
regulator and battery commands to talk to the EC.

Someone interested in using this should add the cros ec tunnel to
their spring device tree and see what happens.  Note that this limited
tunnel doesn't actually export anything that's useful for i2cdetect to
use so to test you pretty much just need to add an sbs_battery and see
if it works.

I may not have tons of time for spinning this series so possibly
someone in the community who is interested in getting
exynos5250-spring supported would be interested in following up with
any needed review feedback.  I certainly wouldn't be offended.

As is probably obvious this patch series is based atop all of the
existing posted (and ACKed) cros_ec cleanups that I've sent up.


Doug Anderson (3):
  regmap: cache: regcache_hw_init() should use regmap_bulk_read()
  mfd: cros_ec: Use the proper size when looking at the cros_ec_i2c
    result
  i2c: cros_ec: Support a limited i2c tunnel for exynos5250-spring

 drivers/base/regmap/regcache.c          |  2 +-
 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 92 ++++++++++++++++++++++++++++++++-
 drivers/mfd/cros_ec_i2c.c               | 15 ++++--
 3 files changed, 104 insertions(+), 5 deletions(-)

-- 
2.0.0.526.g5318336

[PATCH 1/3] regmap: cache: regcache_hw_init() should use regmap_bulk_read()

[Doug Anderson]

We really should be using regmap_bulk_read() in regcache_hw_init().
The regmap_bulk_read() will translate into regmap_raw_read() when
appropriate.  Doing this fixes problems where regmap_smbus() will
crash because they don't implement .read and .write.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/base/regmap/regcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 29b4128..6447486 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -45,7 +45,7 @@ static int regcache_hw_init(struct regmap *map)
 		tmp_buf = kmalloc(map->cache_size_raw, GFP_KERNEL);
 		if (!tmp_buf)
 			return -EINVAL;
-		ret = regmap_raw_read(map, 0, tmp_buf,
+		ret = regmap_bulk_read(map, 0, tmp_buf,
 				      map->num_reg_defaults_raw);
 		map->cache_bypass = cache_bypass;
 		if (ret < 0) {
-- 
2.0.0.526.g5318336

Re: [PATCH 1/3] regmap: cache: regcache_hw_init() should use regmap_bulk_read()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Fri, Jun 27, 2014 at 12:56:11PM -0700, Doug Anderson wrote:

> We really should be using regmap_bulk_read() in regcache_hw_init().
> The regmap_bulk_read() will translate into regmap_raw_read() when
> appropriate.  Doing this fixes problems where regmap_smbus() will
> crash because they don't implement .read and .write.

Not quite - _bulk_read() does do byte swapping so...

> -		ret = regmap_raw_read(map, 0, tmp_buf,
> +		ret = regmap_bulk_read(map, 0, tmp_buf,
>  				      map->num_reg_defaults_raw);
>  		map->cache_bypass = cache_bypass;
>  		if (ret < 0) {

...a direct replacement shouldn't quite work.  We need an actual
fallback to word at a time operation I think.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/3] mfd: cros_ec: Use the proper size when looking at the cros_ec_i2c result

[Doug Anderson]

We know how many bytes the EC should be sending us (which is also the
number of bytes transferred) and also how many bytes the EC actually
wanted to send to us.  When computing the checksum and copying back
data let's make sure we take the lesser of the two of those.  We'll
also complain if the EC tried to send us too many bytes.  The EC
sending us too few bytes is legit for when we send the EC an invalid
command.

This is based on similar code in cros_ec_spi.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mfd/cros_ec_i2c.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index fd7a546..c0c30f4 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -35,6 +35,7 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
 	struct i2c_client *client = ec_dev->priv;
 	int ret = -ENOMEM;
 	int i;
+	int len;
 	int packet_len;
 	u8 *out_buf = NULL;
 	u8 *in_buf = NULL;
@@ -97,21 +98,29 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
 	if (ret)
 		goto done;
 
+	len = in_buf[1];
+	if (len > msg->insize) {
+		dev_err(ec_dev->dev, "packet too long (%d bytes, expected %d)",
+			len, msg->insize);
+		ret = -ENOSPC;
+		goto done;
+	}
+
 	/* copy response packet payload and compute checksum */
 	sum = in_buf[0] + in_buf[1];
-	for (i = 0; i < msg->insize; i++) {
+	for (i = 0; i < len; i++) {
 		msg->indata[i] = in_buf[2 + i];
 		sum += in_buf[2 + i];
 	}
 	dev_dbg(ec_dev->dev, "packet: %*ph, sum = %02x\n",
 		i2c_msg[1].len, in_buf, sum);
-	if (sum != in_buf[2 + msg->insize]) {
+	if (sum != in_buf[2 + len]) {
 		dev_err(ec_dev->dev, "bad packet checksum\n");
 		ret = -EBADMSG;
 		goto done;
 	}
 
-	ret = i2c_msg[1].buf[1];
+	ret = len;
  done:
 	kfree(in_buf);
 	kfree(out_buf);
-- 
2.0.0.526.g5318336

Re: [PATCH 2/3] mfd: cros_ec: Use the proper size when looking at the cros_ec_i2c result

[Simon Glass]

On 27 June 2014 12:56, Doug Anderson <dianders@chromium.org> wrote:
> We know how many bytes the EC should be sending us (which is also the
> number of bytes transferred) and also how many bytes the EC actually
> wanted to send to us.  When computing the checksum and copying back
> data let's make sure we take the lesser of the two of those.  We'll
> also complain if the EC tried to send us too many bytes.  The EC
> sending us too few bytes is legit for when we send the EC an invalid
> command.
>
> This is based on similar code in cros_ec_spi.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 2/3] mfd: cros_ec: Use the proper size when looking at the cros_ec_i2c result

[Lee Jones]

On Fri, 27 Jun 2014, Doug Anderson wrote:

> We know how many bytes the EC should be sending us (which is also the
> number of bytes transferred) and also how many bytes the EC actually
> wanted to send to us.  When computing the checksum and copying back
> data let's make sure we take the lesser of the two of those.  We'll
> also complain if the EC tried to send us too many bytes.  The EC
> sending us too few bytes is legit for when we send the EC an invalid
> command.
> 
> This is based on similar code in cros_ec_spi.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/mfd/cros_ec_i2c.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

Is this patch orthogonal i.e. can it be applied without the other two
patches?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] mfd: cros_ec: Use the proper size when looking at the cros_ec_i2c result

[Doug Anderson]

Lee,

On Wed, Jul 2, 2014 at 12:23 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 27 Jun 2014, Doug Anderson wrote:
>
>> We know how many bytes the EC should be sending us (which is also the
>> number of bytes transferred) and also how many bytes the EC actually
>> wanted to send to us.  When computing the checksum and copying back
>> data let's make sure we take the lesser of the two of those.  We'll
>> also complain if the EC tried to send us too many bytes.  The EC
>> sending us too few bytes is legit for when we send the EC an invalid
>> command.
>>
>> This is based on similar code in cros_ec_spi.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  drivers/mfd/cros_ec_i2c.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> Acked-by: Lee Jones <lee.jones@linaro.org>
>
> Is this patch orthogonal i.e. can it be applied without the other two
> patches?

Yes.  If patch 3/3 had worked out then it would have required patch #1
for proper functioning and patch #2 (this patch) to avoid an ugly
error message in the log.  ...but patch #1 and this patch both can
stand on their own and can be applied.

-Doug

Re: [PATCH 2/3] mfd: cros_ec: Use the proper size when looking at the cros_ec_i2c result

[Lee Jones]

On Wed, 02 Jul 2014, Doug Anderson wrote:
> On Wed, Jul 2, 2014 at 12:23 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 27 Jun 2014, Doug Anderson wrote:
> >
> >> We know how many bytes the EC should be sending us (which is also the
> >> number of bytes transferred) and also how many bytes the EC actually
> >> wanted to send to us.  When computing the checksum and copying back
> >> data let's make sure we take the lesser of the two of those.  We'll
> >> also complain if the EC tried to send us too many bytes.  The EC
> >> sending us too few bytes is legit for when we send the EC an invalid
> >> command.
> >>
> >> This is based on similar code in cros_ec_spi.
> >>
> >> Signed-off-by: Doug Anderson <dianders@chromium.org>
> >> ---
> >>  drivers/mfd/cros_ec_i2c.c | 15 ++++++++++++---
> >>  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> >
> > Is this patch orthogonal i.e. can it be applied without the other two
> > patches?
> 
> Yes.  If patch 3/3 had worked out then it would have required patch #1
> for proper functioning and patch #2 (this patch) to avoid an ugly
> error message in the log.  ...but patch #1 and this patch both can
> stand on their own and can be applied.

Very well, patch applied than.

Clause: There is a chance that this patch might not be seen in -next
for ~24-48hrs.  If it's not there by 72hrs, feel free to poke.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/3] i2c: cros_ec: Support a limited i2c tunnel for exynos5250-spring

[Doug Anderson]

On exynos5250-spring the battery and tps65090 regulator are sitting on
an i2c bus behind the EC (much like on exynos5420-peach-pit).  However
on spring we don't have the full EC_CMD_I2C_PASSTHRU command.

For the production kernel of spring we used a solution like this:
- Fork the tps65090 driver and make a version that talks straight to
  the cros_ec MFD driver and sends special EC commands for talking to
  the regulator.
- Add code into the i2c tunnel to look for i2c transfers and convert
  them into special EC commands for talking to the battery.

For reference:
* http://crosreview.com/66116
* https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/regulator/cros_ec-tps65090.c

The above solution works great but is a bunch of code.  It appears
that we can make do with the limited i2c passthrough commands that
actually happened to be present on the exynos5250-spring EC.  Doing
this we can present ourselves as supporting a small subset of smbus.
We might need to do a few extra transfers here and there but we don't
need any extra code.

Seriss-cc: linux-samsung-soc
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 92 ++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 6d7d009..c52c491 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -38,6 +38,76 @@ struct ec_i2c_device {
 	u8 response_buf[256];
 };
 
+static int ec_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+			   unsigned short flags, char read_write,
+			   u8 command, int size, union i2c_smbus_data *data)
+{
+	struct ec_i2c_device *bus = adap->algo_data;
+	struct cros_ec_command msg = {};
+	int result;
+
+	if (read_write == I2C_SMBUS_WRITE) {
+		struct ec_params_i2c_write params;
+
+		params.addr = addr << 1;
+		params.port = bus->remote_bus;
+		params.offset = command;
+
+		if (size == I2C_SMBUS_WORD_DATA) {
+			params.data = data->word;
+			params.write_size = 16;
+		} else if (size == I2C_SMBUS_BYTE_DATA) {
+			params.data = data->byte;
+			params.write_size = 8;
+		} else {
+			return -EINVAL;
+		}
+
+		msg.command = EC_CMD_I2C_WRITE;
+		msg.outdata = (uint8_t *)&params;
+		msg.outsize = sizeof(params);
+
+		result = bus->ec->cmd_xfer(bus->ec, &msg);
+		if (result < 0)
+			return -EIO;
+
+		return 0;
+	} else if (read_write == I2C_SMBUS_READ) {
+		struct ec_params_i2c_read params;
+		struct ec_response_i2c_read response;
+
+		params.addr = addr << 1;
+		params.port = bus->remote_bus;
+		params.offset = command;
+
+		if (size == I2C_SMBUS_WORD_DATA)
+			params.read_size = 16;
+		else if (size == I2C_SMBUS_BYTE_DATA)
+			params.read_size = 8;
+		else
+			return -EINVAL;
+
+		msg.command = EC_CMD_I2C_READ;
+		msg.outdata = (uint8_t *)&params;
+		msg.outsize = sizeof(params);
+		msg.indata = (uint8_t *)&response;
+		msg.insize = sizeof(response);
+
+		result = bus->ec->cmd_xfer(bus->ec, &msg);
+		if (result < 0)
+			return -EIO;
+
+		if (size == I2C_SMBUS_WORD_DATA)
+			data->word = response.data;
+		else
+			data->byte = response.data;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 /**
  * ec_i2c_count_message - Count bytes needed for ec_i2c_construct_message
  *
@@ -233,6 +303,11 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
 	if (result < 0)
 		goto exit;
 
+	if (msg.result == EC_RES_INVALID_COMMAND) {
+		result = -EINVAL;
+		goto exit;
+	}
+
 	result = ec_i2c_parse_response(response, i2c_msgs, &num);
 	if (result < 0)
 		goto exit;
@@ -258,6 +333,16 @@ static const struct i2c_algorithm ec_i2c_algorithm = {
 	.functionality	= ec_i2c_functionality,
 };
 
+static u32 ec_smbus_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA;
+}
+
+static const struct i2c_algorithm ec_i2c_limited_algorithm = {
+	.smbus_xfer	= ec_smbus_xfer,
+	.functionality	= ec_smbus_functionality,
+};
+
 static int ec_i2c_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -288,11 +373,16 @@ static int ec_i2c_probe(struct platform_device *pdev)
 
 	bus->adap.owner = THIS_MODULE;
 	strlcpy(bus->adap.name, "cros-ec-i2c-tunnel", sizeof(bus->adap.name));
-	bus->adap.algo = &ec_i2c_algorithm;
 	bus->adap.algo_data = bus;
 	bus->adap.dev.parent = &pdev->dev;
 	bus->adap.dev.of_node = np;
 
+	/* Test if we can support full passthrough or just limited */
+	if (ec_i2c_xfer(&bus->adap, NULL, 0) == 0)
+		bus->adap.algo = &ec_i2c_algorithm;
+	else
+		bus->adap.algo = &ec_i2c_limited_algorithm;
+
 	err = i2c_add_adapter(&bus->adap);
 	if (err) {
 		dev_err(dev, "cannot register i2c adapter\n");
-- 
2.0.0.526.g5318336

Re: [PATCH 3/3] i2c: cros_ec: Support a limited i2c tunnel for exynos5250-spring

[Doug Anderson]

Hi,

On Fri, Jun 27, 2014 at 12:56 PM, Doug Anderson <dianders@chromium.org> wrote:
> On exynos5250-spring the battery and tps65090 regulator are sitting on
> an i2c bus behind the EC (much like on exynos5420-peach-pit).  However
> on spring we don't have the full EC_CMD_I2C_PASSTHRU command.
>
> For the production kernel of spring we used a solution like this:
> - Fork the tps65090 driver and make a version that talks straight to
>   the cros_ec MFD driver and sends special EC commands for talking to
>   the regulator.
> - Add code into the i2c tunnel to look for i2c transfers and convert
>   them into special EC commands for talking to the battery.
>
> For reference:
> * http://crosreview.com/66116
> * https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/regulator/cros_ec-tps65090.c
>
> The above solution works great but is a bunch of code.  It appears
> that we can make do with the limited i2c passthrough commands that
> actually happened to be present on the exynos5250-spring EC.  Doing
> this we can present ourselves as supporting a small subset of smbus.
> We might need to do a few extra transfers here and there but we don't
> need any extra code.
>
> Seriss-cc: linux-samsung-soc
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 92 ++++++++++++++++++++++++++++++++-
>  1 file changed, 91 insertions(+), 1 deletion(-)

I just got done talking to Randall and he suggested that this might
not actually work on Spring.  :(

It looks like in the Spring timeframe I2C EC commands were restricted
if the write protect screw was in place.  ...so someone will probably
want to take some of the code referenced above instead of this patch.

Patches #1 and #2 are probably still valid, though.

-Doug