[lm-sensors] local macro

[lm-sensors] local macro

[Paul Thomas]

I'm planning a rev of the ads7871 driver. I want to add in the ability
to set the gain on the chip as well as do differential reads. To do
this I'm adding a bunch of SENSOR_DEVICE_ATTR lines. Awhile ago I had
got a suggestion from Jonathan to use a local macro for these
statements. Would that be a macro that looped through calling the
other macro 8 times with values 0-7? Could someone point me to an
example where this is used?

thanks,
Paul

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

Re: [lm-sensors] local macro

[Paul Thomas]

On Wed, Oct 6, 2010 at 11:24 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 10/06/10 19:13, Paul Thomas wrote:
>> I'm planning a rev of the ads7871 driver. I want to add in the ability
>> to set the gain on the chip as well as do differential reads. To do
>> this I'm adding a bunch of SENSOR_DEVICE_ATTR lines. Awhile ago I had
>> got a suggestion from Jonathan to use a local macro for these
>> statements. Would that be a macro that looped through calling the
>> other macro 8 times with values 0-7? Could someone point me to an
>> example where this is used?
> I suspect I meant something like
>
> #define ADS7871_VOLTAGE_ATTR(n) SENSOR_DEVICE_ATTR(in##n##_input, S_IRUGO, show_voltage, NULL, n);
> But looking back at the usage, it really is a matter of taste, and I'm
> not sure my suggestion was a good one in this case!
>
> Jonathan
>

OK, it's not much to just put in all the lines. So I'm adding
attributes inX_gain and inX_diff for each of the 8 channels. The diff
option sets whether it's a differential read or a single ended read to
ground.

thanks,
Paul

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

Re: [lm-sensors] local macro

[Jonathan Cameron]

On 10/06/10 19:13, Paul Thomas wrote:
> I'm planning a rev of the ads7871 driver. I want to add in the ability
> to set the gain on the chip as well as do differential reads. To do
> this I'm adding a bunch of SENSOR_DEVICE_ATTR lines. Awhile ago I had
> got a suggestion from Jonathan to use a local macro for these
> statements. Would that be a macro that looped through calling the
> other macro 8 times with values 0-7? Could someone point me to an
> example where this is used?
I suspect I meant something like

#define ADS7871_VOLTAGE_ATTR(n) SENSOR_DEVICE_ATTR(in##n##_input, S_IRUGO, show_voltage, NULL, n);
But looking back at the usage, it really is a matter of taste, and I'm
not sure my suggestion was a good one in this case!

Jonathan

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

Re: [lm-sensors] local macro

[Guenter Roeck]

On Wed, 2010-10-06 at 14:23 -0400, Paul Thomas wrote:
> On Wed, Oct 6, 2010 at 11:24 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> > On 10/06/10 19:13, Paul Thomas wrote:
> >> I'm planning a rev of the ads7871 driver. I want to add in the ability
> >> to set the gain on the chip as well as do differential reads. To do
> >> this I'm adding a bunch of SENSOR_DEVICE_ATTR lines. Awhile ago I had
> >> got a suggestion from Jonathan to use a local macro for these
> >> statements. Would that be a macro that looped through calling the
> >> other macro 8 times with values 0-7? Could someone point me to an
> >> example where this is used?
> > I suspect I meant something like
> >
> > #define ADS7871_VOLTAGE_ATTR(n) SENSOR_DEVICE_ATTR(in##n##_input, S_IRUGO, show_voltage, NULL, n);
> > But looking back at the usage, it really is a matter of taste, and I'm
> > not sure my suggestion was a good one in this case!
> >
> > Jonathan
> >
> 
> OK, it's not much to just put in all the lines. So I'm adding
> attributes inX_gain and inX_diff for each of the 8 channels. The diff
> option sets whether it's a differential read or a single ended read to
> ground.
> 
Since you plan to amend the ABI, it might make sense to discuss the
proposed attributes and their usage first.

