Hi Hans,
On Monday 04 November 2013 12:24:10 Hans Verkuil wrote:
> 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@xxxxxx>
> >>>>> ---
> >>>>>
> >>>>> 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.
For what it's worth, that's the solution I was thinking about, so it has my
preference as well.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
