| Message ID | 20240616231350.6787-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: uvc: Probe PLF limits at start-up | expand |
Hi Laurent Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> On Mon, 17 Jun 2024 at 01:14, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > This patch series is a new version of Ricardo's v2 that incorporate my > review feedback and squash v2 7/7 into the appropriate commits. I've > decided to send it as a new version to speed up merging. > > As part of the squashing, patch 1/6 now implements a slightly different > filtering logic by ignoring mappings whose .filter_mapping() function > returns NULL. Apart from that, the series should be functionally > equivalento to v2. > > The patches have been rebased on my UVC -next branch. The base commit > can be found in > git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git. If > this version is acceptable, I will add it to the branch and send a pull > request in the next few days. For reference this is the diff with v2 diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index eb930825c354..79c6dacd516e 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -495,8 +495,8 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = { V4L2_CID_POWER_LINE_FREQUENCY_DISABLED), }; -static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping - (struct uvc_video_chain *chain, struct uvc_control *ctrl) +static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping( + struct uvc_video_chain *chain, struct uvc_control *ctrl) { const struct uvc_control_mapping *out_mapping = &uvc_ctrl_power_line_mapping_uvc11; @@ -2408,8 +2408,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, * Add a control mapping to a given control. */ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, - struct uvc_control *ctrl, - const struct uvc_control_mapping *mapping) + struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) Just curious, do you have a nice vim code formatter that you can share? Or is it just "what looks nicer"? { struct uvc_control_mapping *map; unsigned int size; @@ -2670,14 +2669,14 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, /* Process common mappings. */ for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) { - const struct uvc_control_mapping *mapping = NULL; - - /* Try to get a custom mapping from the device. */ - if (uvc_ctrl_mappings[i].filter_mapping) - mapping = uvc_ctrl_mappings[i].filter_mapping(chain, - ctrl); - if (!mapping) - mapping = &uvc_ctrl_mappings[i]; + const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i]; + + /* Let the device provide a custom mapping. */ + if (mapping->filter_mapping) { + mapping = mapping->filter_mapping(chain, ctrl); + if (!mapping) + continue; + } I guess that if the device is too broken to fail filter_mapping we can skip that control. Thanks! > > Ricardo Ribalda (6): > media: uvcvideo: Allow custom control mapping > media: uvcvideo: Refactor Power Line Frequency limit selection > media: uvcvideo: Probe the PLF characteristics > media: uvcvideo: Cleanup version-specific mapping > media: uvcvideo: Remove PLF device quirking > media: uvcvideo: Remove mappings form uvc_device_info > > drivers/media/usb/uvc/uvc_ctrl.c | 184 ++++++++++++++++------------- > drivers/media/usb/uvc/uvc_driver.c | 131 -------------------- > drivers/media/usb/uvc/uvcvideo.h | 8 +- > 3 files changed, 105 insertions(+), 218 deletions(-) > > > base-commit: 75007ad7544c3a4da6b670983fb41cc4cbe8e9b1 > -- > Regards, > > Laurent Pinchart >
On Mon, Jun 17, 2024 at 09:43:50AM +0200, Ricardo Ribalda wrote: > Hi Laurent > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > On Mon, 17 Jun 2024 at 01:14, Laurent Pinchart wrote: > > > > Hello, > > > > This patch series is a new version of Ricardo's v2 that incorporate my > > review feedback and squash v2 7/7 into the appropriate commits. I've > > decided to send it as a new version to speed up merging. > > > > As part of the squashing, patch 1/6 now implements a slightly different > > filtering logic by ignoring mappings whose .filter_mapping() function > > returns NULL. Apart from that, the series should be functionally > > equivalento to v2. > > > > The patches have been rebased on my UVC -next branch. The base commit > > can be found in > > git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git. If > > this version is acceptable, I will add it to the branch and send a pull > > request in the next few days. > > For reference this is the diff with v2 > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index eb930825c354..79c6dacd516e 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -495,8 +495,8 @@ static const struct uvc_control_mapping > uvc_ctrl_power_line_mapping_uvc15 = { > V4L2_CID_POWER_LINE_FREQUENCY_DISABLED), > }; > > -static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping > - (struct uvc_video_chain *chain, struct uvc_control *ctrl) > +static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping( > + struct uvc_video_chain *chain, struct uvc_control *ctrl) > { > const struct uvc_control_mapping *out_mapping = > &uvc_ctrl_power_line_mapping_uvc11; > @@ -2408,8 +2408,7 @@ static int uvc_ctrl_add_info(struct uvc_device > *dev, struct uvc_control *ctrl, > * Add a control mapping to a given control. > */ > static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, > - const struct uvc_control_mapping *mapping) > + struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) > > Just curious, do you have a nice vim code formatter that you can > share? Or is it just "what looks nicer"? It's the latter I'm afraid, and I'm sure it has changed over time. I would nowadays use the '-' style, not the '+' style here. The diff shows a change, but that's only because splitting patch 7/7 and squashing it in previous patches removed the need to change the __uvc_ctrl_add_mapping() function, so I ended up dropping the style change. > { > struct uvc_control_mapping *map; > unsigned int size; > @@ -2670,14 +2669,14 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, > > /* Process common mappings. */ > for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) { > - const struct uvc_control_mapping *mapping = NULL; > - > - /* Try to get a custom mapping from the device. */ > - if (uvc_ctrl_mappings[i].filter_mapping) > - mapping = uvc_ctrl_mappings[i].filter_mapping(chain, > - ctrl); > - if (!mapping) > - mapping = &uvc_ctrl_mappings[i]; > + const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i]; > + > + /* Let the device provide a custom mapping. */ > + if (mapping->filter_mapping) { > + mapping = mapping->filter_mapping(chain, ctrl); > + if (!mapping) > + continue; > + } > > I guess that if the device is too broken to fail filter_mapping we can > skip that control. And I think we *should* skip it in that case, as exposing the control would lead to trouble. > > Ricardo Ribalda (6): > > media: uvcvideo: Allow custom control mapping > > media: uvcvideo: Refactor Power Line Frequency limit selection > > media: uvcvideo: Probe the PLF characteristics > > media: uvcvideo: Cleanup version-specific mapping > > media: uvcvideo: Remove PLF device quirking > > media: uvcvideo: Remove mappings form uvc_device_info > > > > drivers/media/usb/uvc/uvc_ctrl.c | 184 ++++++++++++++++------------- > > drivers/media/usb/uvc/uvc_driver.c | 131 -------------------- > > drivers/media/usb/uvc/uvcvideo.h | 8 +- > > 3 files changed, 105 insertions(+), 218 deletions(-) > > > > > > base-commit: 75007ad7544c3a4da6b670983fb41cc4cbe8e9b1