I would assume that hardware monitoring sensors have a static gain, ie a
gain which does not have to be set during runtime, but is fixed for a
specific board. I don't know about the diff attribute, but it also seems
to be a board option, not a runtime option.

For such parameters, it might make more sense to define a common hwmon
platform parameter structure and expect it to be set during board
initialization. Works quite nicely for other drivers.

Thanks,
Guenter



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

Re: [lm-sensors] local macro

[Paul Thomas]

On Wed, Oct 6, 2010 at 12:31 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Wed, 2010-10-06 at 14:23 -0400, Paul Thomas wrote:
>> On Wed, Oct 6, 2010 at 11:24 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>> > On 10/06/10 19:13, Paul Thomas wrote:
>> >> I'm planning a rev of the ads7871 driver. I want to add in the ability
>> >> to set the gain on the chip as well as do differential reads. To do
>> >> this I'm adding a bunch of SENSOR_DEVICE_ATTR lines. Awhile ago I had
>> >> got a suggestion from Jonathan to use a local macro for these
>> >> statements. Would that be a macro that looped through calling the
>> >> other macro 8 times with values 0-7? Could someone point me to an
>> >> example where this is used?
>> > I suspect I meant something like
>> >
>> > #define ADS7871_VOLTAGE_ATTR(n) SENSOR_DEVICE_ATTR(in##n##_input, S_IRUGO, show_voltage, NULL, n);
>> > But looking back at the usage, it really is a matter of taste, and I'm
>> > not sure my suggestion was a good one in this case!
>> >
>> > Jonathan
>> >
>>
>> OK, it's not much to just put in all the lines. So I'm adding
>> attributes inX_gain and inX_diff for each of the 8 channels. The diff
>> option sets whether it's a differential read or a single ended read to
>> ground.
>>
> Since you plan to amend the ABI, it might make sense to discuss the
> proposed attributes and their usage first.
>
> I would assume that hardware monitoring sensors have a static gain, ie a
> gain which does not have to be set during runtime, but is fixed for a
> specific board. I don't know about the diff attribute, but it also seems
> to be a board option, not a runtime option.
>
> For such parameters, it might make more sense to define a common hwmon
> platform parameter structure and expect it to be set during board
> initialization. Works quite nicely for other drivers.
>
> Thanks,
> Guenter

No, both of those are runtime options internal to the ads7871, and
because we initiate a read by setting that register anyway there isn't
any additional setup cost (at least there isn't any spi cost, there is
a little additional code in show_voltage).

Additionally gain & diff are initialized to a gain of 1 and single
ended reads which will match how the driver works now.

I'm open to other suggestions also.

thanks,
Paul

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

Re: [lm-sensors] local macro

[Paul Thomas]

> No, both of those are runtime options internal to the ads7871, and
> because we initiate a read by setting that register anyway there isn't
> any additional setup cost (at least there isn't any spi cost, there is
> a little additional code in show_voltage).
>
> Additionally gain & diff are initialized to a gain of 1 and single
> ended reads which will match how the driver works now.
>
> I'm open to other suggestions also.
>
> thanks,
> Paul
>

I had one other question. The chip gives you a valid conversion result
bit. Currently we're not checking it, if It's not a valid conversion,
then what should be returned?

thanks,
Paul

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

Re: [lm-sensors] local macro

[Guenter Roeck]

On Wed, 2010-10-06 at 15:54 -0400, Paul Thomas wrote:
> > No, both of those are runtime options internal to the ads7871, and
> > because we initiate a read by setting that register anyway there isn't
> > any additional setup cost (at least there isn't any spi cost, there is
> > a little additional code in show_voltage).
> >
> > Additionally gain & diff are initialized to a gain of 1 and single
> > ended reads which will match how the driver works now.
> >
> > I'm open to other suggestions also.
> >
> > thanks,
> > Paul
> >
> 
> I had one other question. The chip gives you a valid conversion result
> bit. Currently we're not checking it, if It's not a valid conversion,
> then what should be returned?

Possibly EIO ?

