Re: [PATCH v5 3/3] media: i2c: isl7998x: Add driver for Intersil ISL7998x

[Philipp Zabel <p.zabel@pengutronix.de>]

On Tue, 2021-10-12 at 10:41 +0200, Michael Tretter wrote:
> From: Marek Vasut <marex@denx.de>
> 
> 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@denx.de>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> To: linux-media@vger.kernel.org
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> Changelog:
> 
> 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    |    9 +
>  drivers/media/i2c/Makefile   |    1 +
>  drivers/media/i2c/isl7998x.c | 1416 ++++++++++++++++++++++++++++++++++
>  4 files changed, 1434 insertions(+)
>  create mode 100644 drivers/media/i2c/isl7998x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ee91c5472bc1..60449758d40b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9739,6 +9739,14 @@ L:	linux-iio@vger.kernel.org
>  F:	Documentation/devicetree/bindings/counter/interrupt-counter.yaml
>  F:	drivers/counter/interrupt-cnt.c
>  
> +INTERSIL ISL7998X VIDEO DECODER DRIVER
> +M:	Michael Tretter <m.tretter@pengutronix.de>
> +R:	Pengutronix Kernel Team <kernel@pengutronix.de>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/media/i2c/isil,isl79987.yaml
> +F:	drivers/media/i2c/isl7998x.c
> +
>  INVENSENSE ICM-426xx IMU DRIVER
>  M:	Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>  L:	linux-iio@vger.kernel.org
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index adb348aa8396..ab2d9d45af0e 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -325,6 +325,15 @@ config VIDEO_BT866
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bt866.
>  
> +config VIDEO_ISL7998X
> +	tristate "Intersil ISL7998x video decoder"
> +	depends on VIDEO_V4L2 && I2C
> +	depends on OF_GPIO
> +	select VIDEO_V4L2_SUBDEV_API

This is missing a select MEDIA_CONTROLLER, which VIDEO_V4L2_SUBDEV_API
depends on. As is, the driver doesn't compile without MEDIA_CONTROLLER
enabled.

[...]
> diff --git a/drivers/media/i2c/isl7998x.c b/drivers/media/i2c/isl7998x.c
> new file mode 100644
> index 000000000000..72f5ca63f810
> --- /dev/null
> +++ b/drivers/media/i2c/isl7998x.c
> @@ -0,0 +1,1416 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intersil ISL7998x analog to MIPI CSI-2 or BT.656 decoder driver.
> + *
> + * Copyright (C) 2018-2019 Marek Vasut <marex@denx.de>
> + * Copyright (C) 2021 Michael Tretter <kernel@pengutronix.de>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/log2.h>

Is this header used for anything?

[...]
> +struct isl7998x {
> +	struct v4l2_subdev		subdev;
> +	struct regmap			*regmap;
> +	struct gpio_desc		*pd_gpio;
> +	struct gpio_desc		*rstb_gpio;
> +	unsigned int			nr_mipi_lanes;
> +	u32				nr_inputs;
> +
> +	const struct isl7998x_mode	*mode;
> +	const struct isl7998x_datafmt	*fmt;
> +#ifdef CONFIG_MEDIA_CONTROLLER

This is not required if the driver selects MEDIA_CONTROLLER.
To make it buildable without MEDIA_CONTROLLER, a few more #ifdefs would
be required in other places.

> +	struct media_pad		pads[ISL7998x_NUM_PADS];
> +#endif
> +
> +	struct v4l2_ctrl_handler	ctrl_handler;
> +	/* protect ctrl_handler */
> +	struct mutex			ctrl_mutex;
> +
> +	/* V4L2 Controls */
> +	struct v4l2_ctrl		*link_freq;
> +	u8				test_pattern;
> +	u8				test_pattern_bars;
> +	u8				test_pattern_chans;
> +	u8				test_pattern_color;
> +};
[...]
> +static int isl7998x_wait_power_on(struct isl7998x *isl7998x)
> +{
> +	struct device *dev = isl7998x->subdev.dev;
> +	unsigned int retry;
> +	u32 chip_id;
> +	int ret = -ETIMEDOUT;
> +
> +	for (retry = 10; ret && retry > 0; retry--) {
> +		ret = regmap_read(isl7998x->regmap,
> +				  ISL7998x_REG_P0_PRODUCT_ID_CODE, &chip_id);
> +		usleep_range(1000, 2000);
> +	}

Consider using regmap_read_poll_timeout() here.

> +	if (ret) {
> +		dev_err(dev, "timeout while waiting for ISL7998x\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	dev_dbg(dev, "Found ISL799%x\n", chip_id);
> +
> +	return 0;
> +}
[...]
> +static int isl7998x_update_std(struct isl7998x *isl7998x)
> +{
> +#define ISL7998x_INPUTS			4
> +	struct device *dev = isl7998x->subdev.dev;
> +	unsigned int std_id[ISL7998x_INPUTS];
> +	u8 scanning = GENMASK(ISL7998x_INPUTS - 1, 0);
> +	unsigned int i;
> +	int ret;
> +	u32 reg;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	while (true) {

Should this timeout, just in case the decoder never leaves the detection
state?

> +		for (i = 0; i < ISL7998x_INPUTS; i++) {
> +			ret = regmap_read(isl7998x->regmap,
> +					  ISL7998x_REG_Px_DEC_SDT(i + 1), &reg);
> +			if (ret)
> +				goto out;
> +
> +			/* Detection is still in progress, restart. */
> +			if (reg & ISL7998x_REG_Px_DEC_SDT_DET) {
> +				scanning = GENMASK(ISL7998x_INPUTS - 1, 0);
> +				break;
> +			}
> +
> +			scanning &= ~BIT(i);
> +			std_id[i] = FIELD_GET(ISL7998x_REG_Px_DEC_SDT_NOW, reg);
> +		}
> +
> +		if (!scanning)
> +			break;
> +
> +		/*
> +		 * Duration of standard detection is not documented. Wait for
> +		 * an arbitrary time before checking again.
> +		 */
> +		usleep_range(1000, 2000);
> +	}
[...]
> +static int isl7998x_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct v4l2_fwnode_endpoint endpoint = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY,
> +	};
> +	struct device_node *ep;
> +	struct isl7998x *isl7998x;
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +	int nr_inputs;
> +	int ret;
[...]
> +	ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> +	if (!ep)
> +		return dev_err_probe(dev, -EINVAL, "Missing endpoint node\n");

I think this should use fwnode_graph_get_endpoint_by_id() instead.

> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &endpoint);
> +	of_node_put(ep);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to parse endpoint\n");
> +
> +	if (endpoint.bus.mipi_csi2.num_data_lanes == 0 ||
> +	    endpoint.bus.mipi_csi2.num_data_lanes > 2)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid bus type or number of MIPI lanes\n");

This doesn't check bus type, as stated.

regards
Philipp