Hi Jacopo,
On Tue, Jan 09, 2024 at 12:56:44PM +0100, Jacopo Mondi wrote:
> Hi Sakari
>
> On Mon, Jan 08, 2024 at 09:52:19AM +0200, Sakari Ailus wrote:
> > Use the v4l2_link_freq_to_bitmap() helper to figure out which
> > driver-supported link frequencies can be used on a given system.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/imx334.c | 41 +++++++++++---------------------------
> > 1 file changed, 12 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 6725b3e2a73e..40863d87d341 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -136,7 +136,7 @@ struct imx334_mode {
> > * @vblank: Vertical blanking in lines
> > * @cur_mode: Pointer to current selected sensor mode
> > * @mutex: Mutex for serializing sensor controls
> > - * @menu_skip_mask: Menu skip mask for link_freq_ctrl
> > + * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> > * @cur_code: current selected format code
> > */
> > struct imx334 {
> > @@ -158,7 +158,7 @@ struct imx334 {
> > u32 vblank;
> > const struct imx334_mode *cur_mode;
> > struct mutex mutex;
> > - unsigned long menu_skip_mask;
> > + unsigned long link_freq_bitmap;
> > u32 cur_code;
> > };
> >
> > @@ -954,9 +954,9 @@ static int imx334_init_state(struct v4l2_subdev *sd,
> > imx334_fill_pad_format(imx334, imx334->cur_mode, &fmt);
> >
> > __v4l2_ctrl_modify_range(imx334->link_freq_ctrl, 0,
> > - __fls(imx334->menu_skip_mask),
> > - ~(imx334->menu_skip_mask),
> > - __ffs(imx334->menu_skip_mask));
> > + __fls(imx334->link_freq_bitmap),
> > + ~(imx334->link_freq_bitmap),
> > + __ffs(imx334->link_freq_bitmap));
> >
> > mutex_unlock(&imx334->mutex);
> >
> > @@ -1112,7 +1112,6 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
> > };
> > struct fwnode_handle *ep;
> > unsigned long rate;
> > - unsigned int i, j;
> > int ret;
> >
> > if (!fwnode)
> > @@ -1157,26 +1156,10 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
> > goto done_endpoint_free;
> > }
> >
> > - if (!bus_cfg.nr_of_link_frequencies) {
> > - dev_err(imx334->dev, "no link frequencies defined");
> > - ret = -EINVAL;
> > - goto done_endpoint_free;
> > - }
> > -
> > - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > - for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > - if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > - set_bit(j, &imx334->menu_skip_mask);
> > - break;
> > - }
> > - }
> > -
> > - if (j == ARRAY_SIZE(link_freq)) {
> > - ret = dev_err_probe(imx334->dev, -EINVAL,
> > - "no supported link freq found\n");
> > - goto done_endpoint_free;
> > - }
> > - }
> > + ret = v4l2_link_freq_to_bitmap(imx334->dev, bus_cfg.link_frequencies,
> > + bus_cfg.nr_of_link_frequencies,
> > + link_freq, ARRAY_SIZE(link_freq),
> > + &imx334->link_freq_bitmap);
> >
> > done_endpoint_free:
> > v4l2_fwnode_endpoint_free(&bus_cfg);
> > @@ -1310,8 +1293,8 @@ static int imx334_init_controls(struct imx334 *imx334)
> > imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > &imx334_ctrl_ops,
> > V4L2_CID_LINK_FREQ,
> > - __fls(imx334->menu_skip_mask),
> > - __ffs(imx334->menu_skip_mask),
> > + __fls(imx334->link_freq_bitmap),
> > + __ffs(imx334->link_freq_bitmap),
>
> How does this work if
>
> driver_freqs = {
> FREQ1,
> FREQ2,
> FREQ3,
> FREQ4
> };
>
> fw_freqs = {
> FREQ2,
> FREQ4
> };
>
> Will FREQ3 be selectable by userspace ?
That's a good question. Drivers that support more than two frequencies
should use v4l2_ctrl_modify_range() to provide a bitmap of supported
frequencies, v4l2_ctrl_new_int_menu() doesn't have an argument for that.
Maybe we could add one? :-)
>
> > link_freq);
> >
> > if (imx334->link_freq_ctrl)
> > @@ -1386,7 +1369,7 @@ static int imx334_probe(struct i2c_client *client)
> > }
> >
> > /* Set default mode to max resolution */
> > - imx334->cur_mode = &supported_modes[__ffs(imx334->menu_skip_mask)];
> > + imx334->cur_mode = &supported_modes[__ffs(imx334->link_freq_bitmap)];
> > imx334->cur_code = imx334_mbus_codes[0];
> > imx334->vblank = imx334->cur_mode->vblank;
> >
--
Regards,
Sakari Ailus
