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

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

On 18/08/2021 09:27, 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 ?

Hahah maybe.

>>> 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.

Indeed, well they don't have names in the datasheet was my point, but I
guess we could give them names of their expected usage ...




>>> +	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.

Ok - I missed that, sorry.



>>> +	 */
>>> +	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!

That's what extra eyes are for ;-)


>>> +
>>> +	/* 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.

Ok


>>> +	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 :)

Yes, I suspect it might just be that it's a distinct new thing? ...


>>> +	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.
>>
> 
> Indeed!
> 
> Thanks
>   j
> 
>>
>>> +	{ }
>>> +};
>>> +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");
>>>