[PATCH 1/2] eeprom: ee1004: Call i2c_new_scanned_device to instantiate thermal sensor

[PATCH 1/2] eeprom: ee1004: Call i2c_new_scanned_device to instantiate thermal sensor

[Guenter Roeck]

Instantiating a device by calling i2c_new_client_device() assumes that the
device is not already instantiated. If that is not the case, it will return
an error and generate a misleading kernel log message.

i2c i2c-0: Failed to register i2c client jc42 at 0x18 (-16)

This can be reproduced by unloading the ee1004 driver and loading it again.

Avoid this by calling i2c_new_scanned_device() instead, which returns
silently if a device is already instantiated or does not exist.

Fixes: 393bd1000f81 ("eeprom: ee1004: add support for temperature sensor")
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/misc/eeprom/ee1004.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 21feebc3044c..71ca66d1df82 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -185,6 +185,8 @@ BIN_ATTRIBUTE_GROUPS(ee1004);
 static void ee1004_probe_temp_sensor(struct i2c_client *client)
 {
 	struct i2c_board_info info = { .type = "jc42" };
+	unsigned short addr = 0x18 | (client->addr & 7);
+	unsigned short addr_list[] = { addr, I2C_CLIENT_END };
 	u8 byte14;
 	int ret;
 
@@ -193,9 +195,7 @@ static void ee1004_probe_temp_sensor(struct i2c_client *client)
 	if (ret != 1 || !(byte14 & BIT(7)))
 		return;
 
-	info.addr = 0x18 | (client->addr & 7);
-
-	i2c_new_client_device(client->adapter, &info);
+	i2c_new_scanned_device(client->adapter, &info, addr_list, NULL);
 }
 
 static void ee1004_cleanup(int idx, struct ee1004_bus_data *bd)
-- 
2.39.2

[PATCH 2/2] eeprom: ee1004: Instantiate jc42 devices for DIMMS implementing Rev.1 SPD

[Guenter Roeck]

DDR4 DIMMS implementing SPD Annex L, Revision 1 do not implement SPD byte
14 (Module Temperature Sensor); this byte was only added in revision 2 of
the standard. This only applies to DDR4, not DDR4E or LPDDR4, since those
DDR types were only introduced in revision 3 of the standard.

Use this information to instantiate the jc42 device if the module is a DDR4
following SPD revision 1.0 and a device is detected at the expected thermal
sensor address, even if the Module Temperature Sensor byte suggests that
the thermal sensor is not supported.

Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/misc/eeprom/ee1004.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 71ca66d1df82..a39fefb066cd 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -187,14 +187,31 @@ static void ee1004_probe_temp_sensor(struct i2c_client *client)
 	struct i2c_board_info info = { .type = "jc42" };
 	unsigned short addr = 0x18 | (client->addr & 7);
 	unsigned short addr_list[] = { addr, I2C_CLIENT_END };
-	u8 byte14;
+	u8 data[2];
 	int ret;
 
 	/* byte 14, bit 7 is set if temp sensor is present */
-	ret = ee1004_eeprom_read(client, &byte14, 14, 1);
-	if (ret != 1 || !(byte14 & BIT(7)))
+	ret = ee1004_eeprom_read(client, data, 14, 1);
+	if (ret != 1)
 		return;
 
+	if (!(data[0] & BIT(7))) {
+		/*
+		 * If the SPD data suggests that there is no temperature
+		 * sensor, it may still be there for SPD revision 1.0.
+		 * See SPD Annex L, Revision 1 and 2, for details.
+		 * Check DIMM type and SPD revision; if it is a DDR4
+		 * with SPD revision 1.0, check the thermal sensor address
+		 * and instantiate the jc42 driver if a chip is found at
+		 * that address.
+		 * It is not necessary to check if there is a chip at the
+		 * temperature sensor address since i2c_new_scanned_device()
+		 * will do that and return silently if no chip is found.
+		 */
+		ret = ee1004_eeprom_read(client, data, 1, 2);
+		if (ret != 2 || data[0] != 0x10 || data[1] != 0x0c)
+			return;
+	}
 	i2c_new_scanned_device(client->adapter, &info, addr_list, NULL);
 }
 
-- 
2.39.2

Re: [PATCH 1/2] eeprom: ee1004: Call i2c_new_scanned_device to instantiate thermal sensor

[Heiner Kallweit]

On 29.06.2024 19:37, Guenter Roeck wrote:
> Instantiating a device by calling i2c_new_client_device() assumes that the
> device is not already instantiated. If that is not the case, it will return
> an error and generate a misleading kernel log message.
> 
> i2c i2c-0: Failed to register i2c client jc42 at 0x18 (-16)
> 
> This can be reproduced by unloading the ee1004 driver and loading it again.
> 
> Avoid this by calling i2c_new_scanned_device() instead, which returns
> silently if a device is already instantiated or does not exist.
> 
However i2c_new_scanned_device() runs i2c_default_probe() on the device,
whilst i2c_new_client_device() doesn't access the i2c bus.
If possible I'd like to avoid this overhead.

> Fixes: 393bd1000f81 ("eeprom: ee1004: add support for temperature sensor")
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/misc/eeprom/ee1004.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
> index 21feebc3044c..71ca66d1df82 100644
> --- a/drivers/misc/eeprom/ee1004.c
> +++ b/drivers/misc/eeprom/ee1004.c
> @@ -185,6 +185,8 @@ BIN_ATTRIBUTE_GROUPS(ee1004);
>  static void ee1004_probe_temp_sensor(struct i2c_client *client)
>  {
>  	struct i2c_board_info info = { .type = "jc42" };
> +	unsigned short addr = 0x18 | (client->addr & 7);
> +	unsigned short addr_list[] = { addr, I2C_CLIENT_END };
>  	u8 byte14;
>  	int ret;
>  
> @@ -193,9 +195,7 @@ static void ee1004_probe_temp_sensor(struct i2c_client *client)
>  	if (ret != 1 || !(byte14 & BIT(7)))
>  		return;
>  
> -	info.addr = 0x18 | (client->addr & 7);
> -
> -	i2c_new_client_device(client->adapter, &info);
> +	i2c_new_scanned_device(client->adapter, &info, addr_list, NULL);
>  }
>  
>  static void ee1004_cleanup(int idx, struct ee1004_bus_data *bd)

Re: [PATCH 1/2] eeprom: ee1004: Call i2c_new_scanned_device to instantiate thermal sensor

[Guenter Roeck]

On 6/29/24 15:00, Heiner Kallweit wrote:
> On 29.06.2024 19:37, Guenter Roeck wrote:
>> Instantiating a device by calling i2c_new_client_device() assumes that the
>> device is not already instantiated. If that is not the case, it will return
>> an error and generate a misleading kernel log message.
>>
>> i2c i2c-0: Failed to register i2c client jc42 at 0x18 (-16)
>>
>> This can be reproduced by unloading the ee1004 driver and loading it again.
>>
>> Avoid this by calling i2c_new_scanned_device() instead, which returns
>> silently if a device is already instantiated or does not exist.
>>
> However i2c_new_scanned_device() runs i2c_default_probe() on the device,
> whilst i2c_new_client_device() doesn't access the i2c bus.
> If possible I'd like to avoid this overhead.
> 

I am not as much concerned with that overhead since there are already several
other i2c accesses in both this driver and in the jc42 driver, so it seemed to
me that another quick operation does not make much of a difference. Also, I
wanted to suggest a solution which does not require changing the i2c core.
However, I am not "married" to this solution. Please feel free to suggest
something different, or to drop this patch (and the next one) entirely.

Thanks,
Guenter