[PATCH v1 1/7] platform/x86: serial-multi-instantiate: return -ENOENT when no resources found

[PATCH v1 1/7] platform/x86: serial-multi-instantiate: return -ENOENT when no resources found

[Andy Shevchenko]

Distinguish the case when device is present, but there are no resources.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 1e8063b7c169..97db23243018 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;
 
-- 
2.35.1

[PATCH v1 2/7] 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. Also add a debug message when
none is found.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/serial-multi-instantiate.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
index 97db23243018..e599058196bb 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -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,20 @@ 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);
+		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
+		if (ret && ret != -ENOENT)
+			return ret;
+		ret = smi_spi_probe(pdev, adev, smi, node->instances);
+		if (ret && ret != -ENOENT)
+			return ret;
+		if (ret)
+			return dev_err_probe(dev, ret, "Error No resources found\n");
+		break;
 	default:
 		return -EINVAL;
 	}
 
-	return 0; /* never reached */
+	return 0;
 }
 
 static int smi_remove(struct platform_device *pdev)
-- 
2.35.1

[PATCH v1 3/7] 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>
---
 .../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 e599058196bb..ceb0e414b9f5 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,14 +247,14 @@ 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:
-		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
+		ret = smi_i2c_probe(pdev, smi, node->instances);
 		if (ret && ret != -ENOENT)
 			return ret;
-		ret = smi_spi_probe(pdev, adev, smi, node->instances);
+		ret = smi_spi_probe(pdev, smi, node->instances);
 		if (ret && ret != -ENOENT)
 			return ret;
 		if (ret)
-- 
2.35.1

[PATCH v1 4/7] 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>
---
 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 ceb0e414b9f5..520236b11b2c 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 v1 5/7] 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>
---
 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 520236b11b2c..38bc644a50c3 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 v1 6/7] 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>
---
 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 38bc644a50c3..3f8fc80ec9cc 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 v1 7/7] 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>
---
 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 3f8fc80ec9cc..249570491d4a 100644
--- a/drivers/platform/x86/serial-multi-instantiate.c
+++ b/drivers/platform/x86/serial-multi-instantiate.c
@@ -323,8 +323,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 v1 1/7] platform/x86: serial-multi-instantiate: return -ENOENT when no resources found

[Hans de Goede]

Hi Andy,

On 7/9/22 02:06, Andy Shevchenko wrote:
> Distinguish the case when device is present, but there are no resources.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

IMHO it would have been better to squash this together with patch 2/7,
but think patch 2/7 should be dropped, so this one should be dropped
to IMHO.

Regards,

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 1e8063b7c169..97db23243018 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;
>  

Re: [PATCH v1 2/7] platform/x86: serial-multi-instantiate: Improve autodetection

[Hans de Goede]

Hi,

On 7/9/22 02:06, Andy Shevchenko wrote:
> Instead of calling specific resource counter, let just probe each
> of the type and see what it says. Also add a debug message when
> none is found.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Only probing for I2C resources if some are present is deliberate:

commit 68f201f9061c000d7a4a9f359f021b1cd535d62b
Author: Stefan Binding <sbinding@opensource.cirrus.com>
Date:   Fri Jan 21 17:24:29 2022 +0000

    platform/x86: serial-multi-instantiate: Add SPI support
    
    Add support for spi bus in serial-multi-instantiate driver
    
    Some peripherals can have either a I2C or a SPI connection
    to the host (but not both) but use the same HID for both
    types. So it is not possible to use the HID to determine
    whether it is I2C or SPI. The driver must check the node
    to see if it contains I2cSerialBus or SpiSerialBus entries.
    
    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. If there is ever a need to handle
    a node where both I2C and SPI devices must be instantiated
    this can be added in future.
    
    Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
    Link: https://lore.kernel.org/r/20220121172431.6876-8-sbinding@opensource.cirrus.com
    Reviewed-by: Hans de Goede <hdegoede@redhat.com>
    Signed-off-by: Hans de Goede <hdegoede@redhat.com>

So nack for this change.

Regards,

Hans



> ---
>  drivers/platform/x86/serial-multi-instantiate.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
> index 97db23243018..e599058196bb 100644
> --- a/drivers/platform/x86/serial-multi-instantiate.c
> +++ b/drivers/platform/x86/serial-multi-instantiate.c
> @@ -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,20 @@ 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);
> +		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
> +		if (ret && ret != -ENOENT)
> +			return ret;
> +		ret = smi_spi_probe(pdev, adev, smi, node->instances);
> +		if (ret && ret != -ENOENT)
> +			return ret;
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Error No resources found\n");
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	return 0; /* never reached */
> +	return 0;
>  }
>  
>  static int smi_remove(struct platform_device *pdev)

Re: [PATCH v1 2/7] platform/x86: serial-multi-instantiate: Improve autodetection

[Hans de Goede]

<resend with Stefan added to the To list>

Hi,

On 7/9/22 02:06, Andy Shevchenko wrote:
> Instead of calling specific resource counter, let just probe each
> of the type and see what it says. Also add a debug message when
> none is found.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Only probing for I2C resources if some are present is deliberate:

