| Message ID | 20240610-billion-v2-7-38e861475f85@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvc: Probe PLF limits at start-up | expand |
Hi Ricardo, Thank you for the patch. On Mon, Jun 10, 2024 at 11:09:58PM +0000, Ricardo Ribalda wrote: > If the callback returns a mapping instead of adding it, the codeflow is > more clean and we do not need a forward declaration of > __uvc_ctrl_add_mapping_to_list(). > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> This should have been squashed in the previous patches as appropriate. It's hard to review the new version this way. The diff with v1 looks good, so I don't expect to have further comments. > --- > drivers/media/usb/uvc/uvc_ctrl.c | 40 ++++++++++++++++++---------------------- > drivers/media/usb/uvc/uvcvideo.h | 6 +++--- > 2 files changed, 21 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 1c1710e3c486..4a13f2685d9e 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -495,11 +495,8 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = { > V4L2_CID_POWER_LINE_FREQUENCY_DISABLED), > }; > > -static int __uvc_ctrl_add_mapping_to_list(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping); > - > -static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) > +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; > @@ -509,7 +506,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, > > buf = kmalloc(sizeof(*buf), GFP_KERNEL); > if (!buf) > - return -ENOMEM; > + return NULL; > > /* Save the default PLF value, so we can restore it. */ > ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id, > @@ -517,7 +514,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, > buf, sizeof(*buf)); > /* If we cannot read the control skip it. */ > if (ret) > - return ret; > + return NULL; > init_val = *buf; > > /* If PLF value cannot be set to off, it is limited. */ > @@ -526,8 +523,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, > chain->dev->intfnum, ctrl->info.selector, > buf, sizeof(*buf)); > if (ret) > - return __uvc_ctrl_add_mapping_to_list(chain, ctrl, > - &uvc_ctrl_power_line_mapping_limited); > + return &uvc_ctrl_power_line_mapping_limited; > > /* UVC 1.1 does not define auto, we can exit. */ > if (chain->dev->uvc_version < 0x150) > @@ -548,7 +544,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, > chain->dev->intfnum, ctrl->info.selector, > buf, sizeof(*buf)); > > - return __uvc_ctrl_add_mapping_to_list(chain, ctrl, out_mapping); > + return out_mapping; > } > > static const struct uvc_control_mapping uvc_ctrl_mappings[] = { > @@ -843,7 +839,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = { > { > .entity = UVC_GUID_UVC_PROCESSING, > .selector = UVC_PU_POWER_LINE_FREQUENCY_CONTROL, > - .add_mapping = uvc_ctrl_add_plf_mapping, > + .filter_mapping = uvc_ctrl_filter_plf_mapping, > }, > }; > > @@ -2411,8 +2407,9 @@ 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_to_list(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) > +static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > + struct uvc_control *ctrl, > + const struct uvc_control_mapping *mapping) > { > struct uvc_control_mapping *map; > unsigned int size; > @@ -2485,14 +2482,6 @@ static int __uvc_ctrl_add_mapping_to_list(struct uvc_video_chain *chain, > return -ENOMEM; > } > > -static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) > -{ > - if (mapping && mapping->add_mapping) > - return mapping->add_mapping(chain, ctrl, mapping); > - return __uvc_ctrl_add_mapping_to_list(chain, ctrl, mapping); > -} > - > int uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > const struct uvc_control_mapping *mapping) > { > @@ -2681,7 +2670,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 = &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]; > > if (uvc_entity_match_guid(ctrl->entity, mapping->entity) && > ctrl->info.selector == mapping->selector) > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index ff9545dcf716..a9547795fe22 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -125,9 +125,9 @@ struct uvc_control_mapping { > s32 master_manual; > u32 slave_ids[2]; > > - int (*add_mapping)(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, > - const struct uvc_control_mapping *mapping); > + const struct uvc_control_mapping *(*filter_mapping) > + (struct uvc_video_chain *chain, > + struct uvc_control *ctrl); > s32 (*get)(struct uvc_control_mapping *mapping, u8 query, > const u8 *data); > void (*set)(struct uvc_control_mapping *mapping, s32 value, >
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 1c1710e3c486..4a13f2685d9e 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -495,11 +495,8 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = { V4L2_CID_POWER_LINE_FREQUENCY_DISABLED), }; -static int __uvc_ctrl_add_mapping_to_list(struct uvc_video_chain *chain, - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping); - -static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) +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; @@ -509,7 +506,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, buf = kmalloc(sizeof(*buf), GFP_KERNEL); if (!buf) - return -ENOMEM; + return NULL; /* Save the default PLF value, so we can restore it. */ ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id, @@ -517,7 +514,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, buf, sizeof(*buf)); /* If we cannot read the control skip it. */ if (ret) - return ret; + return NULL; init_val = *buf; /* If PLF value cannot be set to off, it is limited. */ @@ -526,8 +523,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, chain->dev->intfnum, ctrl->info.selector, buf, sizeof(*buf)); if (ret) - return __uvc_ctrl_add_mapping_to_list(chain, ctrl, - &uvc_ctrl_power_line_mapping_limited); + return &uvc_ctrl_power_line_mapping_limited; /* UVC 1.1 does not define auto, we can exit. */ if (chain->dev->uvc_version < 0x150) @@ -548,7 +544,7 @@ static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain, chain->dev->intfnum, ctrl->info.selector, buf, sizeof(*buf)); - return __uvc_ctrl_add_mapping_to_list(chain, ctrl, out_mapping); + return out_mapping; } static const struct uvc_control_mapping uvc_ctrl_mappings[] = { @@ -843,7 +839,7 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = { { .entity = UVC_GUID_UVC_PROCESSING, .selector = UVC_PU_POWER_LINE_FREQUENCY_CONTROL, - .add_mapping = uvc_ctrl_add_plf_mapping, + .filter_mapping = uvc_ctrl_filter_plf_mapping, }, }; @@ -2411,8 +2407,9 @@ 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_to_list(struct uvc_video_chain *chain, - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) +static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, + struct uvc_control *ctrl, + const struct uvc_control_mapping *mapping) { struct uvc_control_mapping *map; unsigned int size; @@ -2485,14 +2482,6 @@ static int __uvc_ctrl_add_mapping_to_list(struct uvc_video_chain *chain, return -ENOMEM; } -static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, - struct uvc_control *ctrl, const struct uvc_control_mapping *mapping) -{ - if (mapping && mapping->add_mapping) - return mapping->add_mapping(chain, ctrl, mapping); - return __uvc_ctrl_add_mapping_to_list(chain, ctrl, mapping); -} - int uvc_ctrl_add_mapping(struct uvc_video_chain *chain, const struct uvc_control_mapping *mapping) { @@ -2681,7 +2670,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 = &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]; if (uvc_entity_match_guid(ctrl->entity, mapping->entity) && ctrl->info.selector == mapping->selector) diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index ff9545dcf716..a9547795fe22 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -125,9 +125,9 @@ struct uvc_control_mapping { s32 master_manual; u32 slave_ids[2]; - int (*add_mapping)(struct uvc_video_chain *chain, - struct uvc_control *ctrl, - const struct uvc_control_mapping *mapping); + const struct uvc_control_mapping *(*filter_mapping) + (struct uvc_video_chain *chain, + struct uvc_control *ctrl); s32 (*get)(struct uvc_control_mapping *mapping, u8 query, const u8 *data); void (*set)(struct uvc_control_mapping *mapping, s32 value,
If the callback returns a mapping instead of adding it, the codeflow is more clean and we do not need a forward declaration of __uvc_ctrl_add_mapping_to_list(). Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 40 ++++++++++++++++++---------------------- drivers/media/usb/uvc/uvcvideo.h | 6 +++--- 2 files changed, 21 insertions(+), 25 deletions(-)