Re: [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI)

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-acpi@vger.kernel.org, linux-media@vger.kernel.org,
	andriy.shevchenko@linux.intel.com, rafael@kernel.org
Subject: Re: [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI)
Date: Fri, 20 Aug 2021 00:27:23 +0300	[thread overview]
Message-ID: <YR7Mu76nlw4kKwE5@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210819201936.7390-2-sakari.ailus@linux.intel.com>

Hi Sakari,

Thank you for the patch.

On Thu, Aug 19, 2021 at 11:19:34PM +0300, Sakari Ailus wrote:
> Return -EPROBE_DEFER if probing the device fails because of the I²C
> transaction (-EIO only). This generally happens when the power on sequence
> of the device has not been fully performed yet due to later probing of
> other drivers.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/imx258.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index c249507aa2db..2751c12b6029 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -1109,6 +1109,14 @@ static int imx258_identify_module(struct imx258 *imx258)
>  
>  	ret = imx258_read_reg(imx258, IMX258_REG_CHIP_ID,
>  			      IMX258_REG_VALUE_16BIT, &val);
> +	if (ret == -EIO && is_acpi_device_node(dev_fwnode(&client->dev))) {
> +		/*
> +		 * If we get -EIO here and it's an ACPI device, there's a fair
> +		 * likelihood it's because the drivers required to power this
> +		 * device on have not probed yet. Thus return -EPROBE_DEFER.
> +		 */
> +		return -EPROBE_DEFER;

That's really a hack :-( The driver shouldn't have to deal with this. If
power management is handled transparently for the driver, which is
what's meant to happen with ACPI, then it should be fully transparent.
An -EIO error may mean a real communication issue, turning it into
infinite probe deferring isn't right. The ACPI subsystem should figure
this out and not probe the driver until all the required resources that
are managed transparently for the driver are available.

If this was a one-off hack I may be able to pretend I haven't noticed,
but this would need to be copied to every single sensor driver, even
every single I2C device driver. It should be fixed properly in the ACPI
subsystem instead.

> +	}
>  	if (ret) {
>  		dev_err(&client->dev, "failed to read chip id %x\n",
>  			IMX258_CHIP_ID);

-- 
Regards,

Laurent Pinchart