Message-ID: <4cb013f2-fd65-9628-e250-bbf7b9709353@collabora.com>
Date:   Tue, 13 Jun 2017 10:23:40 -0300
From:   Helen Koike <helen.koike@...labora.com>
To:     Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-kernel@...r.kernel.org
Cc:     jgebben@...eaurora.org, mchehab@....samsung.com,
        Sakari Ailus <sakari.ailus@....fi>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [RFC PATCH v3 09/11] [media] vimc: Subdevices as modules

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

Powered by blists - more mailing lists