| Message ID | 20241204-ub9xx-fixes-v3-7-a933c109b323@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: ds90ub9xx: Misc fixes and improvements | expand |
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? > + ub960_write(priv, UB960_SR_I2C_RX_ID(i), > + (UB960_DEBUG_I2C_RX_ID + i) << 1); > +#endif
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 > > + ub960_write(priv, UB960_SR_I2C_RX_ID(i), > > + (UB960_DEBUG_I2C_RX_ID + i) << 1); > > +#endif > > -- > With Best Regards, > Andy Shevchenko > >
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
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
On Thu, Dec 05, 2024 at 03:59:58PM +0200, Tomi Valkeinen wrote: > On 05/12/2024 10:31, Andy Shevchenko wrote: > > On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote: ... > > > #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: Had I talked about tables? :-) I was only commented the calculations.
On 05/12/2024 21:36, Andy Shevchenko wrote: > On Thu, Dec 05, 2024 at 03:59:58PM +0200, Tomi Valkeinen wrote: >> On 05/12/2024 10:31, Andy Shevchenko wrote: >>> On Wed, Dec 04, 2024 at 01:05:21PM +0200, Tomi Valkeinen wrote: > > ... > >>>> #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: > > Had I talked about tables? :-) > I was only commented the calculations. I see your point now =) Tomi
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 98d815526341..03938def6ae9 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -55,6 +55,15 @@ #define MHZ(v) ((u32)((v) * 1000000U)) +/* + * If this is defined, the i2c addresses from UB960_DEBUG_I2C_RX_ID to + * UB960_DEBUG_I2C_RX_ID + 3 can be used to access the paged RX port registers + * directly. + * + * Only for debug purposes. + */ +/* #define UB960_DEBUG_I2C_RX_ID 0x40 */ + #define UB960_POLL_TIME_MS 500 #define UB960_MAX_RX_NPORTS 4 @@ -351,7 +360,7 @@ #define UB960_SR_FPD3_RX_ID(n) (0xf0 + (n)) #define UB960_SR_FPD3_RX_ID_LEN 6 -#define UB960_SR_I2C_RX_ID(n) (0xf8 + (n)) /* < UB960_FPD_RX_NPORTS */ +#define UB960_SR_I2C_RX_ID(n) (0xf8 + (n)) #define UB9702_SR_REFCLK_FREQ 0x3d @@ -4001,6 +4010,12 @@ static int ub960_probe(struct i2c_client *client) schedule_delayed_work(&priv->poll_work, msecs_to_jiffies(UB960_POLL_TIME_MS)); +#ifdef UB960_DEBUG_I2C_RX_ID + for (unsigned int i = 0; i < 4; i++) + ub960_write(priv, UB960_SR_I2C_RX_ID(i), + (UB960_DEBUG_I2C_RX_ID + i) << 1); +#endif + return 0; err_free_sers:
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. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/i2c/ds90ub960.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)