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

[Guennadi Liakhovetski]

On Tue, 31 Jul 2012, Laurent Pinchart wrote:

> 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 at samsung.com>
> > > > > > > > >> Signed-off-by: Bartlomiej
> > > > > > > > >> Zolnierkiewicz<b.zolnierkie at samsung.com>
> > > > > > > > >> Signed-off-by: Kyungmin Park<kyungmin.park at 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.

There's however at least one more gotcha that occurs to me with this 
approach: if clients fail to probe, how do we find out about that and turn 
clocks back off? One improvement to turning clocks on immediately in 
host's probe() is to only do it in a BUS_NOTIFY_BIND_DRIVER notifier. But 
how do we find out, that probing failed? No notifier is called in this 
case. We could use a time-out, but that's ugly. I think, we could ever 
request a new notifier for this case. We could also require client drivers 
to call a V4L2 function in this case, but that's not very pretty either.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/