Hi Julien,
On Tue, Feb 18, 2025 at 03:46:02PM +0100, Julien Massot wrote:
> 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
Ok, I'll add support for pinctrl in a different patch. Though, I don't
believe a separate patch is needed for adding the gpiochip request
callback.
>
>
>
> >
> > 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.
Now that you made the point of the serializer not being aware of FSYNC,
it makes sense to change this in order to allow more forwarded pins.
>
> > };
> >
> > 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.
OK
>
> >
> > +
> > + 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.
I will have a look at this and, hopefully, come up with a separate patch
adding support for pinctrl.
>
> > +
> > + 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.
OK.
Thanks,
Laurentiu
>
> > ret = max96717_register_clkout(priv);
> > if (ret)
> > return dev_err_probe(dev, ret, "Failed to register clkout\n");
>
> Best regards,
> --
> Julien
