On Wed, May 29, 2013 at 04:02:56PM -0700, Robert Coulson wrote:
> On Tue, May 28, 2013 at 7:52 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> > On Tue, May 28, 2013 at 04:56:01PM -0700, Robert Coulson wrote:
> > > These changes add DS1731 chip support to the ds1621 driver,
> > > Kconfig, and documentation.
> > >
> > > Signed-off-by: Robert Coulson <rob.coulson@xxxxxxxxx>
> >
> > Hi Rob,
> >
> > Nitpicks ...
> >
>
> Good afternoon Guenter,
>
>
> >
> > > ---
> > > Documentation/hwmon/ds1621 | 22 ++++++++++++++--------
> > > drivers/hwmon/Kconfig | 1 +
> > > drivers/hwmon/ds1621.c | 12 +++++++-----
> > > 3 files changed, 22 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/Documentation/hwmon/ds1621 b/Documentation/hwmon/ds1621
> > > index 2022379..64ee39f 100644
> > > --- a/Documentation/hwmon/ds1621
> > > +++ b/Documentation/hwmon/ds1621
> > > @@ -22,6 +22,11 @@ Supported chips:
> > > Addresses scanned: none
> > > Datasheet: Publicly available from www.maximintegrated.com
> > >
> > > + * Maxim Integrated DS1731
> > > + Prefix: 'ds1731'
> > > + Addresses scanned: none
> > > + Datasheet: Publicly available from www.maximintegrated.com
> > > +
> > > Authors:
> > > Christian W. Zuckschwerdt <zany@xxxxxxxx>
> > > valuable contributions by Jan M. Sendler <sendler@xxxxxxxxxx>
> > > @@ -72,7 +77,7 @@ Temperature conversion of the DS1621 takes up to
> > 1000ms; internal access to
> > > non-volatile registers may last for 10ms or below.
> > >
> > > The DS1625 is pin compatible and functionally equivalent with the
> > DS1621,
> > > -but the DS1621 is meant to replace it. The DS1631 and DS1721 are also
> > > +but the DS1621 is meant to replace it. The DS1631, DS1721, & DS1731 are
> > also
> > > pin compatible with the DS1621, but provide multi-resolution support.
> > >
> > > Since there is no version or vendor identification register, there is
> > > @@ -96,14 +101,15 @@ One additional note about the ds1721 is that
> > although the data sheet says
> > > the temperature flags (THF and TLF) are used internally, these flags do
> > > get set and cleared as the actual temperature crosses the min or max
> > settings.
> > >
> > > -The DS1631 is also pin compatible with the DS1621 and feature
> > compatible with
> > > -the DS1721, however the DS1631 accuracy is +/- 0.5 degree Celius over
> > the
> > > -same range.
> > > +The DS1631 and DS1731 are pin compatible with the DS1621 and feature
> > compatible
> > > +with the DS1721, however, the DS1631 accuracy is +/- 0.5 degree Celius
> > over
> >
> > DS1721. However, ...
> >
>
> fixed.
>
>
> >
> > > +the 0 to +70 degrees, while the DS1731 accuracy is +/-1 degree Celius
> > over the
> >
> > "over the ... degrees" sounds weird. "over the range of", maybe ? Or at
> > least
> > add "range" after "degrees".
> >
>
> how about 'within -10 to +85 degrees'.
>
ok
Guenter
>
> >
> > > +-10 to +85 degree range.
> > >
> > > -Changing the DS1631/1721 resolution mode affects the conversion time
> > and can be
> > > -done from userspace, via the device 'update_interval' sysfs attribute.
> > This
> > > -attribute will normalize range of input values to the device maximum
> > resolution
> > > -values defined in the datasheet as such:
> > > +Changing the DS1631, DS1721, or DS1731 resolution mode affects the
> > conversion
> >
> > Changing the resolution mode for ... affects
> >
>
> agreed.
>
>
> >
> > > +time and can be done from userspace, via the device 'update_interval'
> > sysfs
> > > +attribute. This attribute will normalize range of input values to the
> > device
> > > +maximum resolution values defined in the datasheet as such:
> > >
> > as follows ?
> >
>
> agreed.
>
>
> >
> > > Resolution Conversion Time Input Range
> > > (C/LSB) (msec) (msec)
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 0f20eef..38c27b2 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -324,6 +324,7 @@ config SENSORS_DS1621
> > > - Dallas Semiconductor DS1625
> > > - Maxim Integrated DS1631
> > > - Maxim Integrated DS1721
> > > + - Maxim Integrated DS1731
> > >
> > > This driver can also be built as a module. If so, the module
> > > will be called ds1621.
> > > diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
> > > index 1ecfcd0..c94495e 100644
> > > --- a/drivers/hwmon/ds1621.c
> > > +++ b/drivers/hwmon/ds1621.c
> > > @@ -10,8 +10,8 @@
> > > * resolution, a thermal alarm output (Tout), and user-defined minimum
> > > * and maximum temperature thresholds (TH & TL).
> > > *
> > > - * The DS1625, DS1631, & DS1721 are pin compatible with the DS1621 and
> > > - * similar in operation, with slight variations as noted in the device
> > > + * The DS1625, DS1631, DS1721, & DS1731 are pin compatible with the
> > DS1621
> > > + * and similar in operation, with slight variations as noted in the
> > device
> > > * datasheets (please refer to www.maximintegrated.com for specific
> > > * device information).
> > > *
> > > @@ -47,7 +47,7 @@
> > > #include <linux/kernel.h>
> > >
> > > /* Supported devices */
> > > -enum chips { ds1621, ds1625, ds1631, ds1721 };
> > > +enum chips { ds1621, ds1625, ds1631, ds1721, ds1731 };
> > >
> > > /* Insmod parameters */
> > > static int polarity = -1;
> > > @@ -65,7 +65,7 @@ MODULE_PARM_DESC(polarity, "Output's polarity: 0 =
> > active high, 1 = active low")
> > > * 7 6 5 4 3 2 1 0
> > > * |Done|THF |TLF |NVB | 1 | 0 |POL |1SHOT|
> > > *
> > > - * - DS1631:
> > > + * - DS1631, DS1731:
> > > * 7 6 5 4 3 2 1 0
> > > * |Done|THF |TLF |NVB | R1 | R0 |POL |1SHOT|
> > > *
> > > @@ -140,7 +140,7 @@ static inline int DS1621_TEMP_FROM_REG(u16 reg)
> > > * TEMP: 0.001C/bit (-55C to +125C)
> > > * REG:
> > > * - 1621, 1625: 0.5C/bit
> > > - * - 1631, 1721: 0.0625C/bit
> > > + * - 1631, 1721, 1731: 0.0625C/bit
> > > * Assume highest resolution and let the bits fall where they may..
> > > */
> > > static inline u16 DS1621_TEMP_TO_REG(long temp)
> > > @@ -176,6 +176,7 @@ static void ds1621_init_client(struct i2c_client
> > *client)
> > > break;
> > > case ds1631:
> > > case ds1721:
> > > + case ds1731:
> > > resol = (new_conf & DS1621_REG_CONFIG_RESOL) >>
> > > DS1621_REG_CONFIG_RESOL_SHIFT;
> > > data->update_interval = ds1721_convrates[resol];
> > > @@ -408,6 +409,7 @@ static const struct i2c_device_id ds1621_id[] = {
> > > { "ds1625", ds1625 },
> > > { "ds1631", ds1631 },
> > > { "ds1721", ds1721 },
> > > + { "ds1731", ds1731 },
> > > { }
> > > };
> > > MODULE_DEVICE_TABLE(i2c, ds1621_id);
> > > --
> > > 1.7.9.5
> > >
> > >
> > > _______________________________________________
> > > lm-sensors mailing list
> > > lm-sensors@xxxxxxxxxxxxxx
> > > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> > >
> >
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
