Hi Hans, Michael,
On Wed, Feb 23, 2022 at 12:51:49PM +0100, Hans Verkuil wrote:
> On 2/17/22 16:44, Michael Tretter wrote:
> > From: Marek Vasut <marex@xxxxxxx>
> >
> > Add driver for the Intersil ISL7998x Analog to MIPI CSI-2/BT656 decoder.
> > This chip supports 1/2/4 analog video inputs and converts them into
> > 1/2/4 VCs in MIPI CSI2 stream.
> >
> > This driver currently supports ISL79987 and both 720x480 and 720x576
> > resolutions, however as per specification, all inputs must use the
> > same resolution and standard. The only supported pixel format is now
> > YUYV/YUV422. The chip should support RGB565 on the CSI2 as well, but
> > this is currently unsupported.
> >
> > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Cc: Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > To: linux-media@xxxxxxxxxxxxxxx
> > Signed-off-by: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
> > ---
> > Changelog:
> >
> > v10:
> >
> > - add a lock for subdev calls
> > - remove unnecessary pm_runtime_enabled
> > - fix indentation and format
> > - free controls on error
> > - fix set_standard call
> > - remove camel case in macro definitions
> > - add macros for video formats
> > - rework lookup of video standards
> > - add support for PAL Nc
> > - add explicit trigger for format detection
> >
> > v9: none
> >
> > v8:
> >
> > - fix warning "type qualifiers ignored on function return type"
> >
> > v7:
> >
> > - reserve driver specific controls
> > - add documentation for driver specific controls
> > - implement g_input_status
> > - track device enabled state in driver
> > - store norm instead of mode in driver
> > - select test pattern based on video norm
> > - improve debug message for enabled test pattern
> > - fix off by one with 4 inputs
> > - implement querystd and friends
> > - fix polling condition for standard detection
> > - use v4l2_norm_to_name instead of custom implementation
> >
> > v6:
> >
> > - remove unused log2.h
> > - add select MEDIA_CONTROLLER
> > - use poll_read_timeout to wait for power on
> > - add timeout to polling for video standard
> > - use fwnode_graph_get_endpoint_by_id
> > - fix invalid bus type error message
> >
> > v5: none
> >
> > v4:
> >
> > - fix lines over 80 chars where applicable
> > - fix possible NULL pointer access in link_freq
> > - initialize bus type with CSI2_DPHY
> > - iterate over pads instead of hard coded 4
> > - merge power_{on,off} functions into resume,suspend
> > - switch to v4l2_subdev_state
> > - report field order based on video standard
> > - add error message for timeout
> > - simplify dev_dbg statement in update_std
> > - call v4l2_ctrl_handler_setup
> > - don't set control if pm_runtime is not enabled
> > - fix YUV422 byte order
> > - switch to pre_streamon callback for LP11 mode
> >
> > v3:
> >
> > - follow dt binding change: pd-gpios -> powerdown-gpios
> >
> > v2:
> >
> > - general cleanup
> > - remove isl7998x_g_mbus_config function
> > - implement enum_frame_size function
> > - replace msleep with usleep_range
> > - rework set_fmt/get_fmt functions
> > - calculate number of inputs using number of input ports
> > - switch to runtime_pm
> > - add reset gpio
> > - add adv_debug support
> > - add MAINTAINERS entry
> > ---
> > MAINTAINERS | 8 +
> > drivers/media/i2c/Kconfig | 10 +
> > drivers/media/i2c/Makefile | 1 +
> > drivers/media/i2c/isl7998x.c | 1633 ++++++++++++++++++++++++++++
> > include/uapi/linux/v4l2-controls.h | 6 +
> > 5 files changed, 1658 insertions(+)
> > create mode 100644 drivers/media/i2c/isl7998x.c
> >
>
> <snip>
>
> > +static int isl7998x_pre_streamon(struct v4l2_subdev *sd, u32 flags)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > + struct device *dev = &client->dev;
> > +
> > + if (flags & V4L2_SUBDEV_PRE_STREAMON_FL_MANUAL_LP)
> > + return pm_runtime_resume_and_get(dev);
> > +
> > + return 0;
>
> This feels a bit scary: if V4L2_SUBDEV_PRE_STREAMON_FL_MANUAL_LP is NOT
> set, then pm_runtime_resume_and_get() isn't called, but this function
> still returns success...
Good find.
pm_runtime_resume_and_get() need to be called unconditionally.
Alternatively, store what was done here, and put the PM use count
accordingly below. But I see no reason to do that.
>
> > +}
> > +
> > +static int isl7998x_post_streamoff(struct v4l2_subdev *sd)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > + struct device *dev = &client->dev;
> > +
> > + pm_runtime_put(dev);
>
> ...and pm_runtime_put() is called without the corresponding get.
>
> The documentation in v4l2-subdev.h isn't very clear about what
> pre_streamon should return. I'm inclined to say that it should
> return -EACCES.
>
> Sakari, what do you think?
It should return 0 if successful. Indeed we could make the return type
void, as this is simply tearing down what was set up previously.
--
Kind regards,
Sakari Ailus
