| Message ID | 20211216220946.20771-1-laurent.pinchart+renesas@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: i2c: max9286: Small new features | expand |
Hi LAurent On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Power supplies for the ports can be controlled per port depending on the > hardware design. Support per-port supplies in the DT bindings, mutually > exclusive with the global supply. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../bindings/media/i2c/maxim,max9286.yaml | 25 ++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > index 02f656e78700..33aa307e8ee5 100644 > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > @@ -39,7 +39,7 @@ properties: > maxItems: 1 > > poc-supply: > - description: Regulator providing Power over Coax to the cameras > + description: Regulator providing Power over Coax to all the ports > Can anything but a camera be connected to a port ? > enable-gpios: > description: GPIO connected to the \#PWDN pin with inverted polarity > @@ -160,6 +160,10 @@ properties: > > additionalProperties: false > > +patternProperties: > + "^port[0-3]-poc-supply$": > + description: Regulator providing Power over Coax for a particular port > + > required: > - compatible > - reg > @@ -167,6 +171,25 @@ required: > - i2c-mux > - gpio-controller > > +allOf: > + - if: > + required: > + - poc-supply > + then: > + allOf: > + - not: > + required: > + - port0-poc-supply > + - not: > + required: > + - port1-poc-supply > + - not: > + required: > + - port2-poc-supply > + - not: > + required: > + - port3-poc-supply Isn't this simply expressed as if: required: - poc-supply then: properties: port0-poc-supply: false port1-poc-supply: false port2-poc-supply: false port3-poc-supply: false I tried tweaking the DTS file example with the above applied as poc-supply = <&camera_poc_12v>; port0-poc-supply = <&camera0_poc>; And validation fails as expected .../maxim,max9286.example.dt.yaml: gmsl-deserializer@2c: port0-poc-supply: False schema does not allow [[4294967295]] Also, could you make sure this does not conflict with the introduction of gpio-poc in "dt-bindings: media: max9286: Define 'maxim,gpio-poc'". Thanks j > + > additionalProperties: false > > examples: > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The MAX9286 can generate a framesync signal to synchronize the cameras, > using an internal timer. Support this mode of operation and configure it > through the .s_frameinterval() operation. If the frame interval is not > 0, framesync is switched to manual mode with the specified interval, > otherwise automatic mode is used. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 63 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index fa2f6a823fe6..dce1146635d2 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -174,6 +174,7 @@ struct max9286_priv { > struct v4l2_ctrl *pixelrate; > > struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS]; > + struct v4l2_fract interval; > > /* Protects controls and fmt structures */ > struct mutex mutex; > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv, > return 0; > } > > +static void max9286_set_fsync_period(struct max9286_priv *priv) > +{ > + u32 fsync; > + > + if (!priv->interval.numerator || !priv->interval.denominator) { > + /* > + * Special case, a null interval enables automatic FRAMESYNC > + * mode. FRAMESYNC is taken from the slowest link. > + */ > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > + MAX9286_FSYNCMETH_AUTO); > + return; > + } > + > + /* > + * Manual FRAMESYNC > + * > + * The FRAMESYNC generator is configured with a period expressed as a > + * number of PCLK periods, which runs at 75MHz. > + */ > + fsync = div_u64(75000000ULL * priv->interval.numerator, > + priv->interval.denominator); Don't we calculate the pixel rate dynamically based on the number of enabled sources ? > + > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT | > + MAX9286_FSYNCMETH_MANUAL); > + > + max9286_write(priv, 0x06, (fsync >> 0) & 0xff); > + max9286_write(priv, 0x07, (fsync >> 8) & 0xff); > + max9286_write(priv, 0x08, (fsync >> 16) & 0xff); > +} > + > /* ----------------------------------------------------------------------------- > * V4L2 Subdev > */ > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > int ret; > > if (enable) { > + max9286_set_fsync_period(priv); > + > /* > * The frame sync between cameras is transmitted across the > * reverse channel as GPIO. We must open all channels while > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > return 0; > } > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *interval) > +{ > + struct max9286_priv *priv = sd_to_max9286(sd); > + > + if (interval->pad == MAX9286_SRC_PAD) Ah! I would have expected, as this setting applies to all cameras, and consequentially to the output image stream frame rate, that the operation should have been done on the single source pad... Why is this not the case ? Thanks j > + return -EINVAL; > + > + interval->interval = priv->interval; > + > + return 0; > +} > + > +static int max9286_s_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *interval) > +{ > + struct max9286_priv *priv = sd_to_max9286(sd); > + > + if (interval->pad == MAX9286_SRC_PAD) > + return -EINVAL; > + > + priv->interval = interval->interval; > + > + return 0; > +} > + > static int max9286_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_mbus_code_enum *code) > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd, > > static const struct v4l2_subdev_video_ops max9286_video_ops = { > .s_stream = max9286_s_stream, > + .g_frame_interval = max9286_g_frame_interval, > + .s_frame_interval = max9286_s_frame_interval, > }; > > static const struct v4l2_subdev_pad_ops max9286_pad_ops = { > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv) > MAX9286_CSILANECNT(priv->csi2_data_lanes) | > MAX9286_DATATYPE_YUV422_8BIT); > > - /* Automatic: FRAMESYNC taken from the slowest Link. */ > - max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > - MAX9286_FSYNCMETH_AUTO); > + 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 | > -- > Regards, > > Laurent Pinchart >
On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Power supplies for the ports can be controlled per port depending on the > hardware design. Support per-port supplies in the DT bindings, mutually > exclusive with the global supply. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../bindings/media/i2c/maxim,max9286.yaml | 25 ++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > index 02f656e78700..33aa307e8ee5 100644 > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > @@ -39,7 +39,7 @@ properties: > maxItems: 1 > > poc-supply: > - description: Regulator providing Power over Coax to the cameras > + description: Regulator providing Power over Coax to all the ports > > enable-gpios: > description: GPIO connected to the \#PWDN pin with inverted polarity > @@ -160,6 +160,10 @@ properties: > > additionalProperties: false > > +patternProperties: > + "^port[0-3]-poc-supply$": > + description: Regulator providing Power over Coax for a particular port > + > required: > - compatible > - reg > @@ -167,6 +171,25 @@ required: > - i2c-mux > - gpio-controller > > +allOf: > + - if: > + required: > + - poc-supply > + then: > + allOf: > + - not: > + required: > + - port0-poc-supply > + - not: > + required: > + - port1-poc-supply > + - not: > + required: > + - port2-poc-supply > + - not: > + required: > + - port3-poc-supply I think you can invert the if and move patternProperties to the 'then' and... > + > additionalProperties: false then use unevaluatedProperties here. Rob
Hi Jacopo, On Fri, Dec 17, 2021 at 11:47:10AM +0100, Jacopo Mondi wrote: > On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Power supplies for the ports can be controlled per port depending on the > > hardware design. Support per-port supplies in the DT bindings, mutually > > exclusive with the global supply. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../bindings/media/i2c/maxim,max9286.yaml | 25 ++++++++++++++++++- > > 1 file changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > index 02f656e78700..33aa307e8ee5 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > @@ -39,7 +39,7 @@ properties: > > maxItems: 1 > > > > poc-supply: > > - description: Regulator providing Power over Coax to the cameras > > + description: Regulator providing Power over Coax to all the ports > > > > Can anything but a camera be connected to a port ? > > > enable-gpios: > > description: GPIO connected to the \#PWDN pin with inverted polarity > > @@ -160,6 +160,10 @@ properties: > > > > additionalProperties: false > > > > +patternProperties: > > + "^port[0-3]-poc-supply$": > > + description: Regulator providing Power over Coax for a particular port > > + > > required: > > - compatible > > - reg > > @@ -167,6 +171,25 @@ required: > > - i2c-mux > > - gpio-controller > > > > +allOf: > > + - if: > > + required: > > + - poc-supply > > + then: > > + allOf: > > + - not: > > + required: > > + - port0-poc-supply > > + - not: > > + required: > > + - port1-poc-supply > > + - not: > > + required: > > + - port2-poc-supply > > + - not: > > + required: > > + - port3-poc-supply > > Isn't this simply expressed as > > if: > required: > - poc-supply > then: > properties: > port0-poc-supply: false > port1-poc-supply: false > port2-poc-supply: false > port3-poc-supply: false I would have sworn I had tried that and that it didn't work... I now have allOf: - if: required: - poc-supply then: patternProperties: '^port[0-3]-poc-supply$': false additionalProperties: false and it seems to do the job. I'll use that in a v2. > I tried tweaking the DTS file example with the above applied as > > poc-supply = <&camera_poc_12v>; > port0-poc-supply = <&camera0_poc>; > > And validation fails as expected > .../maxim,max9286.example.dt.yaml: gmsl-deserializer@2c: port0-poc-supply: False schema does not allow [[4294967295]] > > Also, could you make sure this does not conflict with the introduction > of gpio-poc in "dt-bindings: media: max9286: Define 'maxim,gpio-poc'". > > > + > > additionalProperties: false > > > > examples:
Hi Rob, On Fri, Dec 17, 2021 at 08:18:06AM -0600, Rob Herring wrote: > On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Power supplies for the ports can be controlled per port depending on the > > hardware design. Support per-port supplies in the DT bindings, mutually > > exclusive with the global supply. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../bindings/media/i2c/maxim,max9286.yaml | 25 ++++++++++++++++++- > > 1 file changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > index 02f656e78700..33aa307e8ee5 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > @@ -39,7 +39,7 @@ properties: > > maxItems: 1 > > > > poc-supply: > > - description: Regulator providing Power over Coax to the cameras > > + description: Regulator providing Power over Coax to all the ports > > > > enable-gpios: > > description: GPIO connected to the \#PWDN pin with inverted polarity > > @@ -160,6 +160,10 @@ properties: > > > > additionalProperties: false > > > > +patternProperties: > > + "^port[0-3]-poc-supply$": > > + description: Regulator providing Power over Coax for a particular port > > + > > required: > > - compatible > > - reg > > @@ -167,6 +171,25 @@ required: > > - i2c-mux > > - gpio-controller > > > > +allOf: > > + - if: > > + required: > > + - poc-supply > > + then: > > + allOf: > > + - not: > > + required: > > + - port0-poc-supply > > + - not: > > + required: > > + - port1-poc-supply > > + - not: > > + required: > > + - port2-poc-supply > > + - not: > > + required: > > + - port3-poc-supply > > I think you can invert the if and move patternProperties to the 'then' > and... > > > + > > additionalProperties: false > > then use unevaluatedProperties here. I ended up doing the following, which I would have sworn I had tried before: allOf: - if: required: - poc-supply then: patternProperties: '^port[0-3]-poc-supply$': false additionalProperties: false It seems to work, so I'll use it in v2.
Hi Jacopo, On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote: > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > The MAX9286 can generate a framesync signal to synchronize the cameras, > > using an internal timer. Support this mode of operation and configure it > > through the .s_frameinterval() operation. If the frame interval is not > > 0, framesync is switched to manual mode with the specified interval, > > otherwise automatic mode is used. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++-- > > 1 file changed, 63 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index fa2f6a823fe6..dce1146635d2 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -174,6 +174,7 @@ struct max9286_priv { > > struct v4l2_ctrl *pixelrate; > > > > struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS]; > > + struct v4l2_fract interval; > > > > /* Protects controls and fmt structures */ > > struct mutex mutex; > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv, > > return 0; > > } > > > > +static void max9286_set_fsync_period(struct max9286_priv *priv) > > +{ > > + u32 fsync; > > + > > + if (!priv->interval.numerator || !priv->interval.denominator) { > > + /* > > + * Special case, a null interval enables automatic FRAMESYNC > > + * mode. FRAMESYNC is taken from the slowest link. > > + */ > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > + MAX9286_FSYNCMETH_AUTO); > > + return; > > + } > > + > > + /* > > + * Manual FRAMESYNC > > + * > > + * The FRAMESYNC generator is configured with a period expressed as a > > + * number of PCLK periods, which runs at 75MHz. > > + */ > > + fsync = div_u64(75000000ULL * priv->interval.numerator, > > + priv->interval.denominator); > > Don't we calculate the pixel rate dynamically based on the number of > enabled sources ? That's a good point, this hardcodes operation for 4 cameras. I'll see how I can fix that. > > + > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT | > > + MAX9286_FSYNCMETH_MANUAL); > > + > > + max9286_write(priv, 0x06, (fsync >> 0) & 0xff); > > + max9286_write(priv, 0x07, (fsync >> 8) & 0xff); > > + max9286_write(priv, 0x08, (fsync >> 16) & 0xff); > > +} > > + > > /* ----------------------------------------------------------------------------- > > * V4L2 Subdev > > */ > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > int ret; > > > > if (enable) { > > + max9286_set_fsync_period(priv); > > + > > /* > > * The frame sync between cameras is transmitted across the > > * reverse channel as GPIO. We must open all channels while > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > return 0; > > } > > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_frame_interval *interval) > > +{ > > + struct max9286_priv *priv = sd_to_max9286(sd); > > + > > + if (interval->pad == MAX9286_SRC_PAD) > > Ah! > > I would have expected, as this setting applies to all cameras, and > consequentially to the output image stream frame rate, that the > operation should have been done on the single source pad... Why is > this not the case ? I went back and forth between the two, I think it would work fine on the source pad as well. I'll give it a try. > > + return -EINVAL; > > + > > + interval->interval = priv->interval; > > + > > + return 0; > > +} > > + > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_frame_interval *interval) > > +{ > > + struct max9286_priv *priv = sd_to_max9286(sd); > > + > > + if (interval->pad == MAX9286_SRC_PAD) > > + return -EINVAL; > > + > > + priv->interval = interval->interval; > > + > > + return 0; > > +} > > + > > static int max9286_enum_mbus_code(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *sd_state, > > struct v4l2_subdev_mbus_code_enum *code) > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd, > > > > static const struct v4l2_subdev_video_ops max9286_video_ops = { > > .s_stream = max9286_s_stream, > > + .g_frame_interval = max9286_g_frame_interval, > > + .s_frame_interval = max9286_s_frame_interval, > > }; > > > > static const struct v4l2_subdev_pad_ops max9286_pad_ops = { > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv) > > MAX9286_CSILANECNT(priv->csi2_data_lanes) | > > MAX9286_DATATYPE_YUV422_8BIT); > > > > - /* Automatic: FRAMESYNC taken from the slowest Link. */ > > - max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > - MAX9286_FSYNCMETH_AUTO); > > + 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 |
Hi Jacopo, On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote: > On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote: > > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > The MAX9286 can generate a framesync signal to synchronize the cameras, > > > using an internal timer. Support this mode of operation and configure it > > > through the .s_frameinterval() operation. If the frame interval is not > > > 0, framesync is switched to manual mode with the specified interval, > > > otherwise automatic mode is used. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 63 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > > index fa2f6a823fe6..dce1146635d2 100644 > > > --- a/drivers/media/i2c/max9286.c > > > +++ b/drivers/media/i2c/max9286.c > > > @@ -174,6 +174,7 @@ struct max9286_priv { > > > struct v4l2_ctrl *pixelrate; > > > > > > struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS]; > > > + struct v4l2_fract interval; > > > > > > /* Protects controls and fmt structures */ > > > struct mutex mutex; > > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv, > > > return 0; > > > } > > > > > > +static void max9286_set_fsync_period(struct max9286_priv *priv) > > > +{ > > > + u32 fsync; > > > + > > > + if (!priv->interval.numerator || !priv->interval.denominator) { > > > + /* > > > + * Special case, a null interval enables automatic FRAMESYNC > > > + * mode. FRAMESYNC is taken from the slowest link. > > > + */ > > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > > + MAX9286_FSYNCMETH_AUTO); > > > + return; > > > + } > > > + > > > + /* > > > + * Manual FRAMESYNC > > > + * > > > + * The FRAMESYNC generator is configured with a period expressed as a > > > + * number of PCLK periods, which runs at 75MHz. > > > + */ > > > + fsync = div_u64(75000000ULL * priv->interval.numerator, > > > + priv->interval.denominator); > > > > Don't we calculate the pixel rate dynamically based on the number of > > enabled sources ? > > That's a good point, this hardcodes operation for 4 cameras. I'll see > how I can fix that. > > > > + > > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT | > > > + MAX9286_FSYNCMETH_MANUAL); > > > + > > > + max9286_write(priv, 0x06, (fsync >> 0) & 0xff); > > > + max9286_write(priv, 0x07, (fsync >> 8) & 0xff); > > > + max9286_write(priv, 0x08, (fsync >> 16) & 0xff); > > > +} > > > + > > > /* ----------------------------------------------------------------------------- > > > * V4L2 Subdev > > > */ > > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > > int ret; > > > > > > if (enable) { > > > + max9286_set_fsync_period(priv); > > > + > > > /* > > > * The frame sync between cameras is transmitted across the > > > * reverse channel as GPIO. We must open all channels while > > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > > return 0; > > > } > > > > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_frame_interval *interval) > > > +{ > > > + struct max9286_priv *priv = sd_to_max9286(sd); > > > + > > > + if (interval->pad == MAX9286_SRC_PAD) > > > > Ah! > > > > I would have expected, as this setting applies to all cameras, and > > consequentially to the output image stream frame rate, that the > > operation should have been done on the single source pad... Why is > > this not the case ? > > I went back and forth between the two, I think it would work fine on the > source pad as well. I'll give it a try. Note that once we'll move to the muxed streams API, the frame interval will be per stream, so setting it on the source pad won't make much of a difference. What do you think ? > > > + return -EINVAL; > > > + > > > + interval->interval = priv->interval; > > > + > > > + return 0; > > > +} > > > + > > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_frame_interval *interval) > > > +{ > > > + struct max9286_priv *priv = sd_to_max9286(sd); > > > + > > > + if (interval->pad == MAX9286_SRC_PAD) > > > + return -EINVAL; > > > + > > > + priv->interval = interval->interval; > > > + > > > + return 0; > > > +} > > > + > > > static int max9286_enum_mbus_code(struct v4l2_subdev *sd, > > > struct v4l2_subdev_state *sd_state, > > > struct v4l2_subdev_mbus_code_enum *code) > > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd, > > > > > > static const struct v4l2_subdev_video_ops max9286_video_ops = { > > > .s_stream = max9286_s_stream, > > > + .g_frame_interval = max9286_g_frame_interval, > > > + .s_frame_interval = max9286_s_frame_interval, > > > }; > > > > > > static const struct v4l2_subdev_pad_ops max9286_pad_ops = { > > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv) > > > MAX9286_CSILANECNT(priv->csi2_data_lanes) | > > > MAX9286_DATATYPE_YUV422_8BIT); > > > > > > - /* Automatic: FRAMESYNC taken from the slowest Link. */ > > > - max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > > - MAX9286_FSYNCMETH_AUTO); > > > + 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 |
Hi Laurent, On Fri, Dec 17, 2021 at 07:49:07PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote: > > On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote: > > > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote: > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > The MAX9286 can generate a framesync signal to synchronize the cameras, > > > > using an internal timer. Support this mode of operation and configure it > > > > through the .s_frameinterval() operation. If the frame interval is not > > > > 0, framesync is switched to manual mode with the specified interval, > > > > otherwise automatic mode is used. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++-- > > > > 1 file changed, 63 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > > > index fa2f6a823fe6..dce1146635d2 100644 > > > > --- a/drivers/media/i2c/max9286.c > > > > +++ b/drivers/media/i2c/max9286.c > > > > @@ -174,6 +174,7 @@ struct max9286_priv { > > > > struct v4l2_ctrl *pixelrate; > > > > > > > > struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS]; > > > > + struct v4l2_fract interval; > > > > > > > > /* Protects controls and fmt structures */ > > > > struct mutex mutex; > > > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv, > > > > return 0; > > > > } > > > > > > > > +static void max9286_set_fsync_period(struct max9286_priv *priv) > > > > +{ > > > > + u32 fsync; > > > > + > > > > + if (!priv->interval.numerator || !priv->interval.denominator) { > > > > + /* > > > > + * Special case, a null interval enables automatic FRAMESYNC > > > > + * mode. FRAMESYNC is taken from the slowest link. > > > > + */ > > > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > > > + MAX9286_FSYNCMETH_AUTO); > > > > + return; > > > > + } > > > > + > > > > + /* > > > > + * Manual FRAMESYNC > > > > + * > > > > + * The FRAMESYNC generator is configured with a period expressed as a > > > > + * number of PCLK periods, which runs at 75MHz. > > > > + */ > > > > + fsync = div_u64(75000000ULL * priv->interval.numerator, > > > > + priv->interval.denominator); > > > > > > Don't we calculate the pixel rate dynamically based on the number of > > > enabled sources ? > > > > That's a good point, this hardcodes operation for 4 cameras. I'll see > > how I can fix that. > > > > > > + > > > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT | > > > > + MAX9286_FSYNCMETH_MANUAL); > > > > + > > > > + max9286_write(priv, 0x06, (fsync >> 0) & 0xff); > > > > + max9286_write(priv, 0x07, (fsync >> 8) & 0xff); > > > > + max9286_write(priv, 0x08, (fsync >> 16) & 0xff); > > > > +} > > > > + > > > > /* ----------------------------------------------------------------------------- > > > > * V4L2 Subdev > > > > */ > > > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > > > int ret; > > > > > > > > if (enable) { > > > > + max9286_set_fsync_period(priv); > > > > + > > > > /* > > > > * The frame sync between cameras is transmitted across the > > > > * reverse channel as GPIO. We must open all channels while > > > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > > > return 0; > > > > } > > > > > > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_frame_interval *interval) > > > > +{ > > > > + struct max9286_priv *priv = sd_to_max9286(sd); > > > > + > > > > + if (interval->pad == MAX9286_SRC_PAD) > > > > > > Ah! > > > > > > I would have expected, as this setting applies to all cameras, and > > > consequentially to the output image stream frame rate, that the > > > operation should have been done on the single source pad... Why is > > > this not the case ? > > > > I went back and forth between the two, I think it would work fine on the > > source pad as well. I'll give it a try. > > Note that once we'll move to the muxed streams API, the frame interval > will be per stream, so setting it on the source pad won't make much of a > difference. What do you think ? > Per-stream means you have it apply on all the enabled source pads, and potentially configure cameras differently, which is not supported because frame sync is a configuration parameter which applies to all sources. If I were you I would have used the single source pad, as from the user perspective what is relevant is the max9286 produced frame rate, but I understand the counter-argument, so up to you :) Thanks j > > > > + return -EINVAL; > > > > + > > > > + interval->interval = priv->interval; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_frame_interval *interval) > > > > +{ > > > > + struct max9286_priv *priv = sd_to_max9286(sd); > > > > + > > > > + if (interval->pad == MAX9286_SRC_PAD) > > > > + return -EINVAL; > > > > + > > > > + priv->interval = interval->interval; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static int max9286_enum_mbus_code(struct v4l2_subdev *sd, > > > > struct v4l2_subdev_state *sd_state, > > > > struct v4l2_subdev_mbus_code_enum *code) > > > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd, > > > > > > > > static const struct v4l2_subdev_video_ops max9286_video_ops = { > > > > .s_stream = max9286_s_stream, > > > > + .g_frame_interval = max9286_g_frame_interval, > > > > + .s_frame_interval = max9286_s_frame_interval, > > > > }; > > > > > > > > static const struct v4l2_subdev_pad_ops max9286_pad_ops = { > > > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv) > > > > MAX9286_CSILANECNT(priv->csi2_data_lanes) | > > > > MAX9286_DATATYPE_YUV422_8BIT); > > > > > > > > - /* Automatic: FRAMESYNC taken from the slowest Link. */ > > > > - max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > > > - MAX9286_FSYNCMETH_AUTO); > > > > + 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 | > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Dec 20, 2021 at 09:43:57AM +0100, Jacopo Mondi wrote: > On Fri, Dec 17, 2021 at 07:49:07PM +0200, Laurent Pinchart wrote: > > On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote: > > > On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote: > > > > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote: > > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > The MAX9286 can generate a framesync signal to synchronize the cameras, > > > > > using an internal timer. Support this mode of operation and configure it > > > > > through the .s_frameinterval() operation. If the frame interval is not > > > > > 0, framesync is switched to manual mode with the specified interval, > > > > > otherwise automatic mode is used. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++-- > > > > > 1 file changed, 63 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > > > > index fa2f6a823fe6..dce1146635d2 100644 > > > > > --- a/drivers/media/i2c/max9286.c > > > > > +++ b/drivers/media/i2c/max9286.c > > > > > @@ -174,6 +174,7 @@ struct max9286_priv { > > > > > struct v4l2_ctrl *pixelrate; > > > > > > > > > > struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS]; > > > > > + struct v4l2_fract interval; > > > > > > > > > > /* Protects controls and fmt structures */ > > > > > struct mutex mutex; > > > > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv, > > > > > return 0; > > > > > } > > > > > > > > > > +static void max9286_set_fsync_period(struct max9286_priv *priv) > > > > > +{ > > > > > + u32 fsync; > > > > > + > > > > > + if (!priv->interval.numerator || !priv->interval.denominator) { > > > > > + /* > > > > > + * Special case, a null interval enables automatic FRAMESYNC > > > > > + * mode. FRAMESYNC is taken from the slowest link. > > > > > + */ > > > > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > > > > + MAX9286_FSYNCMETH_AUTO); > > > > > + return; > > > > > + } > > > > > + > > > > > + /* > > > > > + * Manual FRAMESYNC > > > > > + * > > > > > + * The FRAMESYNC generator is configured with a period expressed as a > > > > > + * number of PCLK periods, which runs at 75MHz. > > > > > + */ > > > > > + fsync = div_u64(75000000ULL * priv->interval.numerator, > > > > > + priv->interval.denominator); > > > > > > > > Don't we calculate the pixel rate dynamically based on the number of > > > > enabled sources ? > > > > > > That's a good point, this hardcodes operation for 4 cameras. I'll see > > > how I can fix that. > > > > > > > > + > > > > > + max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT | > > > > > + MAX9286_FSYNCMETH_MANUAL); > > > > > + > > > > > + max9286_write(priv, 0x06, (fsync >> 0) & 0xff); > > > > > + max9286_write(priv, 0x07, (fsync >> 8) & 0xff); > > > > > + max9286_write(priv, 0x08, (fsync >> 16) & 0xff); > > > > > +} > > > > > + > > > > > /* ----------------------------------------------------------------------------- > > > > > * V4L2 Subdev > > > > > */ > > > > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > > > > int ret; > > > > > > > > > > if (enable) { > > > > > + max9286_set_fsync_period(priv); > > > > > + > > > > > /* > > > > > * The frame sync between cameras is transmitted across the > > > > > * reverse channel as GPIO. We must open all channels while > > > > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > > > > return 0; > > > > > } > > > > > > > > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd, > > > > > + struct v4l2_subdev_frame_interval *interval) > > > > > +{ > > > > > + struct max9286_priv *priv = sd_to_max9286(sd); > > > > > + > > > > > + if (interval->pad == MAX9286_SRC_PAD) > > > > > > > > Ah! > > > > > > > > I would have expected, as this setting applies to all cameras, and > > > > consequentially to the output image stream frame rate, that the > > > > operation should have been done on the single source pad... Why is > > > > this not the case ? > > > > > > I went back and forth between the two, I think it would work fine on the > > > source pad as well. I'll give it a try. > > > > Note that once we'll move to the muxed streams API, the frame interval > > will be per stream, so setting it on the source pad won't make much of a > > difference. What do you think ? > > > > Per-stream means you have it apply on all the enabled source pads, and > potentially configure cameras differently, which is not supported > because frame sync is a configuration parameter which applies to all > sources. > > If I were you I would have used the single source pad, as from the > user perspective what is relevant is the max9286 produced frame rate, > but I understand the counter-argument, so up to you :) The single source pad will have four streams, so it will still be per-stream, even on a single pad. > > > > > + return -EINVAL; > > > > > + > > > > > + interval->interval = priv->interval; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd, > > > > > + struct v4l2_subdev_frame_interval *interval) > > > > > +{ > > > > > + struct max9286_priv *priv = sd_to_max9286(sd); > > > > > + > > > > > + if (interval->pad == MAX9286_SRC_PAD) > > > > > + return -EINVAL; > > > > > + > > > > > + priv->interval = interval->interval; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static int max9286_enum_mbus_code(struct v4l2_subdev *sd, > > > > > struct v4l2_subdev_state *sd_state, > > > > > struct v4l2_subdev_mbus_code_enum *code) > > > > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd, > > > > > > > > > > static const struct v4l2_subdev_video_ops max9286_video_ops = { > > > > > .s_stream = max9286_s_stream, > > > > > + .g_frame_interval = max9286_g_frame_interval, > > > > > + .s_frame_interval = max9286_s_frame_interval, > > > > > }; > > > > > > > > > > static const struct v4l2_subdev_pad_ops max9286_pad_ops = { > > > > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv) > > > > > MAX9286_CSILANECNT(priv->csi2_data_lanes) | > > > > > MAX9286_DATATYPE_YUV422_8BIT); > > > > > > > > > > - /* Automatic: FRAMESYNC taken from the slowest Link. */ > > > > > - max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ | > > > > > - MAX9286_FSYNCMETH_AUTO); > > > > > + 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 |