commit 68f201f9061c000d7a4a9f359f021b1cd535d62b
Author: Stefan Binding <sbinding@opensource.cirrus.com>
Date:   Fri Jan 21 17:24:29 2022 +0000

    platform/x86: serial-multi-instantiate: Add SPI support
    
    Add support for spi bus in serial-multi-instantiate driver
    
    Some peripherals can have either a I2C or a SPI connection
    to the host (but not both) but use the same HID for both
    types. So it is not possible to use the HID to determine
    whether it is I2C or SPI. The driver must check the node
    to see if it contains I2cSerialBus or SpiSerialBus entries.
    
    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. If there is ever a need to handle
    a node where both I2C and SPI devices must be instantiated
    this can be added in future.
    
    Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
    Link: https://lore.kernel.org/r/20220121172431.6876-8-sbinding@opensource.cirrus.com
    Reviewed-by: Hans de Goede <hdegoede@redhat.com>
    Signed-off-by: Hans de Goede <hdegoede@redhat.com>

So nack for this change.

Regards,

Hans



> ---
>  drivers/platform/x86/serial-multi-instantiate.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
> index 97db23243018..e599058196bb 100644
> --- a/drivers/platform/x86/serial-multi-instantiate.c
> +++ b/drivers/platform/x86/serial-multi-instantiate.c
> @@ -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,20 @@ 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);
> +		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
> +		if (ret && ret != -ENOENT)
> +			return ret;
> +		ret = smi_spi_probe(pdev, adev, smi, node->instances);
> +		if (ret && ret != -ENOENT)
> +			return ret;
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Error No resources found\n");
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	return 0; /* never reached */
> +	return 0;
>  }
>  
>  static int smi_remove(struct platform_device *pdev)

Re: [PATCH v1 3/7] platform/x86: serial-multi-instantiate: Drop duplicate check

[Hans de Goede]

Hi,

On 7/9/22 02:06, Andy Shevchenko wrote:
> 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>

Patches 3-7 look good to me, you may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

after rebasing to drop patches 1 + 2.

Regards,

Hans

> ---
>  .../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 e599058196bb..ceb0e414b9f5 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,14 +247,14 @@ 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:
> -		ret = smi_i2c_probe(pdev, adev, smi, node->instances);
> +		ret = smi_i2c_probe(pdev, smi, node->instances);
>  		if (ret && ret != -ENOENT)
>  			return ret;
> -		ret = smi_spi_probe(pdev, adev, smi, node->instances);
> +		ret = smi_spi_probe(pdev, smi, node->instances);
>  		if (ret && ret != -ENOENT)
>  			return ret;
>  		if (ret)

Re: [PATCH v1 2/7] platform/x86: serial-multi-instantiate: Improve autodetection

[Hans de Goede]

Hi,

On 7/9/22 11:52, Andy Shevchenko wrote:
> 
> 
> On Saturday, July 9, 2022, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> 
>     Hi,
> 
>     On 7/9/22 02:06, Andy Shevchenko wrote:
>     > Instead of calling specific resource counter, let just probe each
>     > of the type and see what it says. Also add a debug message when
>     > none is found.
>     >
>     > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com <mailto:andriy.shevchenko@linux.intel.com>>
> 
>     Only probing for I2C resources if some are present is deliberate:
> 
>     commit 68f201f9061c000d7a4a9f359f021b1cd535d62b
>     Author: Stefan Binding <sbinding@opensource.cirrus.com <mailto:sbinding@opensource.cirrus.com>>
>     Date:   Fri Jan 21 17:24:29 2022 +0000
> 
>         platform/x86: serial-multi-instantiate: Add SPI support
> 
>         Add support for spi bus in serial-multi-instantiate driver
> 
>         Some peripherals can have either a I2C or a SPI connection
>         to the host (but not both) but use the same HID for both
>         types. So it is not possible to use the HID to determine
>         whether it is I2C or SPI. The driver must check the node
>         to see if it contains I2cSerialBus or SpiSerialBus entries.
> 
>         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. If there is ever a need to handle
>         a node where both I2C and SPI devices must be instantiated
>         this can be added in future.
> 
>         Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com <mailto:sbinding@opensource.cirrus.com>>
>         Link: https://lore.kernel.org/r/20220121172431.6876-8-sbinding@opensource.cirrus.com <https://lore.kernel.org/r/20220121172431.6876-8-sbinding@opensource.cirrus.com>
>         Reviewed-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>         Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
> 
>     So nack for this 
> 
> 
> 
> This effectively means nack to the series.
> But it’s easy to fix. I can add check for ret == 0.

I don't see how this is a nack for the series, just drop 1/7 + 2/7
and rebase the rest. Yes there will be conflicts to resolve in
the rebase, but the rest of the cleanups can still go upstream
after the rebase.

Regards,

Hans



>     > ---
>     >  drivers/platform/x86/serial-multi-instantiate.c | 16 +++++++++++-----
>     >  1 file changed, 11 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c
>     > index 97db23243018..e599058196bb 100644
>     > --- a/drivers/platform/x86/serial-multi-instantiate.c
>     > +++ b/drivers/platform/x86/serial-multi-instantiate.c
>     > @@ -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,20 @@ 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);
>     > +             ret = smi_i2c_probe(pdev, adev, smi, node->instances);
>     > +             if (ret && ret != -ENOENT)
>     > +                     return ret;
>     > +             ret = smi_spi_probe(pdev, adev, smi, node->instances);
>     > +             if (ret && ret != -ENOENT)
>     > +                     return ret;
>     > +             if (ret)
>     > +                     return dev_err_probe(dev, ret, "Error No resources found\n");
>     > +             break;
>     >       default:
>     >               return -EINVAL;
>     >       }
>     > 
>     > -     return 0; /* never reached */
>     > +     return 0;
>     >  }
>     > 
>     >  static int smi_remove(struct platform_device *pdev)
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 2/7] platform/x86: serial-multi-instantiate: Improve autodetection

[Andy Shevchenko]

On Sat, Jul 9, 2022 at 1:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 7/9/22 11:52, Andy Shevchenko wrote:
> > On Saturday, July 9, 2022, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> >     On 7/9/22 02:06, Andy Shevchenko wrote:

...

> >     So nack for this
> >
> > This effectively means nack to the series.

> > But it’s easy to fix. I can add check for ret == 0.

So, are you okay with fixing it this way? See below how.

> I don't see how this is a nack for the series, just drop 1/7 + 2/7
> and rebase the rest. Yes there will be conflicts to resolve in
> the rebase, but the rest of the cleanups can still go upstream
> after the rebase.

Because patch 3 makes a little sense on its own if we drop the patch
2. The rest is the simple cleanups which I do not consider as a core
of this series.

> >     >       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);
> >     > +             ret = smi_i2c_probe(pdev, adev, smi, node->instances);
> >     > +             if (ret && ret != -ENOENT)
> >     > +                     return ret;

/*
 * ...comment about why we do the following check...
 */
 if (ret == 0)
  return ret;

> >     > +             ret = smi_spi_probe(pdev, adev, smi, node->instances);
> >     > +             if (ret && ret != -ENOENT)
> >     > +                     return ret;
> >     > +             if (ret)
> >     > +                     return dev_err_probe(dev, ret, "Error No resources found\n");
> >     > +             break;

if (ret == -ENOENT)
  return dev_err_probe(...);
return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/7] platform/x86: serial-multi-instantiate: Improve autodetection

[Hans de Goede]

Hi Andy,

On 7/9/22 13:34, Andy Shevchenko wrote:
> On Sat, Jul 9, 2022 at 1:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 7/9/22 11:52, Andy Shevchenko wrote:
>>> On Saturday, July 9, 2022, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
>>>     On 7/9/22 02:06, Andy Shevchenko wrote:
> 
> ...
> 
>>>     So nack for this
>>>
>>> This effectively means nack to the series.
> 
>>> But it’s easy to fix. I can add check for ret == 0.
> 
> So, are you okay with fixing it this way? See below how.
> 
>> I don't see how this is a nack for the series, just drop 1/7 + 2/7
>> and rebase the rest. Yes there will be conflicts to resolve in
>> the rebase, but the rest of the cleanups can still go upstream
>> after the rebase.
> 
> Because patch 3 makes a little sense on its own if we drop the patch
> 2. The rest is the simple cleanups which I do not consider as a core
> of this series.

Patch 3 just removes the adev == NULL check and pushes
the ACPI_COMPANION(dev) calls into the function needing
the adev, I don't see how that relies on this patch ?

>>>     >       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);
>>>     > +             ret = smi_i2c_probe(pdev, adev, smi, node->instances);
>>>     > +             if (ret && ret != -ENOENT)
>>>     > +                     return ret;
> 
> /*
>  * ...comment about why we do the following check...
>  */
>  if (ret == 0)
>   return ret;

I'm ok with doing things this way. Note you then end up with:

             if (ret && ret != -ENOENT)
                     return ret;

             if (ret == 0)
                     return ret;

Which can be simplified to just:

             if (ret != -ENOENT)
                     return ret;

> 
>>>     > +             ret = smi_spi_probe(pdev, adev, smi, node->instances);
>>>     > +             if (ret && ret != -ENOENT)
>>>     > +                     return ret;
>>>     > +             if (ret)
>>>     > +                     return dev_err_probe(dev, ret, "Error No resources found\n");
>>>     > +             break;
> 
> if (ret == -ENOENT)
>   return dev_err_probe(...);
> return ret;

Hmm, we don't do this dev_err for the SMI_I2C / SMI_SPI cases,
I see 2 options to solve this:

1) Drop the return calls from switch (node->bus_type) {} instead
always set ret and then break. And do the:

 if (ret == -ENOENT)
   return dev_err_probe(...);

outside the switch-case

2) Drop the dev_err, the driver-core will already log an error for
the -ENOENT anyways.

Regards,

Hans