Re: [PATCH 4/5] media/i2c: max96717: add FSYNC support

[Julien Massot <julien.massot@collabora.com>]

Hi Laurentiu,

Thanks for your patch
On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> According to specs:
> 
> """
> Frame synchronization (FSYNC) is used to align images sent from multiple
> sources in surround-view applications and is required for concatenation.
> In FSYNC mode, the GMSL2 CSI-2 quad deserializer sends a sync signal to
> each serializer; the serializers then send the signal to the connected
> sensor.
> """
> 
> Since max96717 can be used in multi-sensor setups, we need FSYNC
> support. For that, I added a DT property("maxim,fsync-config") that will
> be used to configure the frame sync output pin and the RX ID of the
> GPIO as sent by the deserializer chip.
> 
> Also, add the .request() callback for the gpiochip so that we can use
> 'gpio-reserved-ranges' in DT to exclude the pins that are used for
> FSYNC from being used as GPIOs.
> 
> 
> I'm seeing different features in this patch:
- Adding the request callback 
- Adding GPIO forwarding 
- Adding support to some pinctrl features such as pullup/pulldown



> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/media/i2c/max96717.c | 87 ++++++++++++++++++++++++++++++++----
>  1 file changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> index 6a668a004c717..47a3be195a971 100644
> --- a/drivers/media/i2c/max96717.c
> +++ b/drivers/media/i2c/max96717.c
> @@ -70,13 +70,28 @@
>  #define MAX96717_VTX_CHKB_ALT          CCI_REG8(0x275)
>  
>  /* GPIO */
> -#define MAX96717_NUM_GPIO         11
> -#define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
> -#define MAX96717_GPIO_OUT         BIT(4)
> -#define MAX96717_GPIO_IN          BIT(3)
> -#define MAX96717_GPIO_RX_EN       BIT(2)
> -#define MAX96717_GPIO_TX_EN       BIT(1)
> -#define MAX96717_GPIO_OUT_DIS     BIT(0)
> +#define MAX96717_NUM_GPIO		11
> +#define MAX96717_GPIO_REG_A(gpio)	CCI_REG8(0x2be + (gpio) * 3)
> +#define MAX96717_GPIO_OUT		BIT(4)
> +#define MAX96717_GPIO_IN		BIT(3)
> +#define MAX96717_GPIO_RX_EN		BIT(2)
> +#define MAX96717_GPIO_TX_EN		BIT(1)
> +#define MAX96717_GPIO_OUT_DIS		BIT(0)
> +#define MAX96717_GPIO_REG_B(gpio)	CCI_REG8(0x2bf + (gpio) * 3)
> +#define MAX96717_GPIO_TX_ID_MASK	GENMASK(4, 0)
> +#define MAX96717_GPIO_TX_ID_SHIFT	0
> +#define MAX96717_OUT_TYPE		BIT(5)
> +#define MAX96717_OUT_TYPE_PUSH_PULL	BIT(5)
> +#define MAX96717_OUT_TYPE_OPEN_DRAIN	0
> +#define MAX96717_PULL_UPDN_SEL_MASK	GENMASK(7, 6)
> +#define MAX96717_PULL_UPDN_SEL_SHIFT	6
> +#define MAX96717_GPIO_NO_PULL		0
> +#define MAX96717_GPIO_PULL_UP		1
> +#define MAX96717_GPIO_PULL_DOWN		2
> +#define MAX96717_GPIO_REG_C(gpio)	CCI_REG8(0x2c0 + (gpio) * 3)
> +#define MAX96717_GPIO_RX_ID_MASK	GENMASK(4, 0)
> +#define MAX96717_GPIO_RX_ID_SHIFT	0
> +#define MAX96717_OVR_RES_CFG		BIT(7)
>  
>  /* CMU */
>  #define MAX96717_CMU_CMU2		CCI_REG8(0x0302)
> @@ -125,6 +140,11 @@ enum max96717_vpg_mode {
>  	MAX96717_VPG_GRADIENT = 2,
>  };
>  
> +struct max96717_fsync_desc {
> +	int pin;
> +	int rx_id;
> +};
> +
>  struct max96717_priv {
>  	struct i2c_client		  *client;
>  	struct regmap			  *regmap;
> @@ -141,6 +161,7 @@ struct max96717_priv {
>  	struct clk_hw                     clk_hw;
>  	struct gpio_chip                  gpio_chip;
>  	enum max96717_vpg_mode            pattern;
> +	struct max96717_fsync_desc	  fsync;
Here we can have multiple GPIOs forwarded.

>  };
>  
>  static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
> @@ -364,6 +385,7 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
>  	gc->get_direction = max96717_gpio_get_direction;
>  	gc->direction_input = max96717_gpio_direction_in;
>  	gc->direction_output = max96717_gpio_direction_out;
> +	gc->request = gpiochip_generic_request;
>  	gc->set = max96717_gpiochip_set;
>  	gc->get = max96717_gpiochip_get;
>  	gc->of_gpio_n_cells = 2;
> @@ -386,6 +408,26 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
>  	return 0;
>  }
>  
> +static int max96717_fsync_setup(struct max96717_priv *priv)
> +{
> +	int ret = 0;
> +
> +	if (priv->fsync.pin == -1)
> +		return 0;
> +
> +	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_C(priv->fsync.pin),
> +			MAX96717_GPIO_RX_ID_MASK,
> +			priv->fsync.rx_id << MAX96717_GPIO_RX_ID_SHIFT, &ret);
                        FIELD_PREP(MAX96717_GPIO_RX_ID_MASK, priv->fsync.rx_id), &ret);
