Hi Hans,
Thank you for your review. Please check my comments below
On 2017-06-12 07:37 AM, Hans Verkuil wrote:
On 06/03/2017 04:58 AM, Helen Koike wrote:
+static struct component_match *vimc_add_subdevs(struct vimc_device
*vimc)
+{
+ struct component_match *match = NULL;
+ unsigned int i;
+
+ for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
+ dev_dbg(&vimc->pdev.dev, "new pdev for %s\n",
+ vimc->pipe_cfg->ents[i].drv);
+
+ /*
+ * TODO: check if using platform_data is indeed the best way to
+ * pass the name to the driver or if we should add the drv name
+ * in the platform_device_id table
+ */
Didn't you set the drv name in the platform_device_id table already?
I refer to the name of the entity, there is the name that identifies the
driver as "vimc-sensor" that is set in the platform_device_id table but
there is also the custom name of the entity e.g. "My Sensor A" that I
need to inform to the vimc-sensor driver.
Using platform_data feels like an abuse to be honest.
Another option would be to make the vimc-sensor driver to populate the
entity name automatically as "Sensor x", where x could be the entity
number, but I don't think this is a good option.
Creating these components here makes sense. Wouldn't it also make sense
to use
v4l2_async to wait until they have all been bound? It would more closely
emulate
standard drivers. Apologies if I misunderstand what is happening here.
I am using linux/component.h for that, when all devices are present and
all modules are loaded, the component.h system brings up the core by
calling vimc_comp_bind() function, which calls component_bind_all() to
call the binding function of each module, then it finishes registering
the topology.
If any one of the components or module is unload, the component system
takes down the entire topology calling component_unbind_all which calls
the unbind functions from each module.
This makes sure that the media device, subdevices and video device are
only registered in the v4l2 system if all the modules are loaded.
I wans't familiar with v4l2-async.h, but from a quick look it seems that
it only works with struct v4l2_subdev, but I'll also need for struct
video_device (for the capture node for example).
And also, if a module is missing we would have vimc partially
registered, e.g. the debayer could be registered at /dev/subdevX but the
sensor not yet and the media wouldn't be ready, I am not sure if this is
a problem though.
Maybe we can use component.h for now, then I can implement
v4l2_async_{un}register_video_device and migrate to v4l2-sync.h latter.
What do you think?