Re: [RFC 2/5] media: i2c: Add MAX9271 I2C driver

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Laurent,

On Mon, Aug 23, 2021 at 05:22:45AM +0300, Laurent Pinchart wrote:
> On Wed, Aug 18, 2021 at 10:27:00AM +0200, Jacopo Mondi wrote:
> > Hi Kieran, thanks for review!
> >
> > On Tue, Aug 17, 2021 at 04:49:17PM +0100, Kieran Bingham wrote:
> > > Hi Jacopo,
> > >
> > > On 17/08/2021 08:27, Jacopo Mondi wrote:
> > > > The MAX9271 is a GMSL serializer that serializes a video stream
> > > > received from an image sensor through the parallel video bus.
> > >
> > >
> > > https://datasheets.maximintegrated.com/en/ds/MAX9271.pdf calls it a
> > > "16-Bit GMSL Serializer with Coas or STP Cable Drive"
> > >
> >
> > Nice we have a public datasheet now!
> > Nice, well, it might mean GMSL is just old ?
> >
> > >
> > > > The serializer it's usually found embedded with an image sensor and
> > >
> > > s/it's/is/
> > >
> > > > other ancillary chips in camera modules like RDACM20 and RDACM21.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  MAINTAINERS                 |   9 +
> > > >  drivers/media/i2c/Kconfig   |  12 +
> > > >  drivers/media/i2c/Makefile  |   1 +
> > > >  drivers/media/i2c/max9271.c | 756 ++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 778 insertions(+)
> > > >  create mode 100644 drivers/media/i2c/max9271.c
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 7ad89cac19b7..2dab25a08c9c 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -11244,6 +11244,15 @@ F:	Documentation/hwmon/max6697.rst
> > > >  F:	drivers/hwmon/max6697.c
> > > >  F:	include/linux/platform_data/max6697.h
> > > >
> > > > +MAX9271 GMSL SERIALIZER DRIVER
> > > > +M:	Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > +M:	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > > +M:	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > +M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > +L:	linux-media@vger.kernel.org
> > > > +S:	Maintained
> > > > +F:	drivers/media/i2c/max9271.c
> > > > +
> > > >  MAX9286 QUAD GMSL DESERIALIZER DRIVER
> > > >  M:	Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > >  M:	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > index 08feb3e8c1bf..b793d1f322d9 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -1300,6 +1300,18 @@ source "drivers/media/i2c/m5mols/Kconfig"
> > > >  config VIDEO_MAX9271_LIB
> > > >  	tristate
> > > >
> > > > +config VIDEO_MAX9271
> > > > +	tristate "MAX9271 GMSL serializer support"
> > > > +	depends on I2C
> > > > +	select V4L2_FWNODE
> > > > +	select VIDEO_V4L2_SUBDEV_API
> > > > +	select MEDIA_CONTROLLER
> > > > +	help
> > > > +	  This driver supports the Maxim MAX9271 GMSL serializer.
> > > > +
> > > > +	  To compile this driver as a module, choose M here: the
> > > > +	  module will be called max9271.
> > > > +
> > > >  config VIDEO_RDACM20
> > > >  	tristate "IMI RDACM20 camera support"
> > > >  	depends on I2C
> > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > > index 4d879373bd48..37bb51065574 100644
> > > > --- a/drivers/media/i2c/Makefile
> > > > +++ b/drivers/media/i2c/Makefile
> > > > @@ -130,6 +130,7 @@ obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
> > > >  obj-$(CONFIG_VIDEO_IMX412)	+= imx412.o
> > > >  obj-$(CONFIG_VIDEO_MAX9286)	+= max9286.o
> > > >  obj-$(CONFIG_VIDEO_MAX9271_LIB)	+= max9271-lib.o
> > > > +obj-$(CONFIG_VIDEO_MAX9271)	+= max9271.o
> > > >  obj-$(CONFIG_VIDEO_RDACM20)	+= rdacm20.o
> > > >  obj-$(CONFIG_VIDEO_RDACM21)	+= rdacm21.o
> > > >  obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> > > > diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> > > > new file mode 100644
> > > > index 000000000000..64987cba3d3e
> > > > --- /dev/null
> > > > +++ b/drivers/media/i2c/max9271.c
> > > > @@ -0,0 +1,756 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (C) 2017-2021 Jacopo Mondi
> > > > + * Copyright (C) 2017-2020 Kieran Bingham
> > > > + * Copyright (C) 2017-2019 Laurent Pinchart
> > > > + * Copyright (C) 2017-2019 Niklas Söderlund
> > > > + * Copyright (C) 2016 Renesas Electronics Corporation
> > > > + * Copyright (C) 2015 Cogent Embedded, Inc.
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/fwnode.h>
> > > > +#include <linux/init.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/property.h>
> > > > +
> > > > +#include <media/v4l2-async.h>
> > > > +#include <media/v4l2-ctrls.h>
> > > > +#include <media/v4l2-fwnode.h>
> > > > +#include <media/v4l2-subdev.h>
> > > > +
> > > > +#define MAX9271_DEFAULT_ADDR	0x40
> > > > +
> > > > +/* Register 0x02 */
> > > > +#define MAX9271_SPREAD_SPECT_0		(0 << 5)
> > > > +#define MAX9271_SPREAD_SPECT_05		(1 << 5)
> > > > +#define MAX9271_SPREAD_SPECT_15		(2 << 5)
> > > > +#define MAX9271_SPREAD_SPECT_1		(5 << 5)
> > > > +#define MAX9271_SPREAD_SPECT_2		(3 << 5)
> > > > +#define MAX9271_SPREAD_SPECT_3		(6 << 5)
> > > > +#define MAX9271_SPREAD_SPECT_4		(7 << 5)
> > > > +#define MAX9271_R02_RES			BIT(4)
> > > > +#define MAX9271_PCLK_AUTODETECT		(3 << 2)
> > > > +#define MAX9271_SERIAL_AUTODETECT	(0x03)
> > > > +/* Register 0x04 */
> > > > +#define MAX9271_SEREN			BIT(7)
> > > > +#define MAX9271_CLINKEN			BIT(6)
> > > > +#define MAX9271_PRBSEN			BIT(5)
> > > > +#define MAX9271_SLEEP			BIT(4)
> > > > +#define MAX9271_INTTYPE_I2C		(0 << 2)
> > > > +#define MAX9271_INTTYPE_UART		(1 << 2)
> > > > +#define MAX9271_INTTYPE_NONE		(2 << 2)
> > > > +#define MAX9271_REVCCEN			BIT(1)
> > > > +#define MAX9271_FWDCCEN			BIT(0)
> > > > +/* Register 0x07 */
> > > > +#define MAX9271_DBL			BIT(7)
> > > > +#define MAX9271_DRS			BIT(6)
> > > > +#define MAX9271_BWS			BIT(5)
> > > > +#define MAX9271_ES			BIT(4)
> > > > +#define MAX9271_HVEN			BIT(2)
> > > > +#define MAX9271_EDC_1BIT_PARITY		(0 << 0)
> > > > +#define MAX9271_EDC_6BIT_CRC		(1 << 0)
> > > > +#define MAX9271_EDC_6BIT_HAMMING	(2 << 0)
> > > > +/* Register 0x08 */
> > > > +#define MAX9271_INVVS			BIT(7)
> > > > +#define MAX9271_INVHS			BIT(6)
> > > > +#define MAX9271_REV_LOGAIN		BIT(3)
> > > > +#define MAX9271_REV_HIVTH		BIT(0)
> > > > +/* Register 0x09 */
> > > > +#define MAX9271_ID			0x09
> > > > +/* Register 0x0d */
> > > > +#define MAX9271_I2CLOCACK		BIT(7)
> > > > +#define MAX9271_I2CSLVSH_1046NS_469NS	(3 << 5)
> > > > +#define MAX9271_I2CSLVSH_938NS_352NS	(2 << 5)
> > > > +#define MAX9271_I2CSLVSH_469NS_234NS	(1 << 5)
> > > > +#define MAX9271_I2CSLVSH_352NS_117NS	(0 << 5)
> > > > +#define MAX9271_I2CMSTBT_837KBPS	(7 << 2)
> > > > +#define MAX9271_I2CMSTBT_533KBPS	(6 << 2)
> > > > +#define MAX9271_I2CMSTBT_339KBPS	(5 << 2)
> > > > +#define MAX9271_I2CMSTBT_173KBPS	(4 << 2)
> > > > +#define MAX9271_I2CMSTBT_105KBPS	(3 << 2)
> > > > +#define MAX9271_I2CMSTBT_84KBPS		(2 << 2)
> > > > +#define MAX9271_I2CMSTBT_28KBPS		(1 << 2)
> > > > +#define MAX9271_I2CMSTBT_8KBPS		(0 << 2)
> > > > +#define MAX9271_I2CSLVTO_NONE		(3 << 0)
> > > > +#define MAX9271_I2CSLVTO_1024US		(2 << 0)
> > > > +#define MAX9271_I2CSLVTO_256US		(1 << 0)
> > > > +#define MAX9271_I2CSLVTO_64US		(0 << 0)
> > > > +/* Register 0x0f */
> > > > +#define MAX9271_GPIO5OUT		BIT(5)
> > > > +#define MAX9271_GPIO4OUT		BIT(4)
> > > > +#define MAX9271_GPIO3OUT		BIT(3)
> > > > +#define MAX9271_GPIO2OUT		BIT(2)
> > > > +#define MAX9271_GPIO1OUT		BIT(1)
> > > > +#define MAX9271_GPO			BIT(0)
> > > > +/* Register 0x15 */
> > > > +#define MAX9271_PCLKDET			BIT(0)
> > > > +
> > > > +struct max9271_device {
> > > > +	struct device			*dev;
> > > > +	struct i2c_client		*client;
> > > > +	struct v4l2_subdev		sd;
> > > > +#define MAX9271_SOURCE_PAD	0
> > > > +#define MAX9271_SINK_PAD	1
> > > > +	struct media_pad		pads[2];
> > > > +	struct v4l2_async_notifier	notifier;
> > > > +	struct v4l2_async_subdev	*asd;
> > > > +	struct v4l2_ctrl_handler	ctrls;
> > > > +	struct v4l2_subdev		*sensor;
> > > > +};
> > > > +
> > > > +static inline struct max9271_device *sd_to_max9271(struct v4l2_subdev *sd)
> > > > +{
> > > > +	return container_of(sd, struct max9271_device, sd);
> > > > +}
> > > > +
> > > > +static inline struct max9271_device *i2c_to_max9271(struct i2c_client *client)
> > > > +{
> > > > +	return sd_to_max9271(i2c_get_clientdata(client));
> > > > +}
> > > > +
> > > > +static inline struct max9271_device *notifier_to_max9271(
> > > > +						struct v4l2_async_notifier *nf)
> > > > +{
> > > > +	return container_of(nf, struct max9271_device, notifier);
> > > > +}
> > > > +
> > > > +/* --- MAX9271 hardware operations --- */
> > > > +
> > > > +static int max9271_read(struct max9271_device *dev, u8 reg)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	dev_dbg(&dev->client->dev, "%s(0x%02x)\n", __func__, reg);
> > > > +
> > > > +	ret = i2c_smbus_read_byte_data(dev->client, reg);
> > > > +	if (ret < 0)
> > > > +		dev_dbg(&dev->client->dev,
> > > > +			"%s: register 0x%02x read failed (%d)\n",
> > > > +			__func__, reg, ret);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	dev_dbg(&dev->client->dev, "%s(0x%02x, 0x%02x)\n", __func__, reg, val);
> > > > +
> > > > +	ret = i2c_smbus_write_byte_data(dev->client, reg, val);
> > > > +	if (ret < 0)
> > > > +		dev_err(&dev->client->dev,
> > > > +			"%s: register 0x%02x write failed (%d)\n",
> > > > +			__func__, reg, ret);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +/*
> > > > + * max9271_pclk_detect() - Detect valid pixel clock from image sensor
> > > > + *
> > > > + * Wait up to 10ms for a valid pixel clock.
> > > > + *
> > > > + * Returns 0 for success, < 0 for pixel clock not properly detected
> > > > + */
> > > > +static int max9271_pclk_detect(struct max9271_device *dev)
> > > > +{
> > > > +	unsigned int i;
> > > > +	int ret;
> > > > +
> > > > +	for (i = 0; i < 100; i++) {
> > > > +		ret = max9271_read(dev, 0x15);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +
> > > > +		if (ret & MAX9271_PCLKDET)
> > > > +			return 0;
> > > > +
> > > > +		usleep_range(50, 100);
> > > > +	}
> > > > +
> > > > +	dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n");
> > > > +
> > > > +	return -EIO;
> > > > +}
> > > > +
> > > > +static void max9271_wake_up(struct max9271_device *dev)
> > > > +{
> > > > +	/*
> > > > +	 * Use the chip default address as this function has to be called
> > > > +	 * before any other one.
> > > > +	 */
> > > > +	dev->client->addr = MAX9271_DEFAULT_ADDR;
> > > > +	i2c_smbus_read_byte(dev->client);
> > > > +	usleep_range(5000, 8000);
> > > > +}
> > > > +
> > > > +static int max9271_set_serial_link(struct max9271_device *dev, bool enable)
> > > > +{
> > > > +	int ret;
> > > > +	u8 val = MAX9271_REVCCEN | MAX9271_FWDCCEN;
> > > > +
> > > > +	if (enable) {
> > > > +		ret = max9271_pclk_detect(dev);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		val |= MAX9271_SEREN;
> > > > +	} else {
> > > > +		val |= MAX9271_CLINKEN;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * The serializer temporarily disables the reverse control channel for
> > > > +	 * 350µs after starting/stopping the forward serial link, but the
> > > > +	 * deserializer synchronization time isn't clearly documented.
> > > > +	 *
> > > > +	 * According to the serializer datasheet we should wait 3ms, while
> > > > +	 * according to the deserializer datasheet we should wait 5ms.
> > > > +	 *
> > > > +	 * Short delays here appear to show bit-errors in the writes following.
> > > > +	 * Therefore a conservative delay seems best here.
> > > > +	 */
> > > > +	ret = max9271_write(dev, 0x04, val);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	usleep_range(5000, 8000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_configure_i2c(struct max9271_device *dev, u8 i2c_config)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_write(dev, 0x0d, i2c_config);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	/* The delay required after an I2C bus configuration change is not
> > >
> > > /*
> > >  * The delay ... ?
> > >
> >
> > Ah, ups
> >
> > >
> > >
> > > > +	 * characterized in the serializer manual. Sleep up to 5msec to
> > > > +	 * stay safe.
> > > > +	 */
> > > > +	usleep_range(3500, 5000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_read(dev, 0x08);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Enable or disable reverse channel high threshold to increase
> > > > +	 * immunity to power supply noise.
> > > > +	 */
> > > > +	ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	usleep_range(2000, 2500);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_configure_gmsl_link(struct max9271_device *dev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	/*
> > > > +	 * Configure the GMSL link:
> > > > +	 *
> > > > +	 * - Double input mode, high data rate, 24-bit mode
> > > > +	 * - Latch input data on PCLKIN rising edge
> > > > +	 * - Enable HS/VS encoding
> > > > +	 * - 1-bit parity error detection
> > > > +	 *
> > > > +	 * TODO: Make the GMSL link configuration parametric.
> > > > +	 */
> > > > +	ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
> > > > +			    MAX9271_EDC_1BIT_PARITY);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	usleep_range(5000, 8000);
> > > > +
> > > > +	/*
> > > > +	 * Adjust spread spectrum to +4% and auto-detect pixel clock
> > > > +	 * and serial link rate.
> > > > +	 */
> > > > +	ret = max9271_write(dev, 0x02,
> > > > +			    MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
> > > > +			    MAX9271_PCLK_AUTODETECT |
> > > > +			    MAX9271_SERIAL_AUTODETECT);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	usleep_range(5000, 8000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_set_gpios(struct max9271_device *dev, u8 gpio_mask)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_read(dev, 0x0f);
> > >
> > > What is 0x0f?
> > >
> > > Ah - the registers are unnamed...
> > >
> > > Seems a shame..
> >
> > It is a bit. If the overall approach is validated, I'll go and
> > beautify the driver.
> >
> > >
> > >
> > > > +	if (ret < 0)
> > > > +		return 0;
> > > > +
> > > > +	ret |= gpio_mask;
> > > > +	ret = max9271_write(dev, 0x0f, ret);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&dev->client->dev, "Failed to set gpio (%d)\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	usleep_range(3500, 5000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_clear_gpios(struct max9271_device *dev, u8 gpio_mask)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_read(dev, 0x0f);
> > > > +	if (ret < 0)
> > > > +		return 0;
> > > > +
> > > > +	ret &= ~gpio_mask;
> > > > +	ret = max9271_write(dev, 0x0f, ret);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&dev->client->dev, "Failed to clear gpio (%d)\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	usleep_range(3500, 5000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_enable_gpios(struct max9271_device *dev, u8 gpio_mask)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_read(dev, 0x0e);
> > > > +	if (ret < 0)
> > > > +		return 0;
> > > > +
> > > > +	/* BIT(0) reserved: GPO is always enabled. */
> > > > +	ret |= (gpio_mask & ~BIT(0));
> > > > +	ret = max9271_write(dev, 0x0e, ret);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&dev->client->dev, "Failed to enable gpio (%d)\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	usleep_range(3500, 5000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_verify_id(struct max9271_device *dev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_read(dev, 0x1e);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&dev->client->dev, "MAX9271 ID read failed (%d)\n",
> > > > +			ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (ret != MAX9271_ID) {
> > > > +		dev_err(&dev->client->dev, "MAX9271 ID mismatch (0x%02x)\n",
> > > > +			ret);
> > > > +		return -ENXIO;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_set_address(struct max9271_device *dev, u8 addr)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_write(dev, 0x00, addr << 1);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&dev->client->dev,
> > > > +			"MAX9271 I2C address change failed (%d)\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +	usleep_range(3500, 5000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/* --- V4L2 Subdev Ops --- */
> > > > +
> > > > +static int max9271_s_stream(struct v4l2_subdev *sd, int enable)
> > > > +{
> > > > +	struct max9271_device *max9271 = sd_to_max9271(sd);
> > > > +
> > > > +	return max9271_set_serial_link(max9271, enable);
> > > > +}
> > > > +
> > > > +static int max9271_enum_mbus_code(struct v4l2_subdev *sd,
> > > > +				  struct v4l2_subdev_state *sd_state,
> > > > +				  struct v4l2_subdev_mbus_code_enum *code)
> > > > +{
> > > > +	struct max9271_device *max9271 = sd_to_max9271(sd);
> > > > +
> > > > +	return v4l2_subdev_call(max9271->sensor, pad, enum_mbus_code, NULL,
> > > > +				code);
> > > > +}
> > > > +
> > > > +static int max9271_get_fmt(struct v4l2_subdev *sd,
> > > > +			   struct v4l2_subdev_state *sd_state,
> > > > +			   struct v4l2_subdev_format *format)
> > > > +{
> > > > +	struct max9271_device *max9271 = sd_to_max9271(sd);
> > > > +
> > > > +	return v4l2_subdev_call(max9271->sensor, pad, get_fmt, NULL,
> > > > +				format);
> > > > +}
> > > > +
> > > > +static int max9271_set_fmt(struct v4l2_subdev *sd,
> > > > +			   struct v4l2_subdev_state *sd_state,
> > > > +			   struct v4l2_subdev_format *format)
> > > > +{
> > > > +	struct max9271_device *max9271 = sd_to_max9271(sd);
> > > > +
> > > > +	return v4l2_subdev_call(max9271->sensor, pad, set_fmt, NULL,
> > > > +				format);
> > > > +}
> > > > +
> > > > +static int max9271_post_register(struct v4l2_subdev *sd)
> > > > +{
> > > > +	struct max9271_device *max9271 = sd_to_max9271(sd);
> > > > +	int ret;
> > > > +
> > > > +	ret = max9271_verify_id(max9271);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = max9271_enable_gpios(max9271, MAX9271_GPIO1OUT);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = max9271_configure_gmsl_link(max9271);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct v4l2_subdev_video_ops max9271_video_ops = {
> > > > +	.s_stream	= max9271_s_stream,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_pad_ops max9271_subdev_pad_ops = {
> > > > +	.enum_mbus_code = max9271_enum_mbus_code,
> > > > +	.get_fmt	= max9271_get_fmt,
> > > > +	.set_fmt	= max9271_set_fmt,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_core_ops max9271_core_ops = {
> > > > +	.post_register	= max9271_post_register,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_ops max9271_subdev_ops = {
> > > > +	.core		= &max9271_core_ops,
> > > > +	.video		= &max9271_video_ops,
> > > > +	.pad		= &max9271_subdev_pad_ops,
> > > > +};
> > > > +
> > > > +/* --- V4L2 Async Notifier --- */
> > > > +
> > > > +static int max9271_notify_bound(struct v4l2_async_notifier *notifier,
> > > > +				struct v4l2_subdev *subdev,
> > > > +				struct v4l2_async_subdev *asd)
> > > > +{
> > > > +	struct max9271_device *max9271 = notifier_to_max9271(notifier);
> > > > +	int ret, pad;
> > > > +
> > > > +	/*
> > > > +	 * Reserve more space than necessary for controls inherited by the
> > > > +	 * remote subdev.
> > > > +	 */
> > > > +	ret = v4l2_ctrl_handler_init(&max9271->ctrls, 16);
> > > > +	if (ret < 0) {
> > > > +		dev_err(max9271->dev,
> > > > +			"Unable to initialize control handler: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = v4l2_ctrl_add_handler(&max9271->ctrls, subdev->ctrl_handler,
> > > > +				    NULL, true);
> > > > +	if (ret < 0) {
> > > > +		dev_err(max9271->dev,
> > > > +			"Unable to add subdev control handler: %d\n", ret);
> > > > +		goto error_free_handler;
> > > > +	}
> > > > +	max9271->sd.ctrl_handler = &max9271->ctrls;
> > > > +
> > > > +	/* Create media link with the remote sensor source pad. */
> > > > +	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
> > > > +					  MEDIA_PAD_FL_SOURCE);
> > > > +	if (pad < 0) {
> > > > +		dev_err(max9271->dev,
> > > > +			"Failed to find source pad for %s\n", subdev->name);
> > > > +		ret = pad;
> > > > +		goto error_free_handler;
> > > > +	}
> > > > +
> > > > +	ret = media_create_pad_link(&subdev->entity, pad,
> > > > +				    &max9271->sd.entity, MAX9271_SINK_PAD,
> > > > +				    MEDIA_LNK_FL_ENABLED |
> > > > +				    MEDIA_LNK_FL_IMMUTABLE);
> > > > +	if (ret)
> > > > +		goto error_free_handler;
> > > > +
> > > > +	max9271->sensor = subdev;
> > > > +
> > > > +	/*
> > > > +	 * Hold OV10635 in reset during max9271 configuration. The reset signal
> > >
> > > Hold the sensor in reset ...
> > >
> > > But are all sensors guaranteed to be connected to the GPIO in the same way?
> > >
> > > Will this cause us issues? - does the GPIO need to be modelled somehow
> > > to determine what's connected to them?
> > >
> > > > +	 * has to be asserted for at least 200 microseconds.
> > > > +	 */
> > > > +	ret = max9271_clear_gpios(max9271, MAX9271_GPIO1OUT);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	usleep_range(200, 500);
> > > > +
> > > > +	/*
> > > > +	 * Release ov10635 from reset and initialize it. The image sensor
> > > > +	 * requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
> > > > +	 * before being available. Stay safe and wait up to 500 micro-seconds.
> > >
> > > More sensor specific ... what needs to be abstracted here?
> >
> > Yes indeed. As pointed out in the cover letter the handling of the
> > sensor's reset line should go through the gpiod API, and will probably
> > require installing a gpiochip in the max9271 driver.
>
> What would be the point though ? I really think this isn't worth it.

That I might agree

> This could would btw need to move to the sensor driver, we don't want to
> encode the reset duration in the max9271 driver, and we don't want to
> specify it in DT either.

Installing a gpio-controller on the max9271 and having the sensor
driver handling its reset is what I was suggesting.

Thanks
   j

>
> > > > +	 */
> > > > +	ret = max9271_set_gpios(max9271, MAX9271_GPIO1OUT);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	usleep_range(100, 500);
> > > > +
> > > > +	/*
> > > > +	 * Call the sensor post_register operation to complete its
> > > > +	 * initialization.
> > > > +	 */
> > > > +	ret = v4l2_subdev_call(max9271->sensor, core, post_register);
> > > > +	if (ret) {
> > > > +		dev_err(max9271->dev, "Failed to initialize sensor %u\n", ret);
> > > > +		goto error_remove_link;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +
> > > > +error_remove_link:
> > > > +	media_entity_remove_links(&max9271->sd.entity);
> > > > +	max9271->sensor = NULL;
> > > > +
> > > > +error_free_handler:
> > > > +	v4l2_ctrl_handler_free(&max9271->ctrls);
> > > > +	max9271->sd.ctrl_handler = NULL;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void max9271_notify_unbind(struct v4l2_async_notifier *notifier,
> > > > +				  struct v4l2_subdev *subdev,
> > > > +				  struct v4l2_async_subdev *asd)
> > > > +{
> > > > +	struct max9271_device *max9271 = notifier_to_max9271(notifier);
> > > > +
> > > > +	media_entity_remove_links(&max9271->sd.entity);
> > > > +	max9271->sensor = NULL;
> > > > +}
> > > > +
> > > > +static const struct v4l2_async_notifier_operations max9271_notifier_ops = {
> > > > +	.bound = max9271_notify_bound,
> > > > +	.unbind = max9271_notify_unbind,
> > > > +};
> > > > +
> > > > +static int max9271_parse_dt(struct max9271_device *max9271)
> > > > +{
> > > > +	struct fwnode_handle *ep, *remote;
> > > > +	struct v4l2_fwnode_endpoint vep = {
> > > > +		.bus_type = V4L2_MBUS_PARALLEL,
> > > > +	};
> > > > +	int ret;
> > > > +
> > > > +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(max9271->dev), 1, 0, 0);
> > > > +	if (!ep) {
> > > > +		dev_err(max9271->dev, "Unable to get sensor endpoint: %pOF\n",
> > > > +			max9271->dev->of_node);
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	remote = fwnode_graph_get_remote_endpoint(ep);
> > > > +	if (!remote) {
> > > > +		dev_err(max9271->dev, "Unable to get remote endpoint: %pOF\n",
> > > > +			max9271->dev->of_node);
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > > +	fwnode_handle_put(ep);
> > > > +	if (ret) {
> > > > +		fwnode_handle_put(remote);
> > > > +		dev_err(max9271->dev, "Unable to parse endpoint: %pOF\n",
> > > > +			to_of_node(ep));
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	v4l2_async_notifier_init(&max9271->notifier);
> > > > +	max9271->asd = v4l2_async_notifier_add_fwnode_subdev(&max9271->notifier,
> > > > +					      remote, struct v4l2_async_subdev);
> > > > +	fwnode_handle_put(remote);
> > > > +	if (IS_ERR(max9271->asd))
> > > > +		return PTR_ERR(max9271->asd);
> > > > +
> > > > +	max9271->notifier.ops = &max9271_notifier_ops;
> > > > +	max9271->notifier.flags = V4L2_ASYNC_NOTIFIER_DEFER_POST_REGISTER;
> > >
> > > This looks new, I'll have to read up on it ...
> >
> > It is. I am pushing to get feedback on this since a few months now.
> > If you wish to help:
> > https://patchwork.linuxtv.org/project/linux-media/list/?series=5847
> >
> > > > +	ret = v4l2_async_subdev_notifier_register(&max9271->sd,
> > > > +						  &max9271->notifier);
> > > > +	if (ret < 0) {
> > > > +		v4l2_async_notifier_cleanup(&max9271->notifier);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9271_init(struct max9271_device *max9271)
> > > > +{
> > > > +	int ret;
> > > > +	u8 addr;
> > > > +
> > > > +	max9271_wake_up(max9271);
> > >
> > > This call has just set max9271->client->addr = MAX9271_DEFAULT_ADDR, so
> > > the original has now been lost.
> >
> > Doh! I overlooked the fact max9271_wake_up() silently overwrites the
> > i2c client address. For the reasons explained in the cover letter I've
> > been able to test this patch with a single camera only, so the issue
> > went unnoticed here. Thanks for spotting!
> >
> > > > +
> > > > +	/* Re-program the chip address. */
> > > > +	addr = max9271->client->addr;
> > > > +	max9271->client->addr = MAX9271_DEFAULT_ADDR;
> > > > +	ret = max9271_set_address(max9271, addr);
> > >
> > > So this is currently broken :S
> > >
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +	max9271->client->addr = addr;
> > > > +
> > > > +	/* Serial link disabled during conf as it needs a valid pixel clock. */
> > > > +	ret = max9271_set_serial_link(max9271, false);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 *  Ensure that we have a good link configuration before attempting to
> > > > +	 *  identify the device.
> > > > +	 */
> > > > +	ret = max9271_configure_i2c(max9271, MAX9271_I2CSLVSH_469NS_234NS |
> > > > +					     MAX9271_I2CSLVTO_1024US |
> > > > +					     MAX9271_I2CMSTBT_105KBPS);
> > >
> > > Are these parameters tied to the max9286 ?
> >
> > More to the I2c bus configuration and should probably be made
> > configurable through the canonical i2c DTS properties.
> >
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Set reverse channel high threshold to increase noise immunity.
> > > > +	 *
> > > > +	 * This should be compensated by increasing the reverse channel
> > > > +	 * amplitude on the remote deserializer side.
> > > > +	 */
> > > > +	return max9271_set_high_threshold(max9271, true);
> > > > +}
> > > > +
> > > > +static int max9271_probe(struct i2c_client *client)
> > > > +{
> > > > +	struct max9271_device *max9271;
> > > > +	struct fwnode_handle *ep;
> > > > +	int ret;
> > > > +
> > > > +	max9271 = devm_kzalloc(&client->dev, sizeof(*max9271), GFP_KERNEL);
> > > > +	if (!max9271)
> > > > +		return -ENOMEM;
> > > > +	max9271->dev = &client->dev;
> > > > +	max9271->client = client;
> > > > +
> > > > +	/* Initialize and register the subdevice. */
> > > > +	v4l2_i2c_subdev_init(&max9271->sd, client, &max9271_subdev_ops);
> > > > +	max9271->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > > +	max9271->pads[MAX9271_SOURCE_PAD].flags = MEDIA_PAD_FL_SOURCE;
> > > > +	max9271->pads[MAX9271_SINK_PAD].flags = MEDIA_PAD_FL_SINK;
> > > > +	max9271->sd.entity.flags |= MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > >
> > > Is it a formatter - do we need a new/different entity type?
> >
> > I admit I had an hard time choosing the correct entity type :)
> >
> > > > +	ret = media_entity_pads_init(&max9271->sd.entity, 2, max9271->pads);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(max9271->dev), 0, 0, 0);
> > > > +	if (!ep) {
> > > > +		dev_err(max9271->dev, "Unable to get endpoint 0: %pOF\n",
> > > > +			max9271->dev->of_node);
> > > > +		ret = -ENODEV;
> > > > +		goto error_media_entity;
> > > > +	}
> > > > +
> > > > +	max9271->sd.fwnode = ep;
> > > > +	ret = v4l2_async_register_subdev(&max9271->sd);
> > > > +	if (ret)
> > > > +		goto error_put_node;
> > > > +
> > > > +	ret = max9271_parse_dt(max9271);
> > > > +	if (ret)
> > > > +		goto error_unregister_subdev;
> > > > +
> > > > +	ret = max9271_init(max9271);
> > > > +	if (ret)
> > > > +		goto error_unregister_subdev;
> > > > +
> > > > +	return 0;
> > > > +
> > > > +error_unregister_subdev:
> > > > +	v4l2_async_unregister_subdev(&max9271->sd);
> > > > +error_put_node:
> > > > +	fwnode_handle_put(max9271->sd.fwnode);
> > > > +error_media_entity:
> > > > +	media_entity_cleanup(&max9271->sd.entity);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int max9271_remove(struct i2c_client *client)
> > > > +{
> > > > +	struct max9271_device *max9271 = i2c_to_max9271(client);
> > > > +
> > > > +	v4l2_ctrl_handler_free(&max9271->ctrls);
> > > > +	v4l2_async_notifier_cleanup(&max9271->notifier);
> > > > +	v4l2_async_unregister_subdev(&max9271->sd);
> > > > +	fwnode_handle_put(max9271->sd.fwnode);
> > > > +	media_entity_cleanup(&max9271->sd.entity);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id max9271_of_ids[] = {
> > > > +	{ .compatible = "imi,max9271", },
> > >
> > > This should be a 'maxim' prefix now, not IMI.
> > >
> > > > +	{ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, max9271_of_ids);
> > > > +
> > > > +static struct i2c_driver max9271_i2c_driver = {
> > > > +	.driver	= {
> > > > +		.name	= "max9271",
> > > > +		.of_match_table = max9271_of_ids,
> > > > +	},
> > > > +	.probe_new	= max9271_probe,
> > > > +	.remove		= max9271_remove,
> > > > +};
> > > > +
> > > > +module_i2c_driver(max9271_i2c_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("MAX9271 GMSL serializer subdevice driver");
> > > > +MODULE_AUTHOR("Jacopo Mondi");
> > > > +MODULE_LICENSE("GPL");
>
> --
> Regards,
>
> Laurent Pinchart