Guenter




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

Re: [lm-sensors] local macro

[Guenter Roeck]

On Wed, 2010-10-06 at 15:54 -0400, Paul Thomas wrote:
> > No, both of those are runtime options internal to the ads7871, and
> > because we initiate a read by setting that register anyway there isn't
> > any additional setup cost (at least there isn't any spi cost, there is
> > a little additional code in show_voltage).
> >
This isn't a matter of cost, but a matter of maintaining a consistent
ABI.

A brief look into the datasheet shows that the chip is a generic adc. So
I assume its use is not limited to voltage monitoring, and that you
would in fact expect to change the gain during runtime (ie not only at
system startup). Is this a correct assumption ?

Thanks,
Guenter



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

Re: [lm-sensors] local macro

[Jean Delvare]

On Wed, 6 Oct 2010 13:47:59 -0700, Guenter Roeck wrote:
> On Wed, 2010-10-06 at 15:54 -0400, Paul Thomas wrote:
> > > No, both of those are runtime options internal to the ads7871, and
> > > because we initiate a read by setting that register anyway there isn't
> > > any additional setup cost (at least there isn't any spi cost, there is
> > > a little additional code in show_voltage).
> > >
> > > Additionally gain & diff are initialized to a gain of 1 and single
> > > ended reads which will match how the driver works now.
> > >
> > > I'm open to other suggestions also.
> > >
> > > thanks,
> > > Paul
> > >
> > 
> > I had one other question. The chip gives you a valid conversion result
> > bit. Currently we're not checking it, if It's not a valid conversion,
> > then what should be returned?
> 
> Possibly EIO ?

Or EAGAIN if one can hope that the next conversion will be OK.


-- 
Jean Delvare

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

Re: [lm-sensors] local macro

[Jean Delvare]

On Wed, 6 Oct 2010 14:02:26 -0700, Guenter Roeck wrote:
> On Wed, 2010-10-06 at 15:54 -0400, Paul Thomas wrote:
> > > No, both of those are runtime options internal to the ads7871, and
> > > because we initiate a read by setting that register anyway there isn't
> > > any additional setup cost (at least there isn't any spi cost, there is
> > > a little additional code in show_voltage).
> > >
> This isn't a matter of cost, but a matter of maintaining a consistent
> ABI.
> 
> A brief look into the datasheet shows that the chip is a generic adc. So
> I assume its use is not limited to voltage monitoring, and that you
> would in fact expect to change the gain during runtime (ie not only at
> system startup). Is this a correct assumption ?

This is the problem with having ADC drivers in drivers/hwmon. These are
versatile devices, they can be used for voltage monitoring but also for
a variety of other things, and the desired interface depends on that.
Runtime gain changes is not desirable at all for a voltage monitoring
device.

A solution to this may be to move the ADC driver itself somewhere else,
and have a dummy driver in hwmon/ merely adding a hwmon-style interface
on top of the main driver. Other such dummy driver could be written for
other use cases of the ADC. I think this is what the s3c-hwmon driver
is doing, and maybe others.

-- 
Jean Delvare

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

Re: [lm-sensors] local macro

[Guenter Roeck]

On Wed, 2010-10-06 at 17:32 -0400, Jean Delvare wrote:
> On Wed, 6 Oct 2010 14:02:26 -0700, Guenter Roeck wrote:
> > On Wed, 2010-10-06 at 15:54 -0400, Paul Thomas wrote:
> > > > No, both of those are runtime options internal to the ads7871, and
> > > > because we initiate a read by setting that register anyway there isn't
> > > > any additional setup cost (at least there isn't any spi cost, there is
> > > > a little additional code in show_voltage).
> > > >
> > This isn't a matter of cost, but a matter of maintaining a consistent
> > ABI.
> > 
> > A brief look into the datasheet shows that the chip is a generic adc. So
> > I assume its use is not limited to voltage monitoring, and that you
> > would in fact expect to change the gain during runtime (ie not only at
> > system startup). Is this a correct assumption ?
> 
> This is the problem with having ADC drivers in drivers/hwmon. These are
> versatile devices, they can be used for voltage monitoring but also for
> a variety of other things, and the desired interface depends on that.
> Runtime gain changes is not desirable at all for a voltage monitoring
> device.
> 
> A solution to this may be to move the ADC driver itself somewhere else,
> and have a dummy driver in hwmon/ merely adding a hwmon-style interface
> on top of the main driver. Other such dummy driver could be written for
> other use cases of the ADC. I think this is what the s3c-hwmon driver
> is doing, and maybe others.
> 
Agreed.

Guenter



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

Re: [lm-sensors] local macro

[Paul Thomas]

> This is the problem with having ADC drivers in drivers/hwmon. These are
> versatile devices, they can be used for voltage monitoring but also for
> a variety of other things, and the desired interface depends on that.
> Runtime gain changes is not desirable at all for a voltage monitoring
> device.
>
> A solution to this may be to move the ADC driver itself somewhere else,
> and have a dummy driver in hwmon/ merely adding a hwmon-style interface
> on top of the main driver. Other such dummy driver could be written for
> other use cases of the ADC. I think this is what the s3c-hwmon driver
> is doing, and maybe others.
>
> --
> Jean Delvare
>

I think there is slight misunderstanding about where the gain is, and
how I plan on handling it. inX_input always returns in units of
volts*10,000. If you set the gain to 20 the only difference is that if
you try and read a voltage over 0.125 then it will be out of range.

I understand there are hwmon devices that are dependent on external
amplifiers & dividers, but that is not the case here.

thanks,
Paul

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

Re: [lm-sensors] local macro

[Jean Delvare]

Hi Paul,

On Wed, 6 Oct 2010 15:04:36 -0700, Paul Thomas wrote:
> > This is the problem with having ADC drivers in drivers/hwmon. These are
> > versatile devices, they can be used for voltage monitoring but also for
> > a variety of other things, and the desired interface depends on that.
> > Runtime gain changes is not desirable at all for a voltage monitoring
> > device.
> >
> > A solution to this may be to move the ADC driver itself somewhere else,
> > and have a dummy driver in hwmon/ merely adding a hwmon-style interface
> > on top of the main driver. Other such dummy driver could be written for
> > other use cases of the ADC. I think this is what the s3c-hwmon driver
> > is doing, and maybe others.
> 
> I think there is slight misunderstanding about where the gain is, and
> how I plan on handling it. inX_input always returns in units of
> volts*10,000.

I'm not sure what you mean with "units of volts*10,000", I seriously
doubt that the ADC's LSB has a weight of 10,000 Volts. Did you mean a
LSB of 0.0001 Volt? If this is the case then the driver is broken and
has to be fixed, as the sysfs ABI requires that inX files expose values
in units of 0.001 Volt.

> If you set the gain to 20 the only difference is that if
> you try and read a voltage over 0.125 then it will be out of range.
> 
> I understand there are hwmon devices that are dependent on external
> amplifiers & dividers, but that is not the case here.

The point isn't whether the gain is internal or external. The point is
whether the gain should change at run-time or not. When doing voltage
monitoring, you expect a result in a specific, short range, so the gain
should be set for the desired range and never changed afterward. There
are a lot of hardware monitoring devices doing exactly that, internally.

The bottom line is that if your needs are different then your driver
doesn't belong to drivers/hwmon.

-- 
Jean Delvare

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

Re: [lm-sensors] local macro

[Jonathan Cameron]

