Re: [PATCH 4/5] media/i2c: max96717: add FSYNC support
From: Julien Massot
Date: Tue Feb 18 2025 - 09:52:15 EST
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@xxxxxxxxxxx>
> ---
> 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