| Message ID | 20190925152301.21645-2-bparrot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: updates | expand |
Hi Benoit, On Wed, Sep 25, 2019 at 10:22:59AM -0500, Benoit Parrot wrote: > Add v4l2 controls to report the pixel rates of each mode. This is > needed by some CSI2 receiver in order to perform proper DPHY > configuration. > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > --- > drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 500d9bbff10b..c2a44f30d56e 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -193,6 +193,9 @@ struct ov5640_mode_info { > > struct ov5640_ctrls { > struct v4l2_ctrl_handler handler; > + struct { > + struct v4l2_ctrl *pixel_rate; > + }; > struct { > struct v4l2_ctrl *auto_exp; > struct v4l2_ctrl *exposure; > @@ -241,6 +244,7 @@ struct ov5640_dev { > const struct ov5640_mode_info *last_mode; > enum ov5640_frame_rate current_fr; > struct v4l2_fract frame_interval; > + u64 pixel_rate; > > struct ov5640_ctrls ctrls; > > @@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > const struct ov5640_mode_info *new_mode; > struct v4l2_mbus_framefmt *mbus_fmt = &format->format; > struct v4l2_mbus_framefmt *fmt; > + u64 rate; > int ret; > > if (format->pad != 0) > @@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > if (mbus_fmt->code != sensor->fmt.code) > sensor->pending_fmt_change = true; > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot; > + rate *= ov5640_framerates[sensor->current_fr]; > + sensor->pixel_rate = rate; > + > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > + sensor->pixel_rate); > out: > mutex_unlock(&sensor->lock); > return ret; > @@ -2657,6 +2668,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > /* we can use our own mutex for the ctrl lock */ > hdl->lock = &sensor->lock; > > + /* Clock related controls */ > + ctrls->pixel_rate = > + v4l2_ctrl_new_std(hdl, ops, > + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1, > + 55969920); Could you calculate this value instead of using a seemingly random number? > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > /* Auto/manual white balance */ > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > V4L2_CID_AUTO_WHITE_BALANCE, > @@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > struct ov5640_dev *sensor = to_ov5640_dev(sd); > const struct ov5640_mode_info *mode; > int frame_rate, ret = 0; > + u64 rate; > > if (fi->pad != 0) > return -EINVAL; > @@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > sensor->frame_interval = fi->interval; > sensor->current_mode = mode; > sensor->pending_mode_change = true; > + > + rate = sensor->current_mode->vtot * sensor->current_mode->htot; > + rate *= ov5640_framerates[sensor->current_fr]; > + sensor->pixel_rate = rate; I think it'd be better to have a function to calculate the value instead of duplicating the code here. > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > + sensor->pixel_rate); > } > out: > mutex_unlock(&sensor->lock);
Hi Sakari, Thanks for the review. Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Oct-01 10:57:04 +0300]: > Hi Benoit, > > On Wed, Sep 25, 2019 at 10:22:59AM -0500, Benoit Parrot wrote: > > Add v4l2 controls to report the pixel rates of each mode. This is > > needed by some CSI2 receiver in order to perform proper DPHY > > configuration. > > > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > > --- > > drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index 500d9bbff10b..c2a44f30d56e 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -193,6 +193,9 @@ struct ov5640_mode_info { > > > > struct ov5640_ctrls { > > struct v4l2_ctrl_handler handler; > > + struct { > > + struct v4l2_ctrl *pixel_rate; > > + }; > > struct { > > struct v4l2_ctrl *auto_exp; > > struct v4l2_ctrl *exposure; > > @@ -241,6 +244,7 @@ struct ov5640_dev { > > const struct ov5640_mode_info *last_mode; > > enum ov5640_frame_rate current_fr; > > struct v4l2_fract frame_interval; > > + u64 pixel_rate; > > > > struct ov5640_ctrls ctrls; > > > > @@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > > const struct ov5640_mode_info *new_mode; > > struct v4l2_mbus_framefmt *mbus_fmt = &format->format; > > struct v4l2_mbus_framefmt *fmt; > > + u64 rate; > > int ret; > > > > if (format->pad != 0) > > @@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > > if (mbus_fmt->code != sensor->fmt.code) > > sensor->pending_fmt_change = true; > > > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot; > > + rate *= ov5640_framerates[sensor->current_fr]; > > + sensor->pixel_rate = rate; > > + > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > > + sensor->pixel_rate); > > out: > > mutex_unlock(&sensor->lock); > > return ret; > > @@ -2657,6 +2668,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > /* we can use our own mutex for the ctrl lock */ > > hdl->lock = &sensor->lock; > > > > + /* Clock related controls */ > > + ctrls->pixel_rate = > > + v4l2_ctrl_new_std(hdl, ops, > > + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1, > > + 55969920); > > Could you calculate this value instead of using a seemingly random number? Yes I guess I could, especially if I make a helper function for it :). > > > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > /* Auto/manual white balance */ > > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > > V4L2_CID_AUTO_WHITE_BALANCE, > > @@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > const struct ov5640_mode_info *mode; > > int frame_rate, ret = 0; > > + u64 rate; > > > > if (fi->pad != 0) > > return -EINVAL; > > @@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > > sensor->frame_interval = fi->interval; > > sensor->current_mode = mode; > > sensor->pending_mode_change = true; > > + > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot; > > + rate *= ov5640_framerates[sensor->current_fr]; > > + sensor->pixel_rate = rate; > > I think it'd be better to have a function to calculate the value instead of > duplicating the code here. Yes I'll create a helper function. Benoit > > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > > + sensor->pixel_rate); > > } > > out: > > mutex_unlock(&sensor->lock); > > -- > Kind regards, > > Sakari Ailus > sakari.ailus@linux.intel.com
Hi Benoit, +Hugues If you're considering an helper, this thread might be useful to you: https://patchwork.kernel.org/patch/11019673/ Thanks j On Tue, Oct 01, 2019 at 11:23:41AM -0500, Benoit Parrot wrote: > Hi Sakari, > > Thanks for the review. > > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Oct-01 10:57:04 +0300]: > > Hi Benoit, > > > > On Wed, Sep 25, 2019 at 10:22:59AM -0500, Benoit Parrot wrote: > > > Add v4l2 controls to report the pixel rates of each mode. This is > > > needed by some CSI2 receiver in order to perform proper DPHY > > > configuration. > > > > > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > > > --- > > > drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++ > > > 1 file changed, 25 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > > index 500d9bbff10b..c2a44f30d56e 100644 > > > --- a/drivers/media/i2c/ov5640.c > > > +++ b/drivers/media/i2c/ov5640.c > > > @@ -193,6 +193,9 @@ struct ov5640_mode_info { > > > > > > struct ov5640_ctrls { > > > struct v4l2_ctrl_handler handler; > > > + struct { > > > + struct v4l2_ctrl *pixel_rate; > > > + }; > > > struct { > > > struct v4l2_ctrl *auto_exp; > > > struct v4l2_ctrl *exposure; > > > @@ -241,6 +244,7 @@ struct ov5640_dev { > > > const struct ov5640_mode_info *last_mode; > > > enum ov5640_frame_rate current_fr; > > > struct v4l2_fract frame_interval; > > > + u64 pixel_rate; > > > > > > struct ov5640_ctrls ctrls; > > > > > > @@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > > > const struct ov5640_mode_info *new_mode; > > > struct v4l2_mbus_framefmt *mbus_fmt = &format->format; > > > struct v4l2_mbus_framefmt *fmt; > > > + u64 rate; > > > int ret; > > > > > > if (format->pad != 0) > > > @@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > > > if (mbus_fmt->code != sensor->fmt.code) > > > sensor->pending_fmt_change = true; > > > > > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot; > > > + rate *= ov5640_framerates[sensor->current_fr]; > > > + sensor->pixel_rate = rate; > > > + > > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > > > + sensor->pixel_rate); > > > out: > > > mutex_unlock(&sensor->lock); > > > return ret; > > > @@ -2657,6 +2668,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > > /* we can use our own mutex for the ctrl lock */ > > > hdl->lock = &sensor->lock; > > > > > > + /* Clock related controls */ > > > + ctrls->pixel_rate = > > > + v4l2_ctrl_new_std(hdl, ops, > > > + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1, > > > + 55969920); > > > > Could you calculate this value instead of using a seemingly random number? > > Yes I guess I could, especially if I make a helper function for it :). > > > > > > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > + > > > /* Auto/manual white balance */ > > > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > > > V4L2_CID_AUTO_WHITE_BALANCE, > > > @@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > > const struct ov5640_mode_info *mode; > > > int frame_rate, ret = 0; > > > + u64 rate; > > > > > > if (fi->pad != 0) > > > return -EINVAL; > > > @@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > > > sensor->frame_interval = fi->interval; > > > sensor->current_mode = mode; > > > sensor->pending_mode_change = true; > > > + > > > + rate = sensor->current_mode->vtot * sensor->current_mode->htot; > > > + rate *= ov5640_framerates[sensor->current_fr]; > > > + sensor->pixel_rate = rate; > > > > I think it'd be better to have a function to calculate the value instead of > > duplicating the code here. > > Yes I'll create a helper function. > > Benoit > > > > > > + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, > > > + sensor->pixel_rate); > > > } > > > out: > > > mutex_unlock(&sensor->lock); > > > > -- > > Kind regards, > > > > Sakari Ailus > > sakari.ailus@linux.intel.com
Hi Jacopo, Maybe, I miss spoke when I mentioned a helper I did not intent a framework level generic function. Just a function to help in this case :) That being said, I re-read the thread you mentioned. And as Hughes pointed out dynamically generating a "working" link frequency value which can be used by a CSI2 receiver to properly configure its PHY is not trivial. When I created this patch, I also had another to add V4L2_CID_LINK_FREQ support. I am testing this against the TI CAL CSI2 receiver, which already uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch to add support for V4L2_CID_LINK_FREQ to that driver as well. Unfortunately, similar to Hughes' findings I was not able to make it "work" with all supported resolution/framerate. Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the same receiver. So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on V4L2_CID_PIXEL_RATE instead. Regard, Benoit Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]: > Hi Benoit, > +Hugues > > If you're considering an helper, this thread might be useful to you: > https://patchwork.kernel.org/patch/11019673/ > > Thanks > j >
Hi Benoit, On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote: > Hi Jacopo, > > Maybe, I miss spoke when I mentioned a helper I did not intent a framework > level generic function. Just a function to help in this case :) > > That being said, I re-read the thread you mentioned. And as Hughes pointed > out dynamically generating a "working" link frequency value which can be > used by a CSI2 receiver to properly configure its PHY is not trivial. > > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ > support. I am testing this against the TI CAL CSI2 receiver, which already > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch > to add support for V4L2_CID_LINK_FREQ to that driver as well. > > Unfortunately, similar to Hughes' findings I was not able to make it "work" > with all supported resolution/framerate. > > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the > same receiver. > > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on > V4L2_CID_PIXEL_RATE instead. It shouldn't make a difference which one you use; if you know the bus type (and if it's CSI-2 with D-PHY, number of lanes and how many bits per pixel the media bus format has), you can convert fairly trivially between the two.
Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Wed [2019-Oct-02 15:35:13 +0300]: > Hi Benoit, > > On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote: > > Hi Jacopo, > > > > Maybe, I miss spoke when I mentioned a helper I did not intent a framework > > level generic function. Just a function to help in this case :) > > > > That being said, I re-read the thread you mentioned. And as Hughes pointed > > out dynamically generating a "working" link frequency value which can be > > used by a CSI2 receiver to properly configure its PHY is not trivial. > > > > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ > > support. I am testing this against the TI CAL CSI2 receiver, which already > > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch > > to add support for V4L2_CID_LINK_FREQ to that driver as well. > > > > Unfortunately, similar to Hughes' findings I was not able to make it "work" > > with all supported resolution/framerate. > > > > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the > > same receiver. > > > > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on > > V4L2_CID_PIXEL_RATE instead. > > It shouldn't make a difference which one you use; if you know the bus type > (and if it's CSI-2 with D-PHY, number of lanes and how many bits per pixel > the media bus format has), you can convert fairly trivially between the > two. Sakari, Yes I get that, but at this point as they say I will leave that as an exercise to the reader... Benoit > > -- > Sakari Ailus > sakari.ailus@linux.intel.com
Hi Benoit, On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote: > Hi Jacopo, > > Maybe, I miss spoke when I mentioned a helper I did not intent a framework > level generic function. Just a function to help in this case :) Yes indeed, the discussion thread I linked here was mostly interesting because Hugues tried to do the same for LINK_FREQ iirc, and there where some usefult pointers. > > That being said, I re-read the thread you mentioned. And as Hughes pointed > out dynamically generating a "working" link frequency value which can be > used by a CSI2 receiver to properly configure its PHY is not trivial. > > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ > support. I am testing this against the TI CAL CSI2 receiver, which already > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch > to add support for V4L2_CID_LINK_FREQ to that driver as well. > > Unfortunately, similar to Hughes' findings I was not able to make it "work" > with all supported resolution/framerate. As reported by Hugues findings, the PLL calculation procedure might be faulty, and the actuall frequencies on the bus are different from the calculated ones. I wish I had more time to re-look at that, as they worked for my and Sam's use case, but deserve some rework. > > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the > same receiver. > It seems to me you're reporting a fixed rate. It might make your receiver happy, but does not report what is acutally put on the bus. Am I missing something ? > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on > V4L2_CID_PIXEL_RATE instead. > As Sakari pointed out, going from one to the other is trivial and could be done on top. Thanks j > Regard, > Benoit > > Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]: > > Hi Benoit, > > +Hugues > > > > If you're considering an helper, this thread might be useful to you: > > https://patchwork.kernel.org/patch/11019673/ > > > > Thanks > > j > > > >
Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 16:32:26 +0200]: > Hi Benoit, > > On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote: > > Hi Jacopo, > > > > Maybe, I miss spoke when I mentioned a helper I did not intent a framework > > level generic function. Just a function to help in this case :) > > Yes indeed, the discussion thread I linked here was mostly interesting > because Hugues tried to do the same for LINK_FREQ iirc, and there > where some usefult pointers. > > > > > That being said, I re-read the thread you mentioned. And as Hughes pointed > > out dynamically generating a "working" link frequency value which can be > > used by a CSI2 receiver to properly configure its PHY is not trivial. > > > > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ > > support. I am testing this against the TI CAL CSI2 receiver, which already > > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch > > to add support for V4L2_CID_LINK_FREQ to that driver as well. > > > > Unfortunately, similar to Hughes' findings I was not able to make it "work" > > with all supported resolution/framerate. > > As reported by Hugues findings, the PLL calculation procedure might be > faulty, and the actuall frequencies on the bus are different from the > calculated ones. > > I wish I had more time to re-look at that, as they worked for my and > Sam's use case, but deserve some rework. > > > > > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the > > same receiver. > > > > It seems to me you're reporting a fixed rate. It might make your > receiver happy, but does not report what is acutally put on the bus. > Am I missing something ? No it is not fixed, the only fixed value was the initial value (which representative of the initial/default resolution and framerate), I fixed this in v2. The reported PIXEL_RATE is re-calculated every time there is a s_fmt and/or framerate change and the V4L2_CID_PIXEL_RATE control value is updated accordingly. > > > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on > > V4L2_CID_PIXEL_RATE instead. > > > > As Sakari pointed out, going from one to the other is trivial and > could be done on top. As you said it could be done on top. :) Benoit > > Thanks > j > > > Regard, > > Benoit > > > > Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]: > > > Hi Benoit, > > > +Hugues > > > > > > If you're considering an helper, this thread might be useful to you: > > > https://patchwork.kernel.org/patch/11019673/ > > > > > > Thanks > > > j > > > > > > >
Hi Benoit, On Wed, Oct 02, 2019 at 10:06:15AM -0500, Benoit Parrot wrote: > Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 16:32:26 +0200]: > > Hi Benoit, > > > > On Wed, Oct 02, 2019 at 07:14:38AM -0500, Benoit Parrot wrote: > > > Hi Jacopo, > > > > > > Maybe, I miss spoke when I mentioned a helper I did not intent a framework > > > level generic function. Just a function to help in this case :) > > > > Yes indeed, the discussion thread I linked here was mostly interesting > > because Hugues tried to do the same for LINK_FREQ iirc, and there > > where some usefult pointers. > > > > > > > > That being said, I re-read the thread you mentioned. And as Hughes pointed > > > out dynamically generating a "working" link frequency value which can be > > > used by a CSI2 receiver to properly configure its PHY is not trivial. > > > > > > When I created this patch, I also had another to add V4L2_CID_LINK_FREQ > > > support. I am testing this against the TI CAL CSI2 receiver, which already > > > uses the V4L2_CID_PIXEL_RATE value for that purpose, so I also had a patch > > > to add support for V4L2_CID_LINK_FREQ to that driver as well. > > > > > > Unfortunately, similar to Hughes' findings I was not able to make it "work" > > > with all supported resolution/framerate. > > > > As reported by Hugues findings, the PLL calculation procedure might be > > faulty, and the actuall frequencies on the bus are different from the > > calculated ones. > > > > I wish I had more time to re-look at that, as they worked for my and > > Sam's use case, but deserve some rework. > > > > > > > > Unlike my V4L2_CID_PIXEL_RATE solution which now works in all mode with the > > > same receiver. > > > > > > > It seems to me you're reporting a fixed rate. It might make your > > receiver happy, but does not report what is acutally put on the bus. > > Am I missing something ? > > No it is not fixed, the only fixed value was the initial value (which > representative of the initial/default resolution and framerate), I > fixed this in v2. The reported PIXEL_RATE is re-calculated every time there > is a s_fmt and/or framerate change and the V4L2_CID_PIXEL_RATE control > value is updated accordingly. Oh I missed v2! I only seen this one. I'll reply there. Thanks j > > > > > > So long story short I dropped the V4L2_CID_LINK_FREQ patch and focused on > > > V4L2_CID_PIXEL_RATE instead. > > > > > > > As Sakari pointed out, going from one to the other is trivial and > > could be done on top. > > As you said it could be done on top. :) > > Benoit > > > > > Thanks > > j > > > > > Regard, > > > Benoit > > > > > > Jacopo Mondi <jacopo@jmondi.org> wrote on Wed [2019-Oct-02 09:59:51 +0200]: > > > > Hi Benoit, > > > > +Hugues > > > > > > > > If you're considering an helper, this thread might be useful to you: > > > > https://patchwork.kernel.org/patch/11019673/ > > > > > > > > Thanks > > > > j > > > > > > > > > > > >
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 500d9bbff10b..c2a44f30d56e 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -193,6 +193,9 @@ struct ov5640_mode_info { struct ov5640_ctrls { struct v4l2_ctrl_handler handler; + struct { + struct v4l2_ctrl *pixel_rate; + }; struct { struct v4l2_ctrl *auto_exp; struct v4l2_ctrl *exposure; @@ -241,6 +244,7 @@ struct ov5640_dev { const struct ov5640_mode_info *last_mode; enum ov5640_frame_rate current_fr; struct v4l2_fract frame_interval; + u64 pixel_rate; struct ov5640_ctrls ctrls; @@ -2202,6 +2206,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, const struct ov5640_mode_info *new_mode; struct v4l2_mbus_framefmt *mbus_fmt = &format->format; struct v4l2_mbus_framefmt *fmt; + u64 rate; int ret; if (format->pad != 0) @@ -2233,6 +2238,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, if (mbus_fmt->code != sensor->fmt.code) sensor->pending_fmt_change = true; + rate = sensor->current_mode->vtot * sensor->current_mode->htot; + rate *= ov5640_framerates[sensor->current_fr]; + sensor->pixel_rate = rate; + + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, + sensor->pixel_rate); out: mutex_unlock(&sensor->lock); return ret; @@ -2657,6 +2668,13 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) /* we can use our own mutex for the ctrl lock */ hdl->lock = &sensor->lock; + /* Clock related controls */ + ctrls->pixel_rate = + v4l2_ctrl_new_std(hdl, ops, + V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1, + 55969920); + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY; + /* Auto/manual white balance */ ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTO_WHITE_BALANCE, @@ -2782,6 +2800,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, struct ov5640_dev *sensor = to_ov5640_dev(sd); const struct ov5640_mode_info *mode; int frame_rate, ret = 0; + u64 rate; if (fi->pad != 0) return -EINVAL; @@ -2816,6 +2835,12 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, sensor->frame_interval = fi->interval; sensor->current_mode = mode; sensor->pending_mode_change = true; + + rate = sensor->current_mode->vtot * sensor->current_mode->htot; + rate *= ov5640_framerates[sensor->current_fr]; + sensor->pixel_rate = rate; + __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, + sensor->pixel_rate); } out: mutex_unlock(&sensor->lock);
Add v4l2 controls to report the pixel rates of each mode. This is needed by some CSI2 receiver in order to perform proper DPHY configuration. Signed-off-by: Benoit Parrot <bparrot@ti.com> --- drivers/media/i2c/ov5640.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)