[PATCH v2 0/2] hwmon: (oxp-sensors) Refactor probe() and init() and remove devm_add_groups()

[PATCH v2 0/2] hwmon: (oxp-sensors) Refactor probe() and init() and remove devm_add_groups()

[Joaquín Ignacio Aramendía]

Remove the use of devm_add_groups() in favour of dev_groups in platform
driver structure. This will allow for removal of the function as it was
intended in Greg's email[1].

Also move board detection to the init() instead of probe() function so we
don't instantiate the driver if the detection fails.

V2 drops the 3rd patch that removed the probe() function.

[1] Link: https://lore.kernel.org/linux-hwmon/ZKW7WuP0T9QdCR+G@google.com/

Joaquín Ignacio Aramendía (2):
  hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups
  hwmon: (oxp-sensors) Move board detection to the init function

 drivers/hwmon/oxp-sensors.c | 67 +++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 28 deletions(-)

-- 
2.41.0

[PATCH v2 1/2] hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups

[Joaquín Ignacio Aramendía]

A driver should not be manually adding groups in its probe function (it will
race with userspace), so replace the call to devm_device_add_groups() to use
the platform dev_groups callback instead.

This will allow for removal of the devm_device_add_groups() function.

Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
---
 drivers/hwmon/oxp-sensors.c | 38 +++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index e1a907cae820..1e1cc67bcdea 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -220,6 +220,20 @@ static int tt_toggle_disable(void)
 }
 
 /* Callbacks for turbo toggle attribute */
+static umode_t tt_toggle_is_visible(struct kobject *kobj,
+				    struct attribute *attr, int n)
+{
+	switch (board) {
+	case aok_zoe_a1:
+	case oxp_mini_amd_a07:
+	case oxp_mini_amd_pro:
+		return attr->mode;
+	default:
+		break;
+	}
+	return 0;
+}
+
 static ssize_t tt_toggle_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t count)
@@ -396,7 +410,15 @@ static struct attribute *oxp_ec_attrs[] = {
 	NULL
 };
 
-ATTRIBUTE_GROUPS(oxp_ec);
+static struct attribute_group oxp_ec_attribute_group = {
+	.is_visible = tt_toggle_is_visible,
+	.attrs = oxp_ec_attrs,
+};
+
+static const struct attribute_group *oxp_ec_groups[] = {
+	&oxp_ec_attribute_group,
+	NULL
+};
 
 static const struct hwmon_ops oxp_ec_hwmon_ops = {
 	.is_visible = oxp_ec_hwmon_is_visible,
@@ -415,7 +437,6 @@ static int oxp_platform_probe(struct platform_device *pdev)
 	const struct dmi_system_id *dmi_entry;
 	struct device *dev = &pdev->dev;
 	struct device *hwdev;
-	int ret;
 
 	/*
 	 * Have to check for AMD processor here because DMI strings are the
@@ -430,18 +451,6 @@ static int oxp_platform_probe(struct platform_device *pdev)
 
 	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
 
-	switch (board) {
-	case aok_zoe_a1:
-	case oxp_mini_amd_a07:
-	case oxp_mini_amd_pro:
-		ret = devm_device_add_groups(dev, oxp_ec_groups);
-		if (ret)
-			return ret;
-		break;
-	default:
-		break;
-	}
-
 	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
 						     &oxp_ec_chip_info, NULL);
 
@@ -451,6 +460,7 @@ static int oxp_platform_probe(struct platform_device *pdev)
 static struct platform_driver oxp_platform_driver = {
 	.driver = {
 		.name = "oxp-platform",
+		.dev_groups = oxp_ec_groups,
 	},
 	.probe = oxp_platform_probe,
 };
-- 
2.41.0

Re: [PATCH v2 1/2] hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups

[Greg KH]

On Mon, Jul 17, 2023 at 07:25:15PM -0300, Joaquín Ignacio Aramendía wrote:
> A driver should not be manually adding groups in its probe function (it will
> race with userspace), so replace the call to devm_device_add_groups() to use
> the platform dev_groups callback instead.
> 
> This will allow for removal of the devm_device_add_groups() function.
> 
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> ---
>  drivers/hwmon/oxp-sensors.c | 38 +++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2 1/2] hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups

[Guenter Roeck]

On Mon, Jul 17, 2023 at 07:25:15PM -0300, Joaquín Ignacio Aramendía wrote:
> A driver should not be manually adding groups in its probe function (it will
> race with userspace), so replace the call to devm_device_add_groups() to use
> the platform dev_groups callback instead.
> 
> This will allow for removal of the devm_device_add_groups() function.
> 
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/oxp-sensors.c | 38 +++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index e1a907cae820..1e1cc67bcdea 100644
> --- a/drivers/hwmon/oxp-sensors.c
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -220,6 +220,20 @@ static int tt_toggle_disable(void)
>  }
>  
>  /* Callbacks for turbo toggle attribute */
> +static umode_t tt_toggle_is_visible(struct kobject *kobj,
> +				    struct attribute *attr, int n)
> +{
> +	switch (board) {
> +	case aok_zoe_a1:
> +	case oxp_mini_amd_a07:
> +	case oxp_mini_amd_pro:
> +		return attr->mode;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
>  static ssize_t tt_toggle_store(struct device *dev,
>  			       struct device_attribute *attr, const char *buf,
>  			       size_t count)
> @@ -396,7 +410,15 @@ static struct attribute *oxp_ec_attrs[] = {
>  	NULL
>  };
>  
> -ATTRIBUTE_GROUPS(oxp_ec);
> +static struct attribute_group oxp_ec_attribute_group = {
> +	.is_visible = tt_toggle_is_visible,
> +	.attrs = oxp_ec_attrs,
> +};
> +
> +static const struct attribute_group *oxp_ec_groups[] = {
> +	&oxp_ec_attribute_group,
> +	NULL
> +};
>  
>  static const struct hwmon_ops oxp_ec_hwmon_ops = {
>  	.is_visible = oxp_ec_hwmon_is_visible,
> @@ -415,7 +437,6 @@ static int oxp_platform_probe(struct platform_device *pdev)
>  	const struct dmi_system_id *dmi_entry;
>  	struct device *dev = &pdev->dev;
>  	struct device *hwdev;
> -	int ret;
>  
>  	/*
>  	 * Have to check for AMD processor here because DMI strings are the
> @@ -430,18 +451,6 @@ static int oxp_platform_probe(struct platform_device *pdev)
>  
>  	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
>  
> -	switch (board) {
> -	case aok_zoe_a1:
> -	case oxp_mini_amd_a07:
> -	case oxp_mini_amd_pro:
> -		ret = devm_device_add_groups(dev, oxp_ec_groups);
> -		if (ret)
> -			return ret;
> -		break;
> -	default:
> -		break;
> -	}
> -
>  	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
>  						     &oxp_ec_chip_info, NULL);
>  
> @@ -451,6 +460,7 @@ static int oxp_platform_probe(struct platform_device *pdev)
>  static struct platform_driver oxp_platform_driver = {
>  	.driver = {
>  		.name = "oxp-platform",
> +		.dev_groups = oxp_ec_groups,
>  	},
>  	.probe = oxp_platform_probe,
>  };

[PATCH v2 2/2] hwmon: (oxp-sensors) Move board detection to the init function

[Joaquín Ignacio Aramendía]

Move detection logic to the start of init() function so we won't
instantiate the driver if the board is not compatible.

Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
---
 drivers/hwmon/oxp-sensors.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index 1e1cc67bcdea..ea9602063eab 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -434,23 +434,9 @@ static const struct hwmon_chip_info oxp_ec_chip_info = {
 /* Initialization logic */
 static int oxp_platform_probe(struct platform_device *pdev)
 {
-	const struct dmi_system_id *dmi_entry;
 	struct device *dev = &pdev->dev;
 	struct device *hwdev;
 
-	/*
-	 * Have to check for AMD processor here because DMI strings are the
-	 * same between Intel and AMD boards, the only way to tell them apart
-	 * is the CPU.
-	 * Intel boards seem to have different EC registers and values to
-	 * read/write.
-	 */
-	dmi_entry = dmi_first_match(dmi_table);
-	if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
-		return -ENODEV;
-
-	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
-
 	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
 						     &oxp_ec_chip_info, NULL);
 
@@ -469,6 +455,21 @@ static struct platform_device *oxp_platform_device;
 
 static int __init oxp_platform_init(void)
 {
+	const struct dmi_system_id *dmi_entry;
+
+	/*
+	 * Have to check for AMD processor here because DMI strings are the
+	 * same between Intel and AMD boards, the only way to tell them apart
+	 * is the CPU.
+	 * Intel boards seem to have different EC registers and values to
+	 * read/write.
+	 */
+	dmi_entry = dmi_first_match(dmi_table);
+	if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return -ENODEV;
+
+	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
+
 	oxp_platform_device =
 		platform_create_bundle(&oxp_platform_driver,
 				       oxp_platform_probe, NULL, 0, NULL, 0);
-- 
2.41.0

Re: [PATCH v2 2/2] hwmon: (oxp-sensors) Move board detection to the init function

[Greg KH]

On Mon, Jul 17, 2023 at 07:25:16PM -0300, Joaquín Ignacio Aramendía wrote:
> Move detection logic to the start of init() function so we won't
> instantiate the driver if the board is not compatible.
> 
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> ---
>  drivers/hwmon/oxp-sensors.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2 2/2] hwmon: (oxp-sensors) Move board detection to the init function

[Guenter Roeck]

On Mon, Jul 17, 2023 at 07:25:16PM -0300, Joaquín Ignacio Aramendía wrote:
> Move detection logic to the start of init() function so we won't
> instantiate the driver if the board is not compatible.
> 
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied.

Thanks,
Guenter