* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
@ 2010-05-26 11:31 ` Jean Delvare
2010-05-26 15:42 ` Ira W. Snyder
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2010-05-26 11:31 UTC (permalink / raw)
To: lm-sensors
Hi Ira,
Sorry for the late answer.
On Tue, 13 Apr 2010 15:59:29 -0700, Ira W. Snyder wrote:
> Add support for exposing all GPIO pins as analog voltages. Though this is
> not an ideal use of the chip, some hardware engineers may decide that the
> LTC4245 meets their design requirements when studying the datasheet.
>
> The GPIO pins are sampled in round-robin fashion, meaning that a slow
> reader will see stale data. A userspace application can detect this,
> because it will get -EAGAIN when reading from a sysfs file which contains
> stale data.
A userspace application _or library_. In practice, most monitoring
applications will rely on libsensors.
>
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
> Documentation/hwmon/ltc4245 | 13 +++++-
> drivers/hwmon/Kconfig | 10 +++++
> drivers/hwmon/ltc4245.c | 95 ++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 116 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/hwmon/ltc4245 b/Documentation/hwmon/ltc4245
> index 86b5880..1c0599f 100644
> --- a/Documentation/hwmon/ltc4245
> +++ b/Documentation/hwmon/ltc4245
> @@ -72,9 +72,20 @@ in6_min_alarm 5v output undervoltage alarm
> in7_min_alarm 3v output undervoltage alarm
> in8_min_alarm Vee (-12v) output undervoltage alarm
>
> -in9_input GPIO voltage data
> +in9_input GPIO voltage data (see note 1)
> +in10_input GPIO voltage data (see note 1)
> +in11_input GPIO voltage data (see note 1)
>
> power1_input 12v power usage (mW)
> power2_input 5v power usage (mW)
> power3_input 3v power usage (mW)
> power4_input Vee (-12v) power usage (mW)
> +
> +
> +Note 1
> +------
> +
> +If you have configured your kernel with CONFIG_SENSORS_LTC4245_EXTRA_GPIOS=y
> +then all three GPIO voltage lines will be sampled in round-robin fashion. If
> +the data becomes stale, -EAGAIN will be returned when you read the sysfs
> +attribute containing the sensor reading.
Please also clarify the case CONFIG_SENSORS_LTC4245_EXTRA_GPIOS=n.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e4595e6..e9c99cb 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -623,6 +623,16 @@ config SENSORS_LTC4245
> This driver can also be built as a module. If so, the module will
> be called ltc4245.
>
> +config SENSORS_LTC4245_EXTRA_GPIOS
> + bool "LTC4245: sample all GPIO pins as analog voltages"
> + depends on SENSORS_LTC4245
> + default n
> + help
> + If you say yes here, the LTC4245 driver will read all of the GPIO
> + pins in rotation, reporting them as extra voltage channels.
> +
> + Please read the Documentation/hwmon/ltc4245 file for more details.
> +
I have to admit that I am surprised that you made this a build-time
configuration option. This isn't flexible. Distributions will have to
make a decision for all their users at once. Not only this, but the
behavior will have to be the same for all LTC4245 chips in a given
system, even when using a custom kernel (not that I really expect
multiple LTC4245 chips on the same board, but you never know...)
I presume that your idea was to make this feature as little intrusive
as possible for regular users? I thank you for this, but I'm not quite
sure this is the best approach. A module option, or a per-device
attribute, would be more flexible. That being said, you're the one in
need for this option, so I won't argue further. I'm fine taking
whatever approach seems preferable to you.
> config SENSORS_LM95241
> tristate "National Semiconductor LM95241 sensor chip"
> depends on I2C
> diff --git a/drivers/hwmon/ltc4245.c b/drivers/hwmon/ltc4245.c
> index 84e6f32..33fdc02 100644
> --- a/drivers/hwmon/ltc4245.c
> +++ b/drivers/hwmon/ltc4245.c
> @@ -60,8 +60,70 @@ struct ltc4245_data {
>
> /* Voltage registers */
> u8 vregs[0x0d];
> +
> + /* GPIO ADC registers */
> + unsigned int last_gpio;
This field is never initialized, meaning it starts with value 0
(GPIO1). There is, however, no guarantee that the chip has GPIO1
routed to the ADC when the driver is loaded. OTOH, you read the GPIO
configuration in cregs[LTC4245_GPIO] already, so last_gpio is somewhat
redundant, and you could instead use cregs[LTC4245_GPIO] directly.
> + int gpios[3];
> };
>
> +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> +/*
> + * Update the readings from all three GPIO pins. This means that the old
> + * readings will be delayed.
> + *
> + * LOCKING: must hold data->update_lock
> + */
> +static void ltc4245_update_gpios(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ltc4245_data *data = i2c_get_clientdata(client);
> + u8 gpio_curr, gpio_next, gpio, ctl;
> + int i;
> +
> + /*
> + * If the last reading was 10 seconds ago, then we mark all old GPIO
> + * readings as stale by setting them to zero volts
> + */
> + if (time_after(jiffies, data->last_updated + 10 * HZ)) {
10 seconds is much. If one reads the values every 10 seconds, you will
report 20-seconds old readings without complaining. I would lower this
delay to 5 seconds, tops.
> + dev_dbg(&client->dev, "Marking GPIOs invalid\n");
> + for (i = 0; i < ARRAY_SIZE(data->gpios); i++)
> + data->gpios[i] = -EAGAIN;
> + }
> +
> + /* Read the GPIO voltage from the GPIOADC register */
> + gpio_curr = data->last_gpio;
> + data->gpios[gpio_curr] = data->vregs[LTC4245_GPIOADC - 0x10];
> +
> + /* Find the next GPIO pin to read */
> + gpio_next = (data->last_gpio + 1) % ARRAY_SIZE(data->gpios);
> +
> + /*
> + * Calculate the correct setting for the GPIO register so it will
> + * sample the next GPIO pin
> + */
> + gpio = (data->cregs[LTC4245_GPIO] & 0x3f) | ((gpio_next + 1) << 6);
> + ctl = data->cregs[LTC4245_CONTROL];
> +
> + /* Update the GPIO register */
> + i2c_smbus_write_byte_data(client, LTC4245_CONTROL, ctl | 0x80);
> + i2c_smbus_write_byte_data(client, LTC4245_GPIO, gpio);
> + i2c_smbus_write_byte_data(client, LTC4245_CONTROL, ctl);
Not sure why you toggle bit C7? The datasheet says that it must be
toggled for writing to registers 0x10 to 0x1F, but LTC4245_GPIO is
register 0x06. Is there any other reason I'm missing?
> +
> + /* Update saved data */
> + data->cregs[LTC4245_GPIO] = gpio;
> + data->last_gpio = gpio_next;
> +}
> +#else
> +static void ltc4245_update_gpios(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ltc4245_data *data = i2c_get_clientdata(client);
> +
> + /* Just copy the data from the the GPIOADC register */
> + data->gpios[0] = data->vregs[LTC4245_GPIOADC - 0x10];
> +}
> +#endif
Not sure why you define this stub function. If you want your patch to
be as little intrusive as possible, you can skip ltc4245_update_gpios
altogether in the general case, and handle in9 as every other voltage
input as the driver was doing before your patch.
> +
> static struct ltc4245_data *ltc4245_update_device(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -93,6 +155,9 @@ static struct ltc4245_data *ltc4245_update_device(struct device *dev)
> data->vregs[i] = val;
> }
>
> + /* Update GPIO readings */
> + ltc4245_update_gpios(dev);
> +
> data->last_updated = jiffies;
> data->valid = 1;
> }
> @@ -233,6 +298,22 @@ static ssize_t ltc4245_show_alarm(struct device *dev,
> return snprintf(buf, PAGE_SIZE, "%u\n", (reg & mask) ? 1 : 0);
> }
>
> +static ssize_t ltc4245_show_gpio(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> + struct ltc4245_data *data = ltc4245_update_device(dev);
> + int val = data->gpios[attr->index];
> +
> + /* handle stale GPIO's */
> + if (val < 0)
> + return val;
> +
> + /* Convert to millivolts and print */
> + return snprintf(buf, PAGE_SIZE, "%u\n", val * 10);
> +}
> +
> /* These macros are used below in constructing device attribute objects
> * for use with sysfs_create_group() to make a sysfs device file
> * for each register.
> @@ -254,6 +335,10 @@ static ssize_t ltc4245_show_alarm(struct device *dev,
> static SENSOR_DEVICE_ATTR_2(name, S_IRUGO, \
> ltc4245_show_alarm, NULL, (mask), reg)
>
> +#define LTC4245_GPIO(name, gpio_num) \
> + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> + ltc4245_show_gpio, NULL, gpio_num)
You already have an enum value by that name, this is confusing. That's
what you get for prefixing your register names with only LTC4245_
instead of LTC4245_REG_ as most drivers are doing.
> +
> /* Construct a sensor_device_attribute structure for each register */
>
> /* Input voltages */
> @@ -293,7 +378,11 @@ LTC4245_ALARM(in7_min_alarm, (1 << 2), LTC4245_FAULT2);
> LTC4245_ALARM(in8_min_alarm, (1 << 3), LTC4245_FAULT2);
>
> /* GPIO voltages */
> -LTC4245_VOLTAGE(in9_input, LTC4245_GPIOADC);
> +LTC4245_GPIO(in9_input, 0);
> +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> +LTC4245_GPIO(in10_input, 1);
> +LTC4245_GPIO(in11_input, 2);
> +#endif
>
> /* Power Consumption (virtual) */
> LTC4245_POWER(power1_input, LTC4245_12VSENSE);
> @@ -336,6 +425,10 @@ static struct attribute *ltc4245_attributes[] = {
> &sensor_dev_attr_in8_min_alarm.dev_attr.attr,
>
> &sensor_dev_attr_in9_input.dev_attr.attr,
> +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> + &sensor_dev_attr_in10_input.dev_attr.attr,
> + &sensor_dev_attr_in11_input.dev_attr.attr,
> +#endif
>
> &sensor_dev_attr_power1_input.dev_attr.attr,
> &sensor_dev_attr_power2_input.dev_attr.attr,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
2010-05-26 11:31 ` [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins Jean Delvare
@ 2010-05-26 15:42 ` Ira W. Snyder
2010-05-26 16:36 ` Jean Delvare
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Ira W. Snyder @ 2010-05-26 15:42 UTC (permalink / raw)
To: lm-sensors
On Wed, May 26, 2010 at 01:31:44PM +0200, Jean Delvare wrote:
> Hi Ira,
>
> Sorry for the late answer.
>
> On Tue, 13 Apr 2010 15:59:29 -0700, Ira W. Snyder wrote:
> > Add support for exposing all GPIO pins as analog voltages. Though this is
> > not an ideal use of the chip, some hardware engineers may decide that the
> > LTC4245 meets their design requirements when studying the datasheet.
> >
> > The GPIO pins are sampled in round-robin fashion, meaning that a slow
> > reader will see stale data. A userspace application can detect this,
> > because it will get -EAGAIN when reading from a sysfs file which contains
> > stale data.
>
> A userspace application _or library_. In practice, most monitoring
> applications will rely on libsensors.
>
> >
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> > Documentation/hwmon/ltc4245 | 13 +++++-
> > drivers/hwmon/Kconfig | 10 +++++
> > drivers/hwmon/ltc4245.c | 95 ++++++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 116 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/hwmon/ltc4245 b/Documentation/hwmon/ltc4245
> > index 86b5880..1c0599f 100644
> > --- a/Documentation/hwmon/ltc4245
> > +++ b/Documentation/hwmon/ltc4245
> > @@ -72,9 +72,20 @@ in6_min_alarm 5v output undervoltage alarm
> > in7_min_alarm 3v output undervoltage alarm
> > in8_min_alarm Vee (-12v) output undervoltage alarm
> >
> > -in9_input GPIO voltage data
> > +in9_input GPIO voltage data (see note 1)
> > +in10_input GPIO voltage data (see note 1)
> > +in11_input GPIO voltage data (see note 1)
> >
> > power1_input 12v power usage (mW)
> > power2_input 5v power usage (mW)
> > power3_input 3v power usage (mW)
> > power4_input Vee (-12v) power usage (mW)
> > +
> > +
> > +Note 1
> > +------
> > +
> > +If you have configured your kernel with CONFIG_SENSORS_LTC4245_EXTRA_GPIOS=y
> > +then all three GPIO voltage lines will be sampled in round-robin fashion. If
> > +the data becomes stale, -EAGAIN will be returned when you read the sysfs
> > +attribute containing the sensor reading.
>
> Please also clarify the case CONFIG_SENSORS_LTC4245_EXTRA_GPIOS=n.
>
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index e4595e6..e9c99cb 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -623,6 +623,16 @@ config SENSORS_LTC4245
> > This driver can also be built as a module. If so, the module will
> > be called ltc4245.
> >
> > +config SENSORS_LTC4245_EXTRA_GPIOS
> > + bool "LTC4245: sample all GPIO pins as analog voltages"
> > + depends on SENSORS_LTC4245
> > + default n
> > + help
> > + If you say yes here, the LTC4245 driver will read all of the GPIO
> > + pins in rotation, reporting them as extra voltage channels.
> > +
> > + Please read the Documentation/hwmon/ltc4245 file for more details.
> > +
>
> I have to admit that I am surprised that you made this a build-time
> configuration option. This isn't flexible. Distributions will have to
> make a decision for all their users at once. Not only this, but the
> behavior will have to be the same for all LTC4245 chips in a given
> system, even when using a custom kernel (not that I really expect
> multiple LTC4245 chips on the same board, but you never know...)
>
> I presume that your idea was to make this feature as little intrusive
> as possible for regular users? I thank you for this, but I'm not quite
> sure this is the best approach. A module option, or a per-device
> attribute, would be more flexible. That being said, you're the one in
> need for this option, so I won't argue further. I'm fine taking
> whatever approach seems preferable to you.
>
That's correct. I know you're concerned about code size, so I tried to
keep things as minimal as possible for what you consider to be the
correct use of the chip.
My only board with this chip is a powerpc, and uses the OpenFirmware
(OF) bindings to tell the driver which i2c address the chip is at. I'd
be happier to do this with platform data, but I have absolutely no idea
how to do that with the OF bindings. I'm not sure it is even possible.
> > config SENSORS_LM95241
> > tristate "National Semiconductor LM95241 sensor chip"
> > depends on I2C
> > diff --git a/drivers/hwmon/ltc4245.c b/drivers/hwmon/ltc4245.c
> > index 84e6f32..33fdc02 100644
> > --- a/drivers/hwmon/ltc4245.c
> > +++ b/drivers/hwmon/ltc4245.c
> > @@ -60,8 +60,70 @@ struct ltc4245_data {
> >
> > /* Voltage registers */
> > u8 vregs[0x0d];
> > +
> > + /* GPIO ADC registers */
> > + unsigned int last_gpio;
>
> This field is never initialized, meaning it starts with value 0
> (GPIO1). There is, however, no guarantee that the chip has GPIO1
> routed to the ADC when the driver is loaded. OTOH, you read the GPIO
> configuration in cregs[LTC4245_GPIO] already, so last_gpio is somewhat
> redundant, and you could instead use cregs[LTC4245_GPIO] directly.
>
I'll make an updated patch which uses the register instead.
> > + int gpios[3];
> > };
> >
> > +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> > +/*
> > + * Update the readings from all three GPIO pins. This means that the old
> > + * readings will be delayed.
> > + *
> > + * LOCKING: must hold data->update_lock
> > + */
> > +static void ltc4245_update_gpios(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ltc4245_data *data = i2c_get_clientdata(client);
> > + u8 gpio_curr, gpio_next, gpio, ctl;
> > + int i;
> > +
> > + /*
> > + * If the last reading was 10 seconds ago, then we mark all old GPIO
> > + * readings as stale by setting them to zero volts
> > + */
> > + if (time_after(jiffies, data->last_updated + 10 * HZ)) {
>
> 10 seconds is much. If one reads the values every 10 seconds, you will
> report 20-seconds old readings without complaining. I would lower this
> delay to 5 seconds, tops.
>
Ok, 5 seconds it is.
> > + dev_dbg(&client->dev, "Marking GPIOs invalid\n");
> > + for (i = 0; i < ARRAY_SIZE(data->gpios); i++)
> > + data->gpios[i] = -EAGAIN;
> > + }
> > +
> > + /* Read the GPIO voltage from the GPIOADC register */
> > + gpio_curr = data->last_gpio;
> > + data->gpios[gpio_curr] = data->vregs[LTC4245_GPIOADC - 0x10];
> > +
> > + /* Find the next GPIO pin to read */
> > + gpio_next = (data->last_gpio + 1) % ARRAY_SIZE(data->gpios);
> > +
> > + /*
> > + * Calculate the correct setting for the GPIO register so it will
> > + * sample the next GPIO pin
> > + */
> > + gpio = (data->cregs[LTC4245_GPIO] & 0x3f) | ((gpio_next + 1) << 6);
> > + ctl = data->cregs[LTC4245_CONTROL];
> > +
> > + /* Update the GPIO register */
> > + i2c_smbus_write_byte_data(client, LTC4245_CONTROL, ctl | 0x80);
> > + i2c_smbus_write_byte_data(client, LTC4245_GPIO, gpio);
> > + i2c_smbus_write_byte_data(client, LTC4245_CONTROL, ctl);
>
> Not sure why you toggle bit C7? The datasheet says that it must be
> toggled for writing to registers 0x10 to 0x1F, but LTC4245_GPIO is
> register 0x06. Is there any other reason I'm missing?
>
Nope, you're exactly right. I'll remove the write to the LTC4245_CONTROL
register.
I guess I read the note about setting/clearing the bit for the
LTC4245_GPIOADC (0x1c) register, and then my mind carried it over to the
LTC4245_GPIO (0x06) register too. Good catch.
> > +
> > + /* Update saved data */
> > + data->cregs[LTC4245_GPIO] = gpio;
> > + data->last_gpio = gpio_next;
> > +}
> > +#else
> > +static void ltc4245_update_gpios(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ltc4245_data *data = i2c_get_clientdata(client);
> > +
> > + /* Just copy the data from the the GPIOADC register */
> > + data->gpios[0] = data->vregs[LTC4245_GPIOADC - 0x10];
> > +}
> > +#endif
>
> Not sure why you define this stub function. If you want your patch to
> be as little intrusive as possible, you can skip ltc4245_update_gpios
> altogether in the general case, and handle in9 as every other voltage
> input as the driver was doing before your patch.
>
I did it so ltc4245_update_gpios() could be called in both cases,
without any conditional logic. See the hunk below. I just call
ltc4245_update_gpios() without any #ifdef, it "just works" in both
cases.
This is the usual thing Linux does with headers. For example, see
include/linux/pci.h pci_enable_msix() function. When CONFIG_PCI_MSI=n
they stub out the function so it does the right thing: return an error.
> > +
> > static struct ltc4245_data *ltc4245_update_device(struct device *dev)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > @@ -93,6 +155,9 @@ static struct ltc4245_data *ltc4245_update_device(struct device *dev)
> > data->vregs[i] = val;
> > }
> >
> > + /* Update GPIO readings */
> > + ltc4245_update_gpios(dev);
> > +
> > data->last_updated = jiffies;
> > data->valid = 1;
> > }
> > @@ -233,6 +298,22 @@ static ssize_t ltc4245_show_alarm(struct device *dev,
> > return snprintf(buf, PAGE_SIZE, "%u\n", (reg & mask) ? 1 : 0);
> > }
> >
> > +static ssize_t ltc4245_show_gpio(struct device *dev,
> > + struct device_attribute *da,
> > + char *buf)
> > +{
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> > + struct ltc4245_data *data = ltc4245_update_device(dev);
> > + int val = data->gpios[attr->index];
> > +
> > + /* handle stale GPIO's */
> > + if (val < 0)
> > + return val;
> > +
> > + /* Convert to millivolts and print */
> > + return snprintf(buf, PAGE_SIZE, "%u\n", val * 10);
> > +}
> > +
> > /* These macros are used below in constructing device attribute objects
> > * for use with sysfs_create_group() to make a sysfs device file
> > * for each register.
> > @@ -254,6 +335,10 @@ static ssize_t ltc4245_show_alarm(struct device *dev,
> > static SENSOR_DEVICE_ATTR_2(name, S_IRUGO, \
> > ltc4245_show_alarm, NULL, (mask), reg)
> >
> > +#define LTC4245_GPIO(name, gpio_num) \
> > + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> > + ltc4245_show_gpio, NULL, gpio_num)
>
> You already have an enum value by that name, this is confusing. That's
> what you get for prefixing your register names with only LTC4245_
> instead of LTC4245_REG_ as most drivers are doing.
>
The driver works, so I guess the compiler isn't confused. How about I
rename the macro? Is there a different name you would suggest?
> > +
> > /* Construct a sensor_device_attribute structure for each register */
> >
> > /* Input voltages */
> > @@ -293,7 +378,11 @@ LTC4245_ALARM(in7_min_alarm, (1 << 2), LTC4245_FAULT2);
> > LTC4245_ALARM(in8_min_alarm, (1 << 3), LTC4245_FAULT2);
> >
> > /* GPIO voltages */
> > -LTC4245_VOLTAGE(in9_input, LTC4245_GPIOADC);
> > +LTC4245_GPIO(in9_input, 0);
> > +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> > +LTC4245_GPIO(in10_input, 1);
> > +LTC4245_GPIO(in11_input, 2);
> > +#endif
> >
> > /* Power Consumption (virtual) */
> > LTC4245_POWER(power1_input, LTC4245_12VSENSE);
> > @@ -336,6 +425,10 @@ static struct attribute *ltc4245_attributes[] = {
> > &sensor_dev_attr_in8_min_alarm.dev_attr.attr,
> >
> > &sensor_dev_attr_in9_input.dev_attr.attr,
> > +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> > + &sensor_dev_attr_in10_input.dev_attr.attr,
> > + &sensor_dev_attr_in11_input.dev_attr.attr,
> > +#endif
> >
> > &sensor_dev_attr_power1_input.dev_attr.attr,
> > &sensor_dev_attr_power2_input.dev_attr.attr,
>
One more question: if I use a module parameter or platform data to
enable the extra GPIOs, how would I handle this array? I don't see how I
can make it work other than generating two different (mostly redundant)
arrays and registering a different one for each configuration.
Ira
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
2010-05-26 11:31 ` [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins Jean Delvare
2010-05-26 15:42 ` Ira W. Snyder
@ 2010-05-26 16:36 ` Jean Delvare
2010-05-26 17:22 ` Ira W. Snyder
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2010-05-26 16:36 UTC (permalink / raw)
To: lm-sensors
Hi Ira,
On Wed, 26 May 2010 08:42:26 -0700, Ira W. Snyder wrote:
> On Wed, May 26, 2010 at 01:31:44PM +0200, Jean Delvare wrote:
> > On Tue, 13 Apr 2010 15:59:29 -0700, Ira W. Snyder wrote:
> > > +
> > > + /* Update saved data */
> > > + data->cregs[LTC4245_GPIO] = gpio;
> > > + data->last_gpio = gpio_next;
> > > +}
> > > +#else
> > > +static void ltc4245_update_gpios(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct ltc4245_data *data = i2c_get_clientdata(client);
> > > +
> > > + /* Just copy the data from the the GPIOADC register */
> > > + data->gpios[0] = data->vregs[LTC4245_GPIOADC - 0x10];
> > > +}
> > > +#endif
> >
> > Not sure why you define this stub function. If you want your patch to
> > be as little intrusive as possible, you can skip ltc4245_update_gpios
> > altogether in the general case, and handle in9 as every other voltage
> > input as the driver was doing before your patch.
>
> I did it so ltc4245_update_gpios() could be called in both cases,
> without any conditional logic. See the hunk below. I just call
> ltc4245_update_gpios() without any #ifdef, it "just works" in both
> cases.
>
> This is the usual thing Linux does with headers. For example, see
> include/linux/pci.h pci_enable_msix() function. When CONFIG_PCI_MSI=n
> they stub out the function so it does the right thing: return an error.
Sorry for not being clear. I wasn't objecting to the stub's existence,
but to the fact that it was doing something. I expected it to do
nothing at all, and ltc4245_show_voltage() would be used for in9.
> > > (...)
> > > +#define LTC4245_GPIO(name, gpio_num) \
> > > + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> > > + ltc4245_show_gpio, NULL, gpio_num)
> >
> > You already have an enum value by that name, this is confusing. That's
> > what you get for prefixing your register names with only LTC4245_
> > instead of LTC4245_REG_ as most drivers are doing.
>
> The driver works, so I guess the compiler isn't confused. How about I
> rename the macro? Is there a different name you would suggest?
I was about to suggest LTC4245_GPIOADC, you that one already exists
too. LTC4245_VOLTAGE_GPIO maybe?
> > > @@ -336,6 +425,10 @@ static struct attribute *ltc4245_attributes[] = {
> > > &sensor_dev_attr_in8_min_alarm.dev_attr.attr,
> > >
> > > &sensor_dev_attr_in9_input.dev_attr.attr,
> > > +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> > > + &sensor_dev_attr_in10_input.dev_attr.attr,
> > > + &sensor_dev_attr_in11_input.dev_attr.attr,
> > > +#endif
> > >
> > > &sensor_dev_attr_power1_input.dev_attr.attr,
> > > &sensor_dev_attr_power2_input.dev_attr.attr,
> >
>
> One more question: if I use a module parameter or platform data to
> enable the extra GPIOs, how would I handle this array? I don't see how I
> can make it work other than generating two different (mostly redundant)
> arrays and registering a different one for each configuration.
No, you would move in10 and in11 to a separate array of attributes,
with a dedicated attribute group. The general code path would register
ltc4245_group, and your platform specific code path would additionally
register that other group. See drivers/hwmon/adm1025.c for an example,
the in4* attributes are in a separate group.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
` (2 preceding siblings ...)
2010-05-26 16:36 ` Jean Delvare
@ 2010-05-26 17:22 ` Ira W. Snyder
2010-05-26 18:42 ` Jean Delvare
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Ira W. Snyder @ 2010-05-26 17:22 UTC (permalink / raw)
To: lm-sensors
On Wed, May 26, 2010 at 06:36:59PM +0200, Jean Delvare wrote:
> Hi Ira,
>
> On Wed, 26 May 2010 08:42:26 -0700, Ira W. Snyder wrote:
> > On Wed, May 26, 2010 at 01:31:44PM +0200, Jean Delvare wrote:
> > > On Tue, 13 Apr 2010 15:59:29 -0700, Ira W. Snyder wrote:
> > > > +
> > > > + /* Update saved data */
> > > > + data->cregs[LTC4245_GPIO] = gpio;
> > > > + data->last_gpio = gpio_next;
> > > > +}
> > > > +#else
> > > > +static void ltc4245_update_gpios(struct device *dev)
> > > > +{
> > > > + struct i2c_client *client = to_i2c_client(dev);
> > > > + struct ltc4245_data *data = i2c_get_clientdata(client);
> > > > +
> > > > + /* Just copy the data from the the GPIOADC register */
> > > > + data->gpios[0] = data->vregs[LTC4245_GPIOADC - 0x10];
> > > > +}
> > > > +#endif
> > >
> > > Not sure why you define this stub function. If you want your patch to
> > > be as little intrusive as possible, you can skip ltc4245_update_gpios
> > > altogether in the general case, and handle in9 as every other voltage
> > > input as the driver was doing before your patch.
> >
> > I did it so ltc4245_update_gpios() could be called in both cases,
> > without any conditional logic. See the hunk below. I just call
> > ltc4245_update_gpios() without any #ifdef, it "just works" in both
> > cases.
> >
> > This is the usual thing Linux does with headers. For example, see
> > include/linux/pci.h pci_enable_msix() function. When CONFIG_PCI_MSI=n
> > they stub out the function so it does the right thing: return an error.
>
> Sorry for not being clear. I wasn't objecting to the stub's existence,
> but to the fact that it was doing something. I expected it to do
> nothing at all, and ltc4245_show_voltage() would be used for in9.
>
Oh, ok. The ltc4245_show_voltage() function reads the voltage register
values directly from data->vregs[]. When the extra GPIO inputs are
enabled, I have two choices:
1) store them in the new data->gpios[] array
2) store them in data->vregs[]
For #1, ltc4245_show_voltage() doesn't work anymore, since it reads
voltage register values from data->vregs[].
For #2, I would have to re-expand data->vregs[] to include all of the
GPIO inputs, like it was before. Also, I would need to make
ltc4245_get_voltage() handle the -EAGAIN error code.
I think the code is easier to understand if all the GPIOs are treated in
the same way, and not completely special for GPIOADC1 vs GPIOADC[23].
That's why I chose #1.
If I do a hybrid approach (store GPIOADC1 in data->vregs[] and
GPIOADC[23] in data->gpios[]), then I have to make ltc4245_get_voltage()
robust against errors too. Is this what you're suggesting?
> > > > (...)
> > > > +#define LTC4245_GPIO(name, gpio_num) \
> > > > + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> > > > + ltc4245_show_gpio, NULL, gpio_num)
> > >
> > > You already have an enum value by that name, this is confusing. That's
> > > what you get for prefixing your register names with only LTC4245_
> > > instead of LTC4245_REG_ as most drivers are doing.
> >
> > The driver works, so I guess the compiler isn't confused. How about I
> > rename the macro? Is there a different name you would suggest?
>
> I was about to suggest LTC4245_GPIOADC, you that one already exists
> too. LTC4245_VOLTAGE_GPIO maybe?
>
Seems fine.
> > > > @@ -336,6 +425,10 @@ static struct attribute *ltc4245_attributes[] = {
> > > > &sensor_dev_attr_in8_min_alarm.dev_attr.attr,
> > > >
> > > > &sensor_dev_attr_in9_input.dev_attr.attr,
> > > > +#ifdef CONFIG_SENSORS_LTC4245_EXTRA_GPIOS
> > > > + &sensor_dev_attr_in10_input.dev_attr.attr,
> > > > + &sensor_dev_attr_in11_input.dev_attr.attr,
> > > > +#endif
> > > >
> > > > &sensor_dev_attr_power1_input.dev_attr.attr,
> > > > &sensor_dev_attr_power2_input.dev_attr.attr,
> > >
> >
> > One more question: if I use a module parameter or platform data to
> > enable the extra GPIOs, how would I handle this array? I don't see how I
> > can make it work other than generating two different (mostly redundant)
> > arrays and registering a different one for each configuration.
>
> No, you would move in10 and in11 to a separate array of attributes,
> with a dedicated attribute group. The general code path would register
> ltc4245_group, and your platform specific code path would additionally
> register that other group. See drivers/hwmon/adm1025.c for an example,
> the in4* attributes are in a separate group.
>
Ok.
Any thoughts about the kernel config option vs. platform data, and how
it relates to the OF bindings?
Ira
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
` (3 preceding siblings ...)
2010-05-26 17:22 ` Ira W. Snyder
@ 2010-05-26 18:42 ` Jean Delvare
2010-05-26 19:39 ` Ira W. Snyder
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2010-05-26 18:42 UTC (permalink / raw)
To: lm-sensors
On Wed, 26 May 2010 10:22:44 -0700, Ira W. Snyder wrote:
> On Wed, May 26, 2010 at 06:36:59PM +0200, Jean Delvare wrote:
> > Sorry for not being clear. I wasn't objecting to the stub's existence,
> > but to the fact that it was doing something. I expected it to do
> > nothing at all, and ltc4245_show_voltage() would be used for in9.
>
> Oh, ok. The ltc4245_show_voltage() function reads the voltage register
> values directly from data->vregs[]. When the extra GPIO inputs are
> enabled, I have two choices:
>
> 1) store them in the new data->gpios[] array
> 2) store them in data->vregs[]
>
> For #1, ltc4245_show_voltage() doesn't work anymore, since it reads
> voltage register values from data->vregs[].
>
> For #2, I would have to re-expand data->vregs[] to include all of the
> GPIO inputs, like it was before. Also, I would need to make
> ltc4245_get_voltage() handle the -EAGAIN error code.
>
> I think the code is easier to understand if all the GPIOs are treated in
> the same way, and not completely special for GPIOADC1 vs GPIOADC[23].
> That's why I chose #1.
>
> If I do a hybrid approach (store GPIOADC1 in data->vregs[] and
> GPIOADC[23] in data->gpios[]), then I have to make ltc4245_get_voltage()
> robust against errors too. Is this what you're suggesting?
No. But let's wait and see if you move to OF platform data, the whole
question will be moot. If you keep the config option approach, I'll
propose an iterative patch and we can discuss it then.
> (...)
> Any thoughts about the kernel config option vs. platform data, and how
> it relates to the OF bindings?
I would prefer platform-data-based. But I fear I don't know anything
about OF bindings. Better ask the embedded folks (e.g. Grant Likely),
they will know.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
` (4 preceding siblings ...)
2010-05-26 18:42 ` Jean Delvare
@ 2010-05-26 19:39 ` Ira W. Snyder
2010-05-26 20:37 ` Grant Likely
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Ira W. Snyder @ 2010-05-26 19:39 UTC (permalink / raw)
To: lm-sensors
On Wed, May 26, 2010 at 08:42:22PM +0200, Jean Delvare wrote:
> On Wed, 26 May 2010 10:22:44 -0700, Ira W. Snyder wrote:
> > On Wed, May 26, 2010 at 06:36:59PM +0200, Jean Delvare wrote:
> > > Sorry for not being clear. I wasn't objecting to the stub's existence,
> > > but to the fact that it was doing something. I expected it to do
> > > nothing at all, and ltc4245_show_voltage() would be used for in9.
> >
> > Oh, ok. The ltc4245_show_voltage() function reads the voltage register
> > values directly from data->vregs[]. When the extra GPIO inputs are
> > enabled, I have two choices:
> >
> > 1) store them in the new data->gpios[] array
> > 2) store them in data->vregs[]
> >
> > For #1, ltc4245_show_voltage() doesn't work anymore, since it reads
> > voltage register values from data->vregs[].
> >
> > For #2, I would have to re-expand data->vregs[] to include all of the
> > GPIO inputs, like it was before. Also, I would need to make
> > ltc4245_get_voltage() handle the -EAGAIN error code.
> >
> > I think the code is easier to understand if all the GPIOs are treated in
> > the same way, and not completely special for GPIOADC1 vs GPIOADC[23].
> > That's why I chose #1.
> >
> > If I do a hybrid approach (store GPIOADC1 in data->vregs[] and
> > GPIOADC[23] in data->gpios[]), then I have to make ltc4245_get_voltage()
> > robust against errors too. Is this what you're suggesting?
>
> No. But let's wait and see if you move to OF platform data, the whole
> question will be moot. If you keep the config option approach, I'll
> propose an iterative patch and we can discuss it then.
>
> > (...)
> > Any thoughts about the kernel config option vs. platform data, and how
> > it relates to the OF bindings?
>
> I would prefer platform-data-based. But I fear I don't know anything
> about OF bindings. Better ask the embedded folks (e.g. Grant Likely),
> they will know.
>
Grant, you're listed in MAINTAINERS as the OF bindings maintainer. Is it
currently possible for the i2c OF binding to pass platform_data to hwmon
drivers that are probed via the device tree? If so, can you point me to
an example. If not, can you suggest what I should do?
Here is a short example from my device tree (a lightly modified 834x_mds
device tree):
i2c@3100 {
#address-cells = <1>;
#size-cells = <0>;
cell-index = <1>;
compatible = "fsl-i2c";
reg = <0x3100 0x100>;
interrupts = <15 0x8>;
interrupt-parent = <&ipic>;
clock-frequency = <400000>;
ltc4245@XX {
compatible = "LLTC,ltc4245";
reg = <0>; // from bootloader
};
};
I'd like to be able to send platform data to the drivers/hwmon/ltc4245.c
driver. On my platform, this is probed automatically using the device
tree snippet above.
Thanks,
Ira
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
` (5 preceding siblings ...)
2010-05-26 19:39 ` Ira W. Snyder
@ 2010-05-26 20:37 ` Grant Likely
2010-05-26 20:56 ` Ira W. Snyder
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2010-05-26 20:37 UTC (permalink / raw)
To: lm-sensors
On Wed, May 26, 2010 at 1:39 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> On Wed, May 26, 2010 at 08:42:22PM +0200, Jean Delvare wrote:
>> On Wed, 26 May 2010 10:22:44 -0700, Ira W. Snyder wrote:
>> > On Wed, May 26, 2010 at 06:36:59PM +0200, Jean Delvare wrote:
>> > > Sorry for not being clear. I wasn't objecting to the stub's existence,
>> > > but to the fact that it was doing something. I expected it to do
>> > > nothing at all, and ltc4245_show_voltage() would be used for in9.
>> >
>> > Oh, ok. The ltc4245_show_voltage() function reads the voltage register
>> > values directly from data->vregs[]. When the extra GPIO inputs are
>> > enabled, I have two choices:
>> >
>> > 1) store them in the new data->gpios[] array
>> > 2) store them in data->vregs[]
>> >
>> > For #1, ltc4245_show_voltage() doesn't work anymore, since it reads
>> > voltage register values from data->vregs[].
>> >
>> > For #2, I would have to re-expand data->vregs[] to include all of the
>> > GPIO inputs, like it was before. Also, I would need to make
>> > ltc4245_get_voltage() handle the -EAGAIN error code.
>> >
>> > I think the code is easier to understand if all the GPIOs are treated in
>> > the same way, and not completely special for GPIOADC1 vs GPIOADC[23].
>> > That's why I chose #1.
>> >
>> > If I do a hybrid approach (store GPIOADC1 in data->vregs[] and
>> > GPIOADC[23] in data->gpios[]), then I have to make ltc4245_get_voltage()
>> > robust against errors too. Is this what you're suggesting?
>>
>> No. But let's wait and see if you move to OF platform data, the whole
>> question will be moot. If you keep the config option approach, I'll
>> propose an iterative patch and we can discuss it then.
>>
>> > (...)
>> > Any thoughts about the kernel config option vs. platform data, and how
>> > it relates to the OF bindings?
>>
>> I would prefer platform-data-based. But I fear I don't know anything
>> about OF bindings. Better ask the embedded folks (e.g. Grant Likely),
>> they will know.
>>
>
> Grant, you're listed in MAINTAINERS as the OF bindings maintainer. Is it
> currently possible for the i2c OF binding to pass platform_data to hwmon
> drivers that are probed via the device tree? If so, can you point me to
> an example. If not, can you suggest what I should do?
>
> Here is a short example from my device tree (a lightly modified 834x_mds
> device tree):
>
> i2c@3100 {
> #address-cells = <1>;
> #size-cells = <0>;
> cell-index = <1>;
> compatible = "fsl-i2c";
> reg = <0x3100 0x100>;
> interrupts = <15 0x8>;
> interrupt-parent = <&ipic>;
> clock-frequency = <400000>;
>
> ltc4245@XX {
> compatible = "LLTC,ltc4245";
> reg = <0>; // from bootloader
> };
> };
>
> I'd like to be able to send platform data to the drivers/hwmon/ltc4245.c
> driver. On my platform, this is probed automatically using the device
> tree snippet above.
What is in the platform data? As of the .35 merge window, the device
node pointer lives in struct device, so it is available to all Linux
device drivers if CONFIG_OF is set. If it is simply data that needs
to be passed, then the driver itself can extract it from the tree in
the absence of pdata. If you need to pass function pointers or the
like, then probably the best thing to do is to register a bus notifier
in the platform code before registering devices. That way the
platform code can intercept the device and register pdata before the
device is bound to a driver.
g.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
` (6 preceding siblings ...)
2010-05-26 20:37 ` Grant Likely
@ 2010-05-26 20:56 ` Ira W. Snyder
2010-05-26 21:00 ` Jean Delvare
2010-05-26 21:10 ` Grant Likely
9 siblings, 0 replies; 11+ messages in thread
From: Ira W. Snyder @ 2010-05-26 20:56 UTC (permalink / raw)
To: lm-sensors
On Wed, May 26, 2010 at 02:37:28PM -0600, Grant Likely wrote:
> On Wed, May 26, 2010 at 1:39 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> > On Wed, May 26, 2010 at 08:42:22PM +0200, Jean Delvare wrote:
> >> On Wed, 26 May 2010 10:22:44 -0700, Ira W. Snyder wrote:
> >> > On Wed, May 26, 2010 at 06:36:59PM +0200, Jean Delvare wrote:
> >> > > Sorry for not being clear. I wasn't objecting to the stub's existence,
> >> > > but to the fact that it was doing something. I expected it to do
> >> > > nothing at all, and ltc4245_show_voltage() would be used for in9.
> >> >
> >> > Oh, ok. The ltc4245_show_voltage() function reads the voltage register
> >> > values directly from data->vregs[]. When the extra GPIO inputs are
> >> > enabled, I have two choices:
> >> >
> >> > 1) store them in the new data->gpios[] array
> >> > 2) store them in data->vregs[]
> >> >
> >> > For #1, ltc4245_show_voltage() doesn't work anymore, since it reads
> >> > voltage register values from data->vregs[].
> >> >
> >> > For #2, I would have to re-expand data->vregs[] to include all of the
> >> > GPIO inputs, like it was before. Also, I would need to make
> >> > ltc4245_get_voltage() handle the -EAGAIN error code.
> >> >
> >> > I think the code is easier to understand if all the GPIOs are treated in
> >> > the same way, and not completely special for GPIOADC1 vs GPIOADC[23].
> >> > That's why I chose #1.
> >> >
> >> > If I do a hybrid approach (store GPIOADC1 in data->vregs[] and
> >> > GPIOADC[23] in data->gpios[]), then I have to make ltc4245_get_voltage()
> >> > robust against errors too. Is this what you're suggesting?
> >>
> >> No. But let's wait and see if you move to OF platform data, the whole
> >> question will be moot. If you keep the config option approach, I'll
> >> propose an iterative patch and we can discuss it then.
> >>
> >> > (...)
> >> > Any thoughts about the kernel config option vs. platform data, and how
> >> > it relates to the OF bindings?
> >>
> >> I would prefer platform-data-based. But I fear I don't know anything
> >> about OF bindings. Better ask the embedded folks (e.g. Grant Likely),
> >> they will know.
> >>
> >
> > Grant, you're listed in MAINTAINERS as the OF bindings maintainer. Is it
> > currently possible for the i2c OF binding to pass platform_data to hwmon
> > drivers that are probed via the device tree? If so, can you point me to
> > an example. If not, can you suggest what I should do?
> >
> > Here is a short example from my device tree (a lightly modified 834x_mds
> > device tree):
> >
> > i2c@3100 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > cell-index = <1>;
> > compatible = "fsl-i2c";
> > reg = <0x3100 0x100>;
> > interrupts = <15 0x8>;
> > interrupt-parent = <&ipic>;
> > clock-frequency = <400000>;
> >
> > ltc4245@XX {
> > compatible = "LLTC,ltc4245";
> > reg = <0>; // from bootloader
> > };
> > };
> >
> > I'd like to be able to send platform data to the drivers/hwmon/ltc4245.c
> > driver. On my platform, this is probed automatically using the device
> > tree snippet above.
>
> What is in the platform data?
A bool/int should be sufficient.
> As of the .35 merge window, the device
> node pointer lives in struct device, so it is available to all Linux
> device drivers if CONFIG_OF is set. If it is simply data that needs
> to be passed, then the driver itself can extract it from the tree in
> the absence of pdata. If you need to pass function pointers or the
> like, then probably the best thing to do is to register a bus notifier
> in the platform code before registering devices. That way the
> platform code can intercept the device and register pdata before the
> device is bound to a driver.
Ok, so I should wrap the OF detection code in CONFIG_OF in my driver,
right? I just quickly browsed through the code, and it seems like it
should be straightforward to use pdata and then fall back on OF data.
Jean, give me a few hours to cook up another patch, now that I have this
information from Grant.
Thanks for the quick reply!
Ira
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
` (7 preceding siblings ...)
2010-05-26 20:56 ` Ira W. Snyder
@ 2010-05-26 21:00 ` Jean Delvare
2010-05-26 21:10 ` Grant Likely
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2010-05-26 21:00 UTC (permalink / raw)
To: lm-sensors
On Wed, 26 May 2010 13:56:54 -0700, Ira W. Snyder wrote:
> Jean, give me a few hours to cook up another patch, now that I have this
> information from Grant.
I can give you 9, as I'm on my way to bed.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins
2010-04-13 22:59 [lm-sensors] [PATCH 2/2] hwmon: (ltc4245) expose all GPIO pins as Ira W. Snyder
` (8 preceding siblings ...)
2010-05-26 21:00 ` Jean Delvare
@ 2010-05-26 21:10 ` Grant Likely
9 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2010-05-26 21:10 UTC (permalink / raw)
To: lm-sensors
On Wed, May 26, 2010 at 2:56 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> On Wed, May 26, 2010 at 02:37:28PM -0600, Grant Likely wrote:
>> On Wed, May 26, 2010 at 1:39 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
>> > On Wed, May 26, 2010 at 08:42:22PM +0200, Jean Delvare wrote:
>> >> On Wed, 26 May 2010 10:22:44 -0700, Ira W. Snyder wrote:
>> >> > On Wed, May 26, 2010 at 06:36:59PM +0200, Jean Delvare wrote:
>> >> > > Sorry for not being clear. I wasn't objecting to the stub's existence,
>> >> > > but to the fact that it was doing something. I expected it to do
>> >> > > nothing at all, and ltc4245_show_voltage() would be used for in9.
>> >> >
>> >> > Oh, ok. The ltc4245_show_voltage() function reads the voltage register
>> >> > values directly from data->vregs[]. When the extra GPIO inputs are
>> >> > enabled, I have two choices:
>> >> >
>> >> > 1) store them in the new data->gpios[] array
>> >> > 2) store them in data->vregs[]
>> >> >
>> >> > For #1, ltc4245_show_voltage() doesn't work anymore, since it reads
>> >> > voltage register values from data->vregs[].
>> >> >
>> >> > For #2, I would have to re-expand data->vregs[] to include all of the
>> >> > GPIO inputs, like it was before. Also, I would need to make
>> >> > ltc4245_get_voltage() handle the -EAGAIN error code.
>> >> >
>> >> > I think the code is easier to understand if all the GPIOs are treated in
>> >> > the same way, and not completely special for GPIOADC1 vs GPIOADC[23].
>> >> > That's why I chose #1.
>> >> >
>> >> > If I do a hybrid approach (store GPIOADC1 in data->vregs[] and
>> >> > GPIOADC[23] in data->gpios[]), then I have to make ltc4245_get_voltage()
>> >> > robust against errors too. Is this what you're suggesting?
>> >>
>> >> No. But let's wait and see if you move to OF platform data, the whole
>> >> question will be moot. If you keep the config option approach, I'll
>> >> propose an iterative patch and we can discuss it then.
>> >>
>> >> > (...)
>> >> > Any thoughts about the kernel config option vs. platform data, and how
>> >> > it relates to the OF bindings?
>> >>
>> >> I would prefer platform-data-based. But I fear I don't know anything
>> >> about OF bindings. Better ask the embedded folks (e.g. Grant Likely),
>> >> they will know.
>> >>
>> >
>> > Grant, you're listed in MAINTAINERS as the OF bindings maintainer. Is it
>> > currently possible for the i2c OF binding to pass platform_data to hwmon
>> > drivers that are probed via the device tree? If so, can you point me to
>> > an example. If not, can you suggest what I should do?
>> >
>> > Here is a short example from my device tree (a lightly modified 834x_mds
>> > device tree):
>> >
>> > i2c@3100 {
>> > á á á á#address-cells = <1>;
>> > á á á á#size-cells = <0>;
>> > á á á ácell-index = <1>;
>> > á á á ácompatible = "fsl-i2c";
>> > á á á áreg = <0x3100 0x100>;
>> > á á á áinterrupts = <15 0x8>;
>> > á á á áinterrupt-parent = <&ipic>;
>> > á á á áclock-frequency = <400000>;
>> >
>> > á á á áltc4245@XX {
>> > á á á á á á á ácompatible = "LLTC,ltc4245";
>> > á á á á á á á áreg = <0>; á á á// from bootloader
>> > á á á á};
>> > };
>> >
>> > I'd like to be able to send platform data to the drivers/hwmon/ltc4245.c
>> > driver. On my platform, this is probed automatically using the device
>> > tree snippet above.
>>
>> What is in the platform data?
>
> A bool/int should be sufficient.
>
>> As of the .35 merge window, the device
>> node pointer lives in struct device, so it is available to all Linux
>> device drivers if CONFIG_OF is set. If it is simply data that needs
>> to be passed, then the driver itself can extract it from the tree in
>> the absence of pdata. If you need to pass function pointers or the
>> like, then probably the best thing to do is to register a bus notifier
>> in the platform code before registering devices. That way the
>> platform code can intercept the device and register pdata before the
>> device is bound to a driver.
>
> Ok, so I should wrap the OF detection code in CONFIG_OF in my driver,
> right? I just quickly browsed through the code, and it seems like it
> should be straightforward to use pdata and then fall back on OF data.
Correct. The goal is to allow the driver-specific OF adapter code to
live with the driver, while still having as little as possible impact
on driver design as a whole.
g.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread