Re: [Gta04-owner] [PATCH 0/3] tty slave device support - version 3.

[Sebastian Reichel]

Hi,

On Fri, Mar 20, 2015 at 02:57:12PM +0100, Dr. H. Nikolaus Schaller wrote:
> > On Fri, Mar 20, 2015 at 09:54:21AM +0100, Dr. H. Nikolaus Schaller wrote:
> >> [...]
> >> And we do not write a driver for the w2sg0004 but the regulator inside that
> >> chip.
> > 
> > DT describes the hardware structure and not the Linux driver
> > structure.
> 
> I am not advocating for describing the Linux driver structure in DT.
> Rather I suggest that the DT should describe hardware and there
> is a regulator inside that chipâ

some drivers also have embedded clocks, which are not described,
since they are only useful for the device itself. Exposing them
is not useful.

> Linux drivers can follow that or mix everything into a single
> driver if that is better.

The DT binding is completly independent of drivers.

> >> You also wonât expect that the omap3 driver hides everything. You
> >> expect that the twl4030 provides some regulators that can be enabled
> >> by subsystems inside the omap3. And if I remember correctly there are
> >> regulators inside the omap3 which have explicit regulator nodes in the DT.
> > 
> > so let's have a look at twl regulators. They can be found below
> > the twl node, so they will look similar to
> > 
> > / -> omap3 -> i2c -> twl -> regulator
> > 
> > So properly mapped to the w2sg0004 device this would put your
> > regulator to
> > 
> > / -> omap3 -> serial -> w2sg0004 -> regulator
> 
> Yes. Indeed.
> 
> Currently it is
> 
> omap3 -> serial -> serial-power-manager (trying to cover a multitude
> of different chips).

No. The DT is omap3 -> serial -> w2sg0004 and w2sg0004 is
handled by a generic driver. If the kernel ever gets more
specific support for UART attached gps you can simply remove
the compatible value from the generic driver and create a
specific one.

> > This will gain you nothing as far as I can tell.
> 
> And because of that I think Neilâs argument isnât for or against
> anything.
> 
> > 
> > Please note that subdevices under the serial node are pretty useful,
> > since then the kernel can e.g. automatically setup correct line
> > disciplines for serial attached bluetooth chips and make bluetooth
> > work out of the box.
> 
> I donât doubt that such subnodes are useful. I just object mixing
> different chips and controls into a single subnode driver.

Well the binding allows to split this into multiple drivers
in the future (if needed).

> And as soon as you want to control line disciplines from such
> a subnode, you indeed need a driver for each variant.

Obviously I do not use the generic driver, but a specific one for
this task. Very simple devices (from kernel's point of view) can
still be handled by a generic driver (as Neil does).

> So a GPS chip needing some line discipline could be
> 
> > / -> omap3 -> serial -> w2sg0004 -> line-discipline
> > / -> omap3 -> serial -> w2sg0004 -> regulator

Actually no. The line discipline does not describe HW, but just
how linux handles the device. It should not be part of DT at all.
The kernel will see compatible string for w2sg0004 and load the
correct line discipline.

> And if omap3 -> serial can directly control a regulator, it can
> easily be the w2sg0004 -> regulator

sure.

> > I am aware, that the Linux kernel has no such thing for serial
> > attached GPS devices, but that's Linux specific and irrelevant
> > for the DT description.
> > 
> >> The w2sg0004-regulator approach just follows this principle.
> >> Anyone willing to control the power of the w2sg0004 can use this
> >> regulator interface.
> > 
> > From a HW perspective your regulator "hides" the information, that
> > there is a device attached to the serial port and instead claims,
> > that a regulator is needed to make the serial port work.
> 
> Yes, without this regulator the serial port does not work. It is IMHO
> more important than stating the obvious that a serial device is
> connected to a serial port.

This is actually not correct though. The serial port does work
without the regulator. The remote side does not work without the
regulator. So the regulator should be referenced by the w2sg0004
node, so it would be

serial-port {
    w2sg0004 {
        w2sg_vdd: gpio-regulator { enable-gpio = <&some_gpio> };
        vdd-supply = <&w2sg_vdd>;
    }
}

And at this point you can simply drop the regulator stuff from DT,
since its device internal (and only adds complexity). For other
devices these kind of ressources are also not exposed in DT (e.g.
internal clocks).

> And if there is nothing to hide (the obvious serial interface
> wiring), why describe it at all?
> 
> Anyways, in my proposal there will be a subnode of the uart, the
> one where it is specified that it should control the regulator of the
> w2sg.
> 
> Basically, Neilâs proposal already covers this case. His bluetooth
> subnode just controls &vaux4 which turns on power for the w2cbw003
> chip.
> 
> So for my view it is not really understandable why one uart subnode
> can control a regulator, and the other canât or shouldnât and why
> this regulator function canât be divided from the serial management
> into a regulator driver.
>
> > 
> > Apart from that this interface is limited in its features. I'm
> > currently working on N900's bluetooth chip. This one needs to set a
> > gpio before data is sent data to the chip, has a reset gpio and
> > a gpio to wakeup the OMAP SoC from idle states to avoid data loss
> > on the UART.
> 
> Yes, that looks equally complicated to solve if not even more than
> the w2sg chip.
> 
> But what would you prefer:
> 
> * extend the drivers/tty/slave/serial-power-manager.c to also cover your
>   special case

I would simply create a new slave driver, which takes care of the
gpios and setups the line discipline.

> * instantiate a drivers/misc/n900-bluetooth-regulator.c and hook it up
>   exactly like the vaux4 of the gta04 bluetooth chip? Just referencing
>   a different regulator node?

The gpios in the n900's case are definitely not regulators. They are
used for waking up a device, not for supplying them with energy.

> [...]

-- Sebastian

Attachment: signature.asc
Description: Digital signature