Re: [PATCH v4 2/2] hwmon: (ads7828) add support for ADS7830

[Guenter Roeck]

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.

How do you use the chip ? Do you need the detect function in your application ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/