| Message ID | 20241204-ub9xx-fixes-v3-0-a933c109b323@ideasonboard.com |
|---|---|
| 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
On Wed, Dec 04, 2024 at 03:44:53PM +0200, Tomi Valkeinen wrote: > Hi, > > On 04/12/2024 15:18, Patchwork Integration wrote: > > Dear Tomi Valkeinen: > > > > Thanks for your patches! Unfortunately media-ci detected some issues: > > > > # Test media-patchstyle:./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch > > WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Sakari. They are already in Signed-off-by > > WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Ailus. They are already in Signed-off-by > > WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc <sakari.ailus@linux.intel.com>. They are already in Signed-off-by > > What could these mean? Cc'd Ricardo.
In your patch you Cc: Sakari Signed-off: Sakari You need to remove the Cc: once someone is already Signed-off: I have improved the error message in media-ci. On Thu, 5 Dec 2024 at 10:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > On Wed, Dec 04, 2024 at 03:44:53PM +0200, Tomi Valkeinen wrote: > > Hi, > > > > On 04/12/2024 15:18, Patchwork Integration wrote: > > > Dear Tomi Valkeinen: > > > > > > Thanks for your patches! Unfortunately media-ci detected some issues: > > > > > > # Test media-patchstyle:./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch > > > WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Sakari. They are already in Signed-off-by > > > WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Ailus. They are already in Signed-off-by > > > WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc <sakari.ailus@linux.intel.com>. They are already in Signed-off-by > > > > What could these mean? > > Cc'd Ricardo. > > -- > Sakari Ailus
Hi, On 05/12/2024 11:22, Ricardo Ribalda wrote: > In your patch you > > Cc: Sakari > Signed-off: Sakari This series has no such lines. Tomi > You need to remove the Cc: once someone is already Signed-off: > > I have improved the error message in media-ci. > > On Thu, 5 Dec 2024 at 10:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: >> >> On Wed, Dec 04, 2024 at 03:44:53PM +0200, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 04/12/2024 15:18, Patchwork Integration wrote: >>>> Dear Tomi Valkeinen: >>>> >>>> Thanks for your patches! Unfortunately media-ci detected some issues: >>>> >>>> # Test media-patchstyle:./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch >>>> WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Sakari. They are already in Signed-off-by >>>> WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Ailus. They are already in Signed-off-by >>>> WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc <sakari.ailus@linux.intel.com>. They are already in Signed-off-by >>> >>> What could these mean? >> >> Cc'd Ricardo. >> >> -- >> Sakari Ailus > > >
Hi Tomi On Thu, 5 Dec 2024 at 10:27, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > > Hi, > > On 05/12/2024 11:22, Ricardo Ribalda wrote: > > In your patch you > > > > Cc: Sakari > > Signed-off: Sakari > > This series has no such lines. > > Tomi > > > You need to remove the Cc: once someone is already Signed-off: > > > > I have improved the error message in media-ci. You are correct, I have just sent a PR to fix it https://gitlab.freedesktop.org/linux-media/media-ci/-/merge_requests/212 Sorry for the inconvenience > > > > On Thu, 5 Dec 2024 at 10:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > >> > >> On Wed, Dec 04, 2024 at 03:44:53PM +0200, Tomi Valkeinen wrote: > >>> Hi, > >>> > >>> On 04/12/2024 15:18, Patchwork Integration wrote: > >>>> Dear Tomi Valkeinen: > >>>> > >>>> Thanks for your patches! Unfortunately media-ci detected some issues: > >>>> > >>>> # Test media-patchstyle:./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch > >>>> WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Sakari. They are already in Signed-off-by > >>>> WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc Ailus. They are already in Signed-off-by > >>>> WARNING: ./0001-media-i2c-ds90ub9x3-Fix-extra-fwnode_handle_put.patch: Don't Cc <sakari.ailus@linux.intel.com>. They are already in Signed-off-by > >>> > >>> What could these mean? > >> > >> Cc'd Ricardo. > >> > >> -- > >> Sakari Ailus > > > > > > >
Hi Ricardo,
On Thu, Dec 05, 2024 at 10:46:31AM +0100, Ricardo Ribalda wrote:
> Sorry for the inconvenience
Thank you for your invaluable work on it.
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 Tomi, On Dec 04, 2024 at 13:05:14 +0200, Tomi Valkeinen wrote: > This series fixes various small issues in the drivers, and adds a few > things (a couple of pixel formats and a debugging feature). > > It also takes a few steps in adding more i2c read/write error handlings > to the drivers, but covers only the easy places. > > Adding error handling to all reads/writes needs more thinking, perhaps > adding a "ret" parameter to the calls, similar to the cci_* functions, > or perhaps adding helpers for writing multiple registers from a given > table. Also, in some places rolling back from an error will require > work. > With the minor comment addressed, for the series: Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > Changes in v3: > - Include bitfield.h for FIELD_PREP() > - Cc stable for relevant fixes > - Link to v2: https://lore.kernel.org/r/20241108-ub9xx-fixes-v2-0-c7db3b2ad89f@ideasonboard.com > > Changes in v2: > - Address comments from Andy > - Add two new patches: > - media: i2c: ds90ub960: Fix shadowing of local variables > - media: i2c: ds90ub960: Use HZ_PER_MHZ > - Link to v1: https://lore.kernel.org/r/20241004-ub9xx-fixes-v1-0-e30a4633c786@ideasonboard.com > > --- > Tomi Valkeinen (15): > media: i2c: ds90ub9x3: Fix extra fwnode_handle_put() > media: i2c: ds90ub960: Fix UB9702 refclk register access > media: i2c: ds90ub960: Fix use of non-existing registers on UB9702 > media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702 > media: i2c: ds90ub960: Fix UB9702 VC map > media: i2c: ds90ub960: Use HZ_PER_MHZ > media: i2c: ds90ub960: Add support for I2C_RX_ID > media: i2c: ds90ub960: Add RGB24, RAW8 and RAW10 formats > media: i2c: ds90ub953: Clear CRC errors in ub953_log_status() > media: i2c: ds90ub960: Drop unused indirect block define > media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks() > media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq() > media: i2c: ds90ub913: Add error handling to ub913_hw_init() > media: i2c: ds90ub953: Add error handling for i2c reads/writes > media: i2c: ds90ub960: Fix shadowing of local variables > > drivers/media/i2c/ds90ub913.c | 26 ++++-- > drivers/media/i2c/ds90ub953.c | 56 +++++++++---- > drivers/media/i2c/ds90ub960.c | 186 ++++++++++++++++++++++++++++-------------- > 3 files changed, 187 insertions(+), 81 deletions(-) > --- > base-commit: adc218676eef25575469234709c2d87185ca223a > change-id: 20241004-ub9xx-fixes-bba80dc48627 > > Best regards, > -- > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >
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
This series fixes various small issues in the drivers, and adds a few things (a couple of pixel formats and a debugging feature). It also takes a few steps in adding more i2c read/write error handlings to the drivers, but covers only the easy places. Adding error handling to all reads/writes needs more thinking, perhaps adding a "ret" parameter to the calls, similar to the cci_* functions, or perhaps adding helpers for writing multiple registers from a given table. Also, in some places rolling back from an error will require work. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- Changes in v3: - Include bitfield.h for FIELD_PREP() - Cc stable for relevant fixes - Link to v2: https://lore.kernel.org/r/20241108-ub9xx-fixes-v2-0-c7db3b2ad89f@ideasonboard.com Changes in v2: - Address comments from Andy - Add two new patches: - media: i2c: ds90ub960: Fix shadowing of local variables - media: i2c: ds90ub960: Use HZ_PER_MHZ - Link to v1: https://lore.kernel.org/r/20241004-ub9xx-fixes-v1-0-e30a4633c786@ideasonboard.com --- Tomi Valkeinen (15): media: i2c: ds90ub9x3: Fix extra fwnode_handle_put() media: i2c: ds90ub960: Fix UB9702 refclk register access media: i2c: ds90ub960: Fix use of non-existing registers on UB9702 media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702 media: i2c: ds90ub960: Fix UB9702 VC map media: i2c: ds90ub960: Use HZ_PER_MHZ media: i2c: ds90ub960: Add support for I2C_RX_ID media: i2c: ds90ub960: Add RGB24, RAW8 and RAW10 formats media: i2c: ds90ub953: Clear CRC errors in ub953_log_status() media: i2c: ds90ub960: Drop unused indirect block define media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks() media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq() media: i2c: ds90ub913: Add error handling to ub913_hw_init() media: i2c: ds90ub953: Add error handling for i2c reads/writes media: i2c: ds90ub960: Fix shadowing of local variables drivers/media/i2c/ds90ub913.c | 26 ++++-- drivers/media/i2c/ds90ub953.c | 56 +++++++++---- drivers/media/i2c/ds90ub960.c | 186 ++++++++++++++++++++++++++++-------------- 3 files changed, 187 insertions(+), 81 deletions(-) --- base-commit: adc218676eef25575469234709c2d87185ca223a change-id: 20241004-ub9xx-fixes-bba80dc48627 Best regards,