On Tue, 12 Oct 2021 15:27:11 +0200, Philipp Zabel wrote:
> 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.
I will select MEDIA_CONTROLLER in v6.
>
> [...]
> > 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?
Currently not. This might be an artifact of a previous version. I will remove
the header.
>
> [...]
> > +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.
See above.
>
> > + 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.
Ack. I forgot about this function.
>
> > + 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?
>From what I have seen so far, the detection algorithm in the decoder always
finishes, even if there is no signal on the input. Adding a timeout doesn't
hurt, though, and I will do so.
>
> > + 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.
Ack. This should also use ISL7998x_PAD_OUT instead of hard coded 0.
>
> > + 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.
Ack. I updated the initialization and check, but missed the error message.
Thanks!
Michael