On 10/07/10 07:49, Jean Delvare wrote:
> Hi Paul,
> 
> On Wed, 6 Oct 2010 15:04:36 -0700, Paul Thomas wrote:
>>> This is the problem with having ADC drivers in drivers/hwmon. These are
>>> versatile devices, they can be used for voltage monitoring but also for
>>> a variety of other things, and the desired interface depends on that.
>>> Runtime gain changes is not desirable at all for a voltage monitoring
>>> device.
>>>
>>> A solution to this may be to move the ADC driver itself somewhere else,
>>> and have a dummy driver in hwmon/ merely adding a hwmon-style interface
>>> on top of the main driver. Other such dummy driver could be written for
>>> other use cases of the ADC. I think this is what the s3c-hwmon driver
>>> is doing, and maybe others.
>>
>> I think there is slight misunderstanding about where the gain is, and
>> how I plan on handling it. inX_input always returns in units of
>> volts*10,000.
> 
> I'm not sure what you mean with "units of volts*10,000", I seriously
> doubt that the ADC's LSB has a weight of 10,000 Volts. Did you mean a
> LSB of 0.0001 Volt? If this is the case then the driver is broken and
> has to be fixed, as the sysfs ABI requires that inX files expose values
> in units of 0.001 Volt.
> 
>> If you set the gain to 20 the only difference is that if
>> you try and read a voltage over 0.125 then it will be out of range.
>>
>> I understand there are hwmon devices that are dependent on external
>> amplifiers & dividers, but that is not the case here.
> 
> The point isn't whether the gain is internal or external. The point is
> whether the gain should change at run-time or not. When doing voltage
> monitoring, you expect a result in a specific, short range, so the gain
> should be set for the desired range and never changed afterward. There
> are a lot of hardware monitoring devices doing exactly that, internally.
> 
> The bottom line is that if your needs are different then your driver
> doesn't belong to drivers/hwmon.
Agree with Jean entirely on this point.

For what it is worth, we do support all these features in IIO (always like to
get a plug in :) )
In our case, the differential channels are separate files called
in0-in1_input etc (where the numbers match the relevant physical pins
as per the in0_input single ended reads and the gain you are talking
about is in_calibscale (and in-in_calibscale if the differential scale
is different).

Hwmon is for monitoring voltages on boards to check health of devices etc.
(and other stuff, but this covers ADCs)
IIO is for more general sensor handling.  Numerous other arch specific
cases exist, often using an an underlying multi function device driver
to handle the different elements. In IIO we have very deliberately matched
hwmon's interfaces wherever the exist and generalize well (few disagreements
on alarm naming, though we are avoiding any naming clashes).
Disadvantage of IIO is that, due to bits that have nothing to do with the
simple sysfs interfaces, it is not yet quite ready for the bigtime.

Jonathan

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

Re: [lm-sensors] local macro

[Paul Thomas]

> I'm not sure what you mean with "units of volts*10,000", I seriously
> doubt that the ADC's LSB has a weight of 10,000 Volts. Did you mean a
> LSB of 0.0001 Volt? If this is the case then the driver is broken and
> has to be fixed, as the sysfs ABI requires that inX files expose values
> in units of 0.001 Volt.
>
>> If you set the gain to 20 the only difference is that if
>> you try and read a voltage over 0.125 then it will be out of range.
>>
>> I understand there are hwmon devices that are dependent on external
>> amplifiers & dividers, but that is not the case here.
>
> The point isn't whether the gain is internal or external. The point is
> whether the gain should change at run-time or not. When doing voltage
> monitoring, you expect a result in a specific, short range, so the gain
> should be set for the desired range and never changed afterward. There
> are a lot of hardware monitoring devices doing exactly that, internally.
>
> The bottom line is that if your needs are different then your driver
> doesn't belong to drivers/hwmon.
>
> --
> Jean Delvare
>

OK, I'll switch to iio. So, yes it looks like the current calculation
is off by a factor of 10. I did this because of the resolution on the
a/d. Also there is an error where if the voltage is below 0 it
calculates a large number (it's not really meant to read voltages
below 0 and there is a protection diode if you drop much below 0).
This is fixed by directly assigning the ads7871_read_reg16(spi,
REG_LS_BYTE) value to a int16_t before doing the other math.

If anyone wants the adjustable gain & differential version they can
contact me directly. I'm not sure when I'll get to the iio version.

thanks,
Paul

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