mbox series

[v3,00/12] media: i2c: max9286: Small new features

Message ID 20221214233825.13050-1-laurent.pinchart+renesas@ideasonboard.com
Headers show
Series media: i2c: max9286: Small new features | expand

Message

Laurent Pinchart Dec. 14, 2022, 11:38 p.m. UTC 
Hello,

This small patch series adds a few new features to the max9286 driver:

- Support for per-port supplies (01/12 and 04/12)
- Remote I2C bus speed selection (02/12 and 09/12)
- GMSL bus width selection (03/12 and 10/12)
- Manual framesync operation (05/12)
- RAW12 support (06/12 and 07/12)

The remaining patches are small cleanups. Please see individual patches
for details.

Compared to v2, I've incorporated all review comments and rebased the
series on top of the latest media tree (with a notable conflict due to
the PoC GPIO support that has been merged in the mainline kernel). Most
of v2 has received Reviewed-by tags, only a few patches are missing
them, so I have good hopes to land this in v6.3.

Laurent Pinchart (11):
  dt-bindings: media: i2c: max9286: Add support for per-port supplies
  dt-bindings: media: i2c: max9286: Add property to select I2C speed
  dt-bindings: media: i2c: max9286: Add property to select bus width
  media: i2c: max9286: Support manual framesync operation
  media: i2c: max9286: Rename MAX9286_DATATYPE_RAW11 to RAW12
  media: i2c: max9286: Support 12-bit raw bayer formats
  media: i2c: max9286: Define macros for all bits of register 0x15
  media: i2c: max9286: Configure remote I2C speed from device tree
  media: i2c: max9286: Configure bus width from device tree
  media: i2c: max9286: Select HS as data enable signal
  media: i2c: max9286: Print power-up GMSL link configuration

Thomas Nizan (1):
  media: i2c: max9286: Add support for port regulators

 .../bindings/media/i2c/maxim,max9286.yaml     |  51 +-
 drivers/media/i2c/max9286.c                   | 465 +++++++++++++++---
 2 files changed, 430 insertions(+), 86 deletions(-)


base-commit: 3178804c64ef7c8c87a53cd5bba0b2942dd64fec

Comments

Jacopo Mondi Dec. 16, 2022, 9:58 a.m. UTC | #1 
Hi Laurent

On Thu, Dec 15, 2022 at 01:38:20AM +0200, Laurent Pinchart wrote:
> Add support for 12-bit raw bayer formats to the driver, configuring the
> GMSL format accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
> Changes since v1:
>
> - Fix incorrect array index when media bus code isn't found
> ---
>  drivers/media/i2c/max9286.c | 128 ++++++++++++++++++++++++++----------
>  1 file changed, 95 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 028fa3547282..3f1228c5053b 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -136,6 +136,11 @@
>  #define MAX9286_N_PADS			5
>  #define MAX9286_SRC_PAD			4
>
> +struct max9286_format_info {
> +	u32 code;
> +	u8 datatype;
> +};
> +
>  struct max9286_source {
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> @@ -218,6 +223,34 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
>  	return container_of(sd, struct max9286_priv, sd);
>  }
>
> +static const struct max9286_format_info max9286_formats[] = {
> +	{
> +		.code = MEDIA_BUS_FMT_UYVY8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_VYUY8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_YUYV8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_YVYU8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	},
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * I2C IO
>   */
> @@ -479,6 +512,38 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  	return 0;
>  }
>
> +static void max9286_set_video_format(struct max9286_priv *priv,
> +				     const struct v4l2_mbus_framefmt *format)
> +{
> +	const struct max9286_format_info *info = NULL;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> +		if (max9286_formats[i].code == format->code) {
> +			info = &max9286_formats[i];
> +			break;
> +		}
> +	}
> +
> +	if (WARN_ON(!info))
> +		return;
> +
> +	/*
> +	 * Video format setup:
> +	 * Disable CSI output, VC is set according to Link number.
> +	 */
> +	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> +
> +	/* Enable CSI-2 Lane D0-D3 only, DBL mode. */
> +	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
> +		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> +		      info->datatype);
> +
> +	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> +	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> +		      MAX9286_HVSRC_D14);
> +}
> +
>  static void max9286_set_fsync_period(struct max9286_priv *priv)
>  {
>  	u32 fsync;
> @@ -697,6 +762,15 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>
>  	if (enable) {
> +		const struct v4l2_mbus_framefmt *format;
> +
> +		/*
> +		 * Get the format from the first used sink pad, as all sink
> +		 * formats must be identical.
> +		 */
> +		format = &priv->fmt[__ffs(priv->bound_sources)];
> +
> +		max9286_set_video_format(priv, format);
>  		max9286_set_fsync_period(priv);
>
>  		/*
> @@ -817,22 +891,20 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
> +	unsigned int i;
>
>  	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;
>
> -	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
> -	switch (format->format.code) {
> -	case MEDIA_BUS_FMT_UYVY8_1X16:
> -	case MEDIA_BUS_FMT_VYUY8_1X16:
> -	case MEDIA_BUS_FMT_YUYV8_1X16:
> -	case MEDIA_BUS_FMT_YVYU8_1X16:
> -		break;
> -	default:
> -		format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> -		break;
> +	/* Validate the format. */
> +	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> +		if (max9286_formats[i].code == format->format.code)
> +			break;
>  	}
>
> +	if (i == ARRAY_SIZE(max9286_formats))
> +		format->format.code = max9286_formats[0].code;
> +
>  	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
>  					 format->which);
>  	if (!cfg_fmt)
> @@ -890,16 +962,20 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
>  	.pad		= &max9286_pad_ops,
>  };
>
> +static const struct v4l2_mbus_framefmt max9286_default_format = {
> +	.width		= 1280,
> +	.height		= 800,
> +	.code		= MEDIA_BUS_FMT_UYVY8_1X16,
> +	.colorspace	= V4L2_COLORSPACE_SRGB,
> +	.field		= V4L2_FIELD_NONE,
> +	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> +	.quantization	= V4L2_QUANTIZATION_DEFAULT,
> +	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> +};
> +
>  static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
>  {
> -	fmt->width		= 1280;
> -	fmt->height		= 800;
> -	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
> -	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
> -	fmt->field		= V4L2_FIELD_NONE;
> -	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
> -	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
> -	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
> +	*fmt = max9286_default_format;
>  }
>
>  static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> @@ -1063,23 +1139,9 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
>  	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
>
> -	/*
> -	 * Video format setup:
> -	 * Disable CSI output, VC is set according to Link number.
> -	 */
> -	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> -
> -	/* Enable CSI-2 Lane D0-D3 only, DBL mode, YUV422 8-bit. */
> -	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
> -		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> -		      MAX9286_DATATYPE_YUV422_8BIT);
> -
> +	max9286_set_video_format(priv, &max9286_default_format);
>  	max9286_set_fsync_period(priv);
>
> -	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> -	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> -		      MAX9286_HVSRC_D14);
> -
>  	/*
>  	 * The overlap window seems to provide additional validation by tracking
>  	 * the delay between vsync and frame sync, generating an error if the
> --
> Regards,
>
> Laurent Pinchart
>
Jacopo Mondi Dec. 16, 2022, 10:03 a.m. UTC | #2 
Hi Laurent

On Thu, Dec 15, 2022 at 01:38:22AM +0200, Laurent Pinchart wrote:
> Read the maxim,i2c-clock-frequency DT property that specifies the speed
> of the remote I2C bus, and configure the MAX9286 accordingly. The remote
> serializers must all have a matching configuration.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
>
> - Rename DT property to "maxim,i2c-remote-bus-hz"
> ---
>  drivers/media/i2c/max9286.c | 56 +++++++++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index e78456c8d24c..fffb0d2da416 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -144,6 +144,11 @@ struct max9286_format_info {
>  	u8 datatype;
>  };
>
> +struct max9286_i2c_speed {
> +	u32 rate;
> +	u8 mstbt;
> +};
> +
>  struct max9286_source {
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> @@ -177,6 +182,7 @@ struct max9286_priv {
>  	/* The initial reverse control channel amplitude. */
>  	u32 init_rev_chan_mv;
>  	u32 rev_chan_mv;
> +	u8 i2c_mstbt;
>
>  	bool use_gpio_poc;
>  	u32 gpio_poc[2];
> @@ -254,6 +260,17 @@ static const struct max9286_format_info max9286_formats[] = {
>  	},
>  };
>
> +static const struct max9286_i2c_speed max9286_i2c_speeds[] = {
> +	{ .rate =   8470, .mstbt = MAX9286_I2CMSTBT_8KBPS },
> +	{ .rate =  28300, .mstbt = MAX9286_I2CMSTBT_28KBPS },
> +	{ .rate =  84700, .mstbt = MAX9286_I2CMSTBT_84KBPS },
> +	{ .rate = 105000, .mstbt = MAX9286_I2CMSTBT_105KBPS },
> +	{ .rate = 173000, .mstbt = MAX9286_I2CMSTBT_173KBPS },
> +	{ .rate = 339000, .mstbt = MAX9286_I2CMSTBT_339KBPS },
> +	{ .rate = 533000, .mstbt = MAX9286_I2CMSTBT_533KBPS },
> +	{ .rate = 837000, .mstbt = MAX9286_I2CMSTBT_837KBPS },
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * I2C IO
>   */
> @@ -374,7 +391,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv)
>  static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
>  {
>  	u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
> -		    MAX9286_I2CMSTBT_105KBPS;
> +		    priv->i2c_mstbt;
>
>  	if (localack)
>  		config |= MAX9286_I2CLOCACK;
> @@ -1387,6 +1404,8 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	struct device_node *node = NULL;
>  	unsigned int i2c_mux_mask = 0;
>  	u32 reverse_channel_microvolt;
> +	u32 i2c_clk_freq = 105000;
> +	unsigned int i;
>
>  	/* Balance the of_node_put() performed by of_find_node_by_name(). */
>  	of_node_get(dev->of_node);
> @@ -1477,6 +1496,23 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	}
>  	of_node_put(node);
>
> +	of_property_read_u32(dev->of_node, "maxim,i2c-remote-bus-hz",
> +			     &i2c_clk_freq);

