Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753256Ab2JCFHB (ORCPT ); Wed, 3 Oct 2012 01:07:01 -0400 Received: from mail.active-venture.com ([67.228.131.205]:57179 "EHLO mail.active-venture.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751448Ab2JCFHA (ORCPT ); Wed, 3 Oct 2012 01:07:00 -0400 X-Originating-IP: 108.223.40.66 Date: Tue, 2 Oct 2012 22:07:39 -0700 From: Guenter Roeck To: Vivien Didelot Cc: lm-sensors@lm-sensors.org, Guillaume Roguez , Jean Delvare , linux-kernel@vger.kernel.org, Steve Hardy Subject: Re: [PATCH v4 2/2] hwmon: (ads7828) add support for ADS7830 Message-ID: <20121003050739.GB7491@roeck-us.net> References: <1349235207-7517-1-git-send-email-vivien.didelot@savoirfairelinux.com> <1349235207-7517-2-git-send-email-vivien.didelot@savoirfairelinux.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1349235207-7517-2-git-send-email-vivien.didelot@savoirfairelinux.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3520 Lines: 96 On Tue, Oct 02, 2012 at 11:33:27PM -0400, Vivien Didelot wrote: > From: Guillaume Roguez > > 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 > Signed-off-by: Vivien Didelot 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/