On Tue, 2021-10-12 at 10:41 +0200, 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:
>
> 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@xxxxxxxxxxxxxxx
> F: Documentation/devicetree/bindings/counter/interrupt-counter.yaml
> F: drivers/counter/interrupt-cnt.c
>
> +INTERSIL ISL7998X VIDEO DECODER DRIVER
> +M: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
> +R: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> +L: linux-media@xxxxxxxxxxxxxxx
> +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@xxxxxxxxxxxxxx>
> L: linux-iio@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> + * Copyright (C) 2021 Michael Tretter <kernel@xxxxxxxxxxxxxx>
> + */
> +
> +#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), ®);
> + 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
