| Message ID | 20231031085807.618827-1-Delphine_CC_Chiu@wiwynn.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] hwmon: emc1403: Add support for EMC1442 | expand |
On Tue, Oct 31, 2023 at 04:58:06PM +0800, Delphine CC Chiu wrote: > Add support for EMC1442 which is compatible with EMC1403. > Unfortunately, almost no information is available about this chip except that it ships in an 8-pin package. Are you sure the chip supports two external temperature sensors like EMC1403 ? The chip numbering and the number of pins would suggest that it only supports a single external temperature sensor, which would make it compatible to emc1402/emc1422. Guenter > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> > --- > drivers/hwmon/emc1403.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/emc1403.c b/drivers/hwmon/emc1403.c > index bb7c859e799d..c0d2e96c5077 100644 > --- a/drivers/hwmon/emc1403.c > +++ b/drivers/hwmon/emc1403.c > @@ -346,6 +346,9 @@ static int emc1403_detect(struct i2c_client *client, > case 0x27: > strscpy(info->type, "emc1424", I2C_NAME_SIZE); > break; > + case 0x60: > + strscpy(info->type, "emc1442", I2C_NAME_SIZE); > + break; > default: > return -ENODEV; > } > @@ -430,7 +433,7 @@ static int emc1403_probe(struct i2c_client *client) > } > > static const unsigned short emc1403_address_list[] = { > - 0x18, 0x1c, 0x29, 0x4c, 0x4d, 0x5c, I2C_CLIENT_END > + 0x18, 0x1c, 0x29, 0x3c, 0x4c, 0x4d, 0x5c, I2C_CLIENT_END > }; > > /* Last digit of chip name indicates number of channels */ > @@ -444,6 +447,7 @@ static const struct i2c_device_id emc1403_idtable[] = { > { "emc1422", emc1402 }, > { "emc1423", emc1403 }, > { "emc1424", emc1404 }, > + { "emc1442", emc1403 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, emc1403_idtable);
On Tue, Oct 31, 2023 at 04:01:10PM -0700, Guenter Roeck wrote: > On Tue, Oct 31, 2023 at 04:58:06PM +0800, Delphine CC Chiu wrote: > > Add support for EMC1442 which is compatible with EMC1403. > > > > Unfortunately, almost no information is available about this chip > except that it ships in an 8-pin package. Are you sure the chip > supports two external temperature sensors like EMC1403 ? > The chip numbering and the number of pins would suggest that > it only supports a single external temperature sensor, > which would make it compatible to emc1402/emc1422. > > Guenter The datasheet I've seen says: >> The EMC1442 monitors two temperature channels (one >> external and one internal). Based on this, I agree that emc1403 seems wrong. The datasheet also says: >> Pin compatible with EMC1412 > > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> > > --- > > { "emc1422", emc1402 }, > > { "emc1423", emc1403 }, > > { "emc1424", emc1404 }, > > + { "emc1442", emc1403 }, So, emc1402? > > { }
On 10/31/23 19:27, Patrick Williams wrote: > On Tue, Oct 31, 2023 at 04:01:10PM -0700, Guenter Roeck wrote: >> On Tue, Oct 31, 2023 at 04:58:06PM +0800, Delphine CC Chiu wrote: >>> Add support for EMC1442 which is compatible with EMC1403. >>> >> >> Unfortunately, almost no information is available about this chip >> except that it ships in an 8-pin package. Are you sure the chip >> supports two external temperature sensors like EMC1403 ? >> The chip numbering and the number of pins would suggest that >> it only supports a single external temperature sensor, >> which would make it compatible to emc1402/emc1422. >> >> Guenter > > The datasheet I've seen says: > >>> The EMC1442 monitors two temperature channels (one >>> external and one internal). > > Based on this, I agree that emc1403 seems wrong. The datasheet also > says: > >>> Pin compatible with EMC1412 > >> >>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> >>> --- >>> { "emc1422", emc1402 }, >>> { "emc1423", emc1403 }, >>> { "emc1424", emc1404 }, >>> + { "emc1442", emc1403 }, > > So, emc1402? > At the very least, if you are willing to confirm that formally if/when v2 is submitted. I previously rejected a similar patch adding emc1444 because it was impossible to get a datasheet to confirm that the chips are really register compatible. No idea why that has to be so secretive. It is a temperature sensor, for heaven's sake :-( Guenter
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Wednesday, November 1, 2023 12:57 PM > To: Patrick Williams <patrick@stwcx.xyz> > Cc: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>; > Jean Delvare <jdelvare@suse.com>; linux-hwmon@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v1] hwmon: emc1403: Add support for EMC1442 > > Security Reminder: Please be aware that this email is sent by an external > sender. > > On 10/31/23 19:27, Patrick Williams wrote: > > On Tue, Oct 31, 2023 at 04:01:10PM -0700, Guenter Roeck wrote: > >> On Tue, Oct 31, 2023 at 04:58:06PM +0800, Delphine CC Chiu wrote: > >>> Add support for EMC1442 which is compatible with EMC1403. > >>> > >> > >> Unfortunately, almost no information is available about this chip > >> except that it ships in an 8-pin package. Are you sure the chip > >> supports two external temperature sensors like EMC1403 ? > >> The chip numbering and the number of pins would suggest that it only > >> supports a single external temperature sensor, which would make it > >> compatible to emc1402/emc1422. > >> > >> Guenter > > > > The datasheet I've seen says: > > > >>> The EMC1442 monitors two temperature channels (one external and one > >>> internal). > > > > Based on this, I agree that emc1403 seems wrong. The datasheet also > > says: > > > >>> Pin compatible with EMC1412 > > > >> > >>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> > >>> --- > >>> { "emc1422", emc1402 }, > >>> { "emc1423", emc1403 }, > >>> { "emc1424", emc1404 }, > >>> + { "emc1442", emc1403 }, > > > > So, emc1402? > > > > At the very least, if you are willing to confirm that formally if/when v2 is > submitted. > > I previously rejected a similar patch adding emc1444 because it was impossible > to get a datasheet to confirm that the chips are really register compatible. No > idea why that has to be so secretive. It is a temperature sensor, for heaven's > sake :-( > > Guenter Hi Krish, Would need your confirmation of the device on the NIC.
On Tue, Oct 31, 2023 at 09:57:28PM -0700, Guenter Roeck wrote: > On 10/31/23 19:27, Patrick Williams wrote: > > On Tue, Oct 31, 2023 at 04:01:10PM -0700, Guenter Roeck wrote: > >> On Tue, Oct 31, 2023 at 04:58:06PM +0800, Delphine CC Chiu wrote: > >>> Add support for EMC1442 which is compatible with EMC1403. > >>> > >> > >> Unfortunately, almost no information is available about this chip > >> except that it ships in an 8-pin package. Are you sure the chip > >> supports two external temperature sensors like EMC1403 ? > >> The chip numbering and the number of pins would suggest that > >> it only supports a single external temperature sensor, > >> which would make it compatible to emc1402/emc1422. > >> > >> Guenter > > > > The datasheet I've seen says: > > > >>> The EMC1442 monitors two temperature channels (one > >>> external and one internal). > > > > Based on this, I agree that emc1403 seems wrong. The datasheet also > > says: > > > >>> Pin compatible with EMC1412 > > > >> > >>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> > >>> --- > >>> { "emc1422", emc1402 }, > >>> { "emc1423", emc1403 }, > >>> { "emc1424", emc1404 }, > >>> + { "emc1442", emc1403 }, > > > > So, emc1402? > > > > At the very least, if you are willing to confirm that formally > if/when v2 is submitted. Yes, I will confirm from the copy of the datasheet I have available when v2 comes up. > I previously rejected a similar patch adding emc1444 because it was > impossible to get a datasheet to confirm that the chips are really > register compatible. No idea why that has to be so secretive. It is a > temperature sensor, for heaven's sake :-( Delphine, could you reach out to the chip vendor and find out if they are willing to publish a datasheet for this chip? They might not have updated their website yet because EMC1442 shows up on Microchip's site but it has almost no information.
On Wed, Nov 01, 2023 at 08:45:15AM +0000, Delphine_CC_Chiu/WYHQ/Wiwynn wrote: > > >>> --- > > >>> { "emc1422", emc1402 }, > > >>> { "emc1423", emc1403 }, > > >>> { "emc1424", emc1404 }, > > >>> + { "emc1442", emc1403 }, > > > > > > So, emc1402? > > > > > > > At the very least, if you are willing to confirm that formally if/when v2 is > > submitted. > > > > I previously rejected a similar patch adding emc1444 because it was impossible > > to get a datasheet to confirm that the chips are really register compatible. No > > idea why that has to be so secretive. It is a temperature sensor, for heaven's > > sake :-( > > > > Guenter > > Hi Krish, > Would need your confirmation of the device on the NIC. I don't think we need additional feedback from Krish. Please change to emc1402 from emc1403. The datasheet for EMC1442 even has a section titled "Functional Delta from EMC1412 to EMC1442", which is effectively all non-software related. Therefore, the compatibility here should match what was done for "emc1412" which was `emc1402`. I will send my Reviewed-by with a v2 using emc1402.
diff --git a/drivers/hwmon/emc1403.c b/drivers/hwmon/emc1403.c index bb7c859e799d..c0d2e96c5077 100644 --- a/drivers/hwmon/emc1403.c +++ b/drivers/hwmon/emc1403.c @@ -346,6 +346,9 @@ static int emc1403_detect(struct i2c_client *client, case 0x27: strscpy(info->type, "emc1424", I2C_NAME_SIZE); break; + case 0x60: + strscpy(info->type, "emc1442", I2C_NAME_SIZE); + break; default: return -ENODEV; } @@ -430,7 +433,7 @@ static int emc1403_probe(struct i2c_client *client) } static const unsigned short emc1403_address_list[] = { - 0x18, 0x1c, 0x29, 0x4c, 0x4d, 0x5c, I2C_CLIENT_END + 0x18, 0x1c, 0x29, 0x3c, 0x4c, 0x4d, 0x5c, I2C_CLIENT_END }; /* Last digit of chip name indicates number of channels */ @@ -444,6 +447,7 @@ static const struct i2c_device_id emc1403_idtable[] = { { "emc1422", emc1402 }, { "emc1423", emc1403 }, { "emc1424", emc1404 }, + { "emc1442", emc1403 }, { } }; MODULE_DEVICE_TABLE(i2c, emc1403_idtable);
Add support for EMC1442 which is compatible with EMC1403. Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> --- drivers/hwmon/emc1403.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)