Hi Julien,
On Tue, Feb 18, 2025 at 04:21:36PM +0100, Julien Massot wrote:
> Hi Laurentiu,
>
> On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> > There are situations when the CFG pins set the chip up for a certain
> > mode of operation (ie: pixel mode or tunneling mode), because the HW
> > designers decided this way, and we, the users, want to change that. For
> > that, add an optional DT property that would allow toggling the
> > operation mode from the configured one to the other one.
> >
> > The driver still only supports tunneling mode, that didn't change.
> >
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@xxxxxxxxxxx>
> > ---
> > drivers/media/i2c/max96717.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > index 47a3be195a971..a591ca5d5f44f 100644
> > --- a/drivers/media/i2c/max96717.c
> > +++ b/drivers/media/i2c/max96717.c
>
>
> enum gmsl2_mode {
> GMSL2_PIXEL_MODE,
> GMSL2_MODE_TUNNEL,
> };
>
> > @@ -161,6 +161,7 @@ struct max96717_priv {
> > struct clk_hw clk_hw;
> > struct gpio_chip gpio_chip;
> > enum max96717_vpg_mode pattern;
> > + bool mode_override;
> enum gmsl2_mode mode;
> I would prefer to set the mode in an explicit way instead of toggling
> the bit in the register.
>
> > struct max96717_fsync_desc fsync;
> > };
> >
> > @@ -1066,6 +1067,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
> > return dev_err_probe(dev, ret,
> > "Fail to read mipi rx extension");
> >
> > + if (priv->mode_override) {
> if (priv->mode_override && priv->mode == GMSL2_MODE_TUNNEL) {
> > +
> > +
> > + ret = cci_write(priv->regmap, MAX96717_MIPI_RX_EXT11, val, NULL);
> ret = cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, MAX96717_TUN_MODE,
> MAX96717_TUN_MODE, NULL);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Unable to update operation mode\n");
> > + }
> > +
> In case we are overwriting the mode to tunnel mode then no need to read the EXT11 register.
>
> > if (!(val & MAX96717_TUN_MODE))
> > return dev_err_probe(dev, -EOPNOTSUPP,
> > "Only supporting tunnel mode");
>
> In fact the driver can works in pixel mode, but since we don't set the "stream id" the
> deserializer have to configured with the right one :)
>
> > @@ -1101,6 +1110,9 @@ static int max96717_parse_dt(struct max96717_priv *priv)
> >
> > priv->mipi_csi2 = vep.bus.mipi_csi2;
> >
> > + if (fwnode_property_present(dev_fwnode(dev), "maxim,cfg-mode-override"))
> > + priv->mode_override = true;
> > +
> source_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> MAX96717_PAD_SOURCE, 0, 0);
> if (fwnode_property_present(source_fwnode, "maxim,tunnel-mode")) {
> priv->mode_override = true;
> priv->mode = GMSL2_MODE_TUNNEL;
> }
So, I don't think the boolean 'maxim,tunnel-mode' would work well when
the pin configuration is 'tunnel' and the user wants to switch to
'pixel'. Maybe, replace the boolean 'maxim,cfg-mode-override' property
with an optional enum property 'maxim,cfg-mode'? Does that sound better?
Thanks,
Laurentiu
> So we can parse the tunnel property from the GMSL port.
>
> > priv->fsync.pin = -1;
> > count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
> > if (count > 0) {
>
> Best Regards,
>
> --
> Julien
