Re: Re: [RFC] misc: Add Allwinner Q8 tablet hardware manager

[Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On Sat, Sep 10, 2016 at 08:12:32PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 09-09-16 23:41, Rob Herring wrote:
> >On Thu, Sep 1, 2016 at 2:08 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>Allwinnner A13 / A23 / A33 based Q8 tablets are popular cheap 7" tablets
> >>of which a new batch is produced every few weeks. Each batch uses a
> >>different mix of touchscreen, accelerometer and wifi peripherals.
> >>
> >>Given that each batch is different creating a devicetree for each variant
> >>is not desirable. This commit adds a Q8 tablet hardware manager which
> >>auto-detects the touchscreen and accelerometer so that a single generic
> >>dts can be used for these tablets.
> >>
> >>The wifi is connected to a discoverable bus (sdio or usb) and will be
> >>autodetected by the mmc resp. usb subsystems.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>---
> >> .../misc/allwinner,sunxi-q8-hardwaremgr.txt        |  52 +++
> >> drivers/misc/Kconfig                               |  12 +
> >> drivers/misc/Makefile                              |   1 +
> >> drivers/misc/q8-hardwaremgr.c                      | 512 +++++++++++++++++++++
> >> 4 files changed, 577 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/misc/allwinner,sunxi-q8-hardwaremgr.txt
> >> create mode 100644 drivers/misc/q8-hardwaremgr.c
> >>
> >>diff --git a/Documentation/devicetree/bindings/misc/allwinner,sunxi-q8-hardwaremgr.txt b/Documentation/devicetree/bindings/misc/allwinner,sunxi-q8-hardwaremgr.txt
> >>new file mode 100644
> >>index 0000000..f428bf5
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/misc/allwinner,sunxi-q8-hardwaremgr.txt
> >>@@ -0,0 +1,52 @@
> >>+Q8 tablet hardware manager
> >>+--------------------------
> >>+
> >>+Allwinnner A13 / A23 / A33 based Q8 tablets are popular cheap 7" tablets of
> >>+which a new batch is produced every few weeks. Each batch uses a different
> >>+mix of touchscreen, accelerometer and wifi peripherals.
> >>+
> >>+Given that each batch is different creating a devicetree for each variant is
> >>+not desirable. The Q8 tablet hardware manager bindings are bindings for an os
> >>+module which auto-detects the touchscreen so that a single
> >>+generic dts can be used for these tablets.
> >>+
> >>+The wifi is connected to a discoverable bus and will be autodetected by the os.
> >>+
> >>+Required properties:
> >>+ - compatible         : "allwinner,sunxi-q8-hardwaremgr"
> >>+ - touchscreen        : phandle of a template touchscreen node, this must be a
> >>+                       child node of the touchscreen i2c bus
> >>+
> >>+Optional properties:
> >>+ - touchscreen-supply : regulator phandle for the touchscreen vdd supply
> >
> >While I said I think you should be using overlays here, you could also
> >do it without.
> 
> Good, because I believe that doing things without overlays will
> be much easier. If you look at the actual implementation you will
> see that it sets a lot of properties (touchscreen width, height,
> inversion, axis-swapping, firmware-name) based on various sources,
> the explosion of possible overlays from this is huge, and if
> we're going to runtime patch the overlays then why use them at all
> and not just runtime generate all the info ?

That's fine if run-time makes more sense. I'd just like to move in the 
direction of applying a hunk of changes rather than C code creating 
nodes and properties one-by-one.
 
> >However, this node has to go. It is not h/w, and you
> >are putting it here purely to instantiate a driver.
> 
> With "this" in "this node has to go", do you mean the node
> with the "allwinner,sunxi-q8-hardwaremgr" compatible, or
> do you mean the touchscreen template node ?

Given that this binding section above is concerning 
allwinner,sunxi-q8-hardwaremgr, that is what I'm referring to.

> 
> >For the
> >touchscreen property, surely you know where the touchscreen is located
> >in the DT?
> 
> Nope q8 tablets come with A13, A23 or A33 SoCs, A23/A33 are pin
> compatible with each other, but the A13 is not, this leads to
> the touchscreen being on a different i2c bus, so I cannot hardcode
> things, likewise the power-gpios and interrupt found in the
> touchscreen node differ between the A13 vs A23/A33.

Either the touchscreen node name is "touchscreen" or you can have 
compatible string for q8-touchscreen and use of_find_node_by_name or 
of_find_compatible_node. Doesn't matter what bus it is on. It only 
requires that there is only 1 node.

> I actually had a touchscreen-i2c-bus property containing a
> phandle to the i2c controller for the bus which has the
> touchscreen in an earlier revision + touchscreen-gpios
> and touchscreen-supply properties, I can switch to that
> if you prefer that over having a touchscreen template node.
> 
> > If not, of_find_node_by_name()?
> 
> Still a bit confused about what you do not like,
> (touchscreen template node vs q8-hardwaremgr node).
> 
> I guess (based on context) you do not want the
> q8-hardwaremgr node ?
> 
> So the q8-hardwaremgr code should activate based on
> the machine compatible I presume? 

Either that, or if you have the board specific compatible in the 
touchscreen node, then you could have a driver for it. That driver would 
need to probe the device and then create the "real" device.
 
> How does that
> work with module autoloading ?

Can't non-DT based platform drivers do autoloading? Regardless, not 
really my problem from a binding perspective.

> If you do not want the q8-hardwaremgr node and thus
> no touchscreen property them yes I can use
> of_find_node_by_name(), but I thought that was
> generally frowned up on?

Generally we do prefer finding things by compatible strings. But there 
are exceptions and doing fix-ups like this is a good example.

> >For touchscreen-supply, I
> >assume this is to turn on the supply so you can talk to the touch
> >controller. There's no reason the supply can't just be in the
> >touchscreen node itself.
> 
> Only a few q8 tablets actually use the regulator, so
> the hardwaremgr first tries detecting the touchscreen
> without it, and only if it does not find anything then
> tries with it, and adds a property to the touchscreen
> node for it, but I guess I can do this other way
> around and have it be present in the (incomplete /
> template) touchscreen node and delete it from it if
> not necessary.

Right.

Rob