i2c_clk_freq is only overwritten if the property is present, so I
guess this is right.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> +	for (i = 0; i < ARRAY_SIZE(max9286_i2c_speeds); ++i) {
> +		const struct max9286_i2c_speed *speed = &max9286_i2c_speeds[i];
> +
> +		if (speed->rate == i2c_clk_freq) {
> +			priv->i2c_mstbt = speed->mstbt;
> +			break;
> +		}
> +	}
> +
> +	if (i == ARRAY_SIZE(max9286_i2c_speeds)) {
> +		dev_err(dev, "Invalid %s value %u\n", "maxim,i2c-remote-bus-hz",
> +			i2c_clk_freq);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Parse the initial value of the reverse channel amplitude from
>  	 * the firmware interface and convert it to millivolts.
> @@ -1553,10 +1589,16 @@ static int max9286_probe(struct i2c_client *client)
>  	/* GPIO values default to high */
>  	priv->gpio_state = BIT(0) | BIT(1);
>
> +	ret = max9286_parse_dt(priv);
> +	if (ret)
> +		goto err_cleanup_dt;
> +
>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>  						   GPIOD_OUT_HIGH);
> -	if (IS_ERR(priv->gpiod_pwdn))
> -		return PTR_ERR(priv->gpiod_pwdn);
> +	if (IS_ERR(priv->gpiod_pwdn)) {
> +		ret = PTR_ERR(priv->gpiod_pwdn);
> +		goto err_cleanup_dt;
> +	}
>
>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
> @@ -1583,10 +1625,6 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_powerdown;
>
> -	ret = max9286_parse_dt(priv);
> -	if (ret)
> -		goto err_cleanup_dt;
> -
>  	if (!priv->use_gpio_poc) {
>  		ret = max9286_get_poc_supplies(priv);
>  		if (ret)
> @@ -1599,10 +1637,10 @@ static int max9286_probe(struct i2c_client *client)
>
>  	return 0;
>
> -err_cleanup_dt:
> -	max9286_cleanup_dt(priv);
>  err_powerdown:
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> +err_cleanup_dt:
> +	max9286_cleanup_dt(priv);
>
>  	return ret;
>  }
> --
> Regards,
>
> Laurent Pinchart
>