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

[Michael Tretter <m.tretter@pengutronix.de>]

On Wed, 13 Oct 2021 10:05:30 +0200, Michael Tretter wrote:
> 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@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/drivers/media/i2c/isl7998x.c b/drivers/media/i2c/isl7998x.c
> > > new file mode 100644
[...]
> > > +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.

regmap_read_poll_timeout() cannot be used here, because it returns if
regmap_read() returns an error. The driver uses the return value of
regmap_read() to detect, if the chip is available, and should continue polling
if regmap_read() failed. I can implement the necessary behavior with
read_poll_timeout(), but am not sure if it is really worth it.

Michael