| Message ID | 20190823123737.7774-5-ribalda@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/7] media: add V4L2_CID_UNIT_CELL_SIZE control | expand |
On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote: > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, > try to minimize it by adding a new helper. > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++- > include/media/v4l2-ctrls.h | 16 ++++++++++++++++ > 2 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index b3bf458df7f7..33e48f0aec1a 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > } > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > > -/* Helper function for standard integer menu controls */ Why move this ... > struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > const struct v4l2_ctrl_ops *ops, > u32 id, u8 _max, u8 _def, const s64 *qmenu_int) > @@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > } > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > > +static void area_init(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr) > +{ > + memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area)); > +} > + > +static const struct v4l2_ctrl_type_ops area_ops = { > + .init = area_init, > +}; > + > +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > + const struct v4l2_ctrl_ops *ops, > + u32 id, const struct v4l2_area *area) > +{ > + static struct v4l2_ctrl_config ctrl = { > + .id = V4L2_CID_UNIT_CELL_SIZE, > + .type_ops = &area_ops, > + }; > + > + return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); > +} > +EXPORT_SYMBOL(v4l2_ctrl_new_area); > + > +/* Helper function for standard integer menu controls */ ... here? Looks to me like this comment should stay attached to v4l2_ctrl_new_int_menu. regards Philipp
On Fri, Aug 23, 2019 at 2:56 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote: > > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, > > try to minimize it by adding a new helper. > > > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > > --- > > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++- > > include/media/v4l2-ctrls.h | 16 ++++++++++++++++ > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > index b3bf458df7f7..33e48f0aec1a 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > @@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > > } > > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > > > > -/* Helper function for standard integer menu controls */ > > Why move this ... > > > struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > > const struct v4l2_ctrl_ops *ops, > > u32 id, u8 _max, u8 _def, const s64 *qmenu_int) > > @@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > > } > > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > > > > +static void area_init(const struct v4l2_ctrl *ctrl, u32 idx, > > + union v4l2_ctrl_ptr ptr) > > +{ > > + memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area)); > > +} > > + > > +static const struct v4l2_ctrl_type_ops area_ops = { > > + .init = area_init, > > +}; > > + > > +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > > + const struct v4l2_ctrl_ops *ops, > > + u32 id, const struct v4l2_area *area) > > +{ > > + static struct v4l2_ctrl_config ctrl = { > > + .id = V4L2_CID_UNIT_CELL_SIZE, > > + .type_ops = &area_ops, > > + }; > > + > > + return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); > > +} > > +EXPORT_SYMBOL(v4l2_ctrl_new_area); > > + > > +/* Helper function for standard integer menu controls */ > > ... here? Because I screwed up :). Let me fix that sorry. I will push all your changes to: https://github.com/ribalda/linux/tree/unit-size-v4 plus any other comment and then I will wait 2-3 days for resend > > Looks to me like this comment should stay attached to > v4l2_ctrl_new_int_menu. > > regards > Philipp
On Fri, 2019-08-23 at 15:05 +0200, Ricardo Ribalda Delgado wrote: > On Fri, Aug 23, 2019 at 2:56 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote: > > > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, > > > try to minimize it by adding a new helper. > > > > > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > > > --- > > > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++- > > > include/media/v4l2-ctrls.h | 16 ++++++++++++++++ > > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > > index b3bf458df7f7..33e48f0aec1a 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > @@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > > > } > > > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > > > > > > -/* Helper function for standard integer menu controls */ > > > > Why move this ... > > > > > struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > > > const struct v4l2_ctrl_ops *ops, > > > u32 id, u8 _max, u8 _def, const s64 *qmenu_int) > > > @@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > > > } > > > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > > > > > > +static void area_init(const struct v4l2_ctrl *ctrl, u32 idx, > > > + union v4l2_ctrl_ptr ptr) > > > +{ > > > + memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area)); > > > +} > > > + > > > +static const struct v4l2_ctrl_type_ops area_ops = { > > > + .init = area_init, > > > +}; > > > + > > > +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > > > + const struct v4l2_ctrl_ops *ops, > > > + u32 id, const struct v4l2_area *area) > > > +{ > > > + static struct v4l2_ctrl_config ctrl = { > > > + .id = V4L2_CID_UNIT_CELL_SIZE, > > > + .type_ops = &area_ops, > > > + }; > > > + > > > + return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); > > > +} > > > +EXPORT_SYMBOL(v4l2_ctrl_new_area); > > > + > > > +/* Helper function for standard integer menu controls */ > > > > ... here? > > Because I screwed up :). Let me fix that sorry. > > I will push all your changes to: > > https://github.com/ribalda/linux/tree/unit-size-v4 > > plus any other comment and then I will wait 2-3 days for resend Awesome, thanks! Feel free to add Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
Hi Ricardo, On Fri, Aug 23, 2019 at 03:13:36PM +0200, Philipp Zabel wrote: > On Fri, 2019-08-23 at 15:05 +0200, Ricardo Ribalda Delgado wrote: > > On Fri, Aug 23, 2019 at 2:56 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > > On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote: > > > > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, > > > > try to minimize it by adding a new helper. > > > > > > > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > > > > --- > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++- > > > > include/media/v4l2-ctrls.h | 16 ++++++++++++++++ > > > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > index b3bf458df7f7..33e48f0aec1a 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > > > @@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > > > > } > > > > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > > > > > > > > -/* Helper function for standard integer menu controls */ > > > > > > Why move this ... > > > > > > > struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > > > > const struct v4l2_ctrl_ops *ops, > > > > u32 id, u8 _max, u8 _def, const s64 *qmenu_int) > > > > @@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > > > > } > > > > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > > > > > > > > +static void area_init(const struct v4l2_ctrl *ctrl, u32 idx, > > > > + union v4l2_ctrl_ptr ptr) > > > > +{ > > > > + memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area)); > > > > +} > > > > + > > > > +static const struct v4l2_ctrl_type_ops area_ops = { > > > > + .init = area_init, > > > > +}; > > > > + > > > > +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > > > > + const struct v4l2_ctrl_ops *ops, > > > > + u32 id, const struct v4l2_area *area) > > > > +{ > > > > + static struct v4l2_ctrl_config ctrl = { > > > > + .id = V4L2_CID_UNIT_CELL_SIZE, > > > > + .type_ops = &area_ops, > > > > + }; > > > > + > > > > + return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); > > > > +} > > > > +EXPORT_SYMBOL(v4l2_ctrl_new_area); > > > > + > > > > +/* Helper function for standard integer menu controls */ > > > > > > ... here? > > > > Because I screwed up :). Let me fix that sorry. > > > > I will push all your changes to: > > > > https://github.com/ribalda/linux/tree/unit-size-v4 > > > > plus any other comment and then I will wait 2-3 days for resend > > Awesome, thanks! Feel free to add > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> With the issue pointed out by Philipp addressed Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > regards > Philipp
On 8/23/19 2:37 PM, Ricardo Ribalda Delgado wrote: > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, > try to minimize it by adding a new helper. > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++- > include/media/v4l2-ctrls.h | 16 ++++++++++++++++ > 2 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index b3bf458df7f7..33e48f0aec1a 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > } > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > > -/* Helper function for standard integer menu controls */ Huh? > struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > const struct v4l2_ctrl_ops *ops, > u32 id, u8 _max, u8 _def, const s64 *qmenu_int) > @@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > } > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > > +static void area_init(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr) > +{ > + memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area)); This can be used in an array, so you have to honor the idx field. > +} > + > +static const struct v4l2_ctrl_type_ops area_ops = { > + .init = area_init, > +}; This is a standard control type, so just add support for it the std_type_ops functions. > + > +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > + const struct v4l2_ctrl_ops *ops, > + u32 id, const struct v4l2_area *area) > +{ > + static struct v4l2_ctrl_config ctrl = { > + .id = V4L2_CID_UNIT_CELL_SIZE, This should be set to the passed id, not hardcoded. > + .type_ops = &area_ops, And just drop this line. > + }; > + > + return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); Don't pass area as a priv pointer. Instead the core will just initialize the area to some default value (1x1), and then you can call set_ctrl() after creating the control to set it to the proper value. The READ_ONLY flag applies to the public API, but the kernel API still has to be able to change it. > +} > +EXPORT_SYMBOL(v4l2_ctrl_new_area); > + > +/* Helper function for standard integer menu controls */ This comment doesn't belong here. > /* Add the controls from another handler to our own. */ > int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl, > struct v4l2_ctrl_handler *add, > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index 9a3d11350e67..36f0712ea6dd 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -669,6 +669,22 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > u32 id, u8 max, u8 def, > const s64 *qmenu_int); > > +/** > + * v4l2_ctrl_new_area() - Allocate and initialize a V4L2_CTRL_TYPE_AREA control. > + * > + * @hdl: The control handler. > + * @ops: The control ops. > + * @id: The control ID. > + * @area: The width and height of the area in a struct v4l2_area. Specifically, this is the initial width and height. > + * > + * If the &v4l2_ctrl struct could not be allocated then NULL is returned > + * and @hdl->error is set to the error code (if it wasn't set already). > + */ > + > +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > + const struct v4l2_ctrl_ops *ops, > + u32 id, const struct v4l2_area *area); > + > /** > * typedef v4l2_ctrl_filter - Typedef to define the filter function to be > * used when adding a control handler. > Regards, Hans
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index b3bf458df7f7..33e48f0aec1a 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, } EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); -/* Helper function for standard integer menu controls */ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, const struct v4l2_ctrl_ops *ops, u32 id, u8 _max, u8 _def, const s64 *qmenu_int) @@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, } EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); +static void area_init(const struct v4l2_ctrl *ctrl, u32 idx, + union v4l2_ctrl_ptr ptr) +{ + memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area)); +} + +static const struct v4l2_ctrl_type_ops area_ops = { + .init = area_init, +}; + +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, + u32 id, const struct v4l2_area *area) +{ + static struct v4l2_ctrl_config ctrl = { + .id = V4L2_CID_UNIT_CELL_SIZE, + .type_ops = &area_ops, + }; + + return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); +} +EXPORT_SYMBOL(v4l2_ctrl_new_area); + +/* Helper function for standard integer menu controls */ /* Add the controls from another handler to our own. */ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl, struct v4l2_ctrl_handler *add, diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index 9a3d11350e67..36f0712ea6dd 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -669,6 +669,22 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, u32 id, u8 max, u8 def, const s64 *qmenu_int); +/** + * v4l2_ctrl_new_area() - Allocate and initialize a V4L2_CTRL_TYPE_AREA control. + * + * @hdl: The control handler. + * @ops: The control ops. + * @id: The control ID. + * @area: The width and height of the area in a struct v4l2_area. + * + * If the &v4l2_ctrl struct could not be allocated then NULL is returned + * and @hdl->error is set to the error code (if it wasn't set already). + */ + +struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, + u32 id, const struct v4l2_area *area); + /** * typedef v4l2_ctrl_filter - Typedef to define the filter function to be * used when adding a control handler.
Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, try to minimize it by adding a new helper. Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> --- drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++- include/media/v4l2-ctrls.h | 16 ++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-)