2020-06-24 11:27:17

by Ricardo Ribalda Delgado

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

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 <[email protected]>
---
v2:
Changes by Andy Shevchenko <[email protected]>:
- CodeStyle
Changes by kernel test robot <[email protected]>
- 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


2020-06-24 13:26:31

by Andy Shevchenko

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

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


2020-06-24 13:49:15

by Jarkko Nikula

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

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

2020-06-25 13:23:12

by Jarkko Nikula

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

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 <[email protected]>
> ---
> v2:
> Changes by Andy Shevchenko <[email protected]>:
> - CodeStyle
> Changes by kernel test robot <[email protected]>
> - 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