On Tue, Oct 02, 2012 at 10:07:39PM -0700, Guenter Roeck wrote:
> On Tue, Oct 02, 2012 at 11:33:27PM -0400, Vivien Didelot wrote:
> > From: Guillaume Roguez <guillaume.roguez@xxxxxxxxxxxxxxxxxxxx>
> >
> > The ADS7830 device is almost the same as the ADS7828,
> > except that it does 8-bit sampling, instead of 12-bit.
> > This patch extends the ads7828 driver to support this chip.
> >
> > Signed-off-by: Guillaume Roguez <guillaume.roguez@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
>
> Guillaume,
> Vivien,
>
> [ ... ]
>
> > @@ -147,6 +152,7 @@ static int ads7828_detect(struct i2c_client *client,
> > {
> > struct i2c_adapter *adapter = client->adapter;
> > u8 default_cmd_byte = ADS7828_CMD_SD_SE | ADS7828_CMD_PD3;
> > + bool is_8bit = false;
> > int ch;
> >
> > /* Check we have a valid client */
> > @@ -158,7 +164,9 @@ static int ads7828_detect(struct i2c_client *client,
> > * dedicated register so attempt to sanity check using knowledge of
> > * the chip
> > * - Read from the 8 channel addresses
> > - * - Check the top 4 bits of each result are not set (12 data bits)
> > + * - Check the top 4 bits of each result:
> > + * - They should not be set in case of 12-bit samples
> > + * - The two bytes should be equal in case of 8-bit samples
> > */
> > for (ch = 0; ch < ADS7828_NCH; ch++) {
> > u8 cmd = ads7828_cmd_byte(default_cmd_byte, ch);
> > @@ -168,13 +176,20 @@ static int ads7828_detect(struct i2c_client *client,
> > return -ENODEV;
> >
> > if (in_data & 0xF000) {
> > - pr_debug("%s : Doesn't look like an ads7828 device\n",
> > - __func__);
> > - return -ENODEV;
> > + if ((in_data >> 8) == (in_data & 0xFF)) {
> > + /* Seems to be an ADS7830 (8-bit sample) */
> > + is_8bit = true;
> > + } else {
> > + dev_dbg(&client->dev, "doesn't look like an ADS7828 compatible device\n");
> > + return -ENODEV;
> > + }
> > }
> > }
>
> I have been thinking about this. The detection function is already quite weak,
> and this makes it even weaker. Reason is that you conly check for ADS7830 if the
> check for ADS7828 failed, and you repeat the pattern for each channel.
> Unfortunately, that means that you don't check for the ADS7830 condition if the
> value returned for a channel happens to be a valid ADS7828 value, even if it is
> not valid for ADS7830 (and even if you already know that the chip is not a
> ADS7828).
>
> Example:
> ch=0: 0x1818 --> You know it is not ADS7828
> ch=1: 0x0818 --> You know it is not ADS7830, but you don't check for it
>
> I don't know an optimal solution right now, but maybe something like
>
> maybe_7828 = true;
> maybe_7830 = true;
> for (ch = 0; ch < ADS7828_NCH && (maybe_7828 || maybe_7830); ch++) {
> ...
> if (in_data & 0xF000)
> maybe_7828 = false;
> if ((in_data >> 8) != (in_data & 0xFF))
> maybe_7830 = false;
> }
> if (!maybe_7828 && !maybe_7830)
> return -ENODEV;
>
> if (maybe_7828)
> strlcpy(info->type, "ads7828", I2C_NAME_SIZE);
> else
> strlcpy(info->type, "ads7830", I2C_NAME_SIZE);
>
> Frankly I would prefer to get rid of the _detect function entirely, I just don't
> know if that would negatively affect some users. To give you an example for a
> bad result: The function will wrongly detect an ADS7830 as ADS7828 if all ADC
> channels report a value between 0x00 and 0x0f.
I'm no longer involved with development on the platform I wrote this driver for, but FWIW I think weak detection (provided it doesn't result in false negatives) is better than no detection at all (potentially resulting in bogus detection of non ads78* devices via this driver)
Steve
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
