linux-kernel - Re: [PATCH v3 07/15] media: i2c: ds90ub960: Add support for I2C_RX

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: <fa3405ce-a9f4-402d-b29c-d900858df974@ideasonboard.com>
Date: Thu, 5 Dec 2024 15:59:58 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Hans Verkuil <hverkuil-cisco@...all.nl>,
 Jai Luthra <jai.luthra@...asonboard.com>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 07/15] media: i2c: ds90ub960: Add support for I2C_RX_ID

Hi,

On 05/12/2024 10:31, Andy Shevchenko wrote:
> On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote:
>> Normally the driver accesses both the RX and the TX port registers via a
>> paging mechanism: one register is used to select the page (i.e. the
>> port), which dictates the port used when accessing the port specific
>> registers.
>>
>> The downside to this is that while debugging it's almost impossible to
>> access the port specific registers from the userspace, as the driver can
>> change the page at any moment.
>>
>> The hardware supports another access mechanism: using the I2C_RX_ID
>> registers (one for each RX port), i2c addresses can be chosen which,
>> when accessed, will always use the specific port's registers, skipping
>> the paging mechanism.
>>
>> The support is only for the RX port, but it has proven very handy while
>> debugging and testing. So let's add the code for this, but hide it
>> behind a disabled define.
> 
> ...
> 
>>   #define MHZ(v) ((u32)((v) * 1000000U))
> 
> Missed HZ_PER_MHZ from previous patch?

Yes, and no. I did leave the MHZ uses on purpose. I think the use of 
HZ_PER_MHZ was fine in the calculations, but when having table-ish use 
of MHZ, with hardcoded numbers, I found the MHZ() macro much nicer to read:

	case MHZ(1200):

vs.
	case 1200 * HZ_PER_MHZ:

> 
> ...
> 
>> +#ifdef UB960_DEBUG_I2C_RX_ID
>> +	for (unsigned int i = 0; i < 4; i++)
> 
> Should it use _MAX_RX_NPORTS instead of 4?

Indeed, thanks!

> 
>> +		ub960_write(priv, UB960_SR_I2C_RX_ID(i),
>> +			    (UB960_DEBUG_I2C_RX_ID + i) << 1);
>> +#endif
> 

  Tomi