Hi Hans,
On 2017-06-13 03:49 AM, Hans Verkuil wrote:
On 06/12/2017 10:35 PM, Helen Koike wrote:
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.
Ah, so in the TODO you mean:
"the best way to pass the entity name to the driver"
Yes
I got confused there.
Sorry about that, I'll improve this comment (if we don't decide to
remove it)
But in that case I still don't get what you mean with "add the drv name
in the platform_device_id table". Do you mean "entity name" there as
well?
Yes, it is because there is a member of the platform_device_id table
called driver_data and I was wondering if this would be more
appropriated then using the platform_data.
Under Documentation/driver-model/platform.txt it is written:
"In many cases, the memory and IRQ resources associated with the
platform device are not enough to let the device's driver work. Board
setup code
will often provide additional information using the device's platform_data
field to hold additional information."
So I thought that using platform_data to pass the entity's name to the
driver would be acceptable as it seems to be a "Board setup code" for vimc
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.
Why not? Well, probably not the entity number, but a simple instance
counter would do fine (i.e. Sensor 1, 2, 3...).
Because I usually use tests scripts that configure the right pad image
format to a specific entity's name using media-ctl, I would like an
assurance that what identifies the entity doesn't change (that doesn't
depend on the order they are inserted), and I thought that applications
in userspace might want the same.
It can be made fancier later with dynamic reconfiguration where you
might want to use the first unused instance number.
We could use configfs for that, but I was wondering what the names of
the folders that represent an entity would mean, if the user do
$ mkdir vimc-sensor-foo
$ mkdir vimc-sensor-bar
Then foo and bar would unused names as the entities would be first
created under the names "Sensor 1" and "Sensor 2", I think it would be
nice if they were created as "Sensor foo" and "Sensor bar".
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?
That's OK. The v4l2-async mechanism precedes the component API. We should
probably investigate moving over to the component API. I seem to remember
that it didn't have all the features we needed, but it's a long time ago
since someone looked at that and whatever the objections were, they may
no longer be true.
I see, I can try to take a look on this.
Regards,
Hans
Thanks
Helen
