Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Guennadi,

On Tuesday 31 July 2012 13:29:55 Guennadi Liakhovetski wrote:
> On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote:
> > > On Tue, 31 Jul 2012, Laurent Pinchart wrote:
> > > > On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote:
> > > > > On Thu, 26 Jul 2012, Laurent Pinchart wrote:
> > > > > > On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote:
> > > > > > > On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote:
> > > > > > > > On Fri, 25 May 2012, Sylwester Nawrocki wrote:
> > > > > > > >> The driver initializes all board related properties except
> > > > > > > >> the s_power() callback to board code. The platforms that
> > > > > > > >> require this callback are not supported by this driver yet
> > > > > > > >> for CONFIG_OF=y.
> > > > > > > >> 
> > > > > > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> > > > > > > >> Signed-off-by: Bartlomiej
> > > > > > > >> Zolnierkiewicz<b.zolnierkie@samsung.com>
> > > > > > > >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > > > > > >> ---
> > > > > > > >> 
> > > > > > > >>   .../bindings/camera/samsung-s5k6aafx.txt           |   57
> > > > > > > >>   +++++++++
> > > > > > > >>   drivers/media/video/s5k6aa.c                       |  129
> > > > > > > >>   ++++++++++++++------ 2 files changed, 146 insertions(+), 40
> > > > > > > >>   deletions(-)
> > > > > > > >>   create mode 100644
> > > > > > > >>   Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >>   xt>>
> > > > > > > >> 
> > > > > > > >> diff --git
> > > > > > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >> xt
> > > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >> xt
> > > > > > > >> new
> > > > > > > >> file
> > > > > > > >> mode 100644
> > > > > > > >> index 0000000..6685a9c
> > > > > > > >> --- /dev/null
> > > > > > > >> +++
> > > > > > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t
> > > > > > > >> xt
> > > > > > > >> @@ -0,0 +1,57 @@
> > > > > > > >> +Samsung S5K6AAFX camera sensor
> > > > > > > >> +------------------------------
> > > > > > > >> +
> > > > > > > >> +Required properties:
> > > > > > > >> +
> > > > > > > >> +- compatible : "samsung,s5k6aafx";
> > > > > > > >> +- reg : base address of the device on I2C bus;
> > > > > > > > 
> > > > > > > > You said you ended up putting your sensors outside of I2C
> > > > > > > > busses, is this one of changes, that are present in your git-
> > > > > > > > tree but not in this series?
> > > > > > > 
> > > > > > > No, I must have been not clear enough on that. Our idea was to
> > > > > > > keep I2C slave device nodes as an I2C controller's child nodes,
> > > > > > > according to the current convention. The 'sensor' nodes (the
> > > > > > > 'camera''s children) would only contain a phandle to a
> > > > > > > respective I2C slave node.
> > > > > > > 
> > > > > > > This implies that we cannot access I2C bus in I2C client's
> > > > > > > device probe() callback. An actual H/W access could begin only
> > > > > > > from within and after invocation of v4l2_subdev .registered
> > > > > > > callback..
> > > > > > 
> > > > > > That's how I've envisioned the DT bindings for sensors as well,
> > > > > > this sounds good. The real challenge will be to get hold of the
> > > > > > subdev to register it without race conditions.
> > > > > 
> > > > > Hrm... That's how early pre-subdev versions of soc-camera used to
> > > > > work, that's where all the <device>_video_probe() functions come
> > > > > from. But then we switched to dynamic i2c device registration. Do we
> > > > > want to switch all drivers back now?... Couldn't we "temporarily"
> > > > > use references from subdevs to hosts until the clock API is
> > > > > available?
> > > > 
> > > > I don't think that requires a reference from subdevs to hosts in the
> > > > DT. The subdev will need the host to be probed before a clock can be
> > > > available so you won't be able to access the hardware in the probe()
> > > > function in the generic case. You will need to wait until the
> > > > registered() subdev operation is called, at which point the host can
> > > > be accessed through the v4l2_device.
> > > 
> > > Sure, I understand, but that's exactly what we wanted to avoid -
> > > succeeding client's i2c .probe() without even touching the hardware.
> > 
> > But should we allow host probe() to succeed if the sensor isn't present ?
> 
> I think we should, yes. The host hardware is there and functional -
> whether or not all or some of the clients are failing. Theoretically
> clients can also be hot-plugged. Whether and how many video device nodes
> we create, that's a different question.

I think I can agree with you on this (although I could change my mind if this 
architecture turns out to result in unsolvable technical issues). That will 
involve a lot of work though.

-- 
Regards,

Laurent Pinchart