mbox series

[v3,00/15] media: i2c: ds90ub9xx: Misc fixes and improvements

Message ID 20241204-ub9xx-fixes-v3-0-a933c109b323@ideasonboard.com (mailing list archive)
Headers show
Series media: i2c: ds90ub9xx: Misc fixes and improvements | expand

Message

Tomi Valkeinen Dec. 4, 2024, 11:05 a.m. UTC 
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,

Comments

Tomi Valkeinen Dec. 4, 2024, 1:44 p.m. UTC | #1 
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?

  Tomi
Sakari Ailus Dec. 5, 2024, 9:15 a.m. UTC | #2 
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.
Ricardo Ribalda Dec. 5, 2024, 9:22 a.m. UTC | #3 
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
Tomi Valkeinen Dec. 5, 2024, 9:27 a.m. UTC | #4 
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
> 
> 
>
Ricardo Ribalda Dec. 5, 2024, 9:46 a.m. UTC | #5 
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
> >
> >
> >
>
Sakari Ailus Dec. 5, 2024, 9:50 a.m. UTC | #6 
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.
Jai Luthra Dec. 5, 2024, 1:50 p.m. UTC | #7 
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>
>