And you can get rid of the _SHIFT define.

> 
> +
> +	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_B(priv->fsync.pin),
> +			MAX96717_PULL_UPDN_SEL_MASK,
> +			1 << MAX96717_PULL_UPDN_SEL_SHIFT, &ret);

The serializer can't guess what kind of pin configuration is required for the design.
This change deserves his own patch most likely implementing pinconf support.

> +
> +	return cci_update_bits(priv->regmap, MAX96717_GPIO_REG_A(priv->fsync.pin),
> +			       MAX96717_GPIO_RX_EN | MAX96717_GPIO_OUT_DIS,
> +			       MAX96717_GPIO_RX_EN, &ret);
> +}


> +
>  static int _max96717_set_routing(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *state,
>  				 struct v4l2_subdev_krouting *routing)
> @@ -1037,7 +1079,8 @@ static int max96717_parse_dt(struct max96717_priv *priv)
>  	struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
>  	struct fwnode_handle *ep_fwnode;
>  	unsigned char num_data_lanes;
> -	int ret;
> +	int ret, count;
> +	u32 dt_val[2];
>  
>  	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
>  						    MAX96717_PAD_SINK, 0, 0);
> @@ -1058,6 +1101,30 @@ static int max96717_parse_dt(struct max96717_priv *priv)
>  
>  	priv->mipi_csi2 = vep.bus.mipi_csi2;
>  
> +	priv->fsync.pin = -1;
> +	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
> +	if (count > 0) {
> +		ret = fwnode_property_read_u32_array(dev_fwnode(dev), "maxim,fsync-config",
> +						     dt_val, count);
> +		if (ret) {
> +			dev_err(dev, "Unable to read FSYNC config from DT.\n");
> +			return ret;
> +		}
> +
> +		priv->fsync.rx_id = dt_val[0];
> +		if (priv->fsync.rx_id > 31) {
> +			dev_err(dev, "Wrong GPIO RX ID. Allowed: 0 -> 31\n");
> +			return -EINVAL;
> +		}
> +
> +		priv->fsync.pin = dt_val[1];
> +		if (priv->fsync.pin >= MAX96717_NUM_GPIO) {
> +			dev_err(dev, "Wrong GPIO pin used for FSYNC. Allowed: 0 -> %d\n",
> +				MAX96717_NUM_GPIO - 1);
> +			return -EINVAL;
> +		}
> +	}
> 
> +
>  	return 0;
>  }
>  
> @@ -1092,6 +1159,10 @@ static int max96717_probe(struct i2c_client *client)
>  		return dev_err_probe(&client->dev, ret,
>  				     "Failed to init gpiochip\n");
>  
> +	ret = max96717_fsync_setup(priv);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to setup FSYNC\n");
> +
Configuring GPIO forwarding can be done when calling GPIO chip probe.

>  	ret = max96717_register_clkout(priv);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to register clkout\n");

Best regards,
-- 
Julien