[v7,00/19] Small fixes and cleanups (ov2740 and ccs)

Hi folks,

This small set contains fixes and cleanups, mainly for the ccs and ov2740
drivers. I wrote these while working on the metadata set, but these could
and should be merged earlier.

since v6:

- New patch: In scope of the CCS driver, revert the patch moving CCS to
  use pm_runtime_get_and_resume().

- New patch: Drop s_stream re-entrancy in CCS driver.

- New patch: Rename ccs_create_subdev() as ccs_init_subdev.

- New patch: CCS driver rework: move sub-device initialisation earlier in
  probe to address initialisation ordering issues later on in embedded
  data support. This introduces minor changes to the patch adding
  sub-device state support.

since v5:

- Send right patches (was v3 + an additional patch)!

since v4:

- Fix CCS driver active state patch --- media entity was initialised too
  late.

- Rebase on Laurent's ov2740 cleanups.

- Add a new patch for MIPI CSI-2 long packet types.

since v3:

- Don't print frame descriptor entry flags as strings but in a numeric
  form.

- Add a WARN_ON() for string truncation in printing the frame descriptor.

- Use 0 flag in printing hexadecimal values in frame descriptor instead of
  specifying precision.

- Add curly braces around a loop (11th patch).

since v2:

- Wrap init_cfg callback long line.

- Remove "pad_" from variable names in ccs_init_cfg.

- Fix media_entity_pads_init() error handling bug (was introduced in the
  last patch).

- Print frame descriptor in less verbose way.

since v1:

- Add a comment on ov2740 active state patch on serialising sensor access.

- Improved commit message of ov2740 patch enabling runtime PM earlier.

- Added patches for printing and zeroing frame descriptor, (debug)
  printing of frame descriptor, switching ccs to init_cfg and sub-device
  state and checking pad flag validity.

Sakari Ailus (19):
  media: Documentation: Align numbered list, make it a proper ReST
  media: ccs: Fix driver quirk struct documentation
  media: ccs: Correctly initialise try compose rectangle
  media: ccs: Correct error handling in ccs_register_subdev
  media: ccs: Switch to init_cfg
  media: ccs: Rename ccs_create_subdev as ccs_init_subdev
  media: ccs: Move media_entity_pads_init to init from register
  media: ccs: Obtain media bus formats before initialising up
    sub-devices
  media: ccs: Use sub-device active state
  media: ccs: Partially revert "media: i2c: Use
    pm_runtime_resume_and_get()"
  media: ccs: Drop re-entrant s_stream support
  media: ov2740: Enable runtime PM before registering the async subdev
  media: ov2740: Use sub-device active state
  media: ov2740: Return -EPROBE_DEFER if no endpoint is found
  media: v4l: subdev: Clear frame descriptor before get_frame_desc
  media: v4l: subdev: Print debug information on frame descriptor
  media: mc: Check pad flag validity
  media: Add MIPI CSI-2 generic long packet type definition
  media: Documentation: Split camera sensor documentation

 .../driver-api/media/camera-sensor.rst        | 131 ++----
 .../media/drivers/camera-sensor.rst           | 104 +++++
 .../userspace-api/media/drivers/index.rst     |   1 +
 .../userspace-api/media/v4l/control.rst       |   4 +
 .../userspace-api/media/v4l/dev-subdev.rst    |  49 ++-
 drivers/media/i2c/ccs/ccs-core.c              | 374 +++++++-----------
 drivers/media/i2c/ccs/ccs-quirk.h             |   4 +-
 drivers/media/i2c/ccs/ccs.h                   |   4 +-
 drivers/media/i2c/ds90ub913.c                 |   2 -
 drivers/media/i2c/ds90ub953.c                 |   2 -
 drivers/media/i2c/ds90ub960.c                 |   2 -
 drivers/media/i2c/ov2740.c                    | 125 +++---
 drivers/media/mc/mc-entity.c                  |  15 +-
 drivers/media/platform/nxp/imx-mipi-csis.c    |   2 -
 drivers/media/v4l2-core/v4l2-subdev.c         |  38 ++
 include/media/mipi-csi2.h                     |   1 +
 16 files changed, 422 insertions(+), 436 deletions(-)
 create mode 100644 Documentation/userspace-api/media/drivers/camera-sensor.rst

On (24/02/01 09:22), Sakari Ailus wrote:
> Hi Sergey,
> 
> Thanks for the review.

Hi Sakari,

> On Thu, Feb 01, 2024 at 06:17:13PM +0900, Sergey Senozhatsky wrote:
> > On (23/10/02 13:55), Sakari Ailus wrote:
[..]
> > > @@ -197,6 +197,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> > >  	struct media_device *mdev = entity->graph_obj.mdev;
> > >  	struct media_pad *iter;
> > >  	unsigned int i = 0;
> > > +	int ret = 0;
> > >  
> > >  	if (num_pads >= MEDIA_ENTITY_MAX_PADS)
> > >  		return -E2BIG;
> > > @@ -210,15 +211,27 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> > >  	media_entity_for_each_pad(entity, iter) {
> > >  		iter->entity = entity;
> > >  		iter->index = i++;
> > > +
> > > +		if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
> > > +					     MEDIA_PAD_FL_SOURCE)) != 1) {
> > > +			ret = -EINVAL;
> > 
> > Can we please have some sort of WARN_ON() or pr_err() here?
> > This is a pretty big change.
> 
> Doing proper input validation is hardly anything unusual, is it?

Well, function requirements change quite significantly, to the point
that drivers that worked before won't work after.

> I'm fine with a WARN_ON() though, I'll add that to v8.

Thanks!

On (24/02/01 11:05), Sakari Ailus wrote:
[..]
> > > > >  	if (num_pads >= MEDIA_ENTITY_MAX_PADS)
> > > > >  		return -E2BIG;
> > > > > @@ -210,15 +211,27 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> > > > >  	media_entity_for_each_pad(entity, iter) {
> > > > >  		iter->entity = entity;
> > > > >  		iter->index = i++;
> > > > > +
> > > > > +		if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
> > > > > +					     MEDIA_PAD_FL_SOURCE)) != 1) {
> > > > > +			ret = -EINVAL;
> > > > 
> > > > Can we please have some sort of WARN_ON() or pr_err() here?
> > > > This is a pretty big change.
> > > 
> > > Doing proper input validation is hardly anything unusual, is it?
> > 
> > Well, function requirements change quite significantly, to the point
> > that drivers that worked before won't work after.
> > 
> > > I'm fine with a WARN_ON() though, I'll add that to v8.
> > 
> > Thanks!
> 
> Actually this was a patchset that was merged quite some time ago. I'll
> post separate patch on this.

Ack.

I just debugged a driver that miraculously stopped working, and it
turned out to be because of this media_entity_pads_init() change.
I think I would have benefited from WARN_ON() or pr_err() there.