Hi Laurent, On Tue, Aug 27, 2019 at 04:16:20PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Aug 27, 2019 at 11:23:34AM +0200, Jacopo Mondi wrote: > > Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The > > function parses the device node and endpoint firmware properties to > > which a v4l2 control is associated to and registers the control with the > > provided handler. > > > > Signed-off-by: Jacopo Mondi <[email protected]> > > --- > > drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++ > > include/media/v4l2-fwnode.h | 30 ++++++++++++++ > > 2 files changed, 87 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c > > b/drivers/media/v4l2-core/v4l2-fwnode.c > > index 3bd1888787eb..669801fceb64 100644 > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > @@ -25,6 +25,7 @@ > > #include <linux/types.h> > > > > #include <media/v4l2-async.h> > > +#include <media/v4l2-ctrls.h> > > #include <media/v4l2-fwnode.h> > > #include <media/v4l2-subdev.h> > > > > @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link > > *link) > > } > > EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link); > > > > +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode, > > + struct v4l2_ctrl_handler *hdl, > > + const struct v4l2_ctrl_ops *ctrl_ops) > > Passing the ctrl_ops is a bit annoying. Would there be a way to get the > V4L2 control framework to accept NULL ops for read-only controls ? >
That would require a core change that is imho outside the scope of
this patch
> > +{
> > + u32 val;
> > + int ret;
> > +
> > + ret = fwnode_property_read_u32(fwnode, "location", &val);
> > + if (!ret) {
> > + switch (val) {
> > + case V4L2_LOCATION_FRONT:
> > + case V4L2_LOCATION_BACK:
> > + case V4L2_LOCATION_EXTERNAL:
> > + break;
> > + default:
> > + pr_warn("Unsupported location: %u\n", val);
>
> dev_warn() would be nicer. Is there a way we could either pass the
> struct device pointer, or maybe a subdev that would be populated with
> the device, fwnode and hdl pointers ?
>
I went for pr_ as in all other functions of this file it is used when
a fwnode_handle is passed as parameter. I could pass the device node,
but I'm not sure passing an already populated subdevice is a good
idea, as the subdevice ctrl_handler is usually populated after having
made sure all controls have been registered without errors.
> > + return -EINVAL;
> > + }
> > +
> > + if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> > + pr_debug("Skip control '%s': already registered",
> > + v4l2_ctrl_get_name(
> > + V4L2_CID_CAMERA_SENSOR_LOCATION));
> > + else
> > + v4l2_ctrl_new_std(hdl, ctrl_ops,
> > + V4L2_CID_CAMERA_SENSOR_LOCATION,
> > + val, val, 1, val);
> > + }
> > +
> > + ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> > + if (!ret) {
> > + if (val > 360) {
> > + pr_warn("Unsupported rotation: %u\n", val);
> > + return -EINVAL;
> > + }
>
> We need to define the range of allowed values in the control
> documentation. 360 doesn't seem very useful as it's equivalent to 0.
> A few possible options are [0, 360[, [-180, +180[ or ]-180, +180].
>
As asked to Hans, why should we restrict this? I don't see a use case
for a 73 degrees rotated camera, but why prevent it?
> > +
> > + if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> > + pr_debug("Skip control '%s': already registered",
> > + v4l2_ctrl_get_name(
> > + V4L2_CID_CAMERA_SENSOR_ROTATION));
> > + else
> > + v4l2_ctrl_new_std(hdl, ctrl_ops,
> > + V4L2_CID_CAMERA_SENSOR_ROTATION,
> > + val, val, 1, val);
> > + }
> > +
> > + if (hdl->error) {
> > + pr_warn("Failed to register controls from firmware: %d\n",
> > + hdl->error);
> > + return hdl->error;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> > +
> > static int
> > v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> > struct v4l2_async_notifier *notifier,
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index f6a7bcd13197..0dad6968bde9 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -25,6 +25,8 @@
> > struct fwnode_handle;
> > struct v4l2_async_notifier;
> > struct v4l2_async_subdev;
> > +struct v4l2_ctrl_handler;
> > +struct v4l2_ctrl_ops;
> >
> > #define V4L2_FWNODE_CSI2_MAX_DATA_LANES 4
> >
> > @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
> > *fwnode,
> > */
> > void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +/**
> > + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> > + * properties and register a v4l2 control
> > + * for each of them
>
> I don't think that description is accurate.
>
How so? The function parses the device (not yet endpoint, right)
properties and register a control for each of them.
How would you phrase this?
Thanks
j
> > + * @fwnode: pointer to the device fwnode handle
> > + * @hdl: pointer to the v4l2 control handler to register controls with
> > + * @ctrl_ops: pointer to the v4l2 control operations to register with the
> > handler
> > + *
> > + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> > + * is associated and register them with the provided handler @hdl.
> > + * Currently the following v4l2 controls are parsed and registered:
> > + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> > + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> > + *
> > + * Controls already registered by the caller with the @hdl control handler
> > are
> > + * not overwritten. Callers should register the controls they want to
> > handle
> > + * themselves before calling this function.
> > + *
> > + * NOTE: This function locks the @hdl control handler mutex, the caller
> > shall
> > + * not hold the lock when calling this function.
> > + *
> > + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> > + * specified.
> > + */
> > +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> > + struct v4l2_ctrl_handler *hdl,
> > + const struct v4l2_ctrl_ops *ctrl_ops);
> > +
> > /**
> > * typedef parse_endpoint_func - Driver's callback function to be called on
> > * each V4L2 fwnode endpoint.
>
> --
> Regards,
>
> Laurent Pinchart
signature.asc
Description: PGP signature
