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
