| Message ID | 20240108075221.15757-3-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add link frequency to bitmap helper | expand |
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@linux.intel.com> > --- > 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 ? > 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; > > -- > 2.39.2 >
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@linux.intel.com> > > --- > > 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; > >
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@linux.intel.com> > > > --- > > > 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). 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... > > > > > 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
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@linux.intel.com> > > > > --- > > > > 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; > > > > > >
Hi Sakari On Tue, Jan 09, 2024 at 01:44:59PM +0000, Sakari Ailus wrote: > 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@linux.intel.com> > > > > > --- > > > > > 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(). > For this driver it's a single s64, yes, but in the general case the intersection between the driver and the firmware frequencies is a list of 64-bit values.. > > > > 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. > if it's needed in drivers then we're not adding anything useful to the core :) > > > > > > > > > > > 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
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), 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;
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@linux.intel.com> --- drivers/media/i2c/imx334.c | 41 +++++++++++--------------------------- 1 file changed, 12 insertions(+), 29 deletions(-)