Hi Jacopo,
On Tue, Jan 09, 2024 at 01:52:40PM +0100, Jacopo Mondi wrote:
> Hi Sakari
>
> On Tue, Jan 09, 2024 at 12:20:19PM +0000, Sakari Ailus wrote:
> > 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? :-)
> >
>
> How would you feel about v4l2_link_freq_to_bitmap() returning an s64[]
> instead so that the caller can pass it to v4l2_ctrl_new_int_menu()
> directly ? I know, the driver wuold then need to remember to release
> it, which is not an ideal pattern (allocate in the core, release in
> the driver).
Given that this is a single 64-bit value, I don't think it's useful to
allocate it in v4l2_link_freq_to_bitmap().
>
> Alternatively, could v4l2_fwnode_endpoint_alloc_parse() be passed the
> list of driver supported frequencies and store in vep->link_frequencies[]
> the intersection between the driver and firmware frequencies ? After
> all, the _alloc_parse() version of v4l2_fwnode_endpoint_parse() it's
> only there to support link_frequencies parsing, hence passing to it the
> driver's supported frequencies might not look that weird...
It's not a good fit for the functional scope of
v4l2_fwnode_endpoint_alloc_parse(), but maybe we could add a new function
to do both (and possibly more)? If this one isn't eventually be needed in
drivers, it can be made static.
>
> > >
> > > > 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
