Hi Hans, Laurent, On Thu, Aug 29, 2019 at 05:32:52PM +0200, Hans Verkuil wrote: > On 8/29/19 5:05 PM, Laurent Pinchart wrote: > > Hi Hans, > > > > On Thu, Aug 29, 2019 at 03:04:42PM +0200, Hans Verkuil wrote: > >> On 8/29/19 2:45 PM, Jacopo Mondi wrote: > >>> 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 ? > >> > >> Split it. My main problem with this helper is that it is mixing two > >> frameworks. Most of Laurent's comments on this patch just go away if > >> you leave the control creation to the driver. > >> > >> It really isn't much code, and it is much easier to review a driver > >> if all the controls are created in the same place instead of some > >> controls being magically created in a helper function. > > > > But this would require copying the above code in every single camera > > sensor driver. Furthermore, the helper proposed by Jacopo would make > > addition of new firmware properties much simpler, as we wouldn't need to > > modify all sensor drivers. > > > > V4L2 requires lots of boilerplate code for sensor drivers, and I think > > more helper would be useful. There's really not point in doing the same > > thing slightly differently in dozens of drivers. Maybe we could > > experiment with a v4l2_camera_subdev structure ? > > You can make one v4l2_fwnode helper that parses all the sensor properties, > and another helper in v4l2-ctrl.c or v4l2-common.c that uses the parsed > result to create the controls. That way if the driver needs to do something > unusual with the controls it can just create them manually. > > But don't mix the two in one helper. > I see, I like this better than a single helper per property, as this is not going to scale well if the number of static properties increases. > Note that creating some more advanced framework for sensors wouldn't hurt > since it's a bit mess at the moment IMHO. > I would start by defining a v4l2_sensor_properties structure, filled by parsing the fwnode properties and then used by to register controls with the informations there collected. Thanks j > Regards, > > Hans > > > > >>> 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 ? > >> > >> I would start out by enforcing this until someone comes up with a > >> realistic use-case. > >> > >> As long as it is a multiple of 90 degree, then there is a clear interaction > >> with the ROTATE/HFLIP/VFLIP controls. For other angles that gets more confusing > >> and I'd rather avoid that for now. > > > > If we enfore this, then let's update the DT bindings accordingly. yaml > > would help with validation ;-) > > > >>>>> + 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. > > >