[PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/hwmon/lm63.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 508cb29..94e373c 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -483,7 +483,7 @@ static int lm63_detect(struct i2c_client *new_client,
 		return -ENODEV;
 	}
 
-	if (chip_id == 0x41 && address == 0x4c)
+	if ((chip_id == 0x41 || chip_id == 0x49) && address == 0x4c)
 		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
 	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
 		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
-- 
1.7.7.2

Re: [PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[Guenter Roeck]

Hi,

On Mon, 2011-11-07 at 05:20 -0500, Thierry Reding wrote:
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  drivers/hwmon/lm63.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 508cb29..94e373c 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -483,7 +483,7 @@ static int lm63_detect(struct i2c_client *new_client,
>  		return -ENODEV;
>  	}
>  
> -	if (chip_id == 0x41 && address == 0x4c)
> +	if ((chip_id == 0x41 || chip_id == 0x49) && address == 0x4c)
>  		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
>  	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
>  		strlcpy(info->type, "lm64", I2C_NAME_SIZE);

Minor - the new chip should be mentioned in drivers/hwmon/Kconfig and in
Documentation/hwmon/lm63.

My main concern is that LM96163 supports a high resolution PWM mode.
Since that mode is much more convenient than the LM63-compatible mode, I
think it is likely to be enabled in designs using LM96163, and it should
be supported by the driver if we add support for LM96163.

Thanks,
Guenter

Re: [PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

* Guenter Roeck wrote:
> Hi,
> 
> On Mon, 2011-11-07 at 05:20 -0500, Thierry Reding wrote:
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > ---
> >  drivers/hwmon/lm63.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > index 508cb29..94e373c 100644
> > --- a/drivers/hwmon/lm63.c
> > +++ b/drivers/hwmon/lm63.c
> > @@ -483,7 +483,7 @@ static int lm63_detect(struct i2c_client *new_client,
> >  		return -ENODEV;
> >  	}
> >  
> > -	if (chip_id == 0x41 && address == 0x4c)
> > +	if ((chip_id == 0x41 || chip_id == 0x49) && address == 0x4c)
> >  		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
> >  	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
> >  		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
> 
> Minor - the new chip should be mentioned in drivers/hwmon/Kconfig and in
> Documentation/hwmon/lm63.

I don't see how to list this variant meaningfully in the documentation.
Since, as you mention, no functionality specific to the LM96163 is added,
this patch really only adds support for one more lm63 chip.

> My main concern is that LM96163 supports a high resolution PWM mode.
> Since that mode is much more convenient than the LM63-compatible mode, I
> think it is likely to be enabled in designs using LM96163, and it should
> be supported by the driver if we add support for LM96163.

Unfortunately our design doesn't use the PWM at all. It is only used for
temperature measurements, so I'm not able to test any of the PWM code.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[Jean Delvare]

Hi Thierry,

On Tue, 8 Nov 2011 08:30:52 +0100, Thierry Reding wrote:
> * Guenter Roeck wrote:
> > Hi,
> > 
> > On Mon, 2011-11-07 at 05:20 -0500, Thierry Reding wrote:
> > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > > ---
> > >  drivers/hwmon/lm63.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > > index 508cb29..94e373c 100644
> > > --- a/drivers/hwmon/lm63.c
> > > +++ b/drivers/hwmon/lm63.c
> > > @@ -483,7 +483,7 @@ static int lm63_detect(struct i2c_client *new_client,
> > >  		return -ENODEV;
> > >  	}
> > >  
> > > -	if (chip_id == 0x41 && address == 0x4c)
> > > +	if ((chip_id == 0x41 || chip_id == 0x49) && address == 0x4c)
> > >  		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
> > >  	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
> > >  		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
> > 
> > Minor - the new chip should be mentioned in drivers/hwmon/Kconfig and in
> > Documentation/hwmon/lm63.
> 
> I don't see how to list this variant meaningfully in the documentation.
> Since, as you mention, no functionality specific to the LM96163 is added,
> this patch really only adds support for one more lm63 chip.

The new chip name should at least be listed in
Documentation/hwmon/lm63, drivers/hwmon/Kconfig.

> > My main concern is that LM96163 supports a high resolution PWM mode.
> > Since that mode is much more convenient than the LM63-compatible mode, I
> > think it is likely to be enabled in designs using LM96163, and it should
> > be supported by the driver if we add support for LM96163.
> 
> Unfortunately our design doesn't use the PWM at all. It is only used for
> temperature measurements, so I'm not able to test any of the PWM code.

You still need to ensure that nothing bad will happen if users load the
lm63 driver with a LM96163 chip (if nothing else, by disabling the
unimplemented/untested feature.)

-- 
Jean Delvare

Re: [PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

* Jean Delvare wrote:
> Hi Thierry,
> 
> On Tue, 8 Nov 2011 08:30:52 +0100, Thierry Reding wrote:
> > * Guenter Roeck wrote:
> > > Hi,
> > > 
> > > On Mon, 2011-11-07 at 05:20 -0500, Thierry Reding wrote:
> > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > > > ---
> > > >  drivers/hwmon/lm63.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > > > index 508cb29..94e373c 100644
> > > > --- a/drivers/hwmon/lm63.c
> > > > +++ b/drivers/hwmon/lm63.c
> > > > @@ -483,7 +483,7 @@ static int lm63_detect(struct i2c_client *new_client,
> > > >  		return -ENODEV;
> > > >  	}
> > > >  
> > > > -	if (chip_id == 0x41 && address == 0x4c)
> > > > +	if ((chip_id == 0x41 || chip_id == 0x49) && address == 0x4c)
> > > >  		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
> > > >  	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
> > > >  		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
> > > 
> > > Minor - the new chip should be mentioned in drivers/hwmon/Kconfig and in
> > > Documentation/hwmon/lm63.
> > 
> > I don't see how to list this variant meaningfully in the documentation.
> > Since, as you mention, no functionality specific to the LM96163 is added,
> > this patch really only adds support for one more lm63 chip.
> 
> The new chip name should at least be listed in
> Documentation/hwmon/lm63, drivers/hwmon/Kconfig.

Okay.

> > > My main concern is that LM96163 supports a high resolution PWM mode.
> > > Since that mode is much more convenient than the LM63-compatible mode, I
> > > think it is likely to be enabled in designs using LM96163, and it should
> > > be supported by the driver if we add support for LM96163.
> > 
> > Unfortunately our design doesn't use the PWM at all. It is only used for
> > temperature measurements, so I'm not able to test any of the PWM code.
> 
> You still need to ensure that nothing bad will happen if users load the
> lm63 driver with a LM96163 chip (if nothing else, by disabling the
> unimplemented/untested feature.)

Okay. I'll make sure to mention that in the documentation as well.

Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[Guenter Roeck]

On Tue, 2011-11-08 at 02:30 -0500, Thierry Reding wrote:
> * Guenter Roeck wrote:
> > Hi,
> > 
> > On Mon, 2011-11-07 at 05:20 -0500, Thierry Reding wrote:
> > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > > ---
> > >  drivers/hwmon/lm63.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > > index 508cb29..94e373c 100644
> > > --- a/drivers/hwmon/lm63.c
> > > +++ b/drivers/hwmon/lm63.c
> > > @@ -483,7 +483,7 @@ static int lm63_detect(struct i2c_client *new_client,
> > >  		return -ENODEV;
> > >  	}
> > >  
> > > -	if (chip_id == 0x41 && address == 0x4c)
> > > +	if ((chip_id == 0x41 || chip_id == 0x49) && address == 0x4c)
> > >  		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
> > >  	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
> > >  		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
> > 
> > Minor - the new chip should be mentioned in drivers/hwmon/Kconfig and in
> > Documentation/hwmon/lm63.
> 
> I don't see how to list this variant meaningfully in the documentation.
> Since, as you mention, no functionality specific to the LM96163 is added,
> this patch really only adds support for one more lm63 chip.
> 
> > My main concern is that LM96163 supports a high resolution PWM mode.
> > Since that mode is much more convenient than the LM63-compatible mode, I
> > think it is likely to be enabled in designs using LM96163, and it should
> > be supported by the driver if we add support for LM96163.
> 
> Unfortunately our design doesn't use the PWM at all. It is only used for
> temperature measurements, so I'm not able to test any of the PWM code.

I requested samples for both LM63 and LM96163, so I should be able to
test that part.

Guenter

Re: [PATCH] hwmon: lm63: Add support for the NatSemi LM96163.

[Jean Delvare]

On Mon,  7 Nov 2011 11:20:58 +0100, Thierry Reding wrote:
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  drivers/hwmon/lm63.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 508cb29..94e373c 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -483,7 +483,7 @@ static int lm63_detect(struct i2c_client *new_client,
>  		return -ENODEV;
>  	}
>  
> -	if (chip_id == 0x41 && address == 0x4c)
> +	if ((chip_id == 0x41 || chip_id == 0x49) && address == 0x4c)
>  		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
>  	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
>  		strlcpy(info->type, "lm64", I2C_NAME_SIZE);

Oh, BTW... there's really no reason to reuse the "lm63" name when both
chips are not fully compatible. Please introduce a new name for the new
chip. It's fairly easy.

I've added detection support to sensors-detect:
http://dl.lm-sensors.org/lm-sensors/files/sensors-detect

I would appreciate if you could try it and report.

I would also appreciate if you could provide a dump of your chip's
registers (using the i2c-dev driver with i2cdump - part of the
i2c-tools package) for my collection.

-- 
Jean Delvare