linux-kernel - Re: [PATCH v2] media: i2c: alvium: Accelerated alvium_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMGUoQMGZ12oBnpa@kekkonen.localdomain>
Date: Wed, 10 Sep 2025 18:09:21 +0300
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Martin Hecht <mhecht73@...il.com>
Cc: linux-media@...r.kernel.org, michael.roeder@...et.eu,
	martin.hecht@...et.eu, Tommaso Merciai <tomm.merciai@...il.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

Hi Martin,

On Tue, Sep 09, 2025 at 01:22:51PM +0200, Martin Hecht wrote:
> Now alvium_set_power tests if Alvium is up and running already
> instead of waiting for the period of a full reboot. This safes
> about 5-7 seconds delay for each connected camera what is already
> booted especially when using multiple Alvium cameras or using
> camera arrays.
> The new function alvium_check is used by read_poll_timeout to check
> whether a camera is connected on I2C and if it responds already.
> 
> Signed-off-by: Martin Hecht <mhecht73@...il.com>
> ---
> v2:
> - added alvium_check to be used by read_poll_timeout as
>   suggested by Sakari
> ---
>  drivers/media/i2c/alvium-csi2.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c
> index 5c1bab574394..c63af96d3b31 100644
> --- a/drivers/media/i2c/alvium-csi2.c
> +++ b/drivers/media/i2c/alvium-csi2.c
> @@ -443,10 +443,8 @@ static int alvium_is_alive(struct alvium_dev *alvium)
>  
>  	alvium_read(alvium, REG_BCRM_MINOR_VERSION_R, &bcrm, &ret);
>  	alvium_read(alvium, REG_BCRM_HEARTBEAT_RW, &hbeat, &ret);
> -	if (ret)
> -		return ret;
>  
> -	return hbeat;
> +	return ret;
>  }
>  
>  static void alvium_print_avail_mipi_fmt(struct alvium_dev *alvium)
> @@ -2364,8 +2362,25 @@ static int alvium_get_dt_data(struct alvium_dev *alvium)
>  	return -EINVAL;
>  }
>  
> +static int alvium_check(struct alvium_dev *alvium, u64 *bcrm_major)
> +{
> +	struct device *dev = &alvium->i2c_client->dev;
> +	int ret = 0;

No need to assign ret here.

> +
> +	ret = alvium_read(alvium, REG_BCRM_MAJOR_VERSION_R, bcrm_major, NULL);
> +

No need for an empty line here.

But see below...

> +	if (ret)
> +		return ret;
> +
> +	if (*bcrm_major != 0)
> +		return 0;
> +
> +	return -ENODEV;
> +}
> +
>  static int alvium_set_power(struct alvium_dev *alvium, bool on)
>  {
> +	u64 bcrm_major = 0;
>  	int ret;
>  
>  	if (!on)
> @@ -2375,9 +2390,12 @@ static int alvium_set_power(struct alvium_dev *alvium, bool on)
>  	if (ret)
>  		return ret;
>  
> -	/* alvium boot time 7s */
> -	msleep(7000);
> -	return 0;
> +	/* alvium boot time is up to 7.5s but test if its available already */
> +	read_poll_timeout(alvium_check, bcrm_major, (bcrm_major == 0),
> +		250000, 7500000, false,
> +		alvium, &bcrm_major);

I presume bcrm_major needs to be non-zero to proceed rather than zero?

I think you could also do:

	read_poll_timeout(alvium_read, ret, !ret && brcm_major, 250000, 7500000,
			  false, alvium, REG_BCRM_MAJOR_VERSION_R, bcrm_major,
			  NULL);

	return ret ?: brcm_major ? 0 : -ENODEV;

> +
> +	return ret;
>  }
>  
>  static int alvium_runtime_resume(struct device *dev)
> @@ -2442,7 +2460,7 @@ static int alvium_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_powerdown;
>  
> -	if (!alvium_is_alive(alvium)) {
> +	if (alvium_is_alive(alvium)) {

If you prefer to change this, then I'd assign the return value to ret, as
returned by alvium_read() and use it as the error code here, too. But this
should be a separate patch.

>  		ret = -ENODEV;
>  		dev_err_probe(dev, ret, "Device detection failed\n");
>  		goto err_powerdown;

-- 
Kind regards,

Sakari Ailus