Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <edba69a1-3404-d20f-2a9c-6359de37b6ee@redhat.com>
Date:   Sat, 9 Jul 2022 11:48:03 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
From:   Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH v1 2/7] platform/x86: serial-multi-instantiate: Improve
 autodetection
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        Stefan Binding <sbinding@opensource.cirrus.com>
Cc:     Mark Gross <markgross@kernel.org>, patches@opensource.cirrus.com
References: <20220709000636.35550-1-andriy.shevchenko@linux.intel.com>
 <20220709000636.35550-2-andriy.shevchenko@linux.intel.com>
Content-Language: en-US
In-Reply-To: <20220709000636.35550-2-andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

<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)