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] [day] [month] [year] [list]

Message-ID: <7f095cde-cefc-4259-9f7d-9de17c12758b@gmail.com>
Date: Wed, 10 Sep 2025 19:17:01 +0200
From: Martin Hecht <mhecht73@...il.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: 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,
 Ricardo Ribalda <ribalda@...omium.org>
Subject: Re: [PATCH v2] media: i2c: alvium: Accelerated alvium_set_power

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