[PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

[PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

[Martin Hecht]

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@gmail.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;
+
+	ret = alvium_read(alvium, REG_BCRM_MAJOR_VERSION_R, bcrm_major, NULL);
+
+	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);
+
+	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)) {
 		ret = -ENODEV;
 		dev_err_probe(dev, ret, "Device detection failed\n");
 		goto err_powerdown;
-- 
2.43.0

Re: [PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

[kernel test robot]

Hi Martin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on sailus-media-tree/master]
[also build test WARNING on linuxtv-media-pending/master media-tree/master linus/master v6.17-rc5 next-20250909]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Hecht/media-i2c-alvium-Accelerated-alvium_set_power/20250909-193217
base:   git://linuxtv.org/sailus/media_tree.git master
patch link:    https://lore.kernel.org/r/20250909112252.2577949-1-mhecht73%40gmail.com
patch subject: [PATCH v2] media: i2c: alvium: Accelerated alvium_set_power
config: alpha-randconfig-r072-20250910 (https://download.01.org/0day-ci/archive/20250910/202509101013.wDfoHTNv-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250910/202509101013.wDfoHTNv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509101013.wDfoHTNv-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/media/i2c/alvium-csi2.c: In function 'alvium_check':
>> drivers/media/i2c/alvium-csi2.c:2367:24: warning: unused variable 'dev' [-Wunused-variable]
    2367 |         struct device *dev = &alvium->i2c_client->dev;
         |                        ^~~


vim +/dev +2367 drivers/media/i2c/alvium-csi2.c

  2364	
  2365	static int alvium_check(struct alvium_dev *alvium, u64 *bcrm_major)
  2366	{
> 2367		struct device *dev = &alvium->i2c_client->dev;
  2368		int ret = 0;
  2369	
  2370		ret = alvium_read(alvium, REG_BCRM_MAJOR_VERSION_R, bcrm_major, NULL);
  2371	
  2372		if (ret)
  2373			return ret;
  2374	
  2375		if (*bcrm_major != 0)
  2376			return 0;
  2377	
  2378		return -ENODEV;
  2379	}
  2380	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

[Sakari Ailus]

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@gmail.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

Re: [PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

[Martin Hecht]

Hi Sakari,

thank you for your feedback. Please ignore v3 because overlap. I will 
adopt your proposal and send v4. Nevertheless I'm in conversation with 
Ricardo because some eventually misleading feedback from CI to learn how 
to deal with that.

BR Martin

On 9/10/25 17:09, Sakari Ailus wrote:
> 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@gmail.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;
> 

Re: [PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

[Sakari Ailus]

Hi Martin,

On Wed, Sep 10, 2025 at 07:17:01PM +0200, Martin Hecht wrote:
> Hi Sakari,
> 
> thank you for your feedback. Please ignore v3 because overlap. I will adopt
> your proposal and send v4. Nevertheless I'm in conversation with Ricardo
> because some eventually misleading feedback from CI to learn how to deal
> with that.

Ack.

> > 	return ret ?: brcm_major ? 0 : -ENODEV;

Actually -ETIMEDOUT is probably a better choice than -ENODEV in this case.

-- 
Regards,

Sakari Ailus