Re: [PATCH v3 07/15] media: i2c: ds90ub960: Add support for I2C_RX_ID

[Tomi Valkeinen]

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