Re: [PATCH v10 2/2] media: i2c: isl7998x: Add driver for Intersil ISL7998x

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux