Re: [RFC PATCH v3 09/11] [media] vimc: Subdevices as modules

[Hans Verkuil]

On 06/13/17 15:23, Helen Koike wrote:
> 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

OK, now I understand. I am OK with this, but I would prefer if you made a
proper struct for this:

struct vimc_platform_data {
	char entity_name[32];
};

Then point platform_data to that. Also comment clearly why you do it.

Part of the confusion comes from the fact that this is unusual for MC
devices: they all use the device tree instead of platform_data, so seeing
platform_data being used in something like this is unexpected.

> 
>>
>>>
>>>>
>>>> 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.

Good point!

> 
>>
>> 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.

Something for later.

So in other words, this looks good but needs better comments and a proper
platform_data struct.

Regards,

	Hans