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]

 



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...

> +}
> +
> +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?

> +
> +	return 0;
> +}

Regards,

	Hans



[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