Hi Hans, On Thu, Aug 29, 2019 at 12:31:37PM +0200, Hans Verkuil wrote: > On 8/27/19 11:23 AM, 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 > > --- > > 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 > > > > #include > > +#include > > #include > > #include > > > > @@ -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) > > I'm not convinced that this helper is a good idea. > > A helper that parses and validates this information makes sense, > but combining that with creating the controls feels wrong to me. > > You're mixing two very different things in one function. > > I think something like this would work better in a driver: > > if (!v4l2_fwnode_parse_location(&val)) > v4l2_ctrl_new_std(hdl, ctrl_ops, > V4L2_CID_CAMERA_SENSOR_LOCATION, > val, val, 1, val); > if (!v4l2_fwnode_parse_rotation(&val)) > v4l2_ctrl_new_std(hdl, ctrl_ops, > V4L2_CID_CAMERA_SENSOR_ROTATION, > val, val, 1, val); > > Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these > function prototypes!) > Could the control registration being conditional on the presence of the *hdl parameter otherwise, or would you split the two operations (property parsing and control registration) nonetheless ? An helper was suggested in the v1 review, Laurent, Sakari, what do you think here? > > +{ > > + 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); > > + 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) { > > I'd add '|| val % 90' to this condition. > Do we want to enforce this? I can't imagine any use case, but why a camera cannot be rotated of an arbitrary number of degrees ? Thanks j > Regards, > > Hans > > > + pr_warn("Unsupported rotation: %u\n", val); > > + return -EINVAL; > > + } > > + > > + 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 > > + * @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. > > >