[PATCH v2] i2c: designware: platdrv: Set class based on dmi

[PATCH v2] i2c: designware: platdrv: Set class based on dmi

[Ricardo Ribalda]

Current AMD's zen-based APUs use this core for some of its i2c-buses.

With this patch we re-enable autodetection of hwmon-alike devices, so
lm-sensors will be able to work automatically.

It does not affect the boot-time of embedded devices, as the class is
set based on the dmi information.

Signed-off-by: Ricardo Ribalda <ribalda@kernel.org>
---
v2:
Changes by Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
 - CodeStyle
Changes by kernel test robot <lkp@intel.com>
 - Include dmi header to fix build error on arc
 - check if dmi_get_system_info returned NULL
 drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index c2efaaaac252..5892fdba9c25 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -12,6 +12,7 @@
 #include <linux/clk-provider.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
@@ -173,6 +174,19 @@ static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
 		pm_runtime_put_noidle(dev->dev);
 }
 
+static bool dw_i2c_hwmon_bus(void)
+{
+	const char *product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+
+	if (!product_name)
+		return false;
+
+	if (strstr(product_name, "QT5222"))
+		return true;
+
+	return false;
+}
+
 static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev->dev);
@@ -267,7 +281,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 
 	adap = &dev->adapter;
 	adap->owner = THIS_MODULE;
-	adap->class = I2C_CLASS_DEPRECATED;
+	adap->class = dw_i2c_hwmon_bus() ? I2C_CLASS_HWMON : I2C_CLASS_DEPRECATED;
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 	adap->nr = -1;
-- 
2.27.0

Re: [PATCH v2] i2c: designware: platdrv: Set class based on dmi

[Andy Shevchenko]

On Wed, Jun 24, 2020 at 01:25:30PM +0200, Ricardo Ribalda wrote:
> Current AMD's zen-based APUs use this core for some of its i2c-buses.
> 
> With this patch we re-enable autodetection of hwmon-alike devices, so
> lm-sensors will be able to work automatically.
> 
> It does not affect the boot-time of embedded devices, as the class is
> set based on the dmi information.

I think it misses Fixes tag. And...

...

> +static bool dw_i2c_hwmon_bus(void)
> +{

> +	const char *product_name = dmi_get_system_info(DMI_PRODUCT_NAME);

Split this, so the assignment will be attached to the check below.

> +	if (!product_name)
> +		return false;
> +

> +	if (strstr(product_name, "QT5222"))
> +		return true;

I don't like this part at all. Why do you need strstr()? Can you provide in the
commit message relevant fields from dmidecode (or sysfs)?

> +	return false;
> +}

In general it's not how we do DMI based quirks, rather using table and call
match function. In that case you can take class as a driver_data. Much more
flexible in case we need to extend.

...

> -	adap->class = I2C_CLASS_DEPRECATED;
> +	adap->class = dw_i2c_hwmon_bus() ? I2C_CLASS_HWMON : I2C_CLASS_DEPRECATED;

...since the patch has it unconditionally, I would go unconditionally. But if
Wolfram insists, then see above.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] i2c: designware: platdrv: Set class based on dmi

[Jarkko Nikula]

On 6/24/20 4:22 PM, Andy Shevchenko wrote:
> On Wed, Jun 24, 2020 at 01:25:30PM +0200, Ricardo Ribalda wrote:
>> Current AMD's zen-based APUs use this core for some of its i2c-buses.
>>
>> With this patch we re-enable autodetection of hwmon-alike devices, so
>> lm-sensors will be able to work automatically.
>>
>> It does not affect the boot-time of embedded devices, as the class is
>> set based on the dmi information.
> 
> I think it misses Fixes tag. And...
> 
I don't think we have regression here. Commit 70fba8302ade ("i2c: 
i2c-designware-platdrv: Drop class based scanning to improve bootup 
time") was done before any of those AMD ACPI IDs were added.

-- 
Jarkko

Re: [PATCH v2] i2c: designware: platdrv: Set class based on dmi

[Jarkko Nikula]

On 6/24/20 2:25 PM, Ricardo Ribalda wrote:
> Current AMD's zen-based APUs use this core for some of its i2c-buses.
> 
> With this patch we re-enable autodetection of hwmon-alike devices, so
> lm-sensors will be able to work automatically.
> 
> It does not affect the boot-time of embedded devices, as the class is
> set based on the dmi information.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@kernel.org>
> ---
> v2:
> Changes by Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
>   - CodeStyle
> Changes by kernel test robot <lkp@intel.com>
>   - Include dmi header to fix build error on arc
>   - check if dmi_get_system_info returned NULL
>   drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index c2efaaaac252..5892fdba9c25 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -12,6 +12,7 @@
>   #include <linux/clk-provider.h>
>   #include <linux/clk.h>
>   #include <linux/delay.h>
> +#include <linux/dmi.h>
>   #include <linux/err.h>
>   #include <linux/errno.h>
>   #include <linux/i2c.h>
> @@ -173,6 +174,19 @@ static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
>   		pm_runtime_put_noidle(dev->dev);
>   }
>   
> +static bool dw_i2c_hwmon_bus(void)
> +{
> +	const char *product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> +
> +	if (!product_name)
> +		return false;
> +
> +	if (strstr(product_name, "QT5222"))
> +		return true;
> +
> +	return false;
> +}
> +

I'm not too familiar with the DMI stuff but is the more usual way to 
have struct dmi_system_id table and match it with dmi_check_system()? 
Perhaps scales better than individual dmi_get_system_info() and string 
comparison calls.

Andy and I were pondering offline is there any info in ACPI table that 
tells which bus have these sensors or can it be hardcoded with the DMI 
match so that there is no need probe all buses for these sensors on that 
product. But that can be another optimization patch I guess.

-- 
Jarkko