| Message ID | 20240503155127.105235-12-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: renesas: rcar-csi2: Use the subdev active state | expand |
Hi Jacopo, Thank you for the patch. On Fri, May 03, 2024 at 05:51:26PM +0200, Jacopo Mondi wrote: > Use the frame interval stored in the subdev state instead of storing > a copy in the driver private structure. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/max9286.c | 52 +++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 28 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index cc7ee35560fc..4ddbc247395f 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -197,8 +197,6 @@ struct max9286_priv { > struct v4l2_ctrl *pixelrate_ctrl; > unsigned int pixelrate; > > - struct v4l2_fract interval; > - > unsigned int nsources; > unsigned int source_mask; > unsigned int route_mask; > @@ -571,11 +569,14 @@ static void max9286_set_video_format(struct max9286_priv *priv, > MAX9286_INVVS | MAX9286_HVSRC_D14); > } > > -static void max9286_set_fsync_period(struct max9286_priv *priv) > +static void max9286_set_fsync_period(struct max9286_priv *priv, > + struct v4l2_subdev_state *state) > { > + struct v4l2_fract *interval; const > u32 fsync; > > - if (!priv->interval.numerator || !priv->interval.denominator) { > + interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD); > + if (!interval->numerator || !interval->denominator) { > /* > * Special case, a null interval enables automatic FRAMESYNC > * mode. FRAMESYNC is taken from the slowest link. > @@ -591,8 +592,8 @@ static void max9286_set_fsync_period(struct max9286_priv *priv) > * The FRAMESYNC generator is configured with a period expressed as a > * number of PCLK periods. > */ > - fsync = div_u64((u64)priv->pixelrate * priv->interval.numerator, > - priv->interval.denominator); > + fsync = div_u64((u64)priv->pixelrate * interval->numerator, > + interval->denominator); > > dev_dbg(&priv->client->dev, "fsync period %u (pclk %u)\n", fsync, > priv->pixelrate); > @@ -801,7 +802,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD); > > max9286_set_video_format(priv, format); > - max9286_set_fsync_period(priv); > + max9286_set_fsync_period(priv, state); > > /* > * The frame sync between cameras is transmitted across the > @@ -874,19 +875,11 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_interval *interval) > { > - struct max9286_priv *priv = sd_to_max9286(sd); > - > - /* > - * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2 > - * subdev active state API. > - */ > - if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > - return -EINVAL; > - > if (interval->pad != MAX9286_SRC_PAD) > return -EINVAL; > > - interval->interval = priv->interval; > + interval->interval = *v4l2_subdev_state_get_interval(sd_state, > + interval->pad); > > return 0; > } > @@ -895,19 +888,11 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_interval *interval) > { > - struct max9286_priv *priv = sd_to_max9286(sd); > - > - /* > - * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2 > - * subdev active state API. > - */ > - if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) > - return -EINVAL; > - > if (interval->pad != MAX9286_SRC_PAD) > return -EINVAL; > > - priv->interval = interval->interval; > + *v4l2_subdev_state_get_interval(sd_state, > + interval->pad) = interval->interval; > > return 0; > } > @@ -993,12 +978,24 @@ static const struct v4l2_mbus_framefmt max9286_default_format = { > static int max9286_init_state(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state) > { > + struct v4l2_fract *interval; > + > for (unsigned int i = 0; i < MAX9286_N_SINKS; i++) > *v4l2_subdev_state_get_format(state, i) = max9286_default_format; > > *v4l2_subdev_state_get_format(state, > MAX9286_SRC_PAD) = max9286_default_format; > > + /* > + * Special case: a null interval enables automatic FRAMESYNC mode. > + * > + * FRAMESYNC is taken from the slowest link. See register 0x01 > + * configuration. > + */ > + interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD); > + interval->numerator = 0; > + interval->denominator = 0; > + > return 0; > } > > @@ -1145,7 +1142,6 @@ static int max9286_setup(struct max9286_priv *priv) > max9286_write(priv, 0x69, (0xf & ~priv->route_mask)); > > max9286_set_video_format(priv, &max9286_default_format); > - max9286_set_fsync_period(priv); The commit message should indicate why this line can be dropped. > > cfg = max9286_read(priv, 0x1c); > if (cfg < 0)
Hi Laurent On Mon, May 06, 2024 at 12:36:11AM GMT, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, May 03, 2024 at 05:51:26PM +0200, Jacopo Mondi wrote: > > Use the frame interval stored in the subdev state instead of storing > > a copy in the driver private structure. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > drivers/media/i2c/max9286.c | 52 +++++++++++++++++-------------------- > > 1 file changed, 24 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index cc7ee35560fc..4ddbc247395f 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -197,8 +197,6 @@ struct max9286_priv { > > struct v4l2_ctrl *pixelrate_ctrl; > > unsigned int pixelrate; > > > > - struct v4l2_fract interval; > > - > > unsigned int nsources; > > unsigned int source_mask; > > unsigned int route_mask; > > @@ -571,11 +569,14 @@ static void max9286_set_video_format(struct max9286_priv *priv, > > MAX9286_INVVS | MAX9286_HVSRC_D14); > > } > > > > -static void max9286_set_fsync_period(struct max9286_priv *priv) > > +static void max9286_set_fsync_period(struct max9286_priv *priv, > > + struct v4l2_subdev_state *state) > > { > > + struct v4l2_fract *interval; > > const > ../drivers/media/i2c/max9286.c:993:29: error: assignment of member ‘numerator’ in read-only object 993 | interval->numerator = 0;
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index cc7ee35560fc..4ddbc247395f 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -197,8 +197,6 @@ struct max9286_priv { struct v4l2_ctrl *pixelrate_ctrl; unsigned int pixelrate; - struct v4l2_fract interval; - unsigned int nsources; unsigned int source_mask; unsigned int route_mask; @@ -571,11 +569,14 @@ static void max9286_set_video_format(struct max9286_priv *priv, MAX9286_INVVS | MAX9286_HVSRC_D14); } -static void max9286_set_fsync_period(struct max9286_priv *priv) +static void max9286_set_fsync_period(struct max9286_priv *priv, + struct v4l2_subdev_state *state) { + struct v4l2_fract *interval; u32 fsync; - if (!priv->interval.numerator || !priv->interval.denominator) { + interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD); + if (!interval->numerator || !interval->denominator) { /* * Special case, a null interval enables automatic FRAMESYNC * mode. FRAMESYNC is taken from the slowest link. @@ -591,8 +592,8 @@ static void max9286_set_fsync_period(struct max9286_priv *priv) * The FRAMESYNC generator is configured with a period expressed as a * number of PCLK periods. */ - fsync = div_u64((u64)priv->pixelrate * priv->interval.numerator, - priv->interval.denominator); + fsync = div_u64((u64)priv->pixelrate * interval->numerator, + interval->denominator); dev_dbg(&priv->client->dev, "fsync period %u (pclk %u)\n", fsync, priv->pixelrate); @@ -801,7 +802,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD); max9286_set_video_format(priv, format); - max9286_set_fsync_period(priv); + max9286_set_fsync_period(priv, state); /* * The frame sync between cameras is transmitted across the @@ -874,19 +875,11 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_frame_interval *interval) { - struct max9286_priv *priv = sd_to_max9286(sd); - - /* - * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2 - * subdev active state API. - */ - if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) - return -EINVAL; - if (interval->pad != MAX9286_SRC_PAD) return -EINVAL; - interval->interval = priv->interval; + interval->interval = *v4l2_subdev_state_get_interval(sd_state, + interval->pad); return 0; } @@ -895,19 +888,11 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_frame_interval *interval) { - struct max9286_priv *priv = sd_to_max9286(sd); - - /* - * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2 - * subdev active state API. - */ - if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE) - return -EINVAL; - if (interval->pad != MAX9286_SRC_PAD) return -EINVAL; - priv->interval = interval->interval; + *v4l2_subdev_state_get_interval(sd_state, + interval->pad) = interval->interval; return 0; } @@ -993,12 +978,24 @@ static const struct v4l2_mbus_framefmt max9286_default_format = { static int max9286_init_state(struct v4l2_subdev *sd, struct v4l2_subdev_state *state) { + struct v4l2_fract *interval; + for (unsigned int i = 0; i < MAX9286_N_SINKS; i++) *v4l2_subdev_state_get_format(state, i) = max9286_default_format; *v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD) = max9286_default_format; + /* + * Special case: a null interval enables automatic FRAMESYNC mode. + * + * FRAMESYNC is taken from the slowest link. See register 0x01 + * configuration. + */ + interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD); + interval->numerator = 0; + interval->denominator = 0; + return 0; } @@ -1145,7 +1142,6 @@ static int max9286_setup(struct max9286_priv *priv) max9286_write(priv, 0x69, (0xf & ~priv->route_mask)); max9286_set_video_format(priv, &max9286_default_format); - max9286_set_fsync_period(priv); cfg = max9286_read(priv, 0x1c); if (cfg < 0)
Use the frame interval stored in the subdev state instead of storing a copy in the driver private structure. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- drivers/media/i2c/max9286.c | 52 +++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 28 deletions(-)