Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Guennadi,

Sorry for the delay, I only saw this today while I was going through my
mail backlog.

On 10/17/2013 08:24 PM, Guennadi Liakhovetski wrote:
> Hi Hans
> 
> Sorry for reviving this old thread. I was going to resubmit a part of 
> those patches for mainlining and then I found this your comment, which I 
> didn't reply to back then.
> 
> On Fri, 19 Apr 2013, Hans Verkuil wrote:
> 
>> On Fri April 19 2013 09:48:27 Guennadi Liakhovetski wrote:
>>> Hi Hans
>>>
>>> Thanks for reviewing.
>>>
>>> On Fri, 19 Apr 2013, Hans Verkuil wrote:
>>>
>>>> On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
>>>>> This struct shall be used by subdevice drivers to pass per-subdevice data,
>>>>> e.g. power supplies, to generic V4L2 methods, at the same time allowing
>>>>> optional host-specific extensions via the host_priv pointer. To avoid
>>>>> having to pass two pointers to those methods, add a pointer to this new
>>>>> struct to struct v4l2_subdev.
>>>>>
>>>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>>> ---
>>>>>  include/media/v4l2-subdev.h |   13 +++++++++++++
>>>>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>>> index eb91366..b15c6e0 100644
>>>>> --- a/include/media/v4l2-subdev.h
>>>>> +++ b/include/media/v4l2-subdev.h
>>>>> @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
>>>>>  /* Set this flag if this subdev generates events. */
>>>>>  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
>>>>>  
>>>>> +struct regulator_bulk_data;
>>>>> +
>>>>> +struct v4l2_subdev_platform_data {
>>>>> +	/* Optional regulators uset to power on/off the subdevice */
>>>>> +	struct regulator_bulk_data *regulators;
>>>>> +	int num_regulators;
>>>>> +
>>>>> +	/* Per-subdevice data, specific for a certain video host device */
>>>>> +	void *host_priv;
>>>>> +};
>>>>> +
>>>>>  /* Each instance of a subdev driver should create this struct, either
>>>>>     stand-alone or embedded in a larger struct.
>>>>>   */
>>>>> @@ -589,6 +600,8 @@ struct v4l2_subdev {
>>>>>  	/* pointer to the physical device */
>>>>>  	struct device *dev;
>>>>>  	struct v4l2_async_subdev_list asdl;
>>>>> +	/* common part of subdevice platform data */
>>>>> +	struct v4l2_subdev_platform_data *pdata;
>>>>>  };
>>>>>  
>>>>>  static inline struct v4l2_subdev *v4l2_async_to_subdev(
>>>>>
>>>>
>>>> Sorry, this is the wrong approach.
>>>>
>>>> This is data that is of no use to the subdev driver itself. It really is
>>>> v4l2_subdev_host_platform_data, and as such must be maintained by the bridge
>>>> driver.
>>>
>>> I don't think so. It has been discussed and agreed upon, that only 
>>> subdevice drivers know when to switch power on and off, because only they 
>>> know when they need to access the hardware. So, they have to manage 
>>> regulators. In fact, those regulators supply power to respective 
>>> subdevices, e.g. a camera sensor. Why should the bridge driver manage 
>>> them? The V4L2 core can (and probably should) provide helper functions for 
>>> that, like soc-camera currently does, but in any case it's the subdevice 
>>> driver, that has to call them.
>>
>> Ah, OK. I just realized I missed some context there. I didn't pay much
>> attention to the regulator discussions since that's not my area of expertise.
>>
>> In that case my only comment is to drop the host_priv pointer since that just
>> duplicates v4l2_get/set_subdev_hostdata().
> 
> I think it's different. This is _platform_ data, whereas struct 
> v4l2_subdev::host_priv is more like run-time data.

You mean subdev_hostdata() instead of host_priv, right?

> This field is for the 
> per-subdevice host-specific data, that the platform has to pass to the 
> host driver. In the soc-camera case this is the largest bulk of the data, 
> that platforms currently pass to the soc-camera framework in the host part 
> of struct soc_camera_link. This data most importantly includes I2C 
> information. Yes, this _could_ be passed to soc-camera separately from the 
> host driver, but that would involve quite some refactoring of the "legacy" 
> synchronous probing mode, which I'd like to avoid if possible. This won't 
> be used in the asynchronous case. Do you think we can keep this pointer in 
> this sruct? We could rename it to avoid confusion with the field, that you 
> told about.

I'm wondering: do we need host_priv at all? Can't drivers use container_of to
go from struct v4l2_subdev_platform_data to the platform_data struct containing
v4l2_subdev_platform_data?

That would be a cleaner solution IMHO. Using host_priv basically forces you
to split up the platform_data into two parts, and a void pointer isn't very
type-safe.

Regards,

	Hans