[PATCH v2 1/6] platform/x86: serial-multi-instantiate: Improve autodetection

[PATCH v2 1/6] platform/x86: serial-multi-instantiate: Improve autodetection

[Andy Shevchenko]

Instead of calling specific resource counter, let just probe each
of the type and see what it says. Return -ENOENT if no resources
found.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: squashed patch 1, restored behaviour, added comment, dropped message (Hans)
 .../platform/x86/serial-multi-instantiate.c   | 23 ++++++++++++-------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
index 1e8063b7c169..366087a9fce2 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -100,7 +100,7 @@ static int smi_spi_probe(struct platform_device *pdev, struct acpi_device *adev,
 	if (ret < 0)
 		return ret;
 	else if (!ret)
-		return -ENODEV;
+		return -ENOENT;
 
 	count = ret;
 
@@ -184,7 +184,7 @@ static int smi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev,
 	if (ret < 0)
 		return ret;
 	else if (!ret)
-		return -ENODEV;
+		return -ENOENT;
 
 	count = ret;
 
@@ -232,6 +232,7 @@ static int smi_probe(struct platform_device *pdev)
 	const struct smi_node *node;
 	struct acpi_device *adev;
 	struct smi *smi;
+	int ret;
 
 	adev = ACPI_COMPANION(dev);
 	if (!adev)
@@ -255,15 +256,21 @@ static int smi_probe(struct platform_device *pdev)
 	case SMI_SPI:
 		return smi_spi_probe(pdev, adev, smi, node->instances);
 	case SMI_AUTO_DETECT:
-		if (i2c_acpi_client_count(adev) > 0)
-			return smi_i2c_probe(pdev, adev, smi, node->instances);
-		else
-			return smi_spi_probe(pdev, adev, smi, node->instances);
+		/*
+		 * For backwards-compatibility with the existing nodes I2C
+		 * is checked first and if such entries are found ONLY I2C
+		 * devices are created. Since some existing nodes that were
+		 * already handled by this driver could also contain unrelated
+		 * SpiSerialBus nodes that were previously ignored, and this
+		 * preserves that behavior.
+		 */
+		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
+		if (ret != -ENOENT)
+			return ret;
+		return smi_spi_probe(pdev, adev, smi, node->instances);
 	default:
 		return -EINVAL;
 	}
-
-	return 0; /* never reached */
 }
 
 static int smi_remove(struct platform_device *pdev)
-- 
2.35.1

[PATCH v2 2/6] platform/x86: serial-multi-instantiate: Drop duplicate check

[Andy Shevchenko]

The device_get_match_data() checks for firmware node to be present,
there is no need to check for ACPI companion.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: rebased on changed previous patch(es)
 .../platform/x86/serial-multi-instantiate.c   | 21 +++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
index 366087a9fce2..a1e04be858c5 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -81,16 +81,16 @@ static void smi_devs_unregister(struct smi *smi)
 /**
  * smi_spi_probe - Instantiate multiple SPI devices from inst array
  * @pdev:	Platform device
- * @adev:	ACPI device
  * @smi:	Internal struct for Serial multi instantiate driver
  * @inst_array:	Array of instances to probe
  *
  * Returns the number of SPI devices instantiate, Zero if none is found or a negative error code.
  */
-static int smi_spi_probe(struct platform_device *pdev, struct acpi_device *adev, struct smi *smi,
+static int smi_spi_probe(struct platform_device *pdev, struct smi *smi,
 			 const struct smi_instance *inst_array)
 {
 	struct device *dev = &pdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 	struct spi_controller *ctlr;
 	struct spi_device *spi_dev;
 	char name[50];
@@ -166,17 +166,17 @@ static int smi_spi_probe(struct platform_device *pdev, struct acpi_device *adev,
 /**
  * smi_i2c_probe - Instantiate multiple I2C devices from inst array
  * @pdev:	Platform device
- * @adev:	ACPI device
  * @smi:	Internal struct for Serial multi instantiate driver
  * @inst_array:	Array of instances to probe
  *
  * Returns the number of I2C devices instantiate, Zero if none is found or a negative error code.
  */
-static int smi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev, struct smi *smi,
+static int smi_i2c_probe(struct platform_device *pdev, struct smi *smi,
 			 const struct smi_instance *inst_array)
 {
 	struct i2c_board_info board_info = {};
 	struct device *dev = &pdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 	char name[32];
 	int i, ret, count;
 
@@ -230,14 +230,9 @@ static int smi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct smi_node *node;
-	struct acpi_device *adev;
 	struct smi *smi;
 	int ret;
 
-	adev = ACPI_COMPANION(dev);
-	if (!adev)
-		return -ENODEV;
-
 	node = device_get_match_data(dev);
 	if (!node) {
 		dev_dbg(dev, "Error ACPI match data is missing\n");
@@ -252,9 +247,9 @@ static int smi_probe(struct platform_device *pdev)
 
 	switch (node->bus_type) {
 	case SMI_I2C:
-		return smi_i2c_probe(pdev, adev, smi, node->instances);
+		return smi_i2c_probe(pdev, smi, node->instances);
 	case SMI_SPI:
-		return smi_spi_probe(pdev, adev, smi, node->instances);
+		return smi_spi_probe(pdev, smi, node->instances);
 	case SMI_AUTO_DETECT:
 		/*
 		 * For backwards-compatibility with the existing nodes I2C
@@ -264,10 +259,10 @@ static int smi_probe(struct platform_device *pdev)
 		 * SpiSerialBus nodes that were previously ignored, and this
 		 * preserves that behavior.
 		 */
-		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
+		ret = smi_i2c_probe(pdev, smi, node->instances);
 		if (ret != -ENOENT)
 			return ret;
-		return smi_spi_probe(pdev, adev, smi, node->instances);
+		return smi_spi_probe(pdev, smi, node->instances);
 	default:
 		return -EINVAL;
 	}
-- 
2.35.1

[PATCH v2 3/6] platform/x86: serial-multi-instantiate: Improve dev_err_probe() messaging

[Andy Shevchenko]

Drop duplicate print of returned value in the messages and use pattern
return dev_err_probe(...) where it's possible.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: added tag (Hans)
 drivers/platform/x86/serial-multi-instantiate.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
index a1e04be858c5..970ede0ff002 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -61,10 +61,9 @@ static int smi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
 	default:
 		return 0;
 	}
-
 	if (ret < 0)
-		dev_err_probe(&pdev->dev, ret, "Error requesting irq at index %d: %d\n",
-			      inst->irq_idx, ret);
+		return dev_err_probe(&pdev->dev, ret, "Error requesting irq at index %d\n",
+				     inst->irq_idx);
 
 	return ret;
 }
@@ -112,9 +111,8 @@ static int smi_spi_probe(struct platform_device *pdev, struct smi *smi,
 
 		spi_dev = acpi_spi_device_alloc(NULL, adev, i);
 		if (IS_ERR(spi_dev)) {
-			ret = PTR_ERR(spi_dev);
-			dev_err_probe(dev, ret, "failed to allocate SPI device %s from ACPI: %d\n",
-				      dev_name(&adev->dev), ret);
+			ret = dev_err_probe(dev, PTR_ERR(spi_dev), "failed to allocate SPI device %s from ACPI\n",
+					    dev_name(&adev->dev));
 			goto error;
 		}
 
@@ -135,9 +133,8 @@ static int smi_spi_probe(struct platform_device *pdev, struct smi *smi,
 
 		ret = spi_add_device(spi_dev);
 		if (ret) {
-			dev_err_probe(&ctlr->dev, ret,
-				      "failed to add SPI device %s from ACPI: %d\n",
-				      dev_name(&adev->dev), ret);
+			dev_err_probe(&ctlr->dev, ret, "failed to add SPI device %s from ACPI\n",
+				      dev_name(&adev->dev));
 			spi_dev_put(spi_dev);
 			goto error;
 		}
-- 
2.35.1

[PATCH v2 4/6] platform/x86: serial-multi-instantiate: Use while (i--) pattern to clean up

[Andy Shevchenko]

Use more natural while (i--) patter to clean up allocated resources.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: added tag (Hans)
 drivers/platform/x86/serial-multi-instantiate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
index 970ede0ff002..0a2335693f4f 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -70,11 +70,11 @@ static int smi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
 
 static void smi_devs_unregister(struct smi *smi)
 {
-	while (smi->i2c_num > 0)
-		i2c_unregister_device(smi->i2c_devs[--smi->i2c_num]);
+	while (smi->i2c_num--)
+		i2c_unregister_device(smi->i2c_devs[smi->i2c_num]);
 
-	while (smi->spi_num > 0)
-		spi_unregister_device(smi->spi_devs[--smi->spi_num]);
+	while (smi->spi_num--)
+		spi_unregister_device(smi->spi_devs[smi->spi_num]);
 }
 
 /**
-- 
2.35.1

[PATCH v2 5/6] platform/x86: serial-multi-instantiate: Get rid of redundant 'else'

[Andy Shevchenko]

In the snippets like the following

	if (...)
		return / goto / break / continue ...;
	else
		...

the 'else' is redundant. Get rid of it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: added tag (Hans)
 drivers/platform/x86/serial-multi-instantiate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
index 0a2335693f4f..24f915bbdec1 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -98,7 +98,7 @@ static int smi_spi_probe(struct platform_device *pdev, struct smi *smi,
 	ret = acpi_spi_count_resources(adev);
 	if (ret < 0)
 		return ret;
-	else if (!ret)
+	if (!ret)
 		return -ENOENT;
 
 	count = ret;
@@ -180,7 +180,7 @@ static int smi_i2c_probe(struct platform_device *pdev, struct smi *smi,
 	ret = i2c_acpi_client_count(adev);
 	if (ret < 0)
 		return ret;
-	else if (!ret)
+	if (!ret)
 		return -ENOENT;
 
 	count = ret;
-- 
2.35.1

[PATCH v2 6/6] platform/x86: serial-multi-instantiate: Sort ACPI IDs by HID

[Andy Shevchenko]

It's easier to maintain the sorted table.
Keep the sorting order in sync with one in drivers/acpi/scan.c.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: added tag (Hans)
 drivers/platform/x86/serial-multi-instantiate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
index 24f915bbdec1..67feed25c9db 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -324,8 +324,8 @@ static const struct smi_node cs35l41_hda = {
 static const struct acpi_device_id smi_acpi_ids[] = {
 	{ "BSG1160", (unsigned long)&bsg1160_data },
 	{ "BSG2150", (unsigned long)&bsg2150_data },
-	{ "INT3515", (unsigned long)&int3515_data },
 	{ "CSC3551", (unsigned long)&cs35l41_hda },
+	{ "INT3515", (unsigned long)&int3515_data },
 	/* Non-conforming _HID for Cirrus Logic already released */
 	{ "CLSA0100", (unsigned long)&cs35l41_hda },
 	{ }
-- 
2.35.1

Re: [PATCH v2 1/6] platform/x86: serial-multi-instantiate: Improve autodetection

[Hans de Goede]

Hi,

On 7/9/22 23:16, Andy Shevchenko wrote:
> Instead of calling specific resource counter, let just probe each
> of the type and see what it says. Return -ENOENT if no resources
> found.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
> v2: squashed patch 1, restored behaviour, added comment, dropped message (Hans)
>  .../platform/x86/serial-multi-instantiate.c   | 23 ++++++++++++-------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
> index 1e8063b7c169..366087a9fce2 100644
> --- a/drivers/platform/x86/serial-multi-instantiate.c
> +++ b/drivers/platform/x86/serial-multi-instantiate.c
> @@ -100,7 +100,7 @@ static int smi_spi_probe(struct platform_device *pdev, struct acpi_device *adev,
>  	if (ret < 0)
>  		return ret;
>  	else if (!ret)
> -		return -ENODEV;
> +		return -ENOENT;
>  
>  	count = ret;
>  
> @@ -184,7 +184,7 @@ static int smi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev,
>  	if (ret < 0)
>  		return ret;
>  	else if (!ret)
> -		return -ENODEV;
> +		return -ENOENT;
>  
>  	count = ret;
>  
> @@ -232,6 +232,7 @@ static int smi_probe(struct platform_device *pdev)
>  	const struct smi_node *node;
>  	struct acpi_device *adev;
>  	struct smi *smi;
> +	int ret;
>  
>  	adev = ACPI_COMPANION(dev);
>  	if (!adev)
> @@ -255,15 +256,21 @@ static int smi_probe(struct platform_device *pdev)
>  	case SMI_SPI:
>  		return smi_spi_probe(pdev, adev, smi, node->instances);
>  	case SMI_AUTO_DETECT:
> -		if (i2c_acpi_client_count(adev) > 0)
> -			return smi_i2c_probe(pdev, adev, smi, node->instances);
> -		else
> -			return smi_spi_probe(pdev, adev, smi, node->instances);
> +		/*
> +		 * For backwards-compatibility with the existing nodes I2C
> +		 * is checked first and if such entries are found ONLY I2C
> +		 * devices are created. Since some existing nodes that were
> +		 * already handled by this driver could also contain unrelated
> +		 * SpiSerialBus nodes that were previously ignored, and this
> +		 * preserves that behavior.
> +		 */
> +		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
> +		if (ret != -ENOENT)
> +			return ret;
> +		return smi_spi_probe(pdev, adev, smi, node->instances);
>  	default:
>  		return -EINVAL;
>  	}
> -
> -	return 0; /* never reached */
>  }
>  
>  static int smi_remove(struct platform_device *pdev)