Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-15 Thread Jean Delvare
Hi Steve,

On Mon, 14 Jan 2008 22:28:51 +, Steve Hardy wrote:
> > As far as I know boolean parameters take values 0 and 1, not N and Y.
> 
> Not on the systems I am testing on (2.6.24-rc6) - the sysfs interface
> displays boolean parameters as Y/N, and will accept boot parameters as
> either Y/N or 0/1.  Below is a snippet from a test proving this point :
> 
> [EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/se_input
> Y
> [EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
> Y
> 
> after appending the ads7828.int_vref=N parameter to the kernel
> command-line :
> 
> Linux 2.6.24-rc6 (shtest) (16:53 on Saturday, 05 January 2002)
> 
> login: test
> [EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
> N

Oh. This is news to me, thanks for the information.

> Is it more "correct" to refer to 1/0 in the documentation?
> I just naturally used the same syntax provided by the sysfs nodes

Nah, just scratch this comment of mine, I just didn't know that Y/N was
working for booleans. If you prefer that to document that form that's
alright with me.

(I do believe that it was a mistake to allow two different ways to do
the same thing, but that's a different issue.)

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-15 Thread Jean Delvare
Hi Steve,

On Mon, 14 Jan 2008 22:28:51 +, Steve Hardy wrote:
  As far as I know boolean parameters take values 0 and 1, not N and Y.
 
 Not on the systems I am testing on (2.6.24-rc6) - the sysfs interface
 displays boolean parameters as Y/N, and will accept boot parameters as
 either Y/N or 0/1.  Below is a snippet from a test proving this point :
 
 [EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/se_input
 Y
 [EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
 Y
 
 after appending the ads7828.int_vref=N parameter to the kernel
 command-line :
 
 Linux 2.6.24-rc6 (shtest) (16:53 on Saturday, 05 January 2002)
 
 login: test
 [EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
 N

Oh. This is news to me, thanks for the information.

 Is it more correct to refer to 1/0 in the documentation?
 I just naturally used the same syntax provided by the sysfs nodes

Nah, just scratch this comment of mine, I just didn't know that Y/N was
working for booleans. If you prefer that to document that form that's
alright with me.

(I do believe that it was a mistake to allow two different ways to do
the same thing, but that's a different issue.)

-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-14 Thread Steve Hardy

> As far as I know boolean parameters take values 0 and 1, not N and Y.

Not on the systems I am testing on (2.6.24-rc6) - the sysfs interface
displays boolean parameters as Y/N, and will accept boot parameters as
either Y/N or 0/1.  Below is a snippet from a test proving this point :

[EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/se_input
Y
[EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
Y

after appending the ads7828.int_vref=N parameter to the kernel
command-line :

Linux 2.6.24-rc6 (shtest) (16:53 on Saturday, 05 January 2002)

login: test
[EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
N

Is it more "correct" to refer to 1/0 in the documentation?
I just naturally used the same syntax provided by the sysfs nodes

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-14 Thread Steve Hardy

 As far as I know boolean parameters take values 0 and 1, not N and Y.

Not on the systems I am testing on (2.6.24-rc6) - the sysfs interface
displays boolean parameters as Y/N, and will accept boot parameters as
either Y/N or 0/1.  Below is a snippet from a test proving this point :

[EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/se_input
Y
[EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
Y

after appending the ads7828.int_vref=N parameter to the kernel
command-line :

Linux 2.6.24-rc6 (shtest) (16:53 on Saturday, 05 January 2002)

login: test
[EMAIL PROTECTED]:~$ cat /sys/module/ads7828/parameters/int_vref
N

Is it more correct to refer to 1/0 in the documentation?
I just naturally used the same syntax provided by the sysfs nodes

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-10 Thread Jean Delvare
Hi Steve,

On Fri, 4 Jan 2008 07:34:39 +, Steve Hardy wrote:
> Jean - you mentioned declaring the devices as platform data & writing
> a new-style I2C driver - are there any examples of this approach I can
> refer to?  I simply copied what is/was currently in-tree for this
> driver, which seems to work well with the hardware I'm working with (a
> COTS CPCI carrier)

In Linus' tree you can find the following new-style drivers:
drivers/rtc/rtc-ds1307.c
drivers/rtc/rtc-ds1374.c
drivers/rtc/rtc-m41t80.c
drivers/rtc/rtc-rs5c372.c

You may also take a look at one that was submitted for merge:
http://marc.info/?l=linux-kernel=119956436514432=2

This doesn't have to be done now though, if you prefer to submit your
driver as-is for now and convert it later, that's fine with me.

> Since I've dropped Thunderbird I hope the patch mangling will be
> fixed.

Yes formatting looks OK this time.

Overall your driver looks good, I only have minor remarks:

> diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
> linux-2.6.24-rc6/Documentation/hwmon/ads7828 
> linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads7828
> --- linux-2.6.24-rc6/Documentation/hwmon/ads7828  1970-01-01 
> 01:00:00.0 +0100
> +++ linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads7828  2008-01-02 
> 16:30:39.0 +
> @@ -0,0 +1,38 @@
> +Kernel driver ads7828
> +=
> +
> +Supported chips:
> +  * Texas Instruments/Burr-Brown ADS7828
> +Prefix: 'ads7828'
> +Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
> +Datasheet: Publicly available at the Texas Instruments website :

No space before colon.

> +   http://focus.ti.com/lit/ds/symlink/ads7828.pdf
> +
> +Authors:
> +Steve Hardy <[EMAIL PROTECTED]>
> +
> +Module Parameters
> +-
> +
> +* se_input: bool (default Y)
> +  Single ended operation - set to N for differential mode
> +* int_vref: bool (default Y)
> +  Operate with the internal 2.5v reference - set to N for external reference

2.5V

As far as I know boolean parameters take values 0 and 1, not N and Y.

> +* vref_mv: int (default 2500)
> +  If using an external reference, set this to the reference voltage in mv

Correct unit spelling is "mV".

> +
> +Description
> +---
> +
> +This driver implements support for the Texas Instruments ADS7828.
> +
> +This device is a 12bit 8ch A-D converter.

12-bit 8-channel

> +
> +It can operate in single ended mode (8 +ve inputs) or in differential mode,
> +when 4 differential pairs can be measured.

s/when/where/?

If there are only 4 measurement channels in differential mode, then
ideally your driver should only create 4 sysfs files in this mode, and
it should only read 4 values from the chip rather than 8 in the update
function. I2C transactions aren't cheap.

> +
> +The chip also has the facility to use an external voltage reference.  This
> +may be required if your hardware supplies the ADS7828 from a 5v supply, see

5V

> +the datasheet for more details.
> +
> +

You can drop these extra blank lines.

> diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
> linux-2.6.24-rc6/drivers/hwmon/Kconfig 
> linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig
> --- linux-2.6.24-rc6/drivers/hwmon/Kconfig2008-01-02 16:37:43.0 
> +
> +++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig2008-01-02 
> 16:49:42.0 +
> @@ -588,6 +588,16 @@ config SENSORS_SMSC47B397
> This driver can also be built as a module.  If so, the module
> will be called smsc47b397.
>  
> +config SENSORS_ADS7828
> + tristate "Texas Instruments ADS7828"
> + depends on I2C
> + help
> + If you say yes here you get support for Texas Instruments ADS7828
> + 12-bit 8-channel ADC device.
> +
> + This driver can also be built as a module.  If so, the module
> + will be called ads7828.

The help block is traditionally indented with two extra spaces.

> +
>  config SENSORS_THMC50
>   tristate "Texas Instruments THMC50 / Analog Devices ADM1022"
>   depends on I2C && EXPERIMENTAL
> diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
> linux-2.6.24-rc6/drivers/hwmon/Makefile 
> linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile
> --- linux-2.6.24-rc6/drivers/hwmon/Makefile   2008-01-02 16:37:43.0 
> +
> +++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile   2008-01-02 
> 16:35:27.0 +
> @@ -22,6 +22,7 @@ obj-$(CONFIG_SENSORS_ADM1026)   += adm1026
>  obj-$(CONFIG_SENSORS_ADM1029)+= adm1029.o
>  obj-$(CONFIG_SENSORS_ADM1031)+= adm1031.o
>  obj-$(CONFIG_SENSORS_ADM9240)+= adm9240.o
> +obj-$(CONFIG_SENSORS_ADS7828)+= ads7828.o
>  obj-$(CONFIG_SENSORS_ADT7470)+= adt7470.o
>  obj-$(CONFIG_SENSORS_APPLESMC)   += applesmc.o
>  obj-$(CONFIG_SENSORS_AMS)+= ams/
> diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
> linux-2.6.24-rc6/drivers/hwmon/ads7828.c 
> linux-2.6.24-rc6-ads7828/drivers/hwmon/ads7828.c
> --- 

Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-10 Thread Jean Delvare
Hi Steve,

On Fri, 4 Jan 2008 07:34:39 +, Steve Hardy wrote:
 Jean - you mentioned declaring the devices as platform data  writing
 a new-style I2C driver - are there any examples of this approach I can
 refer to?  I simply copied what is/was currently in-tree for this
 driver, which seems to work well with the hardware I'm working with (a
 COTS CPCI carrier)

In Linus' tree you can find the following new-style drivers:
drivers/rtc/rtc-ds1307.c
drivers/rtc/rtc-ds1374.c
drivers/rtc/rtc-m41t80.c
drivers/rtc/rtc-rs5c372.c

You may also take a look at one that was submitted for merge:
http://marc.info/?l=linux-kernelm=119956436514432w=2

This doesn't have to be done now though, if you prefer to submit your
driver as-is for now and convert it later, that's fine with me.

 Since I've dropped Thunderbird I hope the patch mangling will be
 fixed.

Yes formatting looks OK this time.

Overall your driver looks good, I only have minor remarks:

 diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
 linux-2.6.24-rc6/Documentation/hwmon/ads7828 
 linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads7828
 --- linux-2.6.24-rc6/Documentation/hwmon/ads7828  1970-01-01 
 01:00:00.0 +0100
 +++ linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads7828  2008-01-02 
 16:30:39.0 +
 @@ -0,0 +1,38 @@
 +Kernel driver ads7828
 +=
 +
 +Supported chips:
 +  * Texas Instruments/Burr-Brown ADS7828
 +Prefix: 'ads7828'
 +Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
 +Datasheet: Publicly available at the Texas Instruments website :

No space before colon.

 +   http://focus.ti.com/lit/ds/symlink/ads7828.pdf
 +
 +Authors:
 +Steve Hardy [EMAIL PROTECTED]
 +
 +Module Parameters
 +-
 +
 +* se_input: bool (default Y)
 +  Single ended operation - set to N for differential mode
 +* int_vref: bool (default Y)
 +  Operate with the internal 2.5v reference - set to N for external reference

2.5V

As far as I know boolean parameters take values 0 and 1, not N and Y.

 +* vref_mv: int (default 2500)
 +  If using an external reference, set this to the reference voltage in mv

Correct unit spelling is mV.

 +
 +Description
 +---
 +
 +This driver implements support for the Texas Instruments ADS7828.
 +
 +This device is a 12bit 8ch A-D converter.

12-bit 8-channel

 +
 +It can operate in single ended mode (8 +ve inputs) or in differential mode,
 +when 4 differential pairs can be measured.

s/when/where/?

If there are only 4 measurement channels in differential mode, then
ideally your driver should only create 4 sysfs files in this mode, and
it should only read 4 values from the chip rather than 8 in the update
function. I2C transactions aren't cheap.

 +
 +The chip also has the facility to use an external voltage reference.  This
 +may be required if your hardware supplies the ADS7828 from a 5v supply, see

5V

 +the datasheet for more details.
 +
 +

You can drop these extra blank lines.

 diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
 linux-2.6.24-rc6/drivers/hwmon/Kconfig 
 linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig
 --- linux-2.6.24-rc6/drivers/hwmon/Kconfig2008-01-02 16:37:43.0 
 +
 +++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig2008-01-02 
 16:49:42.0 +
 @@ -588,6 +588,16 @@ config SENSORS_SMSC47B397
 This driver can also be built as a module.  If so, the module
 will be called smsc47b397.
  
 +config SENSORS_ADS7828
 + tristate Texas Instruments ADS7828
 + depends on I2C
 + help
 + If you say yes here you get support for Texas Instruments ADS7828
 + 12-bit 8-channel ADC device.
 +
 + This driver can also be built as a module.  If so, the module
 + will be called ads7828.

The help block is traditionally indented with two extra spaces.

 +
  config SENSORS_THMC50
   tristate Texas Instruments THMC50 / Analog Devices ADM1022
   depends on I2C  EXPERIMENTAL
 diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
 linux-2.6.24-rc6/drivers/hwmon/Makefile 
 linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile
 --- linux-2.6.24-rc6/drivers/hwmon/Makefile   2008-01-02 16:37:43.0 
 +
 +++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile   2008-01-02 
 16:35:27.0 +
 @@ -22,6 +22,7 @@ obj-$(CONFIG_SENSORS_ADM1026)   += adm1026
  obj-$(CONFIG_SENSORS_ADM1029)+= adm1029.o
  obj-$(CONFIG_SENSORS_ADM1031)+= adm1031.o
  obj-$(CONFIG_SENSORS_ADM9240)+= adm9240.o
 +obj-$(CONFIG_SENSORS_ADS7828)+= ads7828.o
  obj-$(CONFIG_SENSORS_ADT7470)+= adt7470.o
  obj-$(CONFIG_SENSORS_APPLESMC)   += applesmc.o
  obj-$(CONFIG_SENSORS_AMS)+= ams/
 diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
 linux-2.6.24-rc6/drivers/hwmon/ads7828.c 
 linux-2.6.24-rc6-ads7828/drivers/hwmon/ads7828.c
 --- linux-2.6.24-rc6/drivers/hwmon/ads7828.c  1970-01-01 01:00:00.0 
 +0100
 +++ 

Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-09 Thread Andrew Morton
On Fri, 4 Jan 2008 07:34:39 +
Steve Hardy <[EMAIL PROTECTED]> wrote:

> Andrew/Jean,
> 
> Sorry for the delay - christmas & getting mutt working delayed my
> revised patch.  Here is an updated patch against 2.6.24-rc6, which
> hopefully addresses all comments made so far.
> 
> Jean - you mentioned declaring the devices as platform data & writing
> a new-style I2C driver - are there any examples of this approach I can
> refer to?  I simply copied what is/was currently in-tree for this
> driver, which seems to work well with the hardware I'm working with (a
> COTS CPCI carrier)
> 
> Since I've dropped Thunderbird I hope the patch mangling will be
> fixed.
> 
> Any problem, please let me know.
>

The above doesn't constitute a usable changelog.  I guess we don't really
need anything in this patch but please do remember that the changelog text
should be included in each new rev of a patch.

> 
> Signed-Off by Steve Hardy <[EMAIL PROTECTED]>

"Signed-off-by:", please.

 Documentation/hwmon/ads7828 |   38 
 drivers/hwmon/Kconfig   |   10 +
 drivers/hwmon/Makefile  |1 
 drivers/hwmon/ads7828.c |  296 ++
 4 files changed, 345 insertions(+)

I assumed that this is to be merged via Mark's git-hwmon tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-09 Thread Andrew Morton
On Fri, 4 Jan 2008 07:34:39 +
Steve Hardy [EMAIL PROTECTED] wrote:

 Andrew/Jean,
 
 Sorry for the delay - christmas  getting mutt working delayed my
 revised patch.  Here is an updated patch against 2.6.24-rc6, which
 hopefully addresses all comments made so far.
 
 Jean - you mentioned declaring the devices as platform data  writing
 a new-style I2C driver - are there any examples of this approach I can
 refer to?  I simply copied what is/was currently in-tree for this
 driver, which seems to work well with the hardware I'm working with (a
 COTS CPCI carrier)
 
 Since I've dropped Thunderbird I hope the patch mangling will be
 fixed.
 
 Any problem, please let me know.


The above doesn't constitute a usable changelog.  I guess we don't really
need anything in this patch but please do remember that the changelog text
should be included in each new rev of a patch.

 
 Signed-Off by Steve Hardy [EMAIL PROTECTED]

Signed-off-by:, please.

 Documentation/hwmon/ads7828 |   38 
 drivers/hwmon/Kconfig   |   10 +
 drivers/hwmon/Makefile  |1 
 drivers/hwmon/ads7828.c |  296 ++
 4 files changed, 345 insertions(+)

I assumed that this is to be merged via Mark's git-hwmon tree.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-04 Thread Steve Hardy
Andrew/Jean,

Sorry for the delay - christmas & getting mutt working delayed my
revised patch.  Here is an updated patch against 2.6.24-rc6, which
hopefully addresses all comments made so far.

Jean - you mentioned declaring the devices as platform data & writing
a new-style I2C driver - are there any examples of this approach I can
refer to?  I simply copied what is/was currently in-tree for this
driver, which seems to work well with the hardware I'm working with (a
COTS CPCI carrier)

Since I've dropped Thunderbird I hope the patch mangling will be
fixed.

Any problem, please let me know.

Regards,
Steve

Signed-Off by Steve Hardy <[EMAIL PROTECTED]>

---


diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/Documentation/hwmon/ads7828 
linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads7828
--- linux-2.6.24-rc6/Documentation/hwmon/ads78281970-01-01 
01:00:00.0 +0100
+++ linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads78282008-01-02 
16:30:39.0 +
@@ -0,0 +1,38 @@
+Kernel driver ads7828
+=
+
+Supported chips:
+  * Texas Instruments/Burr-Brown ADS7828
+Prefix: 'ads7828'
+Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
+Datasheet: Publicly available at the Texas Instruments website :
+   http://focus.ti.com/lit/ds/symlink/ads7828.pdf
+
+Authors:
+Steve Hardy <[EMAIL PROTECTED]>
+
+Module Parameters
+-
+
+* se_input: bool (default Y)
+  Single ended operation - set to N for differential mode
+* int_vref: bool (default Y)
+  Operate with the internal 2.5v reference - set to N for external reference
+* vref_mv: int (default 2500)
+  If using an external reference, set this to the reference voltage in mv
+
+Description
+---
+
+This driver implements support for the Texas Instruments ADS7828.
+
+This device is a 12bit 8ch A-D converter.
+
+It can operate in single ended mode (8 +ve inputs) or in differential mode,
+when 4 differential pairs can be measured.
+
+The chip also has the facility to use an external voltage reference.  This
+may be required if your hardware supplies the ADS7828 from a 5v supply, see
+the datasheet for more details.
+
+
diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/drivers/hwmon/Kconfig 
linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig
--- linux-2.6.24-rc6/drivers/hwmon/Kconfig  2008-01-02 16:37:43.0 
+
+++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig  2008-01-02 
16:49:42.0 +
@@ -588,6 +588,16 @@ config SENSORS_SMSC47B397
  This driver can also be built as a module.  If so, the module
  will be called smsc47b397.
 
+config SENSORS_ADS7828
+   tristate "Texas Instruments ADS7828"
+   depends on I2C
+   help
+   If you say yes here you get support for Texas Instruments ADS7828
+   12-bit 8-channel ADC device.
+
+   This driver can also be built as a module.  If so, the module
+   will be called ads7828.
+
 config SENSORS_THMC50
tristate "Texas Instruments THMC50 / Analog Devices ADM1022"
depends on I2C && EXPERIMENTAL
diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/drivers/hwmon/Makefile 
linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile
--- linux-2.6.24-rc6/drivers/hwmon/Makefile 2008-01-02 16:37:43.0 
+
+++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile 2008-01-02 
16:35:27.0 +
@@ -22,6 +22,7 @@ obj-$(CONFIG_SENSORS_ADM1026) += adm1026
 obj-$(CONFIG_SENSORS_ADM1029)  += adm1029.o
 obj-$(CONFIG_SENSORS_ADM1031)  += adm1031.o
 obj-$(CONFIG_SENSORS_ADM9240)  += adm9240.o
+obj-$(CONFIG_SENSORS_ADS7828)  += ads7828.o
 obj-$(CONFIG_SENSORS_ADT7470)  += adt7470.o
 obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
 obj-$(CONFIG_SENSORS_AMS)  += ams/
diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/drivers/hwmon/ads7828.c 
linux-2.6.24-rc6-ads7828/drivers/hwmon/ads7828.c
--- linux-2.6.24-rc6/drivers/hwmon/ads7828.c1970-01-01 01:00:00.0 
+0100
+++ linux-2.6.24-rc6-ads7828/drivers/hwmon/ads7828.c2008-01-02 
16:35:27.0 +
@@ -0,0 +1,296 @@
+/*
+   ads7828.c - lm_sensors driver for ads7828 12bit 8ch ADC
+   (C) 2007 EADS Astrium
+
+   This driver is based on the lm75 and other lm_sensors/hwmon drivers
+
+   Written by Steve Hardy <[EMAIL PROTECTED]>
+
+   Datasheet available at : http://focus.ti.com/lit/ds/symlink/ads7828.pdf
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.See the
+   GNU General Public 

Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2008-01-04 Thread Steve Hardy
Andrew/Jean,

Sorry for the delay - christmas  getting mutt working delayed my
revised patch.  Here is an updated patch against 2.6.24-rc6, which
hopefully addresses all comments made so far.

Jean - you mentioned declaring the devices as platform data  writing
a new-style I2C driver - are there any examples of this approach I can
refer to?  I simply copied what is/was currently in-tree for this
driver, which seems to work well with the hardware I'm working with (a
COTS CPCI carrier)

Since I've dropped Thunderbird I hope the patch mangling will be
fixed.

Any problem, please let me know.

Regards,
Steve

Signed-Off by Steve Hardy [EMAIL PROTECTED]

---


diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/Documentation/hwmon/ads7828 
linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads7828
--- linux-2.6.24-rc6/Documentation/hwmon/ads78281970-01-01 
01:00:00.0 +0100
+++ linux-2.6.24-rc6-ads7828/Documentation/hwmon/ads78282008-01-02 
16:30:39.0 +
@@ -0,0 +1,38 @@
+Kernel driver ads7828
+=
+
+Supported chips:
+  * Texas Instruments/Burr-Brown ADS7828
+Prefix: 'ads7828'
+Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
+Datasheet: Publicly available at the Texas Instruments website :
+   http://focus.ti.com/lit/ds/symlink/ads7828.pdf
+
+Authors:
+Steve Hardy [EMAIL PROTECTED]
+
+Module Parameters
+-
+
+* se_input: bool (default Y)
+  Single ended operation - set to N for differential mode
+* int_vref: bool (default Y)
+  Operate with the internal 2.5v reference - set to N for external reference
+* vref_mv: int (default 2500)
+  If using an external reference, set this to the reference voltage in mv
+
+Description
+---
+
+This driver implements support for the Texas Instruments ADS7828.
+
+This device is a 12bit 8ch A-D converter.
+
+It can operate in single ended mode (8 +ve inputs) or in differential mode,
+when 4 differential pairs can be measured.
+
+The chip also has the facility to use an external voltage reference.  This
+may be required if your hardware supplies the ADS7828 from a 5v supply, see
+the datasheet for more details.
+
+
diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/drivers/hwmon/Kconfig 
linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig
--- linux-2.6.24-rc6/drivers/hwmon/Kconfig  2008-01-02 16:37:43.0 
+
+++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Kconfig  2008-01-02 
16:49:42.0 +
@@ -588,6 +588,16 @@ config SENSORS_SMSC47B397
  This driver can also be built as a module.  If so, the module
  will be called smsc47b397.
 
+config SENSORS_ADS7828
+   tristate Texas Instruments ADS7828
+   depends on I2C
+   help
+   If you say yes here you get support for Texas Instruments ADS7828
+   12-bit 8-channel ADC device.
+
+   This driver can also be built as a module.  If so, the module
+   will be called ads7828.
+
 config SENSORS_THMC50
tristate Texas Instruments THMC50 / Analog Devices ADM1022
depends on I2C  EXPERIMENTAL
diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/drivers/hwmon/Makefile 
linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile
--- linux-2.6.24-rc6/drivers/hwmon/Makefile 2008-01-02 16:37:43.0 
+
+++ linux-2.6.24-rc6-ads7828/drivers/hwmon/Makefile 2008-01-02 
16:35:27.0 +
@@ -22,6 +22,7 @@ obj-$(CONFIG_SENSORS_ADM1026) += adm1026
 obj-$(CONFIG_SENSORS_ADM1029)  += adm1029.o
 obj-$(CONFIG_SENSORS_ADM1031)  += adm1031.o
 obj-$(CONFIG_SENSORS_ADM9240)  += adm9240.o
+obj-$(CONFIG_SENSORS_ADS7828)  += ads7828.o
 obj-$(CONFIG_SENSORS_ADT7470)  += adt7470.o
 obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
 obj-$(CONFIG_SENSORS_AMS)  += ams/
diff -uprN -X linux-2.6.24-rc6/Documentation/dontdiff 
linux-2.6.24-rc6/drivers/hwmon/ads7828.c 
linux-2.6.24-rc6-ads7828/drivers/hwmon/ads7828.c
--- linux-2.6.24-rc6/drivers/hwmon/ads7828.c1970-01-01 01:00:00.0 
+0100
+++ linux-2.6.24-rc6-ads7828/drivers/hwmon/ads7828.c2008-01-02 
16:35:27.0 +
@@ -0,0 +1,296 @@
+/*
+   ads7828.c - lm_sensors driver for ads7828 12bit 8ch ADC
+   (C) 2007 EADS Astrium
+
+   This driver is based on the lm75 and other lm_sensors/hwmon drivers
+
+   Written by Steve Hardy [EMAIL PROTECTED]
+
+   Datasheet available at : http://focus.ti.com/lit/ds/symlink/ads7828.pdf
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.See the
+   GNU General Public License for 

Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-19 Thread Jean Delvare
Hi Steve,

On Mon, 03 Dec 2007 17:59:59 +, Steve Hardy wrote:
> Here is a patch against 2.6.23.9 which adds support for the 
> Burr-Brown/Texas-Instruments
> ADS7828 12-bit 8-channel A-D converter.
> 
> The chip is used for voltage monitoring on the COTS processor card I am
> currently working with.
> 
> The driver simply outputs the current input voltages (in mv as specified
> in the lm-sensors sysfs interface documentation).  Any scaling required for
> a specific board is handled by user-space.  Hopefully this makes the driver
> generic enough to be generally useful.
> 
> The driver is basically a simple rehash of existing code - I used lm75 as
> the starting point, with some inspiration from other existing drivers.
> 
> Please let me know if there are any problems with the patch.

Preliminary note: if the chip is on an embedded platform where you can
declare the devices as platform data, writing a new-style I2C driver
would probably be easier. This would save you the detection step (which
I am not sure is very reliable) and device configuration would also be
much easier.

I have some random comments on top of what Andrew already wrote:

> diff -uprN -X linux-2.6.23.9/Documentation/dontdiff 
> linux-2.6.23.9/drivers/hwmon/Kconfig 
> linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig
> --- linux-2.6.23.9/drivers/hwmon/Kconfig2007-11-26 
> 17:51:43.0 +
> +++ linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig2007-11-28 
> 10:02:02.0 +
> @@ -530,6 +530,16 @@ config SENSORS_THMC50
>This driver can also be built as a module.  If so, the module
>will be called thmc50.
>  
> +config SENSORS_ADS7828
> +tristate "Texas Instruments ADS7828"
> +depends on I2C && EXPERIMENTAL
> +help
> +  If you say yes here you get support for Texas Instruments ADS7828
> +  12-bit 8-channel ADC device

Missing final dot.

> +
> +  This driver can also be built as a module.  If so, the module
> +  will be called ads7828

Ditto.

> +
>  config SENSORS_VIA686A
>  tristate "VIA686A"
>  depends on PCI

> diff -uprN -X linux-2.6.23.9/Documentation/dontdiff 
> linux-2.6.23.9/drivers/hwmon/ads7828.c 
> linux-2.6.23.9-ads7828/drivers/hwmon/ads7828.c
> --- linux-2.6.23.9/drivers/hwmon/ads7828.c1970-01-01 
> 01:00:00.0 +0100
> +++ linux-2.6.23.9-ads7828/drivers/hwmon/ads7828.c2007-11-28 
> 10:02:02.0 +
> @@ -0,0 +1,304 @@
> +/*
> +ads7828.c - lm_sensors driver for ads7828 12bit 8ch ADC
> +(C) 2007 EADS Astrium
> +
> +This driver is based on the lm75 and other lm_sensors/hwmon drivers
> +
> +Written by Steve Hardy <[EMAIL PROTECTED]>
> +
> +Datasheet available at : http://focus.ti.com/lit/ds/symlink/ads7828.pdf
> +
> +This program is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program; if not, write to the Free Software
> +Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +*/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
> +I2C_CLIENT_END };
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_1(ads7828);
> +
> +/* The ADS7828 registers */
> +#define ADS7828_NCH 8 /* 8 channels of 12bit A-D supported */
> +#define ADS7828_CMD_SD_SE 0x80 /* Single ended inputs */
> +#define ADS7828_CMD_SD_DIFF 0x00 /* Differential inputs */
> +#define ADS7828_CMD_PD0 0x0 /* Power Down between A-D conversions */
> +#define ADS7828_CMD_PD1 0x04 /* Internal ref OFF && A-D ON */
> +#define ADS7828_CMD_PD2 0x08 /* Internal ref ON && A-D OFF */
> +#define ADS7828_CMD_PD3 0x0C /* Internal ref ON && A-D ON */
> +#define ADS7828_INT_VREF 2500 /* Internal vref is 2.5v, 2500mV */
> +
> +/* module parameters */
> +static int ads7828_se_input = 1; /* Default is SE, 0 == diff */
> +static int ads7828_int_vref = 1; /* Default is internal ref ON */
> +static int ads7828_vref = ADS7828_INT_VREF; /* set if vref != 2.5v */
> +module_param(ads7828_se_input, bool, S_IRUGO);
> +module_param(ads7828_int_vref, bool, S_IRUGO);
> +module_param(ads7828_vref, int, S_IRUGO);
> +
> +static u8 ads7828_cmd_byte; /* cmd byte without channel bits */
> +static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */

What's the unit of ads7828_lsb_resol?

All these parameters deserve some documentation in

Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-19 Thread Jean Delvare
Hi Steve, Andrew,

Thanks for the review Andrew. I have some more comments, I'll reply to
the original post.

On Tue, 18 Dec 2007 20:56:44 +, Steve Hardy wrote:
> >> +static int ads7828_attach_adapter(struct i2c_adapter *adapter)
> >> +{
> >> +if (!(adapter->class & I2C_CLASS_HWMON))
> >> +return 0;
> > 
> > Can this happen?
> 
> Hmmm, this code exists in pretty much all of the other hwmon
> drivers, so I will leave it in.
> 
> I think it relates to I2C vs ISA connected devices, to avoid
> detecting something with the correct ID on the wrong bus?

Not at all ;) It relates to I2C buses being used in various devices
such as TV adapters, which do not have hardware monitoring chips and do
not like being randomly probed for I2C devices as most hardware
monitoring drivers do when you load them.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-19 Thread Jean Delvare
Hi Steve, Andrew,

Thanks for the review Andrew. I have some more comments, I'll reply to
the original post.

On Tue, 18 Dec 2007 20:56:44 +, Steve Hardy wrote:
  +static int ads7828_attach_adapter(struct i2c_adapter *adapter)
  +{
  +if (!(adapter-class  I2C_CLASS_HWMON))
  +return 0;
  
  Can this happen?
 
 Hmmm, this code exists in pretty much all of the other hwmon
 drivers, so I will leave it in.
 
 I think it relates to I2C vs ISA connected devices, to avoid
 detecting something with the correct ID on the wrong bus?

Not at all ;) It relates to I2C buses being used in various devices
such as TV adapters, which do not have hardware monitoring chips and do
not like being randomly probed for I2C devices as most hardware
monitoring drivers do when you load them.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-19 Thread Jean Delvare
Hi Steve,

On Mon, 03 Dec 2007 17:59:59 +, Steve Hardy wrote:
 Here is a patch against 2.6.23.9 which adds support for the 
 Burr-Brown/Texas-Instruments
 ADS7828 12-bit 8-channel A-D converter.
 
 The chip is used for voltage monitoring on the COTS processor card I am
 currently working with.
 
 The driver simply outputs the current input voltages (in mv as specified
 in the lm-sensors sysfs interface documentation).  Any scaling required for
 a specific board is handled by user-space.  Hopefully this makes the driver
 generic enough to be generally useful.
 
 The driver is basically a simple rehash of existing code - I used lm75 as
 the starting point, with some inspiration from other existing drivers.
 
 Please let me know if there are any problems with the patch.

Preliminary note: if the chip is on an embedded platform where you can
declare the devices as platform data, writing a new-style I2C driver
would probably be easier. This would save you the detection step (which
I am not sure is very reliable) and device configuration would also be
much easier.

I have some random comments on top of what Andrew already wrote:

 diff -uprN -X linux-2.6.23.9/Documentation/dontdiff 
 linux-2.6.23.9/drivers/hwmon/Kconfig 
 linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig
 --- linux-2.6.23.9/drivers/hwmon/Kconfig2007-11-26 
 17:51:43.0 +
 +++ linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig2007-11-28 
 10:02:02.0 +
 @@ -530,6 +530,16 @@ config SENSORS_THMC50
This driver can also be built as a module.  If so, the module
will be called thmc50.
  
 +config SENSORS_ADS7828
 +tristate Texas Instruments ADS7828
 +depends on I2C  EXPERIMENTAL
 +help
 +  If you say yes here you get support for Texas Instruments ADS7828
 +  12-bit 8-channel ADC device

Missing final dot.

 +
 +  This driver can also be built as a module.  If so, the module
 +  will be called ads7828

Ditto.

 +
  config SENSORS_VIA686A
  tristate VIA686A
  depends on PCI

 diff -uprN -X linux-2.6.23.9/Documentation/dontdiff 
 linux-2.6.23.9/drivers/hwmon/ads7828.c 
 linux-2.6.23.9-ads7828/drivers/hwmon/ads7828.c
 --- linux-2.6.23.9/drivers/hwmon/ads7828.c1970-01-01 
 01:00:00.0 +0100
 +++ linux-2.6.23.9-ads7828/drivers/hwmon/ads7828.c2007-11-28 
 10:02:02.0 +
 @@ -0,0 +1,304 @@
 +/*
 +ads7828.c - lm_sensors driver for ads7828 12bit 8ch ADC
 +(C) 2007 EADS Astrium
 +
 +This driver is based on the lm75 and other lm_sensors/hwmon drivers
 +
 +Written by Steve Hardy [EMAIL PROTECTED]
 +
 +Datasheet available at : http://focus.ti.com/lit/ds/symlink/ads7828.pdf
 +
 +This program is free software; you can redistribute it and/or modify
 +it under the terms of the GNU General Public License as published by
 +the Free Software Foundation; either version 2 of the License, or
 +(at your option) any later version.
 +
 +This program is distributed in the hope that it will be useful,
 +but WITHOUT ANY WARRANTY; without even the implied warranty of
 +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.See the
 +GNU General Public License for more details.
 +
 +You should have received a copy of the GNU General Public License
 +along with this program; if not, write to the Free Software
 +Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 +*/
 +
 +#include linux/module.h
 +#include linux/init.h
 +#include linux/slab.h
 +#include linux/jiffies.h
 +#include linux/i2c.h
 +#include linux/hwmon.h
 +#include linux/hwmon-sysfs.h
 +#include linux/err.h
 +#include linux/mutex.h
 +
 +/* Addresses to scan */
 +static unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
 +I2C_CLIENT_END };
 +
 +/* Insmod parameters */
 +I2C_CLIENT_INSMOD_1(ads7828);
 +
 +/* The ADS7828 registers */
 +#define ADS7828_NCH 8 /* 8 channels of 12bit A-D supported */
 +#define ADS7828_CMD_SD_SE 0x80 /* Single ended inputs */
 +#define ADS7828_CMD_SD_DIFF 0x00 /* Differential inputs */
 +#define ADS7828_CMD_PD0 0x0 /* Power Down between A-D conversions */
 +#define ADS7828_CMD_PD1 0x04 /* Internal ref OFF  A-D ON */
 +#define ADS7828_CMD_PD2 0x08 /* Internal ref ON  A-D OFF */
 +#define ADS7828_CMD_PD3 0x0C /* Internal ref ON  A-D ON */
 +#define ADS7828_INT_VREF 2500 /* Internal vref is 2.5v, 2500mV */
 +
 +/* module parameters */
 +static int ads7828_se_input = 1; /* Default is SE, 0 == diff */
 +static int ads7828_int_vref = 1; /* Default is internal ref ON */
 +static int ads7828_vref = ADS7828_INT_VREF; /* set if vref != 2.5v */
 +module_param(ads7828_se_input, bool, S_IRUGO);
 +module_param(ads7828_int_vref, bool, S_IRUGO);
 +module_param(ads7828_vref, int, S_IRUGO);
 +
 +static u8 ads7828_cmd_byte; /* cmd byte without channel bits */
 +static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */

What's the unit of ads7828_lsb_resol?

All these parameters deserve some documentation in

Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-18 Thread Steve Hardy

Andrew,

Thanks your your comments, I'm currently preparing/testing a revised patch 
based on your suggestions, which I will post later this week.

A couple of comments inline.

Steve


I wouldn't bother with EXPERIMENTAL personally.  It seems a farily
pointless thing.



OK, I copied one of the other hwmon drivers, many of which are marked 
experimental.


+static int ads7828_attach_adapter(struct i2c_adapter *adapter);
+static int ads7828_detect(struct i2c_adapter *adapter, int address, int 
kind);

+static void ads7828_init_client(struct i2c_client *client);
+static int ads7828_detach_client(struct i2c_client *client);
+static struct ads7828_data *ads7828_update_device(struct device *dev);
+static u16 ads7828_read_value(struct i2c_client *client, u8 reg);


I do dislike all these forward declarations, but they're all needed here
give the order of the functions.  Maybe from my Pascal-on-pdp11 days..



OK, this is due to re-using the driver structure/style of other hwmon drivers, 
will try to improve.


+static int ads7828_attach_adapter(struct i2c_adapter *adapter)
+{
+if (!(adapter->class & I2C_CLASS_HWMON))
+return 0;


Can this happen?


Hmmm, this code exists in pretty much all of the other hwmon drivers, so I will 
leave it in.

I think it relates to I2C vs ISA connected devices, to avoid detecting 
something with the correct ID on the wrong bus?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-18 Thread Steve Hardy

Andrew,

Thanks your your comments, I'm currently preparing/testing a revised patch 
based on your suggestions, which I will post later this week.

A couple of comments inline.

Steve


I wouldn't bother with EXPERIMENTAL personally.  It seems a farily
pointless thing.



OK, I copied one of the other hwmon drivers, many of which are marked 
experimental.


+static int ads7828_attach_adapter(struct i2c_adapter *adapter);
+static int ads7828_detect(struct i2c_adapter *adapter, int address, int 
kind);

+static void ads7828_init_client(struct i2c_client *client);
+static int ads7828_detach_client(struct i2c_client *client);
+static struct ads7828_data *ads7828_update_device(struct device *dev);
+static u16 ads7828_read_value(struct i2c_client *client, u8 reg);


I do dislike all these forward declarations, but they're all needed here
give the order of the functions.  Maybe from my Pascal-on-pdp11 days..



OK, this is due to re-using the driver structure/style of other hwmon drivers, 
will try to improve.


+static int ads7828_attach_adapter(struct i2c_adapter *adapter)
+{
+if (!(adapter-class  I2C_CLASS_HWMON))
+return 0;


Can this happen?


Hmmm, this code exists in pretty much all of the other hwmon drivers, so I will 
leave it in.

I think it relates to I2C vs ISA connected devices, to avoid detecting 
something with the correct ID on the wrong bus?

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-12 Thread Andrew Morton
On Mon, 03 Dec 2007 17:59:59 + Steve Hardy <[EMAIL PROTECTED]> wrote:

> Hi,
> 
> Here is a patch against 2.6.23.9 which adds support for the 
> Burr-Brown/Texas-Instruments
> ADS7828 12-bit 8-channel A-D converter.
> 
> The chip is used for voltage monitoring on the COTS processor card I am
> currently working with.
> 
> The driver simply outputs the current input voltages (in mv as specified
> in the lm-sensors sysfs interface documentation).  Any scaling required for
> a specific board is handled by user-space.  Hopefully this makes the driver
> generic enough to be generally useful.
> 
> The driver is basically a simple rehash of existing code - I used lm75 as
> the starting point, with some inspiration from other existing drivers.
> 
> ...
>
> diff -uprN -X linux-2.6.23.9/Documentation/dontdiff 
> linux-2.6.23.9/drivers/hwmon/Kconfig 
> linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig
> --- linux-2.6.23.9/drivers/hwmon/Kconfig2007-11-26 
> 17:51:43.0 +
> +++ linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig2007-11-28 

Your email client is wordwrapping the text and it replaces tabs with
spaces.  It will need a resend, please.

> @@ -530,6 +530,16 @@ config SENSORS_THMC50
>This driver can also be built as a module.  If so, the module
>will be called thmc50.
>  
> +config SENSORS_ADS7828
> +tristate "Texas Instruments ADS7828"
> +depends on I2C && EXPERIMENTAL

I wouldn't bother with EXPERIMENTAL personally.  It seems a farily
pointless thing.

> linux-2.6.23.9-ads7828/drivers/hwmon/ads7828.c

Please prepare and test patches against the latest Linus tree, from git or
from ftp://ftp.kernel.org/pub/linux/kernel/v2.6/snapshots

Usually it's OK to develop a new driver against the latest stable release,
but we regularly change interfaces and if we did, this driver won't even
compile.

> +static int ads7828_attach_adapter(struct i2c_adapter *adapter);
> +static int ads7828_detect(struct i2c_adapter *adapter, int address, int 
> kind);
> +static void ads7828_init_client(struct i2c_client *client);
> +static int ads7828_detach_client(struct i2c_client *client);
> +static struct ads7828_data *ads7828_update_device(struct device *dev);
> +static u16 ads7828_read_value(struct i2c_client *client, u8 reg);

I do dislike all these forward declarations, but they're all needed here
give the order of the functions.  Maybe from my Pascal-on-pdp11 days..

> +/* This is the driver that will be inserted */
> +static struct i2c_driver ads7828_driver = {
> +.driver = {
> +.name = "ads7828",
> +},
> +.id = I2C_DRIVERID_ADS7828,
> +.attach_adapter = ads7828_attach_adapter,
> +.detach_client = ads7828_detach_client,
> +};
> +
> +static ssize_t show_in(struct device *dev, struct device_attribute *da,
> +char *buf)
> +{
> +struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +struct ads7828_data *data = ads7828_update_device(dev);
> +/* Print value (in mv as specified in sysctl-interface 
> documentation) */
> +return sprintf(buf, "%d\n", (data->adc_input[attr->index] *
> +ads7828_lsb_resol)/1000);
> +}
> +
> +#define in_reg(offset)\
> +static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
> +NULL, offset);
> +
> +in_reg(0);
> +in_reg(1);
> +in_reg(2);
> +in_reg(3);
> +in_reg(4);
> +in_reg(5);
> +in_reg(6);
> +in_reg(7);
> +
> +static int ads7828_attach_adapter(struct i2c_adapter *adapter)
> +{
> +if (!(adapter->class & I2C_CLASS_HWMON))
> +return 0;

Can this happen?

> +return i2c_probe(adapter, _data, ads7828_detect);
> +}
> +
> +static struct attribute *ads7828_attributes[] = {
> +_dev_attr_in0_input.dev_attr.attr,
> +_dev_attr_in1_input.dev_attr.attr,
> +_dev_attr_in2_input.dev_attr.attr,
> +_dev_attr_in3_input.dev_attr.attr,
> +_dev_attr_in4_input.dev_attr.attr,
> +_dev_attr_in5_input.dev_attr.attr,
> +_dev_attr_in6_input.dev_attr.attr,
> +_dev_attr_in7_input.dev_attr.attr,
> +NULL
> +};
> +
> +static const struct attribute_group ads7828_group = {
> +.attrs = ads7828_attributes,
> +};
> +
> +/* This function is called by i2c_detect */
> +static int ads7828_detect(struct i2c_adapter *adapter, int address, int 
> kind)
> +{
> +struct i2c_client *new_client;
> +struct ads7828_data *data;
> +int err = 0, ch = 0;
> +const char *name = "";
> +u8 cmd;
> +u16 in_data;
> +
> +/* Check we have a valid client */
> +if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA))
> +goto exit;
> +
> +/* OK. For now, we presume we have a valid client. We now create the
> + client structure, even though we cannot fill it completely yet.
> + But it allows us to access ads7828_read_value. */
> +data = kmalloc(sizeof(struct ads7828_data), GFP_KERNEL);
> +if (!data)
> +err = -ENOMEM;
> +goto exit;
> +}
> +memset(data, 0, 

Re: [PATCH 1/1] : hwmon - new chip driver for TI ADS7828 A-D

2007-12-12 Thread Andrew Morton
On Mon, 03 Dec 2007 17:59:59 + Steve Hardy [EMAIL PROTECTED] wrote:

 Hi,
 
 Here is a patch against 2.6.23.9 which adds support for the 
 Burr-Brown/Texas-Instruments
 ADS7828 12-bit 8-channel A-D converter.
 
 The chip is used for voltage monitoring on the COTS processor card I am
 currently working with.
 
 The driver simply outputs the current input voltages (in mv as specified
 in the lm-sensors sysfs interface documentation).  Any scaling required for
 a specific board is handled by user-space.  Hopefully this makes the driver
 generic enough to be generally useful.
 
 The driver is basically a simple rehash of existing code - I used lm75 as
 the starting point, with some inspiration from other existing drivers.
 
 ...

 diff -uprN -X linux-2.6.23.9/Documentation/dontdiff 
 linux-2.6.23.9/drivers/hwmon/Kconfig 
 linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig
 --- linux-2.6.23.9/drivers/hwmon/Kconfig2007-11-26 
 17:51:43.0 +
 +++ linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig2007-11-28 

Your email client is wordwrapping the text and it replaces tabs with
spaces.  It will need a resend, please.

 @@ -530,6 +530,16 @@ config SENSORS_THMC50
This driver can also be built as a module.  If so, the module
will be called thmc50.
  
 +config SENSORS_ADS7828
 +tristate Texas Instruments ADS7828
 +depends on I2C  EXPERIMENTAL

I wouldn't bother with EXPERIMENTAL personally.  It seems a farily
pointless thing.

 linux-2.6.23.9-ads7828/drivers/hwmon/ads7828.c

Please prepare and test patches against the latest Linus tree, from git or
from ftp://ftp.kernel.org/pub/linux/kernel/v2.6/snapshots

Usually it's OK to develop a new driver against the latest stable release,
but we regularly change interfaces and if we did, this driver won't even
compile.

 +static int ads7828_attach_adapter(struct i2c_adapter *adapter);
 +static int ads7828_detect(struct i2c_adapter *adapter, int address, int 
 kind);
 +static void ads7828_init_client(struct i2c_client *client);
 +static int ads7828_detach_client(struct i2c_client *client);
 +static struct ads7828_data *ads7828_update_device(struct device *dev);
 +static u16 ads7828_read_value(struct i2c_client *client, u8 reg);

I do dislike all these forward declarations, but they're all needed here
give the order of the functions.  Maybe from my Pascal-on-pdp11 days..

 +/* This is the driver that will be inserted */
 +static struct i2c_driver ads7828_driver = {
 +.driver = {
 +.name = ads7828,
 +},
 +.id = I2C_DRIVERID_ADS7828,
 +.attach_adapter = ads7828_attach_adapter,
 +.detach_client = ads7828_detach_client,
 +};
 +
 +static ssize_t show_in(struct device *dev, struct device_attribute *da,
 +char *buf)
 +{
 +struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 +struct ads7828_data *data = ads7828_update_device(dev);
 +/* Print value (in mv as specified in sysctl-interface 
 documentation) */
 +return sprintf(buf, %d\n, (data-adc_input[attr-index] *
 +ads7828_lsb_resol)/1000);
 +}
 +
 +#define in_reg(offset)\
 +static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
 +NULL, offset);
 +
 +in_reg(0);
 +in_reg(1);
 +in_reg(2);
 +in_reg(3);
 +in_reg(4);
 +in_reg(5);
 +in_reg(6);
 +in_reg(7);
 +
 +static int ads7828_attach_adapter(struct i2c_adapter *adapter)
 +{
 +if (!(adapter-class  I2C_CLASS_HWMON))
 +return 0;

Can this happen?

 +return i2c_probe(adapter, addr_data, ads7828_detect);
 +}
 +
 +static struct attribute *ads7828_attributes[] = {
 +sensor_dev_attr_in0_input.dev_attr.attr,
 +sensor_dev_attr_in1_input.dev_attr.attr,
 +sensor_dev_attr_in2_input.dev_attr.attr,
 +sensor_dev_attr_in3_input.dev_attr.attr,
 +sensor_dev_attr_in4_input.dev_attr.attr,
 +sensor_dev_attr_in5_input.dev_attr.attr,
 +sensor_dev_attr_in6_input.dev_attr.attr,
 +sensor_dev_attr_in7_input.dev_attr.attr,
 +NULL
 +};
 +
 +static const struct attribute_group ads7828_group = {
 +.attrs = ads7828_attributes,
 +};
 +
 +/* This function is called by i2c_detect */
 +static int ads7828_detect(struct i2c_adapter *adapter, int address, int 
 kind)
 +{
 +struct i2c_client *new_client;
 +struct ads7828_data *data;
 +int err = 0, ch = 0;
 +const char *name = ;
 +u8 cmd;
 +u16 in_data;
 +
 +/* Check we have a valid client */
 +if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
 + I2C_FUNC_SMBUS_WORD_DATA))
 +goto exit;
 +
 +/* OK. For now, we presume we have a valid client. We now create the
 + client structure, even though we cannot fill it completely yet.
 + But it allows us to access ads7828_read_value. */
 +data = kmalloc(sizeof(struct ads7828_data), GFP_KERNEL);
 +if (!data)
 +err = -ENOMEM;
 +goto exit;
 +}
 +memset(data, 0, sizeof(struct ads7828_data));

Use kzalloc() above, remove the memset.

 +