[lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers correctly

[lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers correctly

[Ira W. Snyder]

The GPIO registers on the ltc4245 behave in a strage way. Every time the
ADC updates the values for one GPIO, it updates all three GPIO registers to
the same value.

To read all three GPIO registers correctly, we cache copies of each
register and update the GPIO MUX to read the next GPIO register. Each GPIO
sample will take 3 * HZ to read, but at least we are returning the correct
values to userspace.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---

When I implemented the ltc4245 driver, I did not have the ability to test
the GPIO pins. I now have devices hooked up, and they don't work exactly as
the datasheet describes (though it is vague in this section). See the patch
description for more detail.

I'd really like to see this picked up. I hunch I am the only user of the
driver, or others would have complained. :) I wish the code wasn't as ugly
as it is, but it is the best I can come up with.

Thanks,
Ira

 drivers/hwmon/ltc4245.c |   60 ++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/ltc4245.c b/drivers/hwmon/ltc4245.c
index 65c232a..1efa554 100644
--- a/drivers/hwmon/ltc4245.c
+++ b/drivers/hwmon/ltc4245.c
@@ -68,6 +68,7 @@ static struct ltc4245_data *ltc4245_update_device(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ltc4245_data *data = i2c_get_clientdata(client);
+	u8 gpio, nextgpio, addr, ctl;
 	s32 val;
 	int i;
 
@@ -86,15 +87,68 @@ static struct ltc4245_data *ltc4245_update_device(struct device *dev)
 				data->cregs[i] = val;
 		}
 
-		/* Read voltage registers -- 0x10 to 0x1f */
-		for (i = 0; i < ARRAY_SIZE(data->vregs); i++) {
-			val = i2c_smbus_read_byte_data(client, i+0x10);
+		/* Read non-GPIO voltage registers -- 0x10 to 0x1b */
+		for (i = 0; i < ARRAY_SIZE(data->vregs) - 3; i++) {
+			val = i2c_smbus_read_byte_data(client, i + 0x10);
 			if (unlikely(val < 0))
 				data->vregs[i] = 0;
 			else
 				data->vregs[i] = val;
 		}
 
+		/*
+		 * On the LTC4245, the GPIO registers act stupidly. Changing
+		 * the MUX to read from any GPIO puts the value into all three
+		 * GPIO registers. To avoid this, we cache a copy of the
+		 * current GPIO state, and advance the MUX to the next state.
+		 *
+		 * This will mean that readings can be up to 3 cycles old,
+		 * which shouldn't be a problem. The hwmon interface is not
+		 * expected to be fast.
+		 */
+		gpio = data->cregs[LTC4245_GPIO] & 0xC0;
+		switch (gpio) {
+		default:
+			/*
+			 * this is just to keep the compiler happy, we
+			 * handle every possible state of the top two
+			 * bits in this register
+			 */
+		case 0x00:
+		case 0x40:
+			addr = LTC4245_GPIOADC1;
+			nextgpio = (data->cregs[LTC4245_GPIO] & 0x3f) | 0x80;
+			break;
+		case 0x80:
+			addr = LTC4245_GPIOADC2;
+			nextgpio = (data->cregs[LTC4245_GPIO] & 0x3f) | 0xc0;
+			break;
+		case 0xc0:
+			addr = LTC4245_GPIOADC3;
+			nextgpio = (data->cregs[LTC4245_GPIO] & 0x3f) | 0x40;
+			break;
+		}
+
+		/* Read the current GPIO state, cache the value */
+		val = i2c_smbus_read_byte_data(client, addr);
+		if (unlikely(val < 0))
+			data->vregs[addr - 0x10] = 0;
+		else
+			data->vregs[addr - 0x10] = val;
+
+		/*
+		 * Set the GPIO MUX register to read the next GPIO
+		 *
+		 * According to the datasheet, the ADC must be stopped before
+		 * writing to the control register, so we disable it, and then
+		 * put it back to its earlier state
+		 */
+		ctl = data->cregs[LTC4245_CONTROL];
+		i2c_smbus_write_byte_data(client, LTC4245_CONTROL, ctl | 0x80);
+		i2c_smbus_write_byte_data(client, LTC4245_GPIO, nextgpio);
+		i2c_smbus_write_byte_data(client, LTC4245_CONTROL, ctl);
+		data->cregs[LTC4245_GPIO] = nextgpio;
+
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}
-- 
1.5.4.3


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Jean Delvare]

Hi Ira,

On Tue, 30 Mar 2010 15:57:04 -0700, Ira W. Snyder wrote:
> The GPIO registers on the ltc4245 behave in a strage way. Every time the
> ADC updates the values for one GPIO, it updates all three GPIO registers to
> the same value.

This is not how I read the datasheet. The datasheet says that only one
of the GPIO pins can be routed to the 13th input of the ADC and thus
used as an extra analog voltage input. It also says that there is ONE
register (U) holding the result of the conversion. Apparently many
registers can be access using more than one address, and register U can
be accessed using 4 different addresses (0x1c, 0x1d, 0x1e, 0x1f) but
this is still only one register. So the fact that all addresses return
the same value is not a surprise.

> To read all three GPIO registers correctly, we cache copies of each
> register and update the GPIO MUX to read the next GPIO register. Each GPIO
> sample will take 3 * HZ to read, but at least we are returning the correct
> values to userspace.

I think this is wrong. You should not expose 3 values to user-space,
you should only expose one. The two other GPIOs are supposed to be used
in digital mode. It's not even clear to me that you should always expose
it, as the application may be using all 3 GPIO pins in digital mode and
not be interested in sampling an analog voltage value from any of them.
I admit I am a little surprised that there is no way to disable ADC
operation on the GPIO pin.

> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
> 
> When I implemented the ltc4245 driver, I did not have the ability to test
> the GPIO pins. I now have devices hooked up, and they don't work exactly as
> the datasheet describes (though it is vague in this section). See the patch
> description for more detail.

I think it does behave as the datasheet describes, I'm not sure what
you think is vague, it looks clear to me.

> I'd really like to see this picked up. I hunch I am the only user of the
> driver, or others would have complained. :) I wish the code wasn't as ugly
> as it is, but it is the best I can come up with.

I agree the driver is broken currently, but I don't agree with the
proposed fix. Please just drop the LTC4245_GPIOADC2 and
LTC4245_GPIOADC3 commands, which do not exist, and rename
LTC4245_GPIOADC1 to LTC4245_GPIOADC.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Ira W. Snyder]

On Fri, Apr 02, 2010 at 10:13:41AM +0200, Jean Delvare wrote:
> Hi Ira,
> 
> On Tue, 30 Mar 2010 15:57:04 -0700, Ira W. Snyder wrote:
> > The GPIO registers on the ltc4245 behave in a strage way. Every time the
> > ADC updates the values for one GPIO, it updates all three GPIO registers to
> > the same value.
> 
> This is not how I read the datasheet. The datasheet says that only one
> of the GPIO pins can be routed to the 13th input of the ADC and thus
> used as an extra analog voltage input. It also says that there is ONE
> register (U) holding the result of the conversion. Apparently many
> registers can be access using more than one address, and register U can
> be accessed using 4 different addresses (0x1c, 0x1d, 0x1e, 0x1f) but
> this is still only one register. So the fact that all addresses return
> the same value is not a surprise.
> 

Ok, re-reading tables 6 and 15 in the datasheet, this makes sense now.

> > To read all three GPIO registers correctly, we cache copies of each
> > register and update the GPIO MUX to read the next GPIO register. Each GPIO
> > sample will take 3 * HZ to read, but at least we are returning the correct
> > values to userspace.
> 
> I think this is wrong. You should not expose 3 values to user-space,
> you should only expose one. The two other GPIOs are supposed to be used
> in digital mode. It's not even clear to me that you should always expose
> it, as the application may be using all 3 GPIO pins in digital mode and
> not be interested in sampling an analog voltage value from any of them.
> I admit I am a little surprised that there is no way to disable ADC
> operation on the GPIO pin.
> 

If I do this, how do I make userspace work?

On my board, I have three different devices hooked up to the three GPIO
pins. They're all analog voltages. From userspace, I currently read
in{9,10,11}_input and report those in volts. It works fine, and was
pretty easy to implement.

If I only report a single GPIO value to userspace, then I can only read
one device. How do I switch which GPIO pin is routed to the ADC? I could
open the /dev/i2c-0 device and poke the LTC4245_CONTROL register
directly, but that seems very anti-hwmon, doesn't it?

> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> > 
> > When I implemented the ltc4245 driver, I did not have the ability to test
> > the GPIO pins. I now have devices hooked up, and they don't work exactly as
> > the datasheet describes (though it is vague in this section). See the patch
> > description for more detail.
> 
> I think it does behave as the datasheet describes, I'm not sure what
> you think is vague, it looks clear to me.
> 
> > I'd really like to see this picked up. I hunch I am the only user of the
> > driver, or others would have complained. :) I wish the code wasn't as ugly
> > as it is, but it is the best I can come up with.
> 
> I agree the driver is broken currently, but I don't agree with the
> proposed fix. Please just drop the LTC4245_GPIOADC2 and
> LTC4245_GPIOADC3 commands, which do not exist, and rename
> LTC4245_GPIOADC1 to LTC4245_GPIOADC.
> 

Ok, so I should use LTC4245_GPIOADC (0x1c) to read all of the GPIO
values. That seems fine to me, I can implement that.

See above about how to report this to userspace. I don't know how you
want this exposed in sysfs.

Ira

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Jean Delvare]

On Fri, 2 Apr 2010 09:12:59 -0700, Ira W. Snyder wrote:
> On Fri, Apr 02, 2010 at 10:13:41AM +0200, Jean Delvare wrote:
> > Hi Ira,
> > 
> > On Tue, 30 Mar 2010 15:57:04 -0700, Ira W. Snyder wrote:
> > > The GPIO registers on the ltc4245 behave in a strage way. Every time the
> > > ADC updates the values for one GPIO, it updates all three GPIO registers to
> > > the same value.
> > 
> > This is not how I read the datasheet. The datasheet says that only one
> > of the GPIO pins can be routed to the 13th input of the ADC and thus
> > used as an extra analog voltage input. It also says that there is ONE
> > register (U) holding the result of the conversion. Apparently many
> > registers can be access using more than one address, and register U can
> > be accessed using 4 different addresses (0x1c, 0x1d, 0x1e, 0x1f) but
> > this is still only one register. So the fact that all addresses return
> > the same value is not a surprise.
> > 
> 
> Ok, re-reading tables 6 and 15 in the datasheet, this makes sense now.
> 
> > > To read all three GPIO registers correctly, we cache copies of each
> > > register and update the GPIO MUX to read the next GPIO register. Each GPIO
> > > sample will take 3 * HZ to read, but at least we are returning the correct
> > > values to userspace.
> > 
> > I think this is wrong. You should not expose 3 values to user-space,
> > you should only expose one. The two other GPIOs are supposed to be used
> > in digital mode. It's not even clear to me that you should always expose
> > it, as the application may be using all 3 GPIO pins in digital mode and
> > not be interested in sampling an analog voltage value from any of them.
> > I admit I am a little surprised that there is no way to disable ADC
> > operation on the GPIO pin.
> > 
> 
> If I do this, how do I make userspace work?
> 
> On my board, I have three different devices hooked up to the three GPIO
> pins. They're all analog voltages. From userspace, I currently read
> in{9,10,11}_input and report those in volts. It works fine, and was
> pretty easy to implement.
> 
> If I only report a single GPIO value to userspace, then I can only read
> one device. How do I switch which GPIO pin is routed to the ADC? I could
> open the /dev/i2c-0 device and poke the LTC4245_CONTROL register
> directly, but that seems very anti-hwmon, doesn't it?

You wired your chip in a way it was not meant to be. You shouldn't have.

> 
> > > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > > ---
> > > 
> > > When I implemented the ltc4245 driver, I did not have the ability to test
> > > the GPIO pins. I now have devices hooked up, and they don't work exactly as
> > > the datasheet describes (though it is vague in this section). See the patch
> > > description for more detail.
> > 
> > I think it does behave as the datasheet describes, I'm not sure what
> > you think is vague, it looks clear to me.
> > 
> > > I'd really like to see this picked up. I hunch I am the only user of the
> > > driver, or others would have complained. :) I wish the code wasn't as ugly
> > > as it is, but it is the best I can come up with.
> > 
> > I agree the driver is broken currently, but I don't agree with the
> > proposed fix. Please just drop the LTC4245_GPIOADC2 and
> > LTC4245_GPIOADC3 commands, which do not exist, and rename
> > LTC4245_GPIOADC1 to LTC4245_GPIOADC.
> > 
> 
> Ok, so I should use LTC4245_GPIOADC (0x1c) to read all of the GPIO
> values. That seems fine to me, I can implement that.
> 
> See above about how to report this to userspace. I don't know how you
> want this exposed in sysfs.

I don't want it exposed at all.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Ira W. Snyder]

On Fri, Apr 02, 2010 at 06:24:38PM +0200, Jean Delvare wrote:
> On Fri, 2 Apr 2010 09:12:59 -0700, Ira W. Snyder wrote:
> > On Fri, Apr 02, 2010 at 10:13:41AM +0200, Jean Delvare wrote:
> > > Hi Ira,
> > > 
> > > On Tue, 30 Mar 2010 15:57:04 -0700, Ira W. Snyder wrote:
> > > > The GPIO registers on the ltc4245 behave in a strage way. Every time the
> > > > ADC updates the values for one GPIO, it updates all three GPIO registers to
> > > > the same value.
> > > 
> > > This is not how I read the datasheet. The datasheet says that only one
> > > of the GPIO pins can be routed to the 13th input of the ADC and thus
> > > used as an extra analog voltage input. It also says that there is ONE
> > > register (U) holding the result of the conversion. Apparently many
> > > registers can be access using more than one address, and register U can
> > > be accessed using 4 different addresses (0x1c, 0x1d, 0x1e, 0x1f) but
> > > this is still only one register. So the fact that all addresses return
> > > the same value is not a surprise.
> > > 
> > 
> > Ok, re-reading tables 6 and 15 in the datasheet, this makes sense now.
> > 
> > > > To read all three GPIO registers correctly, we cache copies of each
> > > > register and update the GPIO MUX to read the next GPIO register. Each GPIO
> > > > sample will take 3 * HZ to read, but at least we are returning the correct
> > > > values to userspace.
> > > 
> > > I think this is wrong. You should not expose 3 values to user-space,
> > > you should only expose one. The two other GPIOs are supposed to be used
> > > in digital mode. It's not even clear to me that you should always expose
> > > it, as the application may be using all 3 GPIO pins in digital mode and
> > > not be interested in sampling an analog voltage value from any of them.
> > > I admit I am a little surprised that there is no way to disable ADC
> > > operation on the GPIO pin.
> > > 
> > 
> > If I do this, how do I make userspace work?
> > 
> > On my board, I have three different devices hooked up to the three GPIO
> > pins. They're all analog voltages. From userspace, I currently read
> > in{9,10,11}_input and report those in volts. It works fine, and was
> > pretty easy to implement.
> > 
> > If I only report a single GPIO value to userspace, then I can only read
> > one device. How do I switch which GPIO pin is routed to the ADC? I could
> > open the /dev/i2c-0 device and poke the LTC4245_CONTROL register
> > directly, but that seems very anti-hwmon, doesn't it?
> 
> You wired your chip in a way it was not meant to be. You shouldn't have.
> 

Why do you think it is wired incorrectly? Nothing I've found in the
datasheet suggests that. If you can provide some insight into why you
think this is, maybe we can come to an agreement, instead of just making
me very frustrated.

On Page 24, "General Purpose Input/Outputs", the text says that any of
the three GPIO pins can be connected to the ADC. Page 32, Table 13 "GPIO
Register G" supports this. Each of the three GPIO pins has exactly the
same configurable settings as all the others.

Page 13 "Configuration, GPIO, and Precharge" also states: The GPIO1 to
GPIO3 pins can be used as general purpose inputs or outputs. One of the
pins can also be multiplexed to the GPIO channel of the ADC.

The way I read this, it means: only one of the pins can be multiplexed
onto the ADC ***at a single moment in time***. Why else would the GPIO
register (0x6) exist, other than to let the user switch between them at
runtime? This kind of design sucks, but it is common in the embedded
world, where we are always trying to use fewer parts to do the same job.

Ira

> > 
> > > > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > > > ---
> > > > 
> > > > When I implemented the ltc4245 driver, I did not have the ability to test
> > > > the GPIO pins. I now have devices hooked up, and they don't work exactly as
> > > > the datasheet describes (though it is vague in this section). See the patch
> > > > description for more detail.
> > > 
> > > I think it does behave as the datasheet describes, I'm not sure what
> > > you think is vague, it looks clear to me.
> > > 
> > > > I'd really like to see this picked up. I hunch I am the only user of the
> > > > driver, or others would have complained. :) I wish the code wasn't as ugly
> > > > as it is, but it is the best I can come up with.
> > > 
> > > I agree the driver is broken currently, but I don't agree with the
> > > proposed fix. Please just drop the LTC4245_GPIOADC2 and
> > > LTC4245_GPIOADC3 commands, which do not exist, and rename
> > > LTC4245_GPIOADC1 to LTC4245_GPIOADC.
> > > 
> > 
> > Ok, so I should use LTC4245_GPIOADC (0x1c) to read all of the GPIO
> > values. That seems fine to me, I can implement that.
> > 
> > See above about how to report this to userspace. I don't know how you
> > want this exposed in sysfs.
> 
> I don't want it exposed at all.
> 
> -- 
> Jean Delvare
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Jean Delvare]

Hi Ira,

On Fri, 2 Apr 2010 09:45:07 -0700, Ira W. Snyder wrote:
> On Fri, Apr 02, 2010 at 06:24:38PM +0200, Jean Delvare wrote:
> > You wired your chip in a way it was not meant to be. You shouldn't have.
> 
> Why do you think it is wired incorrectly?

Just look at the code you had to write to get it to work... (And it's
not even covering all use cases, see below.)

> Nothing I've found in the
> datasheet suggests that. If you can provide some insight into why you
> think this is, maybe we can come to an agreement, instead of just making
> me very frustrated.
> 
> On Page 24, "General Purpose Input/Outputs", the text says that any of
> the three GPIO pins can be connected to the ADC. Page 32, Table 13 "GPIO
> Register G" supports this. Each of the three GPIO pins has exactly the
> same configurable settings as all the others.
> 
> Page 13 "Configuration, GPIO, and Precharge" also states: The GPIO1 to
> GPIO3 pins can be used as general purpose inputs or outputs. One of the
> pins can also be multiplexed to the GPIO channel of the ADC.
> 
> The way I read this, it means: only one of the pins can be multiplexed
> onto the ADC ***at a single moment in time***.

The way I read this, it means: you get to choose one GPIO pin to be
routed to the ADC, by configuring register G at initialization time. Of
course nothing prevents you from changing configuration settings later,
but same holds for every other configuration bit, still most of them
make no sense changing after initialization.

> Why else would the GPIO
> register (0x6) exist, other than to let the user switch between them at
> runtime?

Maybe to give some flexibility with wire routing on the board. Or
because GPIO1 has extra features (so that wouldn't be the best choice
for routing to ADC) but GPIO2 and 3 aren't available on the UHF part, so
there was no one-fits-all decision. Or maybe just for maketing, 'cause
now they can claim the part lets you monitor N+2 voltages, so it looks
very cool.

> This kind of design sucks, but it is common in the embedded
> world, where we are always trying to use fewer parts to do the same job.

Of course, it is possible to change the GPIO routing to ADC at
run-time. But then you get a reading for each every 3 refresh cycles.
You say it means every 3 seconds, but you can get much older results if
you do not read the values every second. Someone running "sensors"
manually at a given point in time to get the current state of the
system would end up seeing 2 values which might be hours or days old,
or even zero values if it is the first use since the driver was loaded.
This is very confusing and I certainly don't want any of our drivers to
behave that way.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Ira W. Snyder]

On Fri, Apr 02, 2010 at 07:11:24PM +0200, Jean Delvare wrote:
> Hi Ira,
> 
> On Fri, 2 Apr 2010 09:45:07 -0700, Ira W. Snyder wrote:
> > On Fri, Apr 02, 2010 at 06:24:38PM +0200, Jean Delvare wrote:
> > > You wired your chip in a way it was not meant to be. You shouldn't have.
> > 
> > Why do you think it is wired incorrectly?
> 
> Just look at the code you had to write to get it to work... (And it's
> not even covering all use cases, see below.)
> 
> > Nothing I've found in the
> > datasheet suggests that. If you can provide some insight into why you
> > think this is, maybe we can come to an agreement, instead of just making
> > me very frustrated.
> > 
> > On Page 24, "General Purpose Input/Outputs", the text says that any of
> > the three GPIO pins can be connected to the ADC. Page 32, Table 13 "GPIO
> > Register G" supports this. Each of the three GPIO pins has exactly the
> > same configurable settings as all the others.
> > 
> > Page 13 "Configuration, GPIO, and Precharge" also states: The GPIO1 to
> > GPIO3 pins can be used as general purpose inputs or outputs. One of the
> > pins can also be multiplexed to the GPIO channel of the ADC.
> > 
> > The way I read this, it means: only one of the pins can be multiplexed
> > onto the ADC ***at a single moment in time***.
> 
> The way I read this, it means: you get to choose one GPIO pin to be
> routed to the ADC, by configuring register G at initialization time. Of
> course nothing prevents you from changing configuration settings later,
> but same holds for every other configuration bit, still most of them
> make no sense changing after initialization.
> 

But it *does* make sense to change this one: to let you use all 3 GPIO
pins as voltage sensors. Especially if your board is already very dense,
and you don't have enough space to fit another component.

> > Why else would the GPIO
> > register (0x6) exist, other than to let the user switch between them at
> > runtime?
> 
> Maybe to give some flexibility with wire routing on the board. Or
> because GPIO1 has extra features (so that wouldn't be the best choice
> for routing to ADC) but GPIO2 and 3 aren't available on the UHF part, so
> there was no one-fits-all decision. Or maybe just for maketing, 'cause
> now they can claim the part lets you monitor N+2 voltages, so it looks
> very cool.
> 
> > This kind of design sucks, but it is common in the embedded
> > world, where we are always trying to use fewer parts to do the same job.
> 
> Of course, it is possible to change the GPIO routing to ADC at
> run-time. But then you get a reading for each every 3 refresh cycles.
> You say it means every 3 seconds, but you can get much older results if
> you do not read the values every second. Someone running "sensors"
> manually at a given point in time to get the current state of the
> system would end up seeing 2 values which might be hours or days old,
> or even zero values if it is the first use since the driver was loaded.
> This is very confusing and I certainly don't want any of our drivers to
> behave that way.
> 

Right, in my application, I attempt to read the sensors every 500ms.
Hence the 3s update time for the chip. I'm aware that not everyone reads
their sensors this fast.

Would you accept a patch that zeroed out stale values (older than a few
seconds)? I'm aware that it is suboptimal, but I think that zeroes would
be better than hours-old values. I could add a big warning to the code
and in the Documentation directory.

As the only known user of this driver, I'm trying to play nice and
follow the "get it upstream before you ship it" mantra. Remember the
recent fiasco with the Google Android folks?

The fact is, I've got a board with this chip, and it is wired in a way
you don't like, but is within the capabilities of the chip. I don't want
to forward port these changes forever. Can we please try and decide on
something that will make this chip work the way I have it hooked up?
From my point of view, you're just telling me "I don't like it, go away"
while I'm trying to make a chip work.

Ira

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Jean Delvare]

Hi Ira,

On Fri, 2 Apr 2010 11:09:38 -0700, Ira W. Snyder wrote:
> But it *does* make sense to change this one: to let you use all 3 GPIO
> pins as voltage sensors. Especially if your board is already very dense,
> and you don't have enough space to fit another component.

I think you're off the point. It's not a matter of fitting another
component, it's a matter of picking the right component to start with.
If you had 15 voltage lines you wanted to monitor, you should have
chosen a chip which can monitor 15 voltage lines right away, rather
than one that can monitor "only 13 and maybe 2 more if I add funky code
to the driver and run user-space applications in some specific way".

> Right, in my application, I attempt to read the sensors every 500ms.
> Hence the 3s update time for the chip. I'm aware that not everyone reads
> their sensors this fast.
> 
> Would you accept a patch that zeroed out stale values (older than a few
> seconds)? I'm aware that it is suboptimal, but I think that zeroes would
> be better than hours-old values. I could add a big warning to the code
> and in the Documentation directory.

I don't quite agree. You're trading one evil for another. Instead of
getting an old reading, you'll get a zero which is statistically more
wrong. The only benefit is that it is more obviously incorrect, but it
is also totally unfriendly for monitoring applications. Think of people
running sensord, for example. 2 out of 3 reading cycles will return 0
and the user will end up with totally unusable graphs.

> As the only known user of this driver, I'm trying to play nice and
> follow the "get it upstream before you ship it" mantra.

I fail to see how it matters here.

> Remember the recent fiasco with the Google Android folks?

No, I have no idea what you are talking about, sorry.

> The fact is, I've got a board with this chip, and it is wired in a way
> you don't like, but is within the capabilities of the chip. I don't want
> to forward port these changes forever. Can we please try and decide on
> something that will make this chip work the way I have it hooked up?
> From my point of view, you're just telling me "I don't like it, go away"
> while I'm trying to make a chip work.

You got my point totally right as I see. I understand you don't like
it, I wouldn't like it if I were you.

Don't get me wrong, you're a nice guy, that's nothing personal. Simply,
none of your workaround proposals so far pleased me, and I can't think
of anything better myself. I don't want to have in mainline a driver
which behaves strangely until you read the documentation. Especially if
this would mean making life difficult for users who did wire their own
chip in a smart way (that is, at most one GPIO used for analog voltage
monitoring.)

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Ira W. Snyder]

On Sat, Apr 03, 2010 at 08:08:01PM +0200, Jean Delvare wrote:
> Hi Ira,
> 
> On Fri, 2 Apr 2010 11:09:38 -0700, Ira W. Snyder wrote:
> > But it *does* make sense to change this one: to let you use all 3 GPIO
> > pins as voltage sensors. Especially if your board is already very dense,
> > and you don't have enough space to fit another component.
> 
> I think you're off the point. It's not a matter of fitting another
> component, it's a matter of picking the right component to start with.
> If you had 15 voltage lines you wanted to monitor, you should have
> chosen a chip which can monitor 15 voltage lines right away, rather
> than one that can monitor "only 13 and maybe 2 more if I add funky code
> to the driver and run user-space applications in some specific way".
> 

I guess my hardware engineer would disagree with you ;)

The fact is, our board is packed pretty dense, there isn't much more
space for extra components. Also, I have ~150 of them all wired this
way, so no matter what, I have to support them. If that means I have to
forward port this patch forever, then that's what I'll do.

> > Right, in my application, I attempt to read the sensors every 500ms.
> > Hence the 3s update time for the chip. I'm aware that not everyone reads
> > their sensors this fast.
> > 
> > Would you accept a patch that zeroed out stale values (older than a few
> > seconds)? I'm aware that it is suboptimal, but I think that zeroes would
> > be better than hours-old values. I could add a big warning to the code
> > and in the Documentation directory.
> 
> I don't quite agree. You're trading one evil for another. Instead of
> getting an old reading, you'll get a zero which is statistically more
> wrong. The only benefit is that it is more obviously incorrect, but it
> is also totally unfriendly for monitoring applications. Think of people
> running sensord, for example. 2 out of 3 reading cycles will return 0
> and the user will end up with totally unusable graphs.
> 

What about returning -EINVAL for stale values? I think the sensors tool
and libsensors library will do the right thing. At least if the user is
reading "fast enough" they'll get good values each time. The meaning of
"fast enough" could be 5-10 seconds.

> > As the only known user of this driver, I'm trying to play nice and
> > follow the "get it upstream before you ship it" mantra.
> 
> I fail to see how it matters here.
> 
> > Remember the recent fiasco with the Google Android folks?
> 
> No, I have no idea what you are talking about, sorry.
> 
> > The fact is, I've got a board with this chip, and it is wired in a way
> > you don't like, but is within the capabilities of the chip. I don't want
> > to forward port these changes forever. Can we please try and decide on
> > something that will make this chip work the way I have it hooked up?
> > From my point of view, you're just telling me "I don't like it, go away"
> > while I'm trying to make a chip work.
> 
> You got my point totally right as I see. I understand you don't like
> it, I wouldn't like it if I were you.
> 
> Don't get me wrong, you're a nice guy, that's nothing personal. Simply,
> none of your workaround proposals so far pleased me, and I can't think
> of anything better myself. I don't want to have in mainline a driver
> which behaves strangely until you read the documentation. Especially if
> this would mean making life difficult for users who did wire their own
> chip in a smart way (that is, at most one GPIO used for analog voltage
> monitoring.)
> 

I apologize. I was getting frustrated on Friday in the last few emails.

If you don't like the -EINVAL idea, then I guess I'll just forward port
this forever.

Ira

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Jean Delvare]

Hi Ira,

On Mon, 5 Apr 2010 11:41:14 -0700, Ira W. Snyder wrote:
> On Sat, Apr 03, 2010 at 08:08:01PM +0200, Jean Delvare wrote:
> > Hi Ira,
> > 
> > On Fri, 2 Apr 2010 11:09:38 -0700, Ira W. Snyder wrote:
> > > But it *does* make sense to change this one: to let you use all 3 GPIO
> > > pins as voltage sensors. Especially if your board is already very dense,
> > > and you don't have enough space to fit another component.
> > 
> > I think you're off the point. It's not a matter of fitting another
> > component, it's a matter of picking the right component to start with.
> > If you had 15 voltage lines you wanted to monitor, you should have
> > chosen a chip which can monitor 15 voltage lines right away, rather
> > than one that can monitor "only 13 and maybe 2 more if I add funky code
> > to the driver and run user-space applications in some specific way".
> > 
> 
> I guess my hardware engineer would disagree with you ;)
> 
> The fact is, our board is packed pretty dense, there isn't much more
> space for extra components.

As I already wrote before, the point isn't extra components, but
choosing the right component in the first place.

> Also, I have ~150 of them all wired this
> way, so no matter what, I have to support them. If that means I have to
> forward port this patch forever, then that's what I'll do.

If the hardware is already built, then indeed I can understand that you
are ready to whatever ugly quirks are needed to get them to work.

> > > Right, in my application, I attempt to read the sensors every 500ms.
> > > Hence the 3s update time for the chip. I'm aware that not everyone reads
> > > their sensors this fast.
> > > 
> > > Would you accept a patch that zeroed out stale values (older than a few
> > > seconds)? I'm aware that it is suboptimal, but I think that zeroes would
> > > be better than hours-old values. I could add a big warning to the code
> > > and in the Documentation directory.
> > 
> > I don't quite agree. You're trading one evil for another. Instead of
> > getting an old reading, you'll get a zero which is statistically more
> > wrong. The only benefit is that it is more obviously incorrect, but it
> > is also totally unfriendly for monitoring applications. Think of people
> > running sensord, for example. 2 out of 3 reading cycles will return 0
> > and the user will end up with totally unusable graphs.
> > 
> 
> What about returning -EINVAL for stale values? I think the sensors tool
> and libsensors library will do the right thing.

You'd rather test. I know that libsensors and sensors are notoriously
bad at dealing with some error codes (for example the ones returned by
the thinkpad driver.) So don't assume it will work, test and be ready
to have to fix libsensors and/or sensors.

> At least if the user is
> reading "fast enough" they'll get good values each time. The meaning of
> "fast enough" could be 5-10 seconds.

Which is pretty arbitrary, but would work in practice, I agree.

> > > As the only known user of this driver, I'm trying to play nice and
> > > follow the "get it upstream before you ship it" mantra.
> > 
> > I fail to see how it matters here.
> > 
> > > Remember the recent fiasco with the Google Android folks?
> > 
> > No, I have no idea what you are talking about, sorry.
> > 
> > > The fact is, I've got a board with this chip, and it is wired in a way
> > > you don't like, but is within the capabilities of the chip. I don't want
> > > to forward port these changes forever. Can we please try and decide on
> > > something that will make this chip work the way I have it hooked up?
> > > From my point of view, you're just telling me "I don't like it, go away"
> > > while I'm trying to make a chip work.
> > 
> > You got my point totally right as I see. I understand you don't like
> > it, I wouldn't like it if I were you.
> > 
> > Don't get me wrong, you're a nice guy, that's nothing personal. Simply,
> > none of your workaround proposals so far pleased me, and I can't think
> > of anything better myself. I don't want to have in mainline a driver
> > which behaves strangely until you read the documentation. Especially if
> > this would mean making life difficult for users who did wire their own
> > chip in a smart way (that is, at most one GPIO used for analog voltage
> > monitoring.)
> 
> I apologize. I was getting frustrated on Friday in the last few emails.
> 
> If you don't like the -EINVAL idea, then I guess I'll just forward port
> this forever.

What I want is:
* A patch fixing the current behavior of the driver, because it is
  broken for everybody. The driver should only expose 13 voltage
  channels. This should be fairly easy to fix, and the fix should go
  upstream ASAP.
* That the 13 voltage channels setup stays the default. If you want to
  add a custom mode for your weird hardware setup, it needs to be
  explicitly enabled either by the platform (through device platform
  data) or by the user (through a module parameter or a sysfs attribute.)
  No way it can become the default behavior.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (ltc4245) Read GPIO registers

[Jean Delvare]

Oh, and I forgot:

On Mon, 5 Apr 2010 11:41:14 -0700, Ira W. Snyder wrote:
> What about returning -EINVAL for stale values?

EINVAL is for invalid input, so it's hardly appropriate there. EAGAIN
(try again later) would seem better.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors