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

[Tomi Valkeinen]

Hi,

On 05/12/2024 14:00, Jai Luthra wrote:
> Hi Tomi,
>
> On Dec 05, 2024 at 10:31:42 +0200, 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?
> >
> > ...
> >
> > > +#ifdef UB960_DEBUG_I2C_RX_ID
> > > +	for (unsigned int i = 0; i < 4; i++)
> >
> > Should it use _MAX_RX_NPORTS instead of 4?
>
> Instead of hardcoded value or the macro, it is better to use
> priv->hw_data->num_rxports.
>
> The cut-down variant of this deser only has 2 ports for example.
> https://www.ti.com/lit/gpn/ds90ub954-q1

Yes, that's true. I'll use the hw_data.

 Tomi