Re: [Gta04-owner] [PATCH 0/4] UART slave device support - version 4

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 28.08.2015 um 09:02 schrieb Pavel Machek :

> Hi!
> 
>> we (the developers of the hardware) have proposed an alternative
>> approach to Neil’s implementation - for the same device and solving
>> the same problem (notifying tty open/close and uart activity to the
>> slave device driver), but differently.
>> 
>> See:
>> https://lkml.org/lkml/2015/6/28/91
>> 
>> Discussion has not yet settled on which approach is better. So your
>> opinion of comparing both is welcome.
> 
> Actually, yes, discussion has settled, agreeing that phandle reference
> for the uart is a bad idea, Nikolaus just refuses to listen to anyone,

no, I only refuse to listen to you.

You are neither maintainer for any subsystem that is involved nor have
you contributed technical arguments pro/con and it appears to me that
you refuse to listen to my argumentation.

> asking "device tree maintainer opinion", and then just simply ignoring
> it when he does not like it, and then making promises he did not keep.


Which promises did I not keep? Please be specific, instead of insulting.

I bring up this alternative again, since I get the impression that most readers
are simply not aware of *both* alternative proposals.

> Please don't stall patches just because of that.

Please provide better arguments and don’t spread FUD.

Please have a look into our RFC implementation and study it carefully
to learn why it is the better (IMHO more flexible, easier to maintain, more
modular) approach. Even if you don’t like phandles.

> 
> Best regards,
>   Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 0/4] UART slave device support - version 4

[Dr. H. Nikolaus Schaller]

Hi Linus,

Am 12.08.2015 um 01:20 schrieb NeilBrown :

> On Fri, 7 Aug 2015 15:01:47 +0200 Linus Walleij
>  wrote:
> 
>> Hi Neil,
>> 
>> first, this is a *VERY* interesting and much needed patch series,
>> I intend to look closer at it, and if possible test it with some
>> (heh) board file device. Would be happy of you put me on CC for these.
>> 
>> On Mon, May 11, 2015 at 3:56 AM, NeilBrown  wrote:
>> 
>>> When a device is connected to a UART via RS-232 (or similar), there
>>> is a DTR line that can be used for power management, and other "modem
>>> control" lines.
>>> 
>>> On an embedded board, it is very likely that there is no "DTR", and
>>> any power management need to be done using some completely separate
>>> mechanism.
>>> 
>>> So these "slaves" are really just for devices permanently attached to
>>> UARTs without a full "RS-232" (or similar) connection.  The driver
>>> does all the extra control beyond Tx/Rx.
>> 
>> What is usually happening (and I have seen it in a few places) is that
>> the SoC has *one* fully featured RS232 with CTS/RTS and even
>> DTS,DCD,RI and other esoterica, which is intended to be connected to a
>> host serial port or so, for example if this SoC is to act as a modem
>> or a fax machine, or if it is to drive one.
>> 
>> Then they often have a few more UART blocks, usually identical, which
>> only have RxD+TxD available, so they are "just" UARTs.
>> 
>> To complicate things further, you may wonder what happened with
>> the CTS/RTS (etc) signals from the other blocks. Usually they are there
>> in the silicon but just routed to dead ends.
>> 
>> To complicate it even further, usually all these pins are placed under
>> pin control multiplexing, so in an actual electronic design, the
>> system will mux out CTS/RTS (etc) from the fully featured RS232
>> blocks and only use them as UARTs anyways.
>> 
>> Then there are those who created real simple RxD/TxD-only UARTs
>> ("yeah lets dump this RS232 legacy crap" / "yeah yeah")
>> and then realized they want to drive modems ("oh crap, it seemed
>> like a good idea at the time"). Then they usually take
>> two GPIO pins for CTS/RTS and drive them as GPIOs using
>> software and you have a cheap 4-line modem line. This is what
>> drivers/tty/serial/serial_mctrl_gpio.c is for if you wondered.
>> 
>>> I've tested this set and it seems to work ... except that something
>>> is sadly broken with bluetooth support in 4.1-rc1 so I've only really
>>> tested the GPS driver.  I guess it is time to rebase to -rc3.
>> 
>> You have a hardware taget I see. Which one?
> 
> GTA04 (www.gta04.org - openmoko successor).
> 
> 3 uarts on OMAP3 are wired: one as RS-232 for console, one to bluetooth
> half of a wifi/bluetooth module, and one to a GPS.
> 
> For the GPS, I just want to power on/off when the TTY is opened/closed,
> but the power-on sequence is non-trivial as both "turn on" and
> "turn-off' toggle the same line, so I need to be able to detect current
> state.
> 
> For the bluetooth, the power is a (shared) regulator.  As well as
> power-on when the TTY is opened, I'd like regulator to be turned of
> when I "hciconfig down" - even though the TTY is still open.
> I did a patch a while ago which hooked in to hci_uart_{open,close} to
> make this work, but it isn't a really good patch.
> 
> It would be nice to hide the TTY from user-space in the bluetooth case,
> and have the "hciattach" happen in the kernel, but I think hciattach
> does extra initialisation...
> 
> NeilBrown


we (the developers of the hardware) have proposed an alternative
approach to Neil’s implementation - for the same device and solving
the same problem (notifying tty open/close and uart activity to the
slave device driver), but differently.

See:
https://lkml.org/lkml/2015/6/28/91

Discussion has not yet settled on which approach is better. So your
opinion of comparing both is welcome.

BR,
Nikolaus


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 0/3] UART slave device support

[Dr. H. Nikolaus Schaller]

Hi all,
this patch series is our proposal to add hooks so that the driver for a device 
connected to an UART can
monitor modem control lines and data activity of the connected chip.

It contains an example for such a device driver which needs such sophisticated 
power control: wi2wi,w2sg0004

A remote device connected to a RS232 interface is usually power controlled by 
the DTR line.
The DTR line is managed automatically by the UART driver for open() and close() 
syscalls
and on demand by tcsetattr().

With embedded devices, the serial peripheral might be directly and always 
connected to the UART
and there might be no physical DTR line involved. Power control (on/off) has to 
be done by some
chip specific device driver (which we call "UART slave") through some 
mechanisms (I2C, GPIOs etc.)
not related to the serial interface. Some devices do not tell their power state 
except by sending
or not sending data to the UART. In such a case the device driver must be able 
to monitor data
activity. The role of the device driver is to encapsulate such power control in 
a single place.

This patch series allows to support such UART slave drivers by providing:
* a mechanism that a slave driver can identify the UART instance it is 
connected to
* a mechanism that UART slave drivers can register to be notified
* notfications for DTR (and other modem control) state changes
* notifications that the device has sent some data to the UART

A slave device tree entry simply adds a phandle reference to the UART it is 
connected to, e.g.

gps {
compatible = "wi2wi,w2sg0004";
uart = <&uart1>;
};

The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart 
driver.
devm_serial_get_uart_by_phandle() follows the concept of 
devm_usb_get_phy_by_phandle().

A slave device driver registers itself with serial_register_slave() to receive 
notifications.
Notification handlers can be registered by serial_register_mctrl_notification() 
and
serial_register_rx_notification(). If an UART has a NULL slave or a NULL 
handler registered,
no notifications are sent.

RX notification handlers can define a ktermios setup and modify or decide to 
throw away the
character that is passed upwards.

This all is a follow-up to the w2sg0004 driver submitted in 2014 that did want 
to add an optional
GPIO to DTR in omap-serial.c and required the w2sg0004 driver to present itself 
as a "virtual
GPIO". The idea of a "virtual GPIO"  is not compatible with the concept that DT 
must
describe hardware (and not virtual hardware). So in this new solution DT only 
describes that
the w2sg0004 is connected to some UART and how the power state signalling works 
is left
to the driver implementations.

The rx data notification also removes the concept of having two different 
pinmux states
and make the w2sg0004 driver intercept rx activities by switching the rx line 
to a GPIO
interrupt. This was very OMAP3 specific. The new solution is generic and might 
even be
extensible that the chip driver could filter or consume the rx data before it 
is passed
to the tty layer.

This patch works on linux-next as intended except one major weakness: we have 
to call 
uart_change_speed() each time we open the tty. This is the opposite of what we 
would like
to have: that the slave initializes the uart speed through some termios and the 
tty level just uses
this setting. We have not yet completely understood how to make this work and 
are happy
about help in this area.

And of course we would like to see general comments about the whole 
implementation
approach.


Signed-off-by: H. Nikolaus Schaller 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 06.05.2015 um 11:27 schrieb Pavel Machek :

> On Wed 2015-05-06 07:19:31, Dr. H. Nikolaus Schaller wrote:
>> Hi Peter,
>> 
>> Am 05.05.2015 um 21:54 schrieb Peter Hurley :
>> 
>>> Hi Neil,
>>> 
>>> On 03/18/2015 01:58 AM, NeilBrown wrote:
>>>> here is version 3 of support for tty-slaves.
>>> 
>>> Is there a v4 of this that I missed?
>> 
>> We did have a lengthy discussion about [PATCH 3/3] how to best (1)
>> represent the slave device in the device tree but as far as I am concerned,
>> I do not see that we have a consensus (2) and the device tree maintainers
>> have no comments or clear guidelines so far.
> 
> Yes. Everyone and their dog disagrees

What a wonderful argument…
I still ask myself who the dog is.

> with Nikolaus, who is playing
> devil's advocate

I hope you don't think that Linux users are devils…

No, I am not playing devil’s advocate (which would imply that I am doing this
for fun to tease the dog), but I feel I have to be the advocate of future board
designers who want to easily import an existing board DT and overwrite device
tree nodes to describe design changes, i.e. what slave device is connected to
which uart.

At least in this regard, the alternatives are really differently easy to handle.

And, the alternatives have some influence how a tty driver and a slave device
driver is designed. So that is for me the root question, before discussing 
(some)
implementation details.

Because it is not resolved in a way that convinces me (and future board DT
designers), I bring it up again and again. Even if you and some dog apparently
disagree.

> here, so we clearly have to get confirmation from
> device tree maintainers. And when they disagree with him, we'll need
> to get concensus from Linus, too...
>   Pavel

BR,
Nikolaus--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 27.04.2015 um 22:35 schrieb Pavel Machek :

> Hi!
> 
>>> In my opinion making something a node in / is always the
>>> last resort and in my perspective it has been handled in such a way
>>> so far.
>> 
>> But that contradicts some documents I have found and linked. Please
>> show me a document about DT that supports your view.
>> 
>> I agree that both views can be valid, but in lack of some ?official? 
>> guideline
>> we can?t decide ourselves.
> 
> This is not how kernel development works.
> 
> We can decide ourselves, and DT people will swear at us if we decide wrongly,
> and then Linus swears at them if they do too obvious mistakes.
> 
> Lets KISS.

some time ago you also wrote in this thread:

> Device tree is not operating system specific. 

In other words: this is a discussion about compatibility with the world outside 
of Linux.
And compatibility can not be achieved by decisions about Linux implementation 
details,
even if Linus is deciding. He can just decide to stay incompatible (but then it 
should
IMHO be a general decision and not case specific).

So I think we should follow some basic design principles, if they exist (which 
I don’t know),
to have this compatibility. And then adapt the Linux implementation (using the 
KISS principle).

Anyways I have not yet seen any comment from the DT people or did I miss them?

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 04.04.2015 um 10:16 schrieb Pavel Machek :

> Hi!
> 
 Please propose your own code doing that so that we can test if it is
 better.
>>> 
>>> So, how does this look?
>>> 
>>> It looks to me like you have cca 0.1 Ohm resistance in your system,
>> 
>> This is completely unknown.
>> 
>>> and are using cca 75mA while discharging, and charge by cca 1.4A.
>> 
>> Where do you get these figures from?
> 
> Least squares fitting of my coefficients to your tables.

Ah, I see.

> 
>> A GTA04 board discharges usually between 50 and 400 mA (depending on 
>> activities)
>> and if you turn on WiFi, it will be up to 600 mA. If you use 3G it can draw 
>> even more
>> during operation.
> 
> How big battery do you have?

1200 mAh

> According to least squares fitting,
> assuming maximum charge of .46A, you were taking about 25mA when
> doing the discharge test.

No, that can’t fit well. But I do not remember who has done this calibration in 
which situation
because it was done many months ago for the platform data version. We have just 
reformatted
the table for the DT.

> 
>> Total current going in is 500-800 mA (depending on USB negotiations) and 
>> this is
>> split into system operation and charging current. So 1.4A charging current 
>> is not
>> possible. Rather 200-500 mA.
>> 
>> So it might be that the battery is discharged although the system is 
>> connected
>> to a charger.
> 
> Yah, and your battery meter will be wrong in such case, as will be
> mine, because they are based on same information.

Well, it is not “mine”, it is the platform_data based driver already in 
kernel.org
since ca. 3.12.

We have just updated it to DT to get rid of platform_data in this area as well…

Yes, I see your argument that hiding the tables (two characteristic curves)
into an analytic approximation can be superior. 

The problem I see is just that your formula needs some parameters which
are difficult to derive or estimate. And must be adjusted to the specific
battery and system setup in a non-trivial way.

At least we must supply a tool (in the kernel/scripts directory?) where someone
can can estimate the parameters of the formula from the characteristic curves.

Maybe a tool that automatically runs several charge/discharge cycles at
different system load. And measures time vs. voltage. And assumes a linear
capacity decrease between the end points. (i.e. if it needs 10 hours to charge
from completely empty to full, we can assume 50% after 5,0 hours).

So my goal is to measure all characteristics of the battery - and no need to
study a (potentially non-existing) data sheet.

If you can provide that for all parameters of your algorithm I am fine with it.

> The thing is, mine
> can be improved without adding more tables. 

How can you improve your algorithm without tweaking or adding new parameters?

> 
>>> It is tricky to do a good job near 0%... or anywhere else. See for
>>> example
>>> 
>>> http://cseweb.ucsd.edu/~trosing/lectures/battery.pdf
>>> 
>>> You start a call, percentage goes down, end a call, it will go
>>> back up. I'm pretty sure you will not be able to make a call with "5%"
>>> indication from your code at low battery temperature (say -10C).
>> 
>> The whole system is heating up a little, so that you never have -10C for the
>> battery.
> 
> Umm. When not calling, your phone should not heat itself up.

Yes, in suspend it needs <20 mA which does not heat or course.
But steady operation at 20-400 mA does significantly rise OMAP
temperature beyond environment temperature.

> And you
> definitely can power it down, leave it in outer pocket, then power it
> up. It is actually something people do when they want to preserve battery...
> 
>> I think you are trying to solve situations that don’t exist in practice.
>> 
>> And AFAIK, the GTA04 board is the only user of this driver in case the 
>> battery
>> has no built-in fuel gauge. So why improve it beyond what the GTA04
>> users need?
> 
> Because then other users can share the same code, and because you

But you have ugly parameters in dts that nobody can easily see in the schematics
and that are full of assumptions.

>From a perspective of uncertainty analysis, estimation of the internal 
>parameters
needs a higher precision than the calibration points.

> avoid big ugly tables in dts.

Well, the biggest tables I have seen in dts are keyboard matrix codes…

> 
>> I repeat myself: this driver is not built for highest precision because 
>> there are
>> better methods (fuel gauge chip). These might not be available so this 
>> fall-back
>> driver has been built.
>> 
>> It is definitively better than no driver and worse than the optimum.
> 
> And my email suggested solution better than your driver, so why not
> just use it?

I am not yet convinced that it is better.

It just moves the (unavoidable) limitations (measuring multiple calibration 
points)
just to a different area (measuring the hidden and not precisely known parameter
of the system).

> 
> 
>>> #p

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi,

Am 01.04.2015 um 22:16 schrieb Pavel Machek :

> Hi!
> 
>>> As I explained in some other mail, those tables should not be
>>> neccessary at all. They can be computed from li-ion characteristics
>>> and internal resistance, and assumed current during charge and
>>> discharge.
>> 
>> I already explained that we do not know the charging and discharging
>> current well enough for such a calculation.
>> 
>> And I explained that the “internal resistance” is a system (battery + cables 
>> +
>> connectors + other circuits) parameter that is not easy to derive or measure
>> and type into the .dts source code.
>> 
>> At least I have no idea how I should find it out for my boards. While I can
>> easily determine the curves (and we already have them for the platform_data
>> driver).
>> 
>> Please propose your own code doing that so that we can test if it is
>> better.
> 
> So, how does this look?
> 
> It looks to me like you have cca 0.1 Ohm resistance in your system,

This is completely unknown.

> and are using cca 75mA while discharging, and charge by cca 1.4A.

Where do you get these figures from?

A GTA04 board discharges usually between 50 and 400 mA (depending on activities)
and if you turn on WiFi, it will be up to 600 mA. If you use 3G it can draw 
even more
during operation.

Total current going in is 500-800 mA (depending on USB negotiations) and this is
split into system operation and charging current. So 1.4A charging current is 
not
possible. Rather 200-500 mA.

So it might be that the battery is discharged although the system is connected
to a charger.

> (And
> these are all the coefficients the code should need; rest is battery
> characteristics -- common to all li-ions, and charger characteristics
> -- that will be common to all cellphones. If current can be measured,
> this code should go more precise answers).
> 
> pavel@amd:~/g/tui/ofone$ ./liion_maps
> Charging
> Voltage  4.2 V ; table  100 %  internal voltage 4.18 V current 0.233 A 
> computed  97 %
> Voltage  4.1 V ; table  75 %  internal voltage 4.08 V current 0.233 A 
> computed  87 %
> Voltage  4.0 V ; table  55 %  internal voltage 3.93 V current 0.700 A 
> computed  69 %
> Voltage  3.9 V ; table  25 %  internal voltage 3.76 V current 1.400 A 
> computed  26 %
> Voltage  3.8 V ; table  5 %  internal voltage 3.66 V current 1.400 A computed 
>  3 %
> Voltage  3.7 V ; table  2 %  internal voltage 3.56 V current 1.400 A computed 
>  2 %
> Voltage  3.6 V ; table  1 %  internal voltage 3.46 V current 1.400 A computed 
>  1 %
> Voltage  3.3 V ; table  0 %  internal voltage 3.16 V current 1.400 A computed 
>  -1 %
> Badness  395.4861761427434
> Discharging
> Voltage  4.2 V ; table  100 %  internal voltage 4.21 V current -0.075 A 
> computed  100 %
> Voltage  4.1 V ; table  95 %  internal voltage 4.11 V current -0.075 A 
> computed  91 %
> Voltage  4.0 V ; table  70 %  internal voltage 4.01 V current -0.075 A 
> computed  79 %
> Voltage  3.8 V ; table  50 %  internal voltage 3.81 V current -0.075 A 
> computed  46 %
> Voltage  3.7 V ; table  10 %  internal voltage 3.71 V current -0.075 A 
> computed  3 %
> Voltage  3.6 V ; table  5 %  internal voltage 3.61 V current -0.075 A 
> computed  2 %
> Voltage  3.3 V ; table  0 %  internal voltage 3.31 V current -0.075 A 
> computed  0 %
> Badness  171.69576218433212
> 
>>> Running below 3.3V.. not really. At that point, the battery is really
>>> _empty_, and voltage is going down really really fast.
>> 
>> It is the diffference between 2% and 0% where a fuel indication might
>> be most important…
> 
>>> Plus, you are damaging the battery at that point.
>> 
>> The power controller will shut down - but the driver should report
>> reasonable (but IMHO not necessarily perfect) values until the last
>> moment.
> 
> It is tricky to do a good job near 0%... or anywhere else. See for
> example
> 
> http://cseweb.ucsd.edu/~trosing/lectures/battery.pdf
> 
> You start a call, percentage goes down, end a call, it will go
> back up. I'm pretty sure you will not be able to make a call with "5%"
> indication from your code at low battery temperature (say -10C).

The whole system is heating up a little, so that you never have -10C for the
battery.

I think you are trying to solve situations that don’t exist in practice.

And AFAIK, the GTA04 board is the only user of this driver in case the battery
has no built-in fuel gauge. So why improve it beyond what the GTA04 users need?

I repeat myself: this driver is not built for highest precision because there 
are
better methods (fuel gauge chip). These might not be available so this fall-back
driver has been built.

It is definitively better than no driver and worse than the optimum.

> 
> Anyway, see above, I think I provide reasonable values even in that range.
> 
> Signed-off-by: Pavel Machek 
>   Pavel
> 
> #!/usr/bin/python3
> import math
> 
> def percent_internal(v):
>u = 0.0387-(1.4523*(3.7835-v

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi,

Am 01.04.2015 um 18:30 schrieb Rob Herring :

> On Tue, Mar 10, 2015 at 4:27 PM, Marek Belisko  wrote:
>> Signed-off-by: Marek Belisko 
>> ---
>> .../bindings/power_supply/twl4030_madc_battery.txt | 43 
>> ++
>> 1 file changed, 43 insertions(+)
>> create mode 100644 
>> Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt 
>> b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> new file mode 100644
>> index 000..d3dd9d8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> @@ -0,0 +1,43 @@
>> +twl4030_madc_battery
>> +
>> +Required properties:
>> + - compatible : "ti,twl4030-madc-battery"
> 
> Is this a sub-node of the twl4030 or something? Please define where
> this fits (hint: I would expect to be a sub node of a charging
> controller or battery monitor).

It is a driver connecting some battery parameters with measurements provided
by the twl4030-madc to present a /sys/class/power_supply node for the battery
with a coarse estimate for the charging level.

So maybe the best wording is that it is a battery-driver assuming a twl4030-madc
providing raw measurements (voltage, charging).

Therefore it could as well be battery-twl4030-madc.

> 
>> + - capacity-uah : battery capacity in uAh
>> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
>> +   for charging calibration (see example)
>> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
>> +   for discharging calibration (see example)
> 
> These seem like properties of the battery independent of the
> battery/charging controller which is the twl4030. Ideally we would
> define battery nodes generically and independent of the charge
> controllers. Then there are smart batteries to consider too.

For smart batteries there are completely independent mechanisms
like I2C and HDQ/1-wire communication with fuel gauge chips (e.g. b27xxx).

Those do not need such battery properties because they are stored
in EEPROMs within these chips.

So all this is a quite special solution just for those handful of board that
have no smart battery and use exactly this twl4030 chip.

It is not intended to become a generic charging/fuel solution. Just
make it work with DT (it worked with platform_data since ~3.12).

BR,
Nikolaus



> 
> Rob
> 
>> + - io-channels: Should contain IIO channel specifiers
>> +   for each element in io-channel-names.
>> +- io-channel-names: Should contain the following values:
>> + * "temp" - The ADC channel for temperature reading
>> + * "ichg" - The ADC channel for battery charging status
>> + * "vbat" - The ADC channel to measure the battery voltage
>> +
>> +Example:
>> +   madc-battery {
>> +   compatible = "ti,twl4030-madc-battery";
>> +   capacity-uah = <120>;
>> +   ti,volt-to-capacity-charging-map = <4200 100>,
>> +   <4100 75>,
>> +   <4000 55>,
>> +   <3900 25>,
>> +   <3800 5>,
>> +   <3700 2>,
>> +   <3600 1>,
>> +   <3300 0>;
>> +
>> +   ti,volt-to-capacity-discharging-map = <4200 100>
>> +  <4100 95>,
>> +  <4000 70>,
>> +  <3800 50>,
>> +  <3700 10>,
>> +  <3600 5>,
>> +  <3300 0>;
>> +   io-channels = <&twl_madc 1>,
>> + <&twl_madc 10>,
>> + <&twl_madc 12>;
>> +   io-channel-names = "temp",
>> +  "ichg",
>> +  "vbat";
>> +   };
>> --
>> 1.9.1
>> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 31.03.2015 um 09:26 schrieb Pavel Machek :

> Hi!
> 
> + io-channels = <&twl_madc 1>,
> +   <&twl_madc 10>,
> +   <&twl_madc 12>;
> + io-channel-names = "temp",
> +"ichg",
> +"vbat";
> + };
 
 Rather than just making platform_data into device tree properties..
 
 Can't you hide the these custom properties behind the compatible flag?
 
 You can initialize that data in the driver based on the compatible
 flag and the match data.
 
 This makes sense if you can group things to similar configurations.
>>> 
>>> Maybe I have not completely understood your proposal.
>>> 
>>> Do you mean to go back to have big parameter tables for each device/battery
>>> combination in the driver code and the compatible flag (e.g. compatible = 
>>> “board17”)
>>> chooses the right data set for the charging map and channels?
>> 
>> If you can somehow group them, then yes. Not for every board if there
>> are many of them naturally.
>> 
>>> I thought this is what the DT was introduced for - to have the same driver 
>>> code but adapt to different boards depending on hardware variations.
>> 
>> Yeah but you also need to consider the issues related to introducing
>> new device tree properties. The device tree properties introduced
>> should be generic where possible.
>> 
>>> And batteries have very different characteristics and vary between devices…
>> 
>> Right. Maybe that has been already agreed on to use capacity-uah for
>> batteries in general? In that case I have not problem with that as
>> it's a generic property :)
>> 
>>> The charging maps are depending on the battery type connected to the twl4030
>>> and which madc channel is which value is also a little hardware dependent
>>> (although the twl4030 doesn’t give much choice).
>> 
>> Just to consider alternatives before introducing driver specific
>> property for the maps.. Maybe here you could have few different type
>> of maps and select something safe by default? Of course it could be this
>> is higly board specific, I think some devices may be able to run below
>> 3.3V for example..
> 
> As I explained in some other mail, those tables should not be
> neccessary at all. They can be computed from li-ion characteristics
> and internal resistance, and assumed current during charge and
> discharge.

I already explained that we do not know the charging and discharging
current well enough for such a calculation.

And I explained that the “internal resistance” is a system (battery + cables +
connectors + other circuits) parameter that is not easy to derive or measure
and type into the .dts source code.

At least I have no idea how I should find it out for my boards. While I can
easily determine the curves (and we already have them for the platform_data
driver).

Please propose your own code doing that so that we can test if it is
better.

> 
> Running below 3.3V.. not really. At that point, the battery is really
> _empty_, and voltage is going down really really fast.

It is the diffference between 2% and 0% where a fuel indication might
be most important…

> 
> Plus, you are damaging the battery at that point.

The power controller will shut down - but the driver should report
reasonable (but IMHO not necessarily perfect) values until the last
moment.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Am 27.03.2015 um 17:31 schrieb Sebastian Reichel :

> Hi,
> 
> On Fri, Mar 27, 2015 at 10:22:11AM +0100, Dr. H. Nikolaus Schaller wrote:
>>> Coming back at my sentence: I asked about a non-trivial, non-bus
>>> connected HW, which has a DT binding.
>> 
>> from omap3-beagle-xm.dts:
>> hsusb2_phy (connected through ULPI)
>> gpio-keys (it is not a trivial on/off)
>> tfp410 (DVI encoder only part of the video pipeline)
> 
> These just need a couple of gpios. For me that's trivial compared to
> an interface as complex as UART. Since they do not have any complex
> interface they could be a child of, they must land in /.

ULPI-PHY is not at all “just a couple of GPIOs”. It is a 60 MHz 8 bit wide
parallel protocol with handshake… Sort of 8 UARTs or SPI with common clock.

But still no bus. The bus driven by the ULPI-PHY is “USB2” which is known
not to be a physical bus but a tree. It just becomes a bus by higher level
protocols.

I must admit that I lost the goal that you want to demonstrate with this
question/examples.

> 
>> sound/codec (connected through McBSPs)
> 
> The audio codec is a child device of the TWL. The node you are
> talking about is a ***virtual*** device.

Ah interesting that they exist! A virtual gpio was not acceptable…

> See also
> 
> http://devicetree.org/SoC_Audio
> 
> Note that the audio binding of many devices is not that "clean",
> since quite some relationships are hidden away behind kernel quirks
> and very specific compatible strings.
> 
>> and there are components on the board which are not described at
>> all but work out of the box: audio connectors, usb/ethernet hub
> 
> Sure. Everything that can be identified automatically (USB/Ethernet
> stuff) is not encoded into DT. DT has been created to provide a
> description for devices, which can *not* be identified automatically.

Exactly.

> 
> And passive components like audio connectors are normally not
> encoded into DT, since no configuration is needed and no information
> is gained. Before you argument with video connectors: Yes they are
> different, but IMHO for good reasons:
> 
> 1. The user can see "DVI" label for DVI output and "HDMI" label
>   for HDMI output. This is not known by the display controller.
> 2. The display data channel is i2c and the connector may use a
>   system i2c controller together with the data wires provided by
>   the display controller. The display subsystem must know which
>   i2c controller takes care of the port.

Yes. Indeed.

> 
>> In the meantime, I have proposed an IMHO even better approach.
>> 
>> It is not to make the gps a regulator or a subnode of some “primary 
>> interface”,
>> but give it a reference to the uart it is connected to.
>> 
>> And make it an independent node on DT root level.
>> 
>> Then we are completely independent on how the gps driver presents
>> data to any user space on any OS.
> 
> IMHO that approach is not better per se. The child variant is more
> generic it contains information without the kernel even knowing
> about uart specific stuff.
> 
> Note, that we could always use references in DT. The tree structure
> is not needed at all, since references make it possible to describe
> a graph.

Right.

> Obviously that would defeat the purpose of the tree
> structure.

Which might be there for completeness if something wants to be
better described in a tree structure. Multiple clients of a single bus 
controller
that can be selected (addressed) are obviously better described by
a parent>child relationship (which includes some sort of “address”
to describe how this addressing works).

> In my opinion making something a node in / is always the
> last resort and in my perspective it has been handled in such a way
> so far.

But that contradicts some documents I have found and linked. Please
show me a document about DT that supports your view.

I agree that both views can be valid, but in lack of some “official” guideline
we can’t decide ourselves.

> 
>> What is bad with having a net? Why must we squeeze everything into
>> a tree structure just because it is called such? DT language
>> provides the syntax for references and that allows to make nets.
>> And it is heavily used.
> 
> I think so far references were mainly used when tree structure was
> too limited to describe something.
> 
>>>> You said NAK about referencing a regulator from the UART node and
>>>> now?
>>> 
>>> No. I said NAK about referencing *the* regulator from the BT/GPS
>>> node. My NAK was about referencing a regulator in the UART node
>>> (local side), which is actually needed by the GPS/BT module (

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Hi,

Am 26.03.2015 um 19:08 schrieb Sebastian Reichel :

> Hi,
> 
> On Wed, Mar 25, 2015 at 05:44:42PM +0100, Dr. H. Nikolaus Schaller wrote:
>> Am 25.03.2015 um 16:21 schrieb Sebastian Reichel :
>>> On Wed, Mar 25, 2015 at 08:59:14AM +0100, Dr. H. Nikolaus Schaller wrote:
>>>> Am 25.03.2015 um 02:45 schrieb Sebastian Reichel :
>>>>> On Tue, Mar 24, 2015 at 06:58:15PM +0100, Dr. H. Nikolaus Schaller wrote:
>>>>>> So you propose that the parent->child relationship is “control”? I.e. 
>>>>>> some
>>>>>> channel which allows to address some bus client (through ) and
>>>>>> control that devices.
>>>>>> 
>>>>>> Makes sense. This is how i2c and spi clients are specified.
>>>>>> 
>>>>>>> In the case of our GPS, it receives control over the serial connection 
>>>>>>> from
>>>>>>> the UART,
>>>>>> 
>>>>>> Ahem - does it?
>>>>>> 
>>>>>> AFAIK the chip simply starts to emit NMEA records if powered on.
>>>>>> There is no command going over the serial interface to address it
>>>>>> or control it.
>>>>> 
>>>>> Right, since GPS basically doesn't need any configuration/control.
>>>>> That’s not true for other UART attached devices, though.
>>>> 
>>>> Do you have an example? Usually an UART data stream is transparently
>>>> presented to a /dev/tty - and user-space daemon can configure/control the
>>>> attached device. In most cases it can mix payload data and control command
>>>> by some AT command and escape sequences.
>>> 
>>> Yes, but the configuration is minimal. Anyways as you said there
>>> *is* some kind of control happening over the UART.
>> 
>> Control is happening on a higher network stack level than UART. It
>> control is done through AT commands.
> 
> Yes, obviously UART is only the physical layer. The same is true for
> busses and does not really matter.

Fine that we agree on that. I just mentioned it because we discussed about line
disciplines and tty which are higher level.

> 
>>>>>>> also receives control via a GPIO to the on/off pin, and also needs
>>>>>>> a regulator to power the antenna.
>>>>>>> 
>>>>>>> So should the parent be the uart, the on/off GPIO, or the regulator?
>>>>>>> 
>>>>>>> I would much rather there wasn't a parent, and that each of these were 
>>>>>>> listed
>>>>>>> as ad-hoc attribute assignments.  But device-tree says there must be a 
>>>>>>> parent
>>>>>>> (where possible), and the parent is the thing that is “primarily" in 
>>>>>>> control.
>>>>>> 
>>>>>> Well, IMHO the “parent” could also be the root. Representing the
>>>>>> whole board.
>>>>>> 
>>>>>> Nevertheless, I doubt your rule that “ability to control” defines
>>>>>> the parent>child relation (see below).
>>>>>> 
>>>>>>> 
>>>>>>> I think the GPS is “primarily" a uart-attached device.
>>>>>> 
>>>>>> But not in the same way as an I2C device.
>>>>>> 
>>>>>> Especially the serial interface is not a bus and not used for
>>>>>> signalling and power control. It is payload data (only).
>>>>> 
>>>>> Actually many I2C devices are also powered on/off via a GPIO and
>>>>> even use additional GPIOs for sending interrupts. Nevertheless they
>>>>> are normally identified as an I2C device.
>>>> 
>>>> Because I2C is a bus that can address multiple clients and gpio isn’t
>>>> a bus and a point-to-point connection.
>>>> 
>>>> But IMHO it is not because they (can) send payload data over i2c.
>>> 
>>> From my POV it's not because I2C is a bus, but because the primary
>>> function is happening via I2C (e.g. configuring sensor, gettings its
>>> data). The GPIOs are only needed to compensate some I2C shortcomings
>>> (missing irq feature in I2C) or reduce the complexity of the client
>>> (power GPIO). The actual system interaction with the I2C chip is
>>> going via I2C, though.
>> 
>> Yes, but this is in my new understanding irrelevant for proper

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Hi,

Am 26.03.2015 um 06:56 schrieb Pavel Machek :

> Hi!
> 
> Main reason is, that I would need to go
> through the UART to “communicate" with the w2sg0004.
 
 You can always "communicate” through the UART. Even without DT. As long as
 the connected chip is powered up by any means (could be some 
 fixed-regulator
 or hard wired).
>>> 
>>> But you don't know "how" to communicate through the uart.
>> 
>> Maybe we are talking using different assumptions. As long as you have a user 
>> space
>> gpsd daemon that talks to the gps chip the kernel simply has to 
>> transparently (except
>> line disciplines) pass the data to the uart.
> 
> Forget userspace, some other operating system (or future linux) may
> want to put gps handling into the kernel. (To hide differences between
> different GPSes).
> 
>>> Because we want the phone to boot knowing "I have a bluetooth" or "I
>>> have a GPS", as it would if it was connected using USB, and not having
>>> user figure out what commands he needs to do to enable reasonable
>>> hardware support (and getting it wrong, because you need to specify
>>> _many_ critical parameters to hciattach).
>> 
>> Yes, this is indeed something I also would like to see for the GTA04 (and 
>> other)
>> devices.
>> 
>> So the reason is that some kernel driver wants to use the tty/uart to 
>> communicate
>> directly with the chip. This is very similar to a gpio that some driver 
>> wants to use.
>> 
>> Thus please consider the
>> 
>> / {
>>  bt {
>>  compatible = "vendor,product“;
>>  uart = <&uart1>;
>>  enable = <&gpio17 34 0>;
>>  };
>> };
> 
> Would work, too, but I and everyone knows that subnode is better,
> easier solution.

“Everyone” could be wrong and ignorant.

And I thought we are not looking for the easiest solution but the right one.
Especially if we define something that is for other operating systems as well.

About easier: the one given above allows to modify the driver to present e.g. 
an iio
interface to user space (and no /dev/tty) *without changing the DT*. Because the
driver code decides which interface it presents. And not where it is a subnode 
in DT.

This is the level of abstraction DT nodes should have.


It may be that you did not read my previous argumentation completely.

In short, please see:

http://www.devicetree.org/Device_Tree_Usage#Devices

And check of there is anything that mandates useage of a subnode in this case.
I simply don’t find it.

Rather, although it is also not explicitly excluded, I read hints that it 
should *not* be
done the subnode way.

The reason is that it appears to me that the DT hierarchy is intended to 
describe
how kernel drivers can *address* different components. Nothing less and nothing 
more.

Especially there is no hint that layering in DT has anything to do with data 
flow
hierarchies or a “primary” interface as Sebastian calls it.

> 
>> approach.
>> 
>> And if you want to hide uart1 from the user-space, that should be a property
>> of the uart1 node (whereever it is defined).
> 
> Sorry? That would be one heck of layering violation.

Which layering?

I think you still mix the software/kernel driver/data flow layers with DT 
layers.

DT can and must be independent on that.

BR,
Nikolaus


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Am 25.03.2015 um 21:53 schrieb Pavel Machek :

> Hi!
> 
 AFAIK the chip simply starts to emit NMEA records if powered on.
 There is no command going over the serial interface to address it
 or control it.
>>> 
>>> Right, since GPS basically doesn't need any configuration/control.
>>> That’s not true for other UART attached devices, though.
>> 
>> Do you have an example? Usually an UART data stream is transparently
>> presented to a /dev/tty - and user-space daemon can configure/control the
>> attached device. In most cases it can mix payload data and control command
>> by some AT command and escape sequences.
> 
> Bluetooth H4P protocol is one example.
> 
>>> well parent > child is not about power control, but about "primary"
>>> control.
>> 
>> Can you clearly and precisely define what “primary” control is? I think
>> two people will have three opinions about that.
> 
> This has not be a problem so far...
> 
>> For example I would see the w2sg0004 on/off gpio as the primary “control”
>> which makes the light (NMES records) turned on.
> 
> ...and I don't think it is a problem now.
> 
>>> Main reason is, that I would need to go
>>> through the UART to “communicate" with the w2sg0004.
>> 
>> You can always "communicate” through the UART. Even without DT. As long as
>> the connected chip is powered up by any means (could be some fixed-regulator
>> or hard wired).
> 
> But you don't know "how" to communicate through the uart.

Maybe we are talking using different assumptions. As long as you have a user 
space
gpsd daemon that talks to the gps chip the kernel simply has to transparently 
(except
line disciplines) pass the data to the uart.

For that there is no need to describe anything in DT.

> 
>> Let me raise the question:
>> 
>> Why do we need to describe in the DT (independently of Linux power control
>> structures and drivers!) that the GPS data interface is connected to a 
>> specific UART?
> 
> Because we want the phone to boot knowing "I have a bluetooth" or "I
> have a GPS", as it would if it was connected using USB, and not having
> user figure out what commands he needs to do to enable reasonable
> hardware support (and getting it wrong, because you need to specify
> _many_ critical parameters to hciattach).

Yes, this is indeed something I also would like to see for the GTA04 (and other)
devices.

So the reason is that some kernel driver wants to use the tty/uart to 
communicate
directly with the chip. This is very similar to a gpio that some driver wants 
to use.

Thus please consider the

/ {
bt {
compatible = "vendor,product“;
uart = <&uart1>;
enable = <&gpio17 34 0>;
};
};

approach.

And if you want to hide uart1 from the user-space, that should be a property
of the uart1 node (whereever it is defined).

BR,
Nikolaus--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Hi,

Am 25.03.2015 um 21:42 schrieb Pavel Machek :

> Hi!
> 
>>> In the case of our GPS, it receives control over the serial connection from
>>> the UART,
>> 
>> Ahem - does it?
>> 
>> AFAIK the chip simply starts to emit NMEA records if powered on. There is no
>> command going over the serial interface to address it or control it.
> 
> Well _most_ GPSes enable you to control them over the serial
> line. (Things like sampling rate, AGPS data upload, …)

Yes, I know.

But is this something the kernel is/must be aware? And
must it be represented by the DT? Or does it have to influence
the way the DT is structured?

Well, if GPS data is presented through iio, these commands must
be handled by some driver.

> 
>>> I think the GPS is “primarily" a uart-attached device.
>> 
>> But not in the same way as an I2C device.
>> 
>> Especially the serial interface is not a bus and not used for signalling and
>> power control. It is payload data (only).
> 
> Serial interface looks a lot like a (point-to-point) bus to
> me. Similar to SATA, for example.

In that sense a gpio looks also a lot like a 1-bit bus...

Anyways, there are no subnodes (clients) of a gpio but the
<&gpiocontroller> pattern is used.

In other words: a driver that wants to control a gpio asks for a
reference of the gpio controller and uses the gpiolib to control it.

In the same way, a driver that wants to use an uart could ask
for a reference to the tty/uart driver and use some functions to
control it (and send commands). So a hypothetical gps-iio
driver could set sampling rate etc.

And it can also ask the tty/uart: „please register me and notify if
some syscall wants you to open() or close() so that I (the driver of
the GPS chip) can turn on power of the GPS chip (and only I know
how to do that - nobody else has to care)“.

I am just playing the Devil's advocate to find out what the really
common principle is and how the DT for clients of a serial interface
should really be represented in a DT in a way that is agnostic to
a specific driver structure, driver functions and user space.

And to find out if we need tty/slaves (as proposed in the title of this
patch series) at all. Or if there is a better way to describe the relation
of the gps chip driver and the tty/uart.

BR,
Nikolaus--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Hi,

Am 25.03.2015 um 16:21 schrieb Sebastian Reichel :

> Hi,
> 
> On Wed, Mar 25, 2015 at 08:59:14AM +0100, Dr. H. Nikolaus Schaller wrote:
>> Am 25.03.2015 um 02:45 schrieb Sebastian Reichel :
>>> On Tue, Mar 24, 2015 at 06:58:15PM +0100, Dr. H. Nikolaus Schaller wrote:
>>>> So you propose that the parent->child relationship is “control”? I.e. some
>>>> channel which allows to address some bus client (through ) and
>>>> control that devices.
>>>> 
>>>> Makes sense. This is how i2c and spi clients are specified.
>>>> 
>>>>> 
>>>>> In the case of our GPS, it receives control over the serial connection 
>>>>> from
>>>>> the UART,
>>>> 
>>>> Ahem - does it?
>>>> 
>>>> AFAIK the chip simply starts to emit NMEA records if powered on.
>>>> There is no command going over the serial interface to address it
>>>> or control it.
>>> 
>>> Right, since GPS basically doesn't need any configuration/control.
>>> That’s not true for other UART attached devices, though.
>> 
>> Do you have an example? Usually an UART data stream is transparently
>> presented to a /dev/tty - and user-space daemon can configure/control the
>> attached device. In most cases it can mix payload data and control command
>> by some AT command and escape sequences.
> 
> Yes, but the configuration is minimal. Anyways as you said there
> *is* some kind of control happening over the UART.

Control is happening on a higher network stack level than UART. It
control is done through AT commands.

> 
>>>>> also receives control via a GPIO to the on/off pin, and also needs
>>>>> a regulator to power the antenna.
>>>>> 
>>>>> So should the parent be the uart, the on/off GPIO, or the regulator?
>>>>> 
>>>>> I would much rather there wasn't a parent, and that each of these were 
>>>>> listed
>>>>> as ad-hoc attribute assignments.  But device-tree says there must be a 
>>>>> parent
>>>>> (where possible), and the parent is the thing that is “primarily" in 
>>>>> control.
>>>> 
>>>> Well, IMHO the “parent” could also be the root. Representing the
>>>> whole board.
>>>> 
>>>> Nevertheless, I doubt your rule that “ability to control” defines
>>>> the parent>child relation (see below).
>>>> 
>>>>> 
>>>>> I think the GPS is “primarily" a uart-attached device.
>>>> 
>>>> But not in the same way as an I2C device.
>>>> 
>>>> Especially the serial interface is not a bus and not used for
>>>> signalling and power control. It is payload data (only).
>>> 
>>> Actually many I2C devices are also powered on/off via a GPIO and
>>> even use additional GPIOs for sending interrupts. Nevertheless they
>>> are normally identified as an I2C device.
>> 
>> Because I2C is a bus that can address multiple clients and gpio isn’t
>> a bus and a point-to-point connection.
>> 
>> But IMHO it is not because they (can) send payload data over i2c.
> 
> From my POV it's not because I2C is a bus, but because the primary
> function is happening via I2C (e.g. configuring sensor, gettings its
> data). The GPIOs are only needed to compensate some I2C shortcomings
> (missing irq feature in I2C) or reduce the complexity of the client
> (power GPIO). The actual system interaction with the I2C chip is
> going via I2C, though.

Yes, but this is in my new understanding irrelevant for proper DT description.

> 
>>> Also for non-GPS device the serial connection is used for
>>> controlling and configuring the device.
>> 
>> This assumes that “controls a device” is the criterion for making a device
>> a subnode. I doubt that.
> 
> For me the criterion always was "accessing the device's
> registers/configuration/data" from the system's point of view (so
> your video port does not count, since it models a connection between
> two components without system interaction).

> 
>>>>> So I propose a device-node which describes the GPS, which is a child of 
>>>>> the
>>>>> UART, and explicitly identifies the GPIO it uses to power on/off, the
>>>>> regulator it uses to power the antenna, and how it receives "on or off"
>>>>> status indications from the GPS.
>>>> 
>>>> The more I think abo

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Hi,

Am 25.03.2015 um 02:45 schrieb Sebastian Reichel :

> Hi,
> 
> On Tue, Mar 24, 2015 at 06:58:15PM +0100, Dr. H. Nikolaus Schaller wrote:
>> So you propose that the parent->child relationship is “control”? I.e. some
>> channel which allows to address some bus client (through ) and
>> control that devices.
>> 
>> Makes sense. This is how i2c and spi clients are specified.
>> 
>>> 
>>> In the case of our GPS, it receives control over the serial connection from
>>> the UART,
>> 
>> Ahem - does it?
>> 
>> AFAIK the chip simply starts to emit NMEA records if powered on.
>> There is no command going over the serial interface to address it
>> or control it.
> 
> Right, since GPS basically doesn't need any configuration/control.
> That’s not true for other UART attached devices, though.

Do you have an example? Usually an UART data stream is transparently
presented to a /dev/tty - and user-space daemon can configure/control the
attached device. In most cases it can mix payload data and control command
by some AT command and escape sequences.

> 
>>> also receives control via a GPIO to the on/off pin, and also needs
>>> a regulator to power the antenna.
>>> 
>>> So should the parent be the uart, the on/off GPIO, or the regulator?
>>> 
>>> I would much rather there wasn't a parent, and that each of these were 
>>> listed
>>> as ad-hoc attribute assignments.  But device-tree says there must be a 
>>> parent
>>> (where possible), and the parent is the thing that is “primarily" in 
>>> control.
>> 
>> Well, IMHO the “parent” could also be the root. Representing the
>> whole board.
>> 
>> Nevertheless, I doubt your rule that “ability to control” defines
>> the parent>child relation (see below).
>> 
>>> 
>>> I think the GPS is “primarily" a uart-attached device.
>> 
>> But not in the same way as an I2C device.
>> 
>> Especially the serial interface is not a bus and not used for
>> signalling and power control. It is payload data (only).
> 
> Actually many I2C devices are also powered on/off via a GPIO and
> even use additional GPIOs for sending interrupts. Nevertheless they
> are normally identified as an I2C device.

Because I2C is a bus that can address multiple clients and gpio isn’t
a bus and a point-to-point connection.

But IMHO it is not because they (can) send payload data over i2c.

> 
> Also for non-GPS device the serial connection is used for
> controlling and configuring the device.

This assumes that “controls a device” is the criterion for making a device
a subnode. I doubt that.

> 
>>> So I propose a device-node which describes the GPS, which is a child of the
>>> UART, and explicitly identifies the GPIO it uses to power on/off, the
>>> regulator it uses to power the antenna, and how it receives "on or off"
>>> status indications from the GPS.
>> 
>> The more I think about that, you have given good arguments *not*
>> to use the parent->child relationship for the UART interface of
>> the GPS.
>> 
>> Let me give another example. The 3G Modem has 3 (or 4) interfaces:
>> 1. an USB-Interface for AT signalling and payload
>> 2. some GPIOs for power control.
>> 3. a PCM interface for digital voice. 
>> 4. it might also have a serial interface as alternate AT command and (GPRS
>>low speed) payload.
>> 
>> So which one is the most prominent or most important to make it a child of?
>> 
>> If you use “control” you must make it a child of the USB phy and the serial 
>> interface
>> which requires multiple inheritance…
>> 
>> So I am not sure at all. None is IMHO prominent and unique enough to make it
>> a parent>child relation.
>> 
>> Threrefore, I would be happy to see it as multiple children of /. For 
>> example a
>> MFD with subnodes.
> 
> This scenario has already been seen before and can happen for
> non-UART based devices (e.g. SPI + I2C). So far the decision was to
> postpone the discussion about this kind of devices until one of them
> appears.
> 
>>> It is arguable that the "antenna" should be treated as a separate device - a
>>> child of the GPS - which controls the regulator and also provides a 'extcon'
>>> which reports whether an external GPS antenna is attached, or whether the
>>> internal on is in use.  I haven't made the time to properly explore that
>>> issue yet.
>> 
>> If you assume that parent>child relationship is about control (only). B

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Hi,

Am 21.03.2015 um 00:31 schrieb NeilBrown :

> On Fri, 20 Mar 2015 10:34:18 +0100 "Dr. H. Nikolaus Schaller"
>  wrote:
> 
>> 
>> Am 20.03.2015 um 09:54 schrieb NeilBrown :
> 
>>> There needs to be one device-node for each device, and that device-node 
>>> needs
>>> to be a child of the device-node for the device which is the primary
>>> connection to the child device.
>> 
>> Then please explain to me nodes like
>> 
>> / {
>>  compatible = "ti,omap3-gta04", "ti,omap36xx", "ti,omap3";
>> 
>>  cpus {
>>  cpu@0 {
>>  cpu0-supply = <&vcc>;
>>  };
>>  };
>> 
>> According to the rule you apply here it should be something like
>> 
>>  cpu@0 {
>>  regulator {
>>  …
>>  }
>> 
>> 
> 
> This exactly highlight one of the big problems with device tree as I see it.
> 
> Each device can potentially have relationships with a number of other
> devices, for the supply of power, reset signalling, interrupt handled, data
> transfer, command transfer etc etc etc.

Yes. The network is a mesh.

> 
> devicetree provides two ways to indicated a relationship between devices.
> One way is a parent/child arrangement.  The other way is ad-hoc
>   attribute = <&devicename>
> assignments.

Yes.

> 
> Each device can only have one parent, but can have multiple arbitrary
> assignments.
> 
> I would much rather that the parent/child relationship didn't exist at all.
> Each device should stand alone, and list all relationships explicitly.  But
> that is not the way devicetree works, and we need to live with that.

Is it not how the device tree works or how we use it? Or how you think it
should better be?

What stops us from using it in arbitrary assignments like clocks and
regulators?

GPIOs are also a nice example: you just specify the gpio controller
and the gpio number to use it. There is no parent/child connection.

Why for tty / serial and serial consumers but not for GPIOs?

I have tried to find out the rules when parent>child is used (see below).

> 
> So we need a clear understanding of what the 'parent' of a given device
> should be.
> I don't know what the specifications say, if anything, but what I see is that
> the parent is in practive a device which can ‘address' the child.  

I think this holds only for “bus” devices - where it is really a good and
standard way of structuring the bus and it’s masters/slaves. One child
node per client.

> i.e.
> control signalling is the key parent->child relationship.
> This is consistent with the fact that many device nodes have a 
>   reg=
> attribute which gives the address of the node as seen by it's parent.
> 
> Given that understanding, a regulator must be a child of the device which can
> control it - which can turn it on or off.  Not a child of the device which
> receives power from it.

So you propose that the parent->child relationship is “control”? I.e. some
channel which allows to address some bus client (through ) and
control that devices.

Makes sense. This is how i2c and spi clients are specified.

> 
> In the case of our GPS, it receives control over the serial connection from
> the UART,

Ahem - does it?

AFAIK the chip simply starts to emit NMEA records if powered on. There is no
command going over the serial interface to address it or control it.

> also receives control via a GPIO to the on/off pin, and also needs
> a regulator to power the antenna.
> 
> So should the parent be the uart, the on/off GPIO, or the regulator?
> 
> I would much rather there wasn't a parent, and that each of these were listed
> as ad-hoc attribute assignments.  But device-tree says there must be a parent
> (where possible), and the parent is the thing that is “primarily" in control.

Well, IMHO the “parent” could also be the root. Representing the whole board.

Nevertheless, I doubt your rule that “ability to control” defines the 
parent>child
relation (see below).

> 
> I think the GPS is “primarily" a uart-attached device.

But not in the same way as an I2C device.

Especially the serial interface is not a bus and not used for signalling and
power control. It is payload data (only).

> So I propose a device-node which describes the GPS, which is a child of the
> UART, and explicitly identifies the GPIO it uses to power on/off, the
> regulator it uses to power the antenna, and how it receives "on or off"
> status indications from the GPS.

The more I think about that, you have given good arguments *not* to use the
parent->chi

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

[Dr. H. Nikolaus Schaller]

Hi,

Am 20.03.2015 um 14:08 schrieb Sebastian Reichel :

> Hi,
> 
> 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… Linux drivers can follow that or mix
everything into a single driver if that is better.

> 
>> 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).

> 
> 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.

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

So a GPS chip needing some line discipline could be

> / -> omap3 -> serial -> w2sg0004 -> line-discipline
> / -> omap3 -> serial -> w2sg0004 -> regulator

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

> 
> 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.

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

* 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?

And as said I don’t mind if the regulator is a subnode of the omap3 serial.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Am 20.03.2015 um 09:54 schrieb NeilBrown :

> On Fri, 20 Mar 2015 08:54:38 +0100 "Dr. H. Nikolaus Schaller"
>  wrote:
> 
>> 
>> Am 18.03.2015 um 06:58 schrieb NeilBrown :
>> 
>>> If a platform has a particular device permanently attached to a UART,
>>> there may be out-of-band signaling necessary to power the device
>>> on and off.
>>> 
>>> This driver controls that signalling for a number of different devices.
>>> It can
>>> - enable/disable a regulator
>>> - toggle a GPIO
>>> - register an 'rfkill' which can force the device to be off.
>>> 
>>> When the rfkill is absent or unblocked, the device will be on when the
>>> associated tty device is open, and closed otherwise.
>>> 
>>> Signed-off-by: NeilBrown 
>>> ---
>>> .../bindings/tty_slave/wi2wi,w2cbw003.txt  |   19 +
>>> .../bindings/tty_slave/wi2wi,w2sg0004.txt  |   37 +
>>> .../devicetree/bindings/vendor-prefixes.txt|1 
>>> drivers/tty/slave/Kconfig  |   14 +
>>> drivers/tty/slave/Makefile |2 
>>> drivers/tty/slave/serial-power-manager.c   |  510 
>>> 
>>> 6 files changed, 583 insertions(+)
>>> create mode 100644 
>>> Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt
>>> create mode 100644 
>>> Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt
>>> create mode 100644 drivers/tty/slave/serial-power-manager.c
>>> 
>>> diff --git a/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt 
>>> b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt
>>> new file mode 100644
>>> index ..cfe6ee5e01e9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt
>>> @@ -0,0 +1,19 @@
>>> +wi2wi bluetooth module
>>> +
>>> +This is accessed via a serial port and is largely controlled via that
>>> +link.  Extra configuration is needed to enable power on/off
>>> +
>>> +Required properties:
>>> +- compatible: "wi2wi,w2cbw003"
>>> +- vdd-supply: regulator used to power the device.
>>> +
>>> +The node for this device must be the child of a UART.
>>> +
>>> +Example:
>>> +
>>> +&uart1 {
>>> +   bluetooth {
>>> +   compatible = "wi2wi,w2cbw003";
>>> +   vdd-supply = <&vaux4>;
>>> +   };
>>> +};
>> 
>> Wouldn’t it be easier to simply write
>> 
>> &uart1 {
>>  vdd-suppy = <&vaux4>;
>> }
> 
> Easier to write: certainly.
> Easier to justify? No.

I just justified.

> Easier to get merged upstream?  Definitely not.

Are you the maintainer?

> After all, the uart itself doesn't require a power supply.
> It is the device connected to the uart which requires the power supply.

Yes.

> 
> 
>> 
>>> diff --git a/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt 
>>> b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt
>>> new file mode 100644
>>> index ..fdc52cf56533
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt
>>> @@ -0,0 +1,37 @@
>>> +wi2wi GPS device
>>> +
>>> +This is accessed via a serial port and is largely controlled via that
>>> +link.  Extra configuration is needed to enable power on/off
>>> +
>>> +Required properties:
>>> +- compatible: "wi2wi,w2sg0004"
>>> +- gpios: gpios used to toggle 'on/off' pin
>>> +- interrupts: interrupt generated by RX pin when device
>>> +  should be off
>>> +
>>> +Optional properties:
>>> +- vdd-supply: regulator used to power antenna
>>> +- pinctrl: "default", "off"
>>> +  if "off" setting is provided it is imposed when device should
>>> +  be off.  This can route the RX pin to a GPIO interrupt.
>>> +
>>> +The w2sg0004 uses a pin-toggle both to power-on and to
>>> +power-off, so the driver needs to detect what state it is in.
>>> +It does this by detecting characters on the RX line.
>>> +When it should be off, these can optionally be detected by a GPIO.
>>> +
>>> +The node for this device must be the child of a UART.
>>> +
>>> +Example:

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

[Dr. H. Nikolaus Schaller]

Hi,

Am 20.03.2015 um 09:43 schrieb NeilBrown :

> On Fri, 20 Mar 2015 08:54:35 +0100 "Dr. H. Nikolaus Schaller"
>  wrote:
> 
>> Hi Neil,
>> 
>> Am 18.03.2015 um 06:58 schrieb NeilBrown :
>> 
>>> Hi again,
>>> here is version 3 of support for tty-slaves.
>>> 
>>> This version introduces a new bus-type for tty-slaves,
>> 
>> Hm. I am still not convinced that a tty is a „bus“ and 
>> that this is a solution for a wide-spread problem.
> 
> Don’t read too much into the word "bus".  

Then we should find a different word.

> It is really just a mechanism for
> grouping drivers together - with maybe a hint of a suggestions that it helps
> communicate with the devices which the drivers drive.
> 
> And I'm not particularly interested in solving wide-spread problems, just in
> solving my problem in a suitably idiomatic way.
> 
> 
>> 
>>> and causes
>>> a tty-slave device to appear in /sys/devices between the uart and the
>>> tty.
>>> It effectively intercepts and calls from the tty to the uart (i.e. any
>>> tty_operations) and applies extra functionality at that point.
>> 
>>> 
>>> Currently the only driver intercepts open and close.
>>> It powers on the device on open, and powers off at last-close.
>> 
>> That is what the missing piece in Linux is to make the w2sg0004
>> chip work.
>> 
>>> 
>>> Power can be controlled by a regulator or by toggling a GPIO.
>> 
>> I think such a GPIO logic has nothing to do with serial and
>> should be left over to the regulator logic, i.e. we need a special
>> regulator-w2sg0004 driver.
>> 
>> So I suggest to remove the GPIO logic from your 
>> 
>> drivers/tty/slave/serial-power-manager.c
>> 
>> And then you can even get rid of adding a chip specific „compatible“
>> entry for the subnodes.
> 
> But (nearly) every node has a "compatible" entry.
> Each node describes a device, and the "compatible" string tells us what sort
> of device.

Yes, it should - if necessary.

> Surely you agree that there is a particular device that needs to be
> controlled?

No, my opinion is that a specific regulator should be controlled.

>  In that case there needs to be a node representing it and a
> "compatible" string describing it.  That is how “devicetree" works.

For this there is the vdd-regulator = <®ulator> idiom and that is
how “devicetree” works as well.

> 
> 
>> 
>>> 
>>> I think I've incorporated most of the feed back I received from
>>> previous versions, but if I missed something - I apologize.  If
>>> this approach is structurally acceptable then I can fix up all the
>>> smaller issues.
>> 
>> As said I would prefer that the w2sg0004 driver is just a separate
>> „regulator“ driver as we had proposed before.
> 
> You can prefer that if you wish.  But given that the w2sg0004 is not in fact
> a regulator,

therefore our proposal is to make it a drivers/misc and not a drivers/regulator.
But it has a regulator inside that can obviously be turned on or off. So I want 
to
keep close to the logical energy flow.

> but is in fact a GPS device, I doubt you will find a lot of
> support for your approach (as indeed I didn’t when I tried it...)

We got a lot of support. The main opposition was about using a “virtual”
gpio controller instead of a regulator API to turn the chip on and off as
directed by the tty driver.

And we do not write a driver for the w2sg0004 but the regulator inside that
chip. 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.

The w2sg0004-regulator approach just follows this principle. Anyone willing
to control the power of the w2sg0004 can use this regulator interface.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dr. H. Nikolaus Schaller]

Hi Neil,

Am 18.03.2015 um 06:58 schrieb NeilBrown :

> Hi again,
> here is version 3 of support for tty-slaves.
> 
> This version introduces a new bus-type for tty-slaves,

Hm. I am still not convinced that a tty is a „bus“ and 
that this is a solution for a wide-spread problem.

> and causes
> a tty-slave device to appear in /sys/devices between the uart and the
> tty.
> It effectively intercepts and calls from the tty to the uart (i.e. any
> tty_operations) and applies extra functionality at that point.

> 
> Currently the only driver intercepts open and close.
> It powers on the device on open, and powers off at last-close.

That is what the missing piece in Linux is to make the w2sg0004
chip work.

> 
> Power can be controlled by a regulator or by toggling a GPIO.

I think such a GPIO logic has nothing to do with serial and
should be left over to the regulator logic, i.e. we need a special
regulator-w2sg0004 driver.

So I suggest to remove the GPIO logic from your 

drivers/tty/slave/serial-power-manager.c

And then you can even get rid of adding a chip specific „compatible“
entry for the subnodes.

> 
> I think I've incorporated most of the feed back I received from
> previous versions, but if I missed something - I apologize.  If
> this approach is structurally acceptable then I can fix up all the
> smaller issues.

As said I would prefer that the w2sg0004 driver is just a separate
„regulator“ driver as we had proposed before.

Nikolaus



> 
> Thanks for your review,
> NeilBrown
> 
> 
> ---
> 
> NeilBrown (3):
>  TTY: use class_find_device to find port in uart_suspend/resume.
>  TTY: add support for tty_slave devices.
>  tty/slaves: add a driver to power on/off UART attached devices.
> 
> 
> .../bindings/tty_slave/wi2wi,w2cbw003.txt  |   19 +
> .../bindings/tty_slave/wi2wi,w2sg0004.txt  |   37 +
> .../devicetree/bindings/vendor-prefixes.txt|1 
> drivers/tty/Kconfig|1 
> drivers/tty/Makefile   |1 
> drivers/tty/serial/serial_core.c   |   21 -
> drivers/tty/slave/Kconfig  |   21 +
> drivers/tty/slave/Makefile |4 
> drivers/tty/slave/serial-power-manager.c   |  510 
> drivers/tty/slave/tty_slave_core.c |  136 +
> drivers/tty/tty_io.c   |   60 ++
> include/linux/tty.h|2 
> include/linux/tty_slave.h  |   26 +
> 13 files changed, 813 insertions(+), 26 deletions(-)
> create mode 100644 
> Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt
> create mode 100644 
> Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt
> create mode 100644 drivers/tty/slave/Kconfig
> create mode 100644 drivers/tty/slave/Makefile
> create mode 100644 drivers/tty/slave/serial-power-manager.c
> create mode 100644 drivers/tty/slave/tty_slave_core.c
> create mode 100644 include/linux/tty_slave.h
> 
> --
> Signature
> 
> ___
> Gta04-owner mailing list
> gta04-ow...@goldelico.com
> http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 3/3] tty/slaves: add a driver to power on/off UART attached devices.

[Dr. H. Nikolaus Schaller]

Am 18.03.2015 um 06:58 schrieb NeilBrown :

> If a platform has a particular device permanently attached to a UART,
> there may be out-of-band signaling necessary to power the device
> on and off.
> 
> This driver controls that signalling for a number of different devices.
> It can
> - enable/disable a regulator
> - toggle a GPIO
> - register an 'rfkill' which can force the device to be off.
> 
> When the rfkill is absent or unblocked, the device will be on when the
> associated tty device is open, and closed otherwise.
> 
> Signed-off-by: NeilBrown 
> ---
> .../bindings/tty_slave/wi2wi,w2cbw003.txt  |   19 +
> .../bindings/tty_slave/wi2wi,w2sg0004.txt  |   37 +
> .../devicetree/bindings/vendor-prefixes.txt|1 
> drivers/tty/slave/Kconfig  |   14 +
> drivers/tty/slave/Makefile |2 
> drivers/tty/slave/serial-power-manager.c   |  510 
> 6 files changed, 583 insertions(+)
> create mode 100644 
> Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt
> create mode 100644 
> Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt
> create mode 100644 drivers/tty/slave/serial-power-manager.c
> 
> diff --git a/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt 
> b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt
> new file mode 100644
> index ..cfe6ee5e01e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2cbw003.txt
> @@ -0,0 +1,19 @@
> +wi2wi bluetooth module
> +
> +This is accessed via a serial port and is largely controlled via that
> +link.  Extra configuration is needed to enable power on/off
> +
> +Required properties:
> +- compatible: "wi2wi,w2cbw003"
> +- vdd-supply: regulator used to power the device.
> +
> +The node for this device must be the child of a UART.
> +
> +Example:
> +
> +&uart1 {
> +   bluetooth {
> +   compatible = "wi2wi,w2cbw003";
> +   vdd-supply = <&vaux4>;
> +   };
> +};

Wouldn’t it be easier to simply write

&uart1 {
vdd-suppy = <&vaux4>;
}

> diff --git a/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt 
> b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt
> new file mode 100644
> index ..fdc52cf56533
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty_slave/wi2wi,w2sg0004.txt
> @@ -0,0 +1,37 @@
> +wi2wi GPS device
> +
> +This is accessed via a serial port and is largely controlled via that
> +link.  Extra configuration is needed to enable power on/off
> +
> +Required properties:
> +- compatible: "wi2wi,w2sg0004"
> +- gpios: gpios used to toggle 'on/off' pin
> +- interrupts: interrupt generated by RX pin when device
> +  should be off
> +
> +Optional properties:
> +- vdd-supply: regulator used to power antenna
> +- pinctrl: "default", "off"
> +  if "off" setting is provided it is imposed when device should
> +  be off.  This can route the RX pin to a GPIO interrupt.
> +
> +The w2sg0004 uses a pin-toggle both to power-on and to
> +power-off, so the driver needs to detect what state it is in.
> +It does this by detecting characters on the RX line.
> +When it should be off, these can optionally be detected by a GPIO.
> +
> +The node for this device must be the child of a UART.
> +
> +Example:
> +&uart2 {
> +   gps {
> +   compatible = "wi2iw,w2sg0004";
> +   vdd-supply = <&vsim>;
> +   gpios = <&gpio5 17 0>; /* GPIO_145 */
> +   interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
> +   /* When off, switch RX to be an interrupt */
> +   pinctrl-names = "default", "off";
> +   pinctrl-0 = <&uart2_pins>;
> +   pinctrl-1 = <&uart2_pins_rx_gpio>;
> +   };
> +};

If the wi2wi driver is a regulator driver one would write

/ {
   gps-regulator: gps {
   compatible = "wi2iw,w2sg0004";
   vdd-supply = <&vsim>;
   gpios = <&gpio5 17 0>; /* GPIO_145 */
   interrupts-extended = <&gpio5 19 0>; /* GPIO_147 */
   /* When off, switch RX to be an interrupt */
   pinctrl-names = "default", "off";
   pinctrl-0 = <&uart2_pins>;
   pinctrl-1 = <&uart2_pins_rx_gpio>;
   };
}

&uart2 {
vdd-suppy = <&gps-regulator>;
};

Which IMHO better describes that the uart controls power of a separate driver.

And this pattern for writing a DT would IMHO be more flexible because you
can „connect“ to any regulator, e.g. a regulator for a RS232 level shifter.


> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 389ca1347a77..81d259303710 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -189,6 +189,7 @@ variscite Variscite Ltd.
> via   VIA Technologies, Inc.

Re: [PATCH v3 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 17.03.2015 um 14:59 schrieb Pavel Machek :

> Hi!
> 
>>> to introduce coefficients for temperature and discharge rate?
>> What do you mean? Nothing like that is used in current driver why do
>> we need to add it?
> 
> Well, conversion between Li-ion's voltage and state of charge at 0
> current is well known:
 
 We can’t measure at 0 current since the OMAP is driven from battery
 and charger and may also draw some mA…
>>> 
>>> Yes, but you know how many mA you are taking just now. So if you knew
>>> the internal resistance, you could compute the voltage at 0
>>> current. (And it should also work during charging, as long as you know
>>> how much current is going in.)
>> 
>> As far as I understand the twl4030 charger and MADC it is not possible to
>> separate these values. It is only reporting the inflow from charger to
>> battery + system. So you don’t know how many mA are supplying the system
>> and how many mA are left over for charging.
>> 
>> You can only assume how much the system is drawing while running (something
>> between 50 and 600 mA but this depends on system activities, power state
>> of peripherald and e.g. backlight being switched on).
>> 
>> I think your basic assumption that we know any time how many mA the system
>> is taking is not given.
> 
> So.. you won't be able to get exact value while charging, but you
> get one while discharging, which is what really matters…?

Is it the same during charging?

And, as we already discussed it depends on the situation.

In addition, GSM power consumption runs in bursts and I don’t know if the sample
rate of the MADC is good enough to track that correctly.

> 
>>> Yes, and that coefficient should be internal battery resistance ;-).
>> 
>> But where do you know this value from to write it into a DT file?
>> Usually you can’t measure it easily and for some batteries you don’t have
>> a data sheet.
>> 
>> Contrary, the calibration curves can easily be measured on the device
>> (assuming that the charge level decreases/increases linearly over time
>> between Full and Empty).
> 
> If you can copy it from the data sheet, that's the easiest option. If
> not, you should be able to easily compute it from the charge/discharge
> curves or from measured voltage at different loads.

Well, this again assumes that you can generate/control different loads
easily (e.g. turn on/off this and that peripheral) to check the voltage
drop. Sounds good in theory, but I don’t want to do it in practice to
derive this parameter.

And, don’t forget that the MADC signals are nosiy and don’t have the
best precision. So it is likely that you get wrong values.

As said, I think it is not worth the effort around the imperfect twl4030/MADC
charging system.

If someone wants precise data, that is what fuel gauge chips are good for.
And we already have drivers for them.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 17.03.2015 um 11:37 schrieb Pavel Machek :

> Hi!
> 
> to introduce coefficients for temperature and discharge rate?
 What do you mean? Nothing like that is used in current driver why do
 we need to add it?
>>> 
>>> Well, conversion between Li-ion's voltage and state of charge at 0
>>> current is well known:
>> 
>> We can’t measure at 0 current since the OMAP is driven from battery
>> and charger and may also draw some mA…
> 
> Yes, but you know how many mA you are taking just now. So if you knew
> the internal resistance, you could compute the voltage at 0
> current. (And it should also work during charging, as long as you know
> how much current is going in.)

As far as I understand the twl4030 charger and MADC it is not possible to
separate these values. It is only reporting the inflow from charger to
battery + system. So you don’t know how many mA are supplying the system
and how many mA are left over for charging.

You can only assume how much the system is drawing while running (something
between 50 and 600 mA but this depends on system activities, power state
of peripherald and e.g. backlight being switched on).

I think your basic assumption that we know any time how many mA the system
is taking is not given.

> 
>>>   def percent(m, v):
>>> u = 0.0387-(1.4523*(3.7835-v))
>>>   if u < 0:
>>>   return 0
>>> return (0.1966+math.sqrt(u))*100
>>> 
>>> And there's not much to calibrate there, and it should become library
>>> helper function; there's no need to write it to every single dts.
>>> 
>>> The charge/discharge difference is due to internal battery resistance,
>>> and Ohm's law. (Then, you should not need different values for
>>> charging/discharging case.)
>> 
>> Yes, and that also depends on the board and battery type. So you always
>> need to specify some battery specific coefficient for that in the DT.
> 
> Yes, and that coefficient should be internal battery resistance ;-).

But where do you know this value from to write it into a DT file?
Usually you can’t measure it easily and for some batteries you don’t have
a data sheet.

Contrary, the calibration curves can easily be measured on the device
(assuming that the charge level decreases/increases linearly over time
between Full and Empty).

I tend to make software easy to use for the user (developer of a board support
package) of a driver, not theoretically correct or mathematically elegant.

> 
>> We simply did choose a table, because calculating the right coefficients
>> is more complex and getting the table values can easily be done from
>> running one full charge-discharge-charge cycle.
> 
> Well.. One set of coefficients would be kind of ok. But you are doing
> two, because it really depends on current, not charge/discharge. So
> why not do it right, for all currents…?

Because the right solution for all these issues is to use a fuel gauge chip
(bq27xxx).

This driver is just for systems where the hardware designer did forget
(or did not want to spend money) for such a chip, but user space
expects some /sys/class/power_supply indication (e.g. Android/Replicant).

The missing optimal precision is something we can live with.

> 
>>> With your aproach, you'll get lower percentage when phone is under
>>> load vs. idle. Taking internal resistance into account would give you
>>> more precise readings. (Attached, old patch for zaurus shows the
>>> needed computation).
>> 
>> This is why we have a charging and a discharging table to compensate
>> for this effect.
> 
> Yes, but there's very different current during "idle" phone and during
> call (for example). So yes, you are compensating for different current
> during charge and discharge, but it is possible to do better.

I am not convinced that it can be really done better, considering the
limitations of the twl4030 circuits, and I doubt that it is worth the efforts
of doing it better.

> 
>>> I guess this should go into library somewhere, because many machines
>>> need similar code.
>> 
>> Is a library easier to maintain and handle than just a set of table
>> values?
> 
> I think so, yes, because otherwise you need a set of tables for each
> machine.

Yes, but what is the problem? We have different device trees for each
machine anyways. And as soon as they differ in battery characteristics
the coefficients for your calculation have to be defined for each machine.

So someone has to write in some DT values (difficult to understand
and derive coefficients or two simple mapping tables).

To me it looks as if you want to make it a 100% solution while I am happy
with the 80% solution, which already exists as a platform data driver and
just want to get it back into DT based boards.

So I would suggest that Mareks patches to make the platform data
driver DT compatible are applied first, and you are invited to submit a
technically better solution for testing on real hardware.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscrib

Re: [PATCH v3 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Am 17.03.2015 um 09:47 schrieb Pavel Machek :

> Hi!
> 
 diff --git 
 a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt 
 b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
 new file mode 100644
 index 000..bb3580c
 --- /dev/null
 +++ 
 b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
 @@ -0,0 +1,43 @@
 +twl4030_madc_battery
 +
 +Required properties:
 + - compatible : "ti,twl4030-madc-battery"
 + - capacity-uah : battery capacity in uAh
>>> 
>>> Could we make it capacity-uAh ?
>> This name was suggested by Mark Rutland [1] and naming convention
>> should be all lowercase. There exists already bindings
>> without uppercase (e.g. ti,bb-uamp) so I would keep it consistent.
> 
> Messing up SI units due to "convention" is _stupid_. Don't do it.
> 
>>> to introduce coefficients for temperature and discharge rate?
>> What do you mean? Nothing like that is used in current driver why do
>> we need to add it?
> 
> Well, conversion between Li-ion's voltage and state of charge at 0
> current is well known:
> 
>def percent(m, v):
>   u = 0.0387-(1.4523*(3.7835-v))
>if u < 0:
>return 0
>  return (0.1966+math.sqrt(u))*100

I forgot to ask: does the kernel have a sqrt() function?
> 
> And there's not much to calibrate there, and it should become library
> helper function; there's no need to write it to every single dts.
> 
> The charge/discharge difference is due to internal battery resistance,
> and Ohm's law. (Then, you should not need different values for
> charging/discharging case.)
> 
> With your aproach, you'll get lower percentage when phone is under
> load vs. idle. Taking internal resistance into account would give you
> more precise readings. (Attached, old patch for zaurus shows the
> needed computation).
> 
> And if you wanted even more precise readings... internal resistance
> depends on temperature.
> 
> I guess this should go into library somewhere, because many machines
> need similar code.
> 
>   Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Am 17.03.2015 um 09:48 schrieb Pavel Machek :

>> 
>> Temperature calibration should have already been done in the underlaying 
>> twl4030 iio driver.
>> 
>> Discharge rate is the real current flow reported in uA. Also
>> reported by iio.
> 
> Ack, but there's rather severe temperature dependency of the lithium
> battery, which you should take into account if you want to compute
> “percentage charged".

We just want to estimate it as good as possible. 5-10% is sufficient
and better than no value at all (which is what you get without this
driver).

And, we just convert the (existing) driver from non-DT to DT and to use
iio, so we do not want to change the inner logic what it does and how it
works.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi,

Am 17.03.2015 um 09:47 schrieb Pavel Machek :

> Hi!
> 
 diff --git 
 a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt 
 b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
 new file mode 100644
 index 000..bb3580c
 --- /dev/null
 +++ 
 b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
 @@ -0,0 +1,43 @@
 +twl4030_madc_battery
 +
 +Required properties:
 + - compatible : "ti,twl4030-madc-battery"
 + - capacity-uah : battery capacity in uAh
>>> 
>>> Could we make it capacity-uAh ?
>> This name was suggested by Mark Rutland [1] and naming convention
>> should be all lowercase. There exists already bindings
>> without uppercase (e.g. ti,bb-uamp) so I would keep it consistent.
> 
> Messing up SI units due to "convention" is _stupid_. Don't do it.
> 
>>> to introduce coefficients for temperature and discharge rate?
>> What do you mean? Nothing like that is used in current driver why do
>> we need to add it?
> 
> Well, conversion between Li-ion's voltage and state of charge at 0
> current is well known:

We can’t measure at 0 current since the OMAP is driven from battery
and charger and may also draw some mA…

> 
>def percent(m, v):
>   u = 0.0387-(1.4523*(3.7835-v))
>if u < 0:
>return 0
>  return (0.1966+math.sqrt(u))*100
> 
> And there's not much to calibrate there, and it should become library
> helper function; there's no need to write it to every single dts.
> 
> The charge/discharge difference is due to internal battery resistance,
> and Ohm's law. (Then, you should not need different values for
> charging/discharging case.)

Yes, and that also depends on the board and battery type. So you always
need to specify some battery specific coefficient for that in the DT.

We simply did choose a table, because calculating the right coefficients
is more complex and getting the table values can easily be done from
running one full charge-discharge-charge cycle.

> 
> With your aproach, you'll get lower percentage when phone is under
> load vs. idle. Taking internal resistance into account would give you
> more precise readings. (Attached, old patch for zaurus shows the
> needed computation).

This is why we have a charging and a discharging table to compensate
for this effect.

> 
> And if you wanted even more precise readings... internal resistance
> depends on temperature.

We don’t necessarily know the real battery temperature.

> 
> I guess this should go into library somewhere, because many machines
> need similar code.

Is a library easier to maintain and handle than just a set of table values?

Anyways it ends up in a representation of a mapping function with two
input parameters (voltage and charge/discharge indication).

My proposal: keep it simple for everybody. And I can’t imagine something
easier than a mapping table.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Am 16.03.2015 um 22:20 schrieb Belisko Marek :

> On Mon, Mar 16, 2015 at 10:05 PM, Pavel Machek  wrote:
>> On Wed 2015-02-04 23:14:32, Marek Belisko wrote:
>>> Signed-off-by: Marek Belisko 
>>> ---
>>> .../bindings/power_supply/twl4030_madc_battery.txt | 43 
>>> ++
>>> 1 file changed, 43 insertions(+)
>>> create mode 100644 
>>> Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>> 
>>> diff --git 
>>> a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt 
>>> b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>> new file mode 100644
>>> index 000..bb3580c
>>> --- /dev/null
>>> +++ 
>>> b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>> @@ -0,0 +1,43 @@
>>> +twl4030_madc_battery
>>> +
>>> +Required properties:
>>> + - compatible : "ti,twl4030-madc-battery"
>>> + - capacity-uah : battery capacity in uAh
>> 
>> Could we make it capacity-uAh ?
> This name was suggested by Mark Rutland [1] and naming convention
> should be all lowercase. There exists already bindings
> without uppercase (e.g. ti,bb-uamp) so I would keep it consistent.
>> 
>>> + - volt-to-capacity-charging-map : list of voltage(mV):level(%) values
>>> + for charging calibration (see example)
>>> + - volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
>>> + for discharging calibration (see example)
>> 
>> Would "mV-to-capacity..." be more accurate? Also, would it make sense
> Again maybe mv but it can be added also later.
>> to introduce coefficients for temperature and discharge rate?
> What do you mean? Nothing like that is used in current driver why do
> we need to add it?

Temperature calibration should have already been done in the underlaying 
twl4030 iio driver.

Discharge rate is the real current flow reported in uA. Also reported by iio.

>> 
>> Thanks,
>>Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) 
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 
> [1] - http://lists.openwall.net/linux-kernel/2014/10/09/104
> 
> BR,
> 
> marek
> 

BR,
Nikolaus


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Am 11.03.2015 um 17:44 schrieb Tony Lindgren :

> * Dr. H. Nikolaus Schaller  [150311 09:17]:
>> Hi,
>> 
>> Am 11.03.2015 um 16:24 schrieb Tony Lindgren :
>> 
>>> Hi,
>>> 
>>> * Marek Belisko  [150310 14:28]:
>>>> Signed-off-by: Marek Belisko 
>>>> ---
>>>> .../bindings/power_supply/twl4030_madc_battery.txt | 43 
>>>> ++
>>>> 1 file changed, 43 insertions(+)
>>>> create mode 100644 
>>>> Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>>> 
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt 
>>>> b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>>> new file mode 100644
>>>> index 000..d3dd9d8
>>>> --- /dev/null
>>>> +++ 
>>>> b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>>> @@ -0,0 +1,43 @@
>>>> +twl4030_madc_battery
>>>> +
>>>> +Required properties:
>>>> + - compatible : "ti,twl4030-madc-battery"
>>>> + - capacity-uah : battery capacity in uAh
>>>> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
>>>> +  for charging calibration (see example)
>>>> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) 
>>>> values
>>>> +  for discharging calibration (see example)
>>>> + - io-channels: Should contain IIO channel specifiers
>>>> +  for each element in io-channel-names.
>>>> +- io-channel-names: Should contain the following values:
>>>> + * "temp" - The ADC channel for temperature reading
>>>> + * "ichg" - The ADC channel for battery charging status
>>>> + * "vbat" - The ADC channel to measure the battery voltage
>>>> +
>>>> +Example:
>>>> +  madc-battery {
>>>> +  compatible = "ti,twl4030-madc-battery";
>>>> +  capacity-uah = <120>;
>>>> +  ti,volt-to-capacity-charging-map = <4200 100>,
>>>> +  <4100 75>,
>>>> +  <4000 55>,
>>>> +  <3900 25>,
>>>> +  <3800 5>,
>>>> +  <3700 2>,
>>>> +  <3600 1>,
>>>> +  <3300 0>;
>>>> +
>>>> +  ti,volt-to-capacity-discharging-map = <4200 100>
>>>> + <4100 95>,
>>>> + <4000 70>,
>>>> + <3800 50>,
>>>> + <3700 10>,
>>>> + <3600 5>,
>>>> + <3300 0>;
>>>> +  io-channels = <&twl_madc 1>,
>>>> +<&twl_madc 10>,
>>>> +<&twl_madc 12>;
>>>> +  io-channel-names = "temp",
>>>> + "ichg",
>>>> + "vbat";
>>>> +  };
>>> 
>>> Rather than just making platform_data into device tree properties..
>>> 
>>> Can't you hide the these custom properties behind the compatible flag?
>>> 
>>> You can initialize that data in the driver based on the compatible
>>> flag and the match data.
>>> 
>>> This makes sense if you can group things to similar configurations.
>> 
>> Maybe I have not completely understood your proposal.
>> 
>> Do you mean to go back to have big parameter tables for each device/battery
>> combination in the driver code and the compatible flag (e.g. compatible = 
>> “board17”)
>> chooses the right data set for the charging map and channels?
> 
> If you can somehow group them, then yes.

I don’t see how to group them. Could you make a proposal?

> Not for every board if there
> are many of them naturally.
> 
>> I thought this is what the DT was introduced for - to have the same driver 
>

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi,

Am 11.03.2015 um 16:24 schrieb Tony Lindgren :

> Hi,
> 
> * Marek Belisko  [150310 14:28]:
>> Signed-off-by: Marek Belisko 
>> ---
>> .../bindings/power_supply/twl4030_madc_battery.txt | 43 
>> ++
>> 1 file changed, 43 insertions(+)
>> create mode 100644 
>> Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt 
>> b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> new file mode 100644
>> index 000..d3dd9d8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> @@ -0,0 +1,43 @@
>> +twl4030_madc_battery
>> +
>> +Required properties:
>> + - compatible : "ti,twl4030-madc-battery"
>> + - capacity-uah : battery capacity in uAh
>> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
>> +for charging calibration (see example)
>> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
>> +for discharging calibration (see example)
>> + - io-channels: Should contain IIO channel specifiers
>> +for each element in io-channel-names.
>> +- io-channel-names: Should contain the following values:
>> + * "temp" - The ADC channel for temperature reading
>> + * "ichg" - The ADC channel for battery charging status
>> + * "vbat" - The ADC channel to measure the battery voltage
>> +
>> +Example:
>> +madc-battery {
>> +compatible = "ti,twl4030-madc-battery";
>> +capacity-uah = <120>;
>> +ti,volt-to-capacity-charging-map = <4200 100>,
>> +<4100 75>,
>> +<4000 55>,
>> +<3900 25>,
>> +<3800 5>,
>> +<3700 2>,
>> +<3600 1>,
>> +<3300 0>;
>> +
>> +ti,volt-to-capacity-discharging-map = <4200 100>
>> +   <4100 95>,
>> +   <4000 70>,
>> +   <3800 50>,
>> +   <3700 10>,
>> +   <3600 5>,
>> +   <3300 0>;
>> +io-channels = <&twl_madc 1>,
>> +  <&twl_madc 10>,
>> +  <&twl_madc 12>;
>> +io-channel-names = "temp",
>> +   "ichg",
>> +   "vbat";
>> +};
> 
> Rather than just making platform_data into device tree properties..
> 
> Can't you hide the these custom properties behind the compatible flag?
> 
> You can initialize that data in the driver based on the compatible
> flag and the match data.
> 
> This makes sense if you can group things to similar configurations.

Maybe I have not completely understood your proposal.

Do you mean to go back to have big parameter tables for each device/battery
combination in the driver code and the compatible flag (e.g. compatible = 
“board17”)
chooses the right data set for the charging map and channels?

I thought this is what the DT was introduced for - to have the same driver 
code but adapt to different boards depending on hardware variations.

And batteries have very different characteristics and vary between devices…

The charging maps are depending on the battery type connected to the twl4030
and which madc channel is which value is also a little hardware dependent
(although the twl4030 doesn’t give much choice).

And moving this information into the driver for each board that uses it
would blow up the code.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] ARM: dts: omap3-pandora: add common device tree

[Dr. H. Nikolaus Schaller]

Am 12.02.2015 um 18:47 schrieb Grazvydas Ignotas :

> On Thu, Feb 12, 2015 at 3:03 PM, Marek Belisko  wrote:
>> From: "H. Nikolaus Schaller" 
>> 
>> This device tree allows to boot, supports the panel,
>> framebuffer, touch screen, as well as some more peripherals.
>> Since there is a OMAP3530 based 600 MHz variant and a DM3730 based
>> 1 GHz variant we must include this common device tree code
>> in one of two CPU specific device trees.
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> Signed-off-by: Marek Belisko 
>> ---
>> arch/arm/boot/dts/omap3-pandora-common.dtsi | 641 
>> 
>> 1 file changed, 641 insertions(+)
>> create mode 100644 arch/arm/boot/dts/omap3-pandora-common.dtsi
>> 
>> diff --git a/arch/arm/boot/dts/omap3-pandora-common.dtsi 
>> b/arch/arm/boot/dts/omap3-pandora-common.dtsi
>> new file mode 100644
>> index 000..0a2a878
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/omap3-pandora-common.dtsi
>> @@ -0,0 +1,641 @@
> 
> ...
> 
>> +
>> +   gpio-leds {
>> +
>> +   compatible = "gpio-leds";
>> +
>> +   pinctrl-names = "default";
>> +   pinctrl-0 = <&led_pins>;
>> +
>> +   led@1 {
>> +   label = "pandora::sd1";
>> +   gpios = <&gpio5 0 GPIO_ACTIVE_HIGH>;/* GPIO_128 
>> */
>> +   linux,default-trigger = "mmc0";
>> +   default-state = "off";
>> +   };
>> +
>> +   led@2 {
>> +   label = "pandora::sd2";
>> +   gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;/* GPIO_129 
>> */
>> +   linux,default-trigger = "mmc1";
>> +   default-state = "off";
>> +   };
>> +
>> +   led@3 {
>> +   label = "pandora::bluetooth";
>> +   gpios = <&gpio5 30 GPIO_ACTIVE_HIGH>;   /* GPIO_158 
>> */
>> +   linux,default-trigger = "heartbeat";
> 
> I’d prefer this had no trigger, but no strong feelings here.

Ok. Well, this is more or less our testing setup - so that we did see something 
before
we could fix the display setup. I think practice will show what is better, and 
then
it can be patched. We are at the beginning of Pandora DT work…

> 
>> +   default-state = "off";
>> +   };
>> +
>> +   led@4 {
>> +   label = "pandora::wifi";
>> +   gpios = <&gpio5 31 GPIO_ACTIVE_HIGH>;   /* GPIO_159 
>> */
>> +   linux,default-trigger = "mmc2";
>> +   default-state = "off";
>> +   };
>> +   };
>> +
>> +   gpio-keys {
>> +   compatible = "gpio-keys";
>> +
>> +   pinctrl-names = "default";
>> +   pinctrl-0 = <&button_pins>;
>> +
>> +   up-button {
>> +   label = "up";
>> +   linux,code = ;
>> +   gpios = <&gpio4 14 GPIO_ACTIVE_HIGH>;   /* GPIO_110 
>> */
>> +   gpio-key,wakeup;
>> +   };
>> +
>> +   down-button {
>> +   label = "down";
>> +   linux,code = ;
>> +   gpios = <&gpio4 7 GPIO_ACTIVE_HIGH>;/* GPIO_103 
>> */
>> +   gpio-key,wakeup;
>> +   };
>> +
>> +   left-button {
>> +   label = "left";
>> +   linux,code = ;
>> +   gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;/* GPIO_96 */
>> +   gpio-key,wakeup;
>> +   };
>> +
>> +   right-button {
>> +   label = "right";
>> +   linux,code = ;
>> +   gpios = <&gpio4 2 GPIO_ACTIVE_HIGH>;/* GPIO_98 */
>> +   gpio-key,wakeup;
>> +   };
>> +
>> +   pageup-button {
>> +   label = "game 1";
>> +   linux,code = ;
>> +   gpios = <&gpio4 13 GPIO_ACTIVE_HIGH>;   /* GPIO_109 
>> */
>> +   gpio-key,wakeup;
>> +   };
>> +
>> +   pagedown-button {
>> +   label = "game 3";
>> +   linux,code = ;
>> +   gpios = <&gpio4 10 GPIO_ACTIVE_HIGH>;   /* GPIO_106 
>> */
>> +   gpio-key,wakeup;
>> +   };
>> +
>> +   home-button {
>> +   label = "game 4";
>> +   linux,code = ;
>> +   gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>;/* GPIO_101 
>> */
>> +   gpio-key,wakeup;
>> +   };
>> +
>> +   end-button {
>> +   label = "game 2";
>> +   linux,code = ;
>> +   gpios = <&gpio4 15 GPIO_ACTIVE_HIGH>;   /* GPIO_111 
>> */
>> +

Re: [PATCH 1/4] ARM: dts: omap3-pandora: add common device tree

[Dr. H. Nikolaus Schaller]

Am 12.02.2015 um 17:03 schrieb Tony Lindgren :

> Hi,
> 
> Few comments below.
> 
> * Marek Belisko  [150212 05:07]:
>> +
>> +&omap3_pmx_core {
>> +
>> +mmc1_pins: pinmux_mmc1_pins {
>> +pinctrl-single,pins = <
>> +OMAP3_CORE1_IOPAD(0x2144, PIN_INPUT_PULLUP | MUX_MODE0) 
>> /* sdmmc1_clk.sdmmc1_clk */
>> +OMAP3_CORE1_IOPAD(0x2146, PIN_INPUT_PULLUP | MUX_MODE0) 
>> /* sdmmc1_cmd.sdmmc1_cmd */
>> +OMAP3_CORE1_IOPAD(0x2148, PIN_INPUT_PULLUP | MUX_MODE0) 
>> /* sdmmc1_dat0.sdmmc1_dat0 */
>> +OMAP3_CORE1_IOPAD(0x214a, PIN_INPUT_PULLUP | MUX_MODE0) 
>> /* sdmmc1_dat1.sdmmc1_dat1 */
>> +OMAP3_CORE1_IOPAD(0x214c, PIN_INPUT_PULLUP | MUX_MODE0) 
>> /* sdmmc1_dat2.sdmmc1_dat2 */
>> +OMAP3_CORE1_IOPAD(0x214e, PIN_INPUT_PULLUP | MUX_MODE0) 
>> /* sdmmc1_dat3.sdmmc1_dat3 */
>> +>;
> ...
> 
>> +&omap3_pmx_core2 {
>> +/* define in CPU specific file that includes this one
>> + * use either OMAP3430_CORE2_IOPAD() or OMAP3630_CORE2_IOPAD()
>> + */
>> +};
> 
> OK looks like you have some SoC specific pins too.. I assume you
> guys have checked that all the pins defined in this file are at
> the same offset between 34xx and 36xx variants?

Yes. All in this file are common between 34xx and 36xx.
The 600mhz / 1ghz files cover the differences for pmx_core2 through the 
different macros
(OMAP3430_CORE2_IOPAD vs. OMAP3460_CORE2_IOPAD).

> 
>> +&i2c1 {
>> +clock-frequency = <260>;
>> +
>> +twl: twl@48 {
>> +reg = <0x48>;
>> +interrupts = <7>; /* SYS_NIRQ cascaded to intc */
>> +interrupt-parent = <&intc>;
>> +
>> +twl_power: power {
>> +compatible = "ti,twl4030-power-reset";
>> +ti,use_poweroff;
>> +};
>> +
>> +twl_audio: audio {
>> +compatible = "ti,twl4030-audio";
>> +
>> +codec {
>> +ti,ramp_delay_value = <3>;
>> +};
>> +};
>> +};
>> +};
> 
> Can be done later naturally, but ere you probably want
> ti,twl4030-power-idle or ti,twl4030-power-idle-osc-off
> if the osicllator can be shut down during off-idle.

Good hint. We have to check and test if it can be shut down.
But as said, this is just the beginning of DT support.

> 
>> +&gpmc {
>> +ranges = <0 0 0x3000 0x04>; /* CS0: NAND */
> 
> The ranges here allocate the GPMC partition, so it needs to be
> a minimum of 16MB:
> 
>   ranges = <0 0 0x3000 0x100>,/* CS0: 16MB for NAND */
> 
>> +nand@0,0 {
>> +reg = <0 0 0>; /* CS0, offset 0 */
> 
> The reg is for the device driver to ioremap it's registers,
> for NAND it's just 4:
> 
>   reg = <0 0 4>;  /* CS0, offset 0, IO size 4 */

Ok, that was a typo and we have not tested how compatible it is to the
board file scheme.

> 
>> +filesystem@68 {
>> +label = "rootfs";
>> +reg = <0xc8 0>; /* 0 = MTDPART_SIZ_FULL */
>> +};
>> +};
>> +};
> 
> Is the NAND the same size on all of them?

AFAIK not necessarily.

> I don't think dts
> has a binding for MTDPART_SIZ_FULL type thing..

It does not need a special binding, we just set the size to 0. This
is interpreted as MTDPART_SIZ_FULL later on. For board files,
there is just a #define in include/linux/mtd/partitions.h and it appears
to work.

> 
>> +lcd: lcd@1 {
>> +reg = <1>;  /* CS1 */
>> +compatible ="omapdss,tpo,td043mtea1";
>> +spi-max-frequency = <10>;
>> +spi-cpol;
>> +spi-cpha;
>> +
>> +label = "lcd";
>> +reset-gpios = <&gpio5 29 GPIO_ACTIVE_LOW>;  /* GPIO_157 */
>> +vcc-supply = <&vaux1>;
>> +
>> +port {
>> +lcd_in: endpoint {
>> +remote-endpoint = <&dpi_out>;
>> +};
>> +};
>> +};
> 
> Oh there's already a binding for the LCD too? That’s great :)

Yes, we were happy to find that the panel driver has already been upgraded
for DT use.

> 
> Nine job, good to see this happening!
> 
> Regards,
> 
> 
> Tony

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] input: tsc2007: Add pre-calibration, flipping and rotation

[Dr. H. Nikolaus Schaller]

Am 15.01.2015 um 19:16 schrieb Dmitry Torokhov :

> On Thu, Jan 15, 2015 at 05:14:38PM +0100, Dr. H. Nikolaus Schaller wrote:
>> 
>> Am 15.01.2015 um 15:38 schrieb Sebastian Reichel :
>> 
>>> Hi,
>>> 
>>> On Thu, Jan 15, 2015 at 08:36:44AM +0100, Dr. H. Nikolaus Schaller wrote:
>>>>> 1. Perform conversion in input core rather than individual drivers. I
>>>>> think we should allocate a new bitmaps for some transformations and have
>>>>> the code do X/Y flip/clip of the coordinates.
>>>> 
>>>> Do you have a suggestion where this should be (I have no clue how
>>>> the input system works or is structured - we just know how to extend a
>>>> driver that uses it)?
>>>> 
>>>>> 2. Standardize on bindings. We already have of-touchscreen.c doing
>>>>> rudimentary parsing, we shoudl look into extending it rather than
>>>>> creating myriad of driver-specific bindings.
>>>> 
>>>> Ok, looks reasonable.
>>> 
>>> Documentation is in 
>>> 
>>> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> 
>> I did look into it now. Unfortunately, it does not fit well into my view of 
>> how bindings
>> should be. They should describe hardware (as we are told for many other 
>> kernel
>> subsystems).
>> 
>> Pixels and resolutions are IMHO related to the screen it is glued on - and 
>> that is
>> quite independent.
> 
> Well, I think pixels was the wrong word to be used there. It is meant to
> be native units, as opposed to millimeters, inches, points, etc.

ok.

> 
>> 
>> So I don’t see how they do describe the different ways the touch screen can 
>> be
>> wired to a tsc2007 controller.
>> 
>> Please can you add minimum and maximum properties for us?
>> 
>> Then, inverted-x and inverted-y is redundant because it is the same as having
>> an expected higher value from the ADC for the minimum coordinate and a lower
>> for the maximum.
> 
> I'd rather not add minimum and maximum, but add the touchscreen-start-x and
> touchscreen-start-y instead so that we limit the number of obsolete
> properties.

ok, that should not be too difficult to add.

So we will modify our driver to use the new functions and align 
omap3-gta04.dtsi accordingly.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] input: tsc2007: Add pre-calibration, flipping and rotation

[Dr. H. Nikolaus Schaller]

Am 15.01.2015 um 15:38 schrieb Sebastian Reichel :

> Hi,
> 
> On Thu, Jan 15, 2015 at 08:36:44AM +0100, Dr. H. Nikolaus Schaller wrote:
>>> 1. Perform conversion in input core rather than individual drivers. I
>>> think we should allocate a new bitmaps for some transformations and have
>>> the code do X/Y flip/clip of the coordinates.
>> 
>> Do you have a suggestion where this should be (I have no clue how
>> the input system works or is structured - we just know how to extend a
>> driver that uses it)?
>> 
>>> 2. Standardize on bindings. We already have of-touchscreen.c doing
>>> rudimentary parsing, we shoudl look into extending it rather than
>>> creating myriad of driver-specific bindings.
>> 
>> Ok, looks reasonable.
> 
> Documentation is in 
> 
> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

I did look into it now. Unfortunately, it does not fit well into my view of how 
bindings
should be. They should describe hardware (as we are told for many other kernel
subsystems).

Pixels and resolutions are IMHO related to the screen it is glued on - and that 
is
quite independent.

So I don’t see how they do describe the different ways the touch screen can be
wired to a tsc2007 controller.

Please can you add minimum and maximum properties for us?

Then, inverted-x and inverted-y is redundant because it is the same as having
an expected higher value from the ADC for the minimum coordinate and a lower
for the maximum.

> 
>>> Also, do we need swap and flip or do we simply need rotation (like the
>>> proposed Broadcom iproc driver has)?
>> 
>> Well, since the DT should describe hardware, there are 3 sets of wires which
>> can have a cross-over: X+ and X-, Y+ and Y-, X and Y.
>> 
>> So IMHO hardware has no “rotation”, just crossover of wires. Rotation is an
>> interpretation of the result of these connections in combination with some
>> panel the touch is glued to and should therefore not be represented in the 
>> DT.
>> 
>> As a result we have proposed a scheme without explicit rotation. We specify 
>> what
>> coordinates X- and X+ should report at their ends (min, max) because the DT
>> programmer has to specify them anyways. Flipping is a result of defining 
>> these
>> coordinates in an ascending or descending way. Only swapping of the X and Y
>> wires can’t be implicitly defined so it has its own property. So the scheme 
>> we
>> have proposed tries to optimize the efforts needed to adapt new boards and 
>> write
>> DTs and focus the DT on hardware description.
>> 
>> As a bonus we also specify the min and max value to be reported for the touch
>> pressure (Z axis) using the same basic principle.
>> 
>> And it is a pure add-on on top of the existing driver so that it attempts not
>> to break existing device trees.
> 
> from what I can see there are no in-tree-users using any of the
> new properties.
> 
>> Maybe could you accept it as a first step for this specific driver (and 
>> let’s do
>> the big standardization work later on)?
> 
> That does not work, since you create an ABI.
> 
> -- Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] input: tsc2007: Add pre-calibration, flipping and rotation

[Dr. H. Nikolaus Schaller]

Hi,

Am 15.01.2015 um 15:38 schrieb Sebastian Reichel :

> Hi,
> 
> On Thu, Jan 15, 2015 at 08:36:44AM +0100, Dr. H. Nikolaus Schaller wrote:
>>> 1. Perform conversion in input core rather than individual drivers. I
>>> think we should allocate a new bitmaps for some transformations and have
>>> the code do X/Y flip/clip of the coordinates.
>> 
>> Do you have a suggestion where this should be (I have no clue how
>> the input system works or is structured - we just know how to extend a
>> driver that uses it)?
>> 
>>> 2. Standardize on bindings. We already have of-touchscreen.c doing
>>> rudimentary parsing, we shoudl look into extending it rather than
>>> creating myriad of driver-specific bindings.
>> 
>> Ok, looks reasonable.
> 
> Documentation is in 
> 
> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> 
>>> Also, do we need swap and flip or do we simply need rotation (like the
>>> proposed Broadcom iproc driver has)?
>> 
>> Well, since the DT should describe hardware, there are 3 sets of wires which
>> can have a cross-over: X+ and X-, Y+ and Y-, X and Y.
>> 
>> So IMHO hardware has no “rotation”, just crossover of wires. Rotation is an
>> interpretation of the result of these connections in combination with some
>> panel the touch is glued to and should therefore not be represented in the 
>> DT.
>> 
>> As a result we have proposed a scheme without explicit rotation. We specify 
>> what
>> coordinates X- and X+ should report at their ends (min, max) because the DT
>> programmer has to specify them anyways. Flipping is a result of defining 
>> these
>> coordinates in an ascending or descending way. Only swapping of the X and Y
>> wires can’t be implicitly defined so it has its own property. So the scheme 
>> we
>> have proposed tries to optimize the efforts needed to adapt new boards and 
>> write
>> DTs and focus the DT on hardware description.
>> 
>> As a bonus we also specify the min and max value to be reported for the touch
>> pressure (Z axis) using the same basic principle.
>> 
>> And it is a pure add-on on top of the existing driver so that it attempts not
>> to break existing device trees.
> 
> from what I can see there are no in-tree-users using any of the
> new properties.

Not yet. But our [patch 2/3] of this series defines the DT entry for the GTA04 
devices:

https://lkml.org/lkml/2014/9/30/663

> 
>> Maybe could you accept it as a first step for this specific driver (and 
>> let’s do
>> the big standardization work later on)?
> 
> That does not work, since you create an ABI.

Hm. I don’t understand what you mean with creating an ABI?




--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] input: tsc2007: Add pre-calibration, flipping and rotation

[Dr. H. Nikolaus Schaller]

Hi,

Am 15.01.2015 um 01:59 schrieb Dmitry Torokhov :

> On Sat, Jan 10, 2015 at 03:15:39PM +0100, Belisko Marek wrote:
>> Ping for input maintainer. DT changes was acked. Thanks.
>> 
>> On Tue, Sep 30, 2014 at 10:17 PM, Marek Belisko  wrote:
>>> This patch adds new parameters that allow to address typical hardware
>>> design differences: touch screens may be wired or oriented differently
>>> (portrait or landscape). And usually the active area of the touch is a
>>> little larger than the active area of the LCD. This results in the touch
>>> coordinates that have some significant deviation from LCD coordinates.
>>> Usually this is addressed in user space by a calibration tool (e.g. tslib
>>> or xinput-calibrator) but some systems don't have these tools or require
>>> that the screen is already roughly calibrated (e.g. Replicant) to operate
>>> the device until a better calibration can be done. And, some systems
>>> react very strangely if the touch event stream reports coordinates
>>> outside of the active area.
>>> 
>>> This makes it necessry to be able to configure:
>>> 1. swapping x and y wires (coordinate values)
>>> 2. flipping of x (left - right) or y (top - bottom) or even both
>>> 3. define an active area so that an uncalibrated screen already
>>> roughly matches the LCD to be useful.
>>> 4. clip reported coordinates to the active area.
>>> 
>>> If none of the new parameters is defined (in DT) or set in a board file,
>>> the driver behaves the same as without this patch.
>>> 
>>> Author (1&2): H. Nikolaus Schaller 
>>> Author (3&4): Paul Kocialkowski 
>>> 
>>> Signed-off-by: H. Nikolaus Schaller 
> 
> OK, I was hesitant of adding these as normally we have tslib to perform
> the conversion, but maybe it is time to allow it in the kernel and
> standardize users.

Well, tslib isn’t a good replacement for this problem any more and
pre-initializing tslib makes some deep hardware dependency visible
in user space (each board needs a different tslib config and pointercal
default - and on some user spaces tslib is broken with Xorg).

But the issue to be solved is more hardware related, i.e. if the Y- and Y+
pins of the controller are connected in a swapped way to the resistor
ends of the panel.

Hence in a DT based system, this must IMHO be described by the DT
and can not be left to some user-space functions any more.

> However, this seems like a general issue and we
> should:
> 
> 1. Perform conversion in input core rather than individual drivers. I
> think we should allocate a new bitmaps for some transformations and have
> the code do X/Y flip/clip of the coordinates.

Do you have a suggestion where this should be (I have no clue how
the input system works or is structured - we just know how to extend a
driver that uses it)?

> 
> 2. Standardize on bindings. We already have of-touchscreen.c doing
> rudimentary parsing, we shoudl look into extending it rather than
> creating myriad of driver-specific bindings.

Ok, looks reasonable.

> 
> Also, do we need swap and flip or do we simply need rotation (like the
> proposed Broadcom iproc driver has)?

Well, since the DT should describe hardware, there are 3 sets of wires which
can have a cross-over: X+ and X-, Y+ and Y-, X and Y.

So IMHO hardware has no “rotation”, just crossover of wires. Rotation is an
interpretation of the result of these connections in combination with some
panel the touch is glued to and should therefore not be represented in the DT.

As a result we have proposed a scheme without explicit rotation. We specify what
coordinates X- and X+ should report at their ends (min, max) because the DT
programmer has to specify them anyways. Flipping is a result of defining these
coordinates in an ascending or descending way. Only swapping of the X and Y
wires can’t be implicitly defined so it has its own property. So the scheme we
have proposed tries to optimize the efforts needed to adapt new boards and write
DTs and focus the DT on hardware description.

As a bonus we also specify the min and max value to be reported for the touch
pressure (Z axis) using the same basic principle.

And it is a pure add-on on top of the existing driver so that it attempts not
to break existing device trees.

Maybe could you accept it as a first step for this specific driver (and let’s do
the big standardization work later on)?

— hns


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Gta04-owner] [PATCH 1/2] mmc: core: allow a reset gpio to be configured.

[Dr. H. Nikolaus Schaller]

Hi Neil,

Am 02.12.2014 um 02:55 schrieb NeilBrown :

> On Fri, 28 Nov 2014 12:56:33 +0100 Ulf Hansson  wrote:
> 
>> On 8 November 2014 at 01:14, NeilBrown  wrote:
>>> If the regulator supplying an SDIO device is shared
>>> with another device, the turning the regulator 'on' and 'off'
>>> will not actually cycle power and so will not reset
>>> the device.
>>> 
>>> This is particularly a problem for some wi2si wireless modules which
>>> have a BT module on chip and can share power lines.
>>> Without the power-cycle, subsequent reset commands fail.
>>> 
>>> So allow a 'reset' gpio to be specified.  If provided, the
>>> line is asserted during power-up.
>> 
>> There have been several attempts to fix similar issues as this patch
>> does. The latest one I posted a few month ago, which wasn't accepted.
>> http://comments.gmane.org/gmane.linux.power-management.general/46635
> 
> Thanks for that link.
> 
>> 
>> There has also been some off-list discussions on especially how we
>> should describe this in DT and there were actually some consensus made
>> around that. Still I haven't seen any patches on the mailing lists.
> 
> Wish I could have a link for those off-list discussions :-)
> 
> Based on what I read and my own thoughts about other devices that I'm having
> trouble managing I wonder if the right approach might be to admit that these
> buses are *not* 100% discoverable.
> 
> i.e. you can discover what is there once it is turned on, but you cannot
> discover how to turn it on.

Indeed.

> 
> So the *fix* is to allow attached devices to be explicitly listed.
> In my case I would create a child node of the mmc1 node, which is
> compatible=“libertas,wifi" (or whatever the proper name is).

Sounds like a good idea to me.

> 
> Then when the mmc1 wants to power-up, it does:
> 
>   device_for_each_child(mmc_dev, NULL, power_up)
> 
> where:
> 
> static int power_up(struct device *dev, void *data)
> {
>   pm_runtime_get_sync(dev);
>   return 0;
> }
> 
> Then I can put my reset-line management in the libertas driver instead of
> trying to include some of it in the mmc driver.
> 
> This has the advantage of the devicetree actually describing the hardware
> (there is a libertas wifi SDIO chip attached) rather than the behaviour
> (please turn on this regulator and toggle this GPIO to initialise the device).
> 
> I want to do a very similar thing for UARTs (so my GPS and Bluetooth turn on
> when /dev/ttyO? is opened), and I've been thinking about something similar
> for USB - I have a USB attached GSM module, but it also has an Audio link and
> some reset/interrupt lines that need to be configured.
> If I could say to device tree "This USB port has this device attached", I
> think it would be a step in the right direction.

Thinking a little further, it could either be the core driver of the 
device/bus/protocol
or a special driver that only does power management. Or audio.

And we should consider using a list of strings in the compatible entry so that
several drivers can be loaded if the subsystem structure shows that this is 
simpler.

It could be one for power, one for audio. Or in the case of the libertas a 
separate
power&reset driver for a specific chip that uses the libertas driver so that 
chip
specific reset management is not introduced into the libertas core but separate.

For the usb connected modem the subnode to be attached is likely the PHY
where the self-powered device is connected to.

> 
> 
> 
>> 
>> So to summarize, I am really concerned that we keep having these power
>> sequence issues for SDIO devices and right now the discussion has been
>> on hold. I am considering to hack on it myself, since I am tired of
>> waiting. :-)
> 
> Please Cc me if you do.  Meanwhile I'll try to hack together code supporting
> my latest idea and let you know if I get anywhere.
> 
>> 
>> Regarding this patch, I don't intent to apply it.
> 
> Fair enough, I’m starting to not like it so much anyway.
> 

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver

[Dr. H. Nikolaus Schaller]

Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen :

> On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen :
>> 
>>> On 13/11/14 00:10, Marek Belisko wrote:
>>>> opa362 is amplifier for video and can be connected to the tvout pads
>>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>>> (high impedance).
>>>> 
>>>> Signed-off-by: H. Nikolaus Schaller 
>>>> ---
>>>> drivers/video/fbdev/omap2/displays-new/Kconfig |   6 +
>>>> drivers/video/fbdev/omap2/displays-new/Makefile|   1 +
>>>> .../fbdev/omap2/displays-new/amplifier-opa362.c| 343 
>>>> +
>>> 
>>> I think it would be better to rename this to encoder-opa362.c. It's not
>> 
>> When developing this driver we did simply rename the encoder-tfp410 file,
>> but thent hough that it does not fit into the „encoder“ category, because we
>> would expect something digital or digital to analog „encoding“ which it does 
>> not.
> 
> That is true, but we already have other "encoders" like
> encoder-tpd12s015.c, which also do not encode. "encoder" in this context
> means something that takes a video input, and has a video output. In
> contrast to a panel or a connector.
> 
>>>> +
>>>> +  in->ops.atv->set_timings(in, &ddata->timings);
>>>> +  /* fixme: should we receive the invert from our consumer, i.e. the 
>>>> connector? */
>>>> +  in->ops.atv->invert_vid_out_polarity(in, true);
>>> 
>>> Well this does seem to be broken. I don't know what the answer to the
>>> question above is, but the code doesn't work properly.
>>> 
>>> In the opa362_invert_vid_out_polarity function below, you get the invert
>>> boolean from the connector. This you pass to the OMAP venc. However,
>>> above you always override that value in venc with true.
>>> 
>>> So, either the invert_vid_out_polarity value has to be always true or
>>> false, because _OPA362_ requires it to be true or false, OR you need use
>>> the value from the connector.
>>> 
>>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>>> latter, and the call above should be removed.
>> 
>> Yes, you are right - this is not systematic.
>> 
>> But the problem is that we can’t ask the connector here what it wants
>> to see. It might (or might not) call our opa362_invert_vid_out_polarity() 
>> later
>> which we can then forward to overwrite the inital state of this 
>> opa362_enable().
> 
> You don't need to ask. The connector calls invert_vid_out_polarity
> before enabling the output.

Unfortunately it doesn’t. At least not always.

It does only for pdata systems but not for DT based systems:

if (!ddata->dev->of_node) {
in->ops.atv->set_type(in, ddata->connector_type);
in->ops.atv->invert_vid_out_polarity(in,
ddata->invert_polarity);
}

Not calling is in our case different from calling with ddata->invert_polarity 
== 0.

> You can just pass it forward inverted, as
> you already do in this driver. If it doesn't, it's either a bug or you
> can just rely on the value that is already programmed to venc.

Therefore it is not called with “false” which would make our 
invert_vid_out_polarity
invert it and send “true” towards the VENC. So VENC remains non-inverted.

We will also add a patch for the connector-analog.c

>>> We are going to support only DT boot at some point. Thus I think the
>>> whole platform data code should be left out.
>> 
>> Is there already a decision? I think it should not be done before. And it
>> does not harm to still have it.
> 
> It's just a matter of time. I don't accept any new boards using platform
> data for display, or new display drivers using platform data, because I
> don't want to spend my time converting them later. And as this is a new
> driver, no existing board can be using the opa362 platform_data. So we
> can support DT only.

Ok, that looks reasonable - as long as we can rely on that all mainline DSS
drivers are already fully converted to DT :)

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver

[Dr. H. Nikolaus Schaller]

Hi,

Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen :

> On 13/11/14 00:10, Marek Belisko wrote:
>> opa362 is amplifier for video and can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>> drivers/video/fbdev/omap2/displays-new/Kconfig |   6 +
>> drivers/video/fbdev/omap2/displays-new/Makefile|   1 +
>> .../fbdev/omap2/displays-new/amplifier-opa362.c| 343 
>> +
> 
> I think it would be better to rename this to encoder-opa362.c. It's not

When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the „encoder“ category, because we
would expect something digital or digital to analog „encoding“ which it does 
not.

> encoder as such, but it falls into the same category.

But we can change it.

> 
>> include/video/omap-panel-data.h|  12 +
>> 4 files changed, 362 insertions(+)
>> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> 
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig 
>> b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> index e6cfc38..211b3ec 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
>> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> @@ -1,6 +1,12 @@
>> menu "OMAP Display Device Drivers (new device model)"
>> depends on OMAP2_DSS
>> 
>> +config DISPLAY_AMPLIFIER_OPA362
> 
> Here also use ENCODER instead.
> 
>> +tristate "external analog amplifier with output disable/high-Z 
>> (e.g. OPA362)"
>> +help
>> +  Driver to enable an external analog TV amplifier (e.g. OPA362)
>> +  through a GPIO.
> 
> The indentation above seems funny.
> 
> The text looks a bit odd. So is this a driver for OPA362, or is this a
> generic driver for any similar devices? Most of the names and code makes
> me think this is a driver for OPA362, but the text above quite clearly
> gives the impression that this is a driver for any analog video amp,
> with single enable gpio.

Hm. We can imagine that there are other devices with similar functionality
and gpio but we have not tested any. So it is indeed better to describe it as
a pure OPA362 driver.

> 
>> +
>> config DISPLAY_ENCODER_TFP410
>> tristate "TFP410 DPI to DVI Encoder"
>>  help
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile 
>> b/drivers/video/fbdev/omap2/displays-new/Makefile
>> index 0323a8a..b311542 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
>> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
>> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c 
>> b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> new file mode 100644
>> index 000..8065a28
>> --- /dev/null
>> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * OPA362 analog video amplifier with output/power control
>> + *
>> + * Copyright (C) 2014 Golden Delicious Computers
>> + * Author: H. Nikolaus Schaller 
>> + *
>> + * based on encoder-tfp410
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + * Author: Tomi Valkeinen 
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published 
>> by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +
>> +struct panel_drv_data {
>> +struct omap_dss_device dssdev;
>> +struct omap_dss_device *in;
>> +
>> +int enable_gpio;
>> +
>> +struct omap_video_timings timings;
>> +};
>> +
>> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> +
>> +static int opa362_connect(struct omap_dss_device *dssdev,
>> +struct omap_dss_device *dst)
>> +{
>> +struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +struct omap_dss_device *in = ddata->in;
>> +int r;
>> +
>> +dev_dbg(dssdev->dev, "connect\n");
>> +
>> +if (omapdss_device_is_connected(dssdev))
>> +return -EBUSY;
>> +
>> +r = in->ops.atv->connect(in, dssdev);
>> +if (r)
>> +return r;
>> +
>> +dst->src = dssdev;
>> +dssdev->dst = dst;
>> +
>> +return 0;
>> +}
>> +
>> +static void opa362_disconnect(struct omap_dss_device *dssdev,
>> +struct omap_dss_device *dst)
>> +{
>> +struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +struct omap_dss_device *in = ddata->in;
>> +
>> +dev_dbg(dssdev->dev, "disconnect\n");
>> +
>> +WARN_ON(!omapdss_device_is_connect

Re: [Gta04-owner] [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[Dr. H. Nikolaus Schaller]

Am 10.11.2014 um 00:25 schrieb NeilBrown :

> On Sat, 8 Nov 2014 09:26:22 + Mark Brown  wrote:
> 
>> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
>> 
>>> If voice port on twl4030 is not connected to a McBSP (or similar)
>>> then we cannot configure the format the way we normally do for a DAI.
>> 
>> Yes we can, you need to represent the DAI link to whatever else the
>> device is connected to in the driver like we do anything else - and in
>> any case this isn't a device specific issue so we shouldn't be doing
>> something driver specific to solve it.  Look at something like speyside.
> 
> Hi Mark,
> thanks for the reply ... I might need a little bit more help though.
> 
> I had a look at sound/soc/samsung/speyside.c, but I'm not entirely sure what
> I'm looking for.
> Presumably this is an audio processor not unlike the audio module in the
> twl4030.
> 
> I see that there are 3 dai-links:
>  CPU-DSP
>  DSP-CODEC
>  Baseband
> 
> Presumably "Baseband" is similar, in purpose at least, to the "voice"
> interface on the twl4030.
> 
> Each dai-link has a "cpu_dai_name" and a "codec_dai_name", even though it
> appears that only "CPU-DSP" is connected to the CPU.  Maybe that naming is
> the source of some of my confusion.
> 
> "Baseband" declares
>.cpu_dai_name = "wm8996-aif2",
> so wm8996 is something with 2 audio interfaces, (aif), and this is the second
> one?  Maybe the  wm8996 is the audio module, so what is the "speyside"?
> 
> http://opensource.wolfsonmicro.com/content/speyside-audio
> 
> says it is a "reference platform".  Does that mean it is a board with a bunch
> of chips soldered onto it?  If it were a board it should be described by a
> dts file, not by a pile of C code (I thought), so I must be wrong about that.
> 
> 
> In my case, I have a board with a GSM module and the twl4030 module.  Each
> has an audio interface and these are connected.  I assume that I need to
> express this connection in the dts file.
> The GSM module doesn't currently appear in the dts file as it is usb-attached.
> However I've been thinking that we will need to add it so we can express
> power-on controls (twiddling some GPIOs).  So let's suppose we have the GSM
> module in the dts file (child of a USB interface)

It is a quite complex connection pattern and I am not sure if the modem is 
really
a logical child of the USB interface. Powering up/down the USB interface has 
nothing
to do with the modem power. Rather, it continues to operate if USB is suspended
and the modem notifies USB that it has become active.

The connections on this GTA04 board are:

Modem USB <—> CPU USB
Modem PCM <—> TWL4030 Voice <—> OMAP3 McBSP4 (yes, it is a 3-way connection)
Modem Power control <—> 1 or 2 GPIOs (depending on board variant)

The reason for the 3-way connection is that user space can chose if the GSM
voice is routed directly to the TWL codec (low latency, independent of CPU) or
goes through the CPU (e.g. for DTAM or voice scrambling by software) and
then through the other PCM link between the CPU and the TWL.

This is why I would make the McBSP4 a separate sound card.

And, this is why it needs some control and tri-state of the TWL4030 and OMAP3
McBSP4 since only one can provide a digital PCM stream to the modem.

One more thing to consider for a general solution is that there are modem 
modules
that communicate data through UART or SPI - but otherwise have a similar 
connection
for digital audio. So forcing the power control driver to be a subnode of USB 
doesn’t
appear general enough to me.

Finally, this connection pattern is not specific to this modem (OPTION GTM601) 
on this
GTA04 board. We plan to use a Gemalto PHS8  in the Pyra-Handheld and the Neo900
devices - but the general connection pattern as defined above remains the same.

So my proposal is to have a specific wwan-power driver for this type of modems.
And power control can and should be kept separately from USB (except the case
where only 1 GPIO exists and USB must be tapped to detect the modem power
state). You can find work in progress for this approach here:



> and the twl4030 as well
> (beneath an i2c interface).
> 
> The twl4030 needs to know the master/polarity of the clk/frm lines.  The GSM
> module declares that these are.  So presumably we need some sort of linkage.
> A... I found Documentation/devicetree/bindings/sound/simple-card.txt
> 
> So I need to make the "voice" port on the twl4030 look like a "cpu" end of a
> dai-link, and create a "codec" end in the GSM module, and use "sound-dai" to
> point from the twl4030 to the GSM module.

What I wonder a little is that we have all these dai-links working since your 
3.7
kernel for GTA04. Is it necessary to re-invent a solution or should we just make
the solution device tree compatible?

> Then I use fram

Re: [PATCH 2/2] Documentation: devicetree: Add bindings for Wi2Wi w2sg0004 gps

[Dr. H. Nikolaus Schaller]

ping (questions for directions at the end of the mail).

Am 24.10.2014 um 11:32 schrieb Dr. H. Nikolaus Schaller :

> 
> Am 20.10.2014 um 19:26 schrieb Dr. H. Nikolaus Schaller :
> 
>> Hi,
>> 
>> Am 20.10.2014 um 11:35 schrieb Mark Rutland :
>> 
>>> Hi,
>>> 
>>> On Fri, Oct 17, 2014 at 08:55:50PM +0100, Dr. H. Nikolaus Schaller wrote:
>>>> 
>>>> Am 17.10.2014 um 13:00 schrieb Mark Rutland :
>>>> 
>>>>> On Fri, Oct 17, 2014 at 11:16:42AM +0100, Dr. H. Nikolaus Schaller wrote:
>>>>>> 
>>>>>> Am 17.10.2014 um 11:37 schrieb Mark Rutland :
>>>>>> 
>>>>>>> On Thu, Oct 16, 2014 at 09:26:23PM +0100, Marek Belisko wrote:
>>>>>>>> Signed-off-by: H. Nikolaus Schaller 
>>>>>>>> Signed-off-by: Marek Belisko 
>>>>>>>> ---
>>>>>>>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt| 44 
>>>>>>>> ++
>>>>>>>> 1 file changed, 44 insertions(+)
>>>>>>>> create mode 100644 
>>>>>>>> Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>>> 
>>>>>>>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt 
>>>>>>>> b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 000..e11
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>>> @@ -0,0 +1,44 @@
>>>>>>>> +Wi2Wi GPS module connected through UART
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
>>>>>>>> +- pinctrl: specify two states (default and monitor). One is the 
>>>>>>>> default (UART) mode
>>>>>>>> +  and the other is for monitoring the RX line by an interrupt
>>>>>>>> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
>>>>>>>> +
>>>>>>>> +Optional properties:
>>>>>>>> +- lna-suppy: an (optional) LNA regulator that is enabled together 
>>>>>>>> with the GPS receiver
>>>>>>>> +
>>>>>>>> +example:
>>>>>>>> +
>>>>>>>> +gps_receiver: w2sg0004 {
>>>>>>>> +compatible = "wi2wi,w2sg0004";
>>>>>>> 
>>>>>>> I couldn't spot "wi2wi" in
>>>>>>> Documentation/devicetree/bindings/vendor-prefixes.txt (in mainline).
>>>>>>> 
>>>>>>> Could you please add it?
>>>>>>> 
>>>>>>>> +gpio-controller;
>>>>>>>> +#gpio-cells = <2>;
>>>>>>> 
>>>>>>> As far as I can see, these properties aren't necessary. This only
>>>>>>> consumes a GPIO, it doesn't provide any.
>>>>>> 
>>>>>> Well, it provides one GPIO. Sort of a "virtual“ GPIO. It is needed so 
>>>>>> that 
>>>>>> it can be wired up to the DTR gpio of the UART driver (unfortunately this
>>>>>> patch was reverted some months ago from mainline and we will reintroduce
>>>>>> it soon).
>>>>> 
>>>>> If this GPIO doesn't really exist, then it's a Linux internal
>>>>> implementation detail rather than a description of the hardware, and
>>>>> doesn't really belong in the DT.
>>>> 
>>>> Hm. 
>>>> 
>>>> Let’s describe it differently.
>>>> 
>>>> I can see the Linux driver module as a simple software simulation for a
>>>> piece of hardware that could have been connected between the UART
>>>> and the GPS chip.
>>>> 
>>>> Basically it is a pulse-generator and a flip-flop to detect data flow on 
>>>> the RX
>>>> wire. This could be implemented by an external FPGA or uController. Or as
>>>> it is done on our board for saving hardware by a mix of the main CPU 
>>>> hardware
>>>> (Pinmu

Re: [PATCH 2/2] Documentation: devicetree: Add bindings for Wi2Wi w2sg0004 gps

[Dr. H. Nikolaus Schaller]

Am 20.10.2014 um 19:26 schrieb Dr. H. Nikolaus Schaller :

> Hi,
> 
> Am 20.10.2014 um 11:35 schrieb Mark Rutland :
> 
>> Hi,
>> 
>> On Fri, Oct 17, 2014 at 08:55:50PM +0100, Dr. H. Nikolaus Schaller wrote:
>>> 
>>> Am 17.10.2014 um 13:00 schrieb Mark Rutland :
>>> 
>>>> On Fri, Oct 17, 2014 at 11:16:42AM +0100, Dr. H. Nikolaus Schaller wrote:
>>>>> 
>>>>> Am 17.10.2014 um 11:37 schrieb Mark Rutland :
>>>>> 
>>>>>> On Thu, Oct 16, 2014 at 09:26:23PM +0100, Marek Belisko wrote:
>>>>>>> Signed-off-by: H. Nikolaus Schaller 
>>>>>>> Signed-off-by: Marek Belisko 
>>>>>>> ---
>>>>>>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt| 44 
>>>>>>> ++
>>>>>>> 1 file changed, 44 insertions(+)
>>>>>>> create mode 100644 
>>>>>>> Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>> 
>>>>>>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt 
>>>>>>> b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>> new file mode 100644
>>>>>>> index 000..e11
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>> @@ -0,0 +1,44 @@
>>>>>>> +Wi2Wi GPS module connected through UART
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
>>>>>>> +- pinctrl: specify two states (default and monitor). One is the 
>>>>>>> default (UART) mode
>>>>>>> +  and the other is for monitoring the RX line by an interrupt
>>>>>>> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
>>>>>>> +
>>>>>>> +Optional properties:
>>>>>>> +- lna-suppy: an (optional) LNA regulator that is enabled together with 
>>>>>>> the GPS receiver
>>>>>>> +
>>>>>>> +example:
>>>>>>> +
>>>>>>> +gps_receiver: w2sg0004 {
>>>>>>> +compatible = "wi2wi,w2sg0004";
>>>>>> 
>>>>>> I couldn't spot "wi2wi" in
>>>>>> Documentation/devicetree/bindings/vendor-prefixes.txt (in mainline).
>>>>>> 
>>>>>> Could you please add it?
>>>>>> 
>>>>>>> +gpio-controller;
>>>>>>> +#gpio-cells = <2>;
>>>>>> 
>>>>>> As far as I can see, these properties aren't necessary. This only
>>>>>> consumes a GPIO, it doesn't provide any.
>>>>> 
>>>>> Well, it provides one GPIO. Sort of a "virtual“ GPIO. It is needed so 
>>>>> that 
>>>>> it can be wired up to the DTR gpio of the UART driver (unfortunately this
>>>>> patch was reverted some months ago from mainline and we will reintroduce
>>>>> it soon).
>>>> 
>>>> If this GPIO doesn't really exist, then it's a Linux internal
>>>> implementation detail rather than a description of the hardware, and
>>>> doesn't really belong in the DT.
>>> 
>>> Hm. 
>>> 
>>> Let’s describe it differently.
>>> 
>>> I can see the Linux driver module as a simple software simulation for a
>>> piece of hardware that could have been connected between the UART
>>> and the GPS chip.
>>> 
>>> Basically it is a pulse-generator and a flip-flop to detect data flow on 
>>> the RX
>>> wire. This could be implemented by an external FPGA or uController. Or as
>>> it is done on our board for saving hardware by a mix of the main CPU 
>>> hardware
>>> (Pinmux + GPIO + IRQ) and a kernel driver.
>>> 
>>> The best of course would have been if the w2sg0004 would have a physical
>>> „enable“ GPIO (instead of that on-off control input).
>>> 
>>> Then we would hook up that enable to some physical GPIO of the CPU
>>> and simply refer to it as e.g. <&gpio4 12>. And would not even need a driver
>>> for it (unless we want to make rfkill gps work).
>>>

Re: [PATCH 1/2] misc: Add Wi2Wi w2sc0004 gps driver

[Dr. H. Nikolaus Schaller]

Hi,

Am 21.10.2014 um 12:49 schrieb Pavel Machek :

> Hi!
> 
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -515,6 +515,16 @@ config VEXPRESS_SYSCFG
>>bus. System Configuration interface is one of the possible means
>>of generating transactions on this bus.
>> 
>> +config W2SG0004
>> +tristate "W2SG0004 on/off control"
> 
>   ~~ insert GPS here.

ok!

> And make it bool if it can’t be a module.
> 
>> +depends on GPIOLIB
>> +help
>> +  Enable on/off control of W2SG0004 GPS using a virtual GPIO.
>> +  The virtual GPIO can be connected to a DTR line of a serial
>> +  interface to allow powering up if the /dev/tty$n is opened.
>> +  It also provides a rfkill gps node to control the LNA power.
>> +  NOTE: can’t currently be compiled as module, so please choose Y.

Sorry, this is a bug in the description that was coorect before using the 
pinctrl
framework. The driver has been tested to work compiled as a module.

>> +
> 
>> +++ b/drivers/misc/w2sg0004.c
>> @@ -0,0 +1,512 @@
>> +/*
>> + * w2sg0004.c
>> + * Virtual GPIO of controlling the w2sg0004 GPS receiver.
>> + *
>> + * Copyright (C) 2011 Neil Brown 
>> + *
>> + * This receiver has an ON/OFF pin which must be toggled to
>> + * turn the device 'on' or 'off'.  A high->low->high toggle
>> + * will switch the device on if it is off, and off if it is on.
>> + * It is not possible to directly detect the state of the device.
>> + * However when it is on it will send characters on a UART line
>> + * regularly.
>> + * On the OMAP3, the UART line can also be programmed as a GPIO
>> + * on which we can receive interrupts.
>> + * So when we want the device to be 'off' we can reprogram
>> + * the line, toggle the ON/OFF pin and hope that it is off.
>> + * However if an interrupt arrives we know that it is really on
>> + * and can toggle again.
>> + *
>> + * To enable receiving on/off requests we create a gpio_chip
>> + * with a single 'output' GPIO.  When it is low, the
>> + * GPS is turned off.  When it is high, it is turned on.
>> + * This can be configured as the DTR GPIO on the UART which
>> + * connects the GPS.  Then whenever the tty is open, the GPS
>> + * will be switched on, and whenever it is closed, the GPS will
>> + * be switched off.
>> + *
>> + * In addition we register as a rfkill client so that we can
>> + * control the LNA power.
>> + *
>> + */
> 
> GPL?
> 
>> +/*
>> + * There seems to restrictions on how quickly we can toggle the
>> + * on/off line.  data sheets says "two rtc ticks", whatever that means.
> 
> “seems to"?

-> “seems to be”

> data -> Data?

ok!

> 
>> +enum w2sg_state {
>> +W2SG_IDLE,  /* is not changing state */
>> +W2SG_PULSE, /* activate on/off impulse */
>> +W2SG_NOPULSE/* desctivate on/off impulse */
>> +};
> 
> deactivate.

ok.

> 
>> +
>> +struct gpio_w2sg {
>> +struct  rfkill *rf_kill;
>> +struct  regulator *lna_regulator;
>> +int lna_blocked;/* rfkill block gps active */
>> +int lna_is_off; /* LNA is currently off */
>> +int is_on;  /* current state (0/1) */
>> +unsigned long   last_toggle;
>> +unsigned long   backoff;/* time to wait since last_toggle */
>> +int on_off_gpio;
>> +int rx_irq;
>> +
>> +struct pinctrl *p;
>> +struct pinctrl_state *default_state;/* should be UART mode */
>> +struct pinctrl_state *monitor_state;/* monitor RX as GPIO */
>> +enum w2sg_state state;
>> +int requested;  /* requested state (0/1) */
>> +int suspended;
>> +int rx_redirected;
>> +spinlock_t  lock;
>> +#ifdef CONFIG_GPIOLIB
>> +struct gpio_chip gpio;
>> +const char  *gpio_name[1];
>> +#endif
> 
> Depends on gpiolib, why ifdef?

historic…

> 
> Array of names?

Yes, that is how it should be defined (usually gpio_chips have more than 1 
entry).
Otherwise we have to provide some & operators when passing to the gpiolib 
functions
where all other similar drivers simply use the array name. So it is a matter of 
taste
where we need to introduce confusion. AFAIK the compiler generated binary code
should be identical.

> 
> 
>> +rf_kill = rfkill_alloc("GPS", &pdev->dev, RFKILL_TYPE_GPS,
>> +&gpio_w2sg0004_rfkill_ops, gw2sg);
> 
> Actually, is rfkill interface right one on GPS? GPS is not supposed to
> transmit…

We have not invented RFKILL_TYPE_GPS. We just use it.
And AFAIK no system that transmits GPS runs Linux.

The function here is to stop it from receiving (and allows to power down
any RF amplifiers and active antennas)…

> 
>> +int gpio_base;  /* (not used by DT) - defines the  gpio.base */
> 
> 
> Is non-device tree path still usefull?

Probably not. At least we don’t need it in our projects any more.

> 
>

Re: [PATCH 2/2] Documentation: devicetree: Add bindings for Wi2Wi w2sg0004 gps

[Dr. H. Nikolaus Schaller]

Hi,

Am 20.10.2014 um 11:35 schrieb Mark Rutland :

> Hi,
> 
> On Fri, Oct 17, 2014 at 08:55:50PM +0100, Dr. H. Nikolaus Schaller wrote:
>> 
>> Am 17.10.2014 um 13:00 schrieb Mark Rutland :
>> 
>>> On Fri, Oct 17, 2014 at 11:16:42AM +0100, Dr. H. Nikolaus Schaller wrote:
>>>> 
>>>> Am 17.10.2014 um 11:37 schrieb Mark Rutland :
>>>> 
>>>>> On Thu, Oct 16, 2014 at 09:26:23PM +0100, Marek Belisko wrote:
>>>>>> Signed-off-by: H. Nikolaus Schaller 
>>>>>> Signed-off-by: Marek Belisko 
>>>>>> ---
>>>>>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt| 44 
>>>>>> ++
>>>>>> 1 file changed, 44 insertions(+)
>>>>>> create mode 100644 
>>>>>> Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>> 
>>>>>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt 
>>>>>> b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>> new file mode 100644
>>>>>> index 000..e11
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>> @@ -0,0 +1,44 @@
>>>>>> +Wi2Wi GPS module connected through UART
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
>>>>>> +- pinctrl: specify two states (default and monitor). One is the default 
>>>>>> (UART) mode
>>>>>> +  and the other is for monitoring the RX line by an interrupt
>>>>>> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- lna-suppy: an (optional) LNA regulator that is enabled together with 
>>>>>> the GPS receiver
>>>>>> +
>>>>>> +example:
>>>>>> +
>>>>>> +gps_receiver: w2sg0004 {
>>>>>> +compatible = "wi2wi,w2sg0004";
>>>>> 
>>>>> I couldn't spot "wi2wi" in
>>>>> Documentation/devicetree/bindings/vendor-prefixes.txt (in mainline).
>>>>> 
>>>>> Could you please add it?
>>>>> 
>>>>>> +gpio-controller;
>>>>>> +#gpio-cells = <2>;
>>>>> 
>>>>> As far as I can see, these properties aren't necessary. This only
>>>>> consumes a GPIO, it doesn't provide any.
>>>> 
>>>> Well, it provides one GPIO. Sort of a "virtual“ GPIO. It is needed so that 
>>>> it can be wired up to the DTR gpio of the UART driver (unfortunately this
>>>> patch was reverted some months ago from mainline and we will reintroduce
>>>> it soon).
>>> 
>>> If this GPIO doesn't really exist, then it's a Linux internal
>>> implementation detail rather than a description of the hardware, and
>>> doesn't really belong in the DT.
>> 
>> Hm. 
>> 
>> Let’s describe it differently.
>> 
>> I can see the Linux driver module as a simple software simulation for a
>> piece of hardware that could have been connected between the UART
>> and the GPS chip.
>> 
>> Basically it is a pulse-generator and a flip-flop to detect data flow on the 
>> RX
>> wire. This could be implemented by an external FPGA or uController. Or as
>> it is done on our board for saving hardware by a mix of the main CPU hardware
>> (Pinmux + GPIO + IRQ) and a kernel driver.
>> 
>> The best of course would have been if the w2sg0004 would have a physical
>> „enable“ GPIO (instead of that on-off control input).
>> 
>> Then we would hook up that enable to some physical GPIO of the CPU
>> and simply refer to it as e.g. <&gpio4 12>. And would not even need a driver
>> for it (unless we want to make rfkill gps work).
>> 
>> Therefore the driver we suggest provides an additional gpio controller with a
>> single GPIO so that we can write <&w2sg 0> to refer to this virtual gpio.
>> 
>> So in fact we try to wrap a non-optimal chip design by the driver and make it
>> appear as a standard GPIO interface to the DT and user space and whoever
>> needs simply to enable/disable the GPS chip.
> 
> The fact remains that this doe

Re: [PATCH 1/2] misc: Add Wi2Wi w2sc0004 gps driver

[Dr. H. Nikolaus Schaller]

Hi,

Am 19.10.2014 um 21:51 schrieb Arnd Bergmann :

> On Thursday 16 October 2014 22:26:22 Marek Belisko wrote:
>> This is a driver for the Wi2Wi GPS modules connected through an UART.
>> The tricky part is that the module is turned on or off by an impulse
>> on the control line - but it is only possible to get the real state by 
>> monitoring
>> the UART RX input. Since the kernel can't know in which status the module
>> is brought e.g. by a boot loader or after suspend, we need some logic to 
>> handle
>> this.
>> 
>> The driver allows two different methods to enable (and power up) GPS:
>> a) through rfkill
>> b) as a GPIO
>> 
>> The GPIO enable can be plumbed to other drivers that expect to be able to 
>> control
>> a GPIO. On the GTA04 board this is the DTR-gpio of the connected UART so that
>> opening the UART device enables the receiver and closing it shuts the 
>> receiver down.
>> 
>> Original implementation by Neil Brown, fixes + DT bindings by H. Nikolaus 
>> Schaller
> 
> I’m not sure if this is a good way to model the device.

It is the easiest way we have found after ca. 2 years of working on it :)

> You say that it's
> connected to a UART, but the code itself has no reference to the tty layer
> at all.

Yes.

> I assume the actual driver is in user space and it would open the
> UART and this control device as separate instances handles and then try
> to coordinate them. Is that right?

Yes, it is called gpsd.

> 
> What is the protocol for the GPS driver itself? Is it standardized?

Yes, NMEA records. Like most GPS receivers provide them.

> Would it help to have a TTY line discipline for the GPS mode instead
> so the power management and startup could be integrated into the serial
> port management instead?

I don’t think line disciplines (as far as I understand them) are helpful and
this chip is not special at all regarding the serial interface data. So it 
needs no
“GPS mode”.

It is very similar to connecting some external handheld GPS receiver by a
RS232 cable or RS232-USB adapter. Or GPS “mouse” receivers through
bluetooth.

They all create/show some /dev/tty that you simply open/read/close. And
there is no special processing of the serial data involved.

The only thing this driver needs to solve is to properly power up/down
the chip on demand.

The DTR line of the tty can enable/disable power of a connected device
(being an external modem or this GPS chip). This is what we have made
the driver compatible to by providing a virtual GPIO to enable/disable.

This is done by a patch to the tty driver that already was in the kernel
but removed in 3.15 because the w2sg driver wasn’t submitted at that
time.

Commit: 985bfd54c826c0ba873ca0adfd5589263e0c6ee2

Basically, if our CPU would have a real RS232 interface and we would
put the chip into an external device, it should look exactly the same if
we look at the serial interface part. Since soldering it on the same PCB
or connecting through a connector should not need different serial drivers.
Just different device tree files.

Therefore we have decided not to touch any serial driver code at all,
because it is not needed (except allowing DTR to control some GPIO)
and keeps it simple and manageable.

BR,
Nikolaus


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Documentation: devicetree: Add bindings for Wi2Wi w2sg0004 gps

[Dr. H. Nikolaus Schaller]

Am 17.10.2014 um 13:00 schrieb Mark Rutland :

> On Fri, Oct 17, 2014 at 11:16:42AM +0100, Dr. H. Nikolaus Schaller wrote:
>> 
>> Am 17.10.2014 um 11:37 schrieb Mark Rutland :
>> 
>>> On Thu, Oct 16, 2014 at 09:26:23PM +0100, Marek Belisko wrote:
>>>> Signed-off-by: H. Nikolaus Schaller 
>>>> Signed-off-by: Marek Belisko 
>>>> ---
>>>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt| 44 
>>>> ++
>>>> 1 file changed, 44 insertions(+)
>>>> create mode 100644 
>>>> Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt 
>>>> b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>> new file mode 100644
>>>> index 000..e11
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>> @@ -0,0 +1,44 @@
>>>> +Wi2Wi GPS module connected through UART
>>>> +
>>>> +Required properties:
>>>> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
>>>> +- pinctrl: specify two states (default and monitor). One is the default 
>>>> (UART) mode
>>>> +  and the other is for monitoring the RX line by an interrupt
>>>> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
>>>> +
>>>> +Optional properties:
>>>> +- lna-suppy: an (optional) LNA regulator that is enabled together with 
>>>> the GPS receiver
>>>> +
>>>> +example:
>>>> +
>>>> +gps_receiver: w2sg0004 {
>>>> +compatible = "wi2wi,w2sg0004";
>>> 
>>> I couldn't spot "wi2wi" in
>>> Documentation/devicetree/bindings/vendor-prefixes.txt (in mainline).
>>> 
>>> Could you please add it?
>>> 
>>>> +gpio-controller;
>>>> +#gpio-cells = <2>;
>>> 
>>> As far as I can see, these properties aren't necessary. This only
>>> consumes a GPIO, it doesn't provide any.
>> 
>> Well, it provides one GPIO. Sort of a "virtual“ GPIO. It is needed so that 
>> it can be wired up to the DTR gpio of the UART driver (unfortunately this
>> patch was reverted some months ago from mainline and we will reintroduce
>> it soon).
> 
> If this GPIO doesn't really exist, then it's a Linux internal
> implementation detail rather than a description of the hardware, and
> doesn't really belong in the DT.

Hm. 

Let’s describe it differently.

I can see the Linux driver module as a simple software simulation for a
piece of hardware that could have been connected between the UART
and the GPS chip.

Basically it is a pulse-generator and a flip-flop to detect data flow on the RX
wire. This could be implemented by an external FPGA or uController. Or as
it is done on our board for saving hardware by a mix of the main CPU hardware
(Pinmux + GPIO + IRQ) and a kernel driver.

The best of course would have been if the w2sg0004 would have a physical
„enable“ GPIO (instead of that on-off control input).

Then we would hook up that enable to some physical GPIO of the CPU
and simply refer to it as e.g. <&gpio4 12>. And would not even need a driver
for it (unless we want to make rfkill gps work).

Therefore the driver we suggest provides an additional gpio controller with a
single GPIO so that we can write <&w2sg 0> to refer to this virtual gpio.

So in fact we try to wrap a non-optimal chip design by the driver and make it
appear as a standard GPIO interface to the DT and user space and whoever
needs simply to enable/disable the GPS chip.

> 
> It sounds like what we actually need is the ability to describe devices
> attached to UARTs.

Hm. The purpose of the driver is power control of the chip. Not the serial
interface.

> Then you could have a mechanism whereby the UART
> driver can notify the other device driver regarding events (e.g. the
> UART being opened for access), or the other driver could claim ownership
> of the UART and expose its own interface to userspace.
> 
> That would be independent of the particular UART or other device, and
> the only description necessary in the DT would be an accurate
> representation of the way the hardware is wired.
> 
> There are a few ways that could be done, but I suspect the simplest is
> to just have the device as a sub-node of the UART, like we do for SPI or
> I2C buses:
> 
>   serial@f00 {
>   compatible = "vendor,uart";
>  

Re: [PATCH 2/2] Documentation: devicetree: Add bindings for Wi2Wi w2sg0004 gps

[Dr. H. Nikolaus Schaller]

Am 17.10.2014 um 11:37 schrieb Mark Rutland :

> On Thu, Oct 16, 2014 at 09:26:23PM +0100, Marek Belisko wrote:
>> Signed-off-by: H. Nikolaus Schaller 
>> Signed-off-by: Marek Belisko 
>> ---
>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt| 44 
>> ++
>> 1 file changed, 44 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt 
>> b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> new file mode 100644
>> index 000..e11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>> @@ -0,0 +1,44 @@
>> +Wi2Wi GPS module connected through UART
>> +
>> +Required properties:
>> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
>> +- pinctrl: specify two states (default and monitor). One is the default 
>> (UART) mode
>> +  and the other is for monitoring the RX line by an interrupt
>> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
>> +
>> +Optional properties:
>> +- lna-suppy: an (optional) LNA regulator that is enabled together with the 
>> GPS receiver
>> +
>> +example:
>> +
>> +gps_receiver: w2sg0004 {
>> +compatible = "wi2wi,w2sg0004";
> 
> I couldn't spot "wi2wi" in
> Documentation/devicetree/bindings/vendor-prefixes.txt (in mainline).
> 
> Could you please add it?
> 
>> +gpio-controller;
>> +#gpio-cells = <2>;
> 
> As far as I can see, these properties aren't necessary. This only
> consumes a GPIO, it doesn't provide any.

Well, it provides one GPIO. Sort of a "virtual“ GPIO. It is needed so that 
it can be wired up to the DTR gpio of the UART driver (unfortunately this
patch was reverted some months ago from mainline and we will reintroduce
it soon).

The reason to solve it that way is that we did not want to have a direct link
between this driver and any serial drivers or other mechanisms how drivers
can detect that their serial port (/dev/tty*) is opened.

It is used to power down the w2sg GPS chip if no user space process is
accessing its serial port (or de-asserts DTR through tcsetattr/ioctl).

> 
>> +
>> +pinctrl-names = "default", "monitor";
>> +pinctrl-0 = <&uart2_pins>;
>> +pinctrl-1 = <&uart2_rx_irq_pins>;
>> +
>> +interrupt-parent = <&gpio5>;
>> +interrupts = <19 IRQ_TYPE_EDGE_FALLING>;  /* GPIO_147: RX - 
>> trigger on arrival of start bit */
> 
> While interrupts is a standard property, please describe above how many
> you expect and what their logical function is.
> 
> The only part I'm confused about is how the link to the UART is
> described. I assume I'm just ignorant of some existing pattern.

The serial link itself is not described at all because it is assumed to be a 
„must have“.
The driver only needs to monitor the RX line and needs to switch it between 
UART and
GPIO/IRQ mode. So this monitoring switch is described (with two different 
pinctrl states).

We know that it is a little tricky to control this chip correctly - and we 
think this solution
is the most general (no direct dependency on the serial line, and just to 
pinmux states
and an interrupt).

> 
> Otherwise this looks ok.
> 
> Thanks,
> Mark.

Thanks as well,
Nikolaus

> 
>> +lna-supply = <&vsim>;   /* LNA regulator */
>> +on-off-gpio = <&gpio5 17 0>;/* GPIO_145: trigger 
>> for turning on/off w2sg0004 */
>> +
>> +&pinmux {
>> +
>> +uart2_pins: pinmux_uart2_pins {
>> +pinctrl-single,pins = <
>> +0x14a (PIN_INPUT | MUX_MODE0)   /* 
>> uart2_tx.uart2_rx */
>> +0x148 (PIN_OUTPUT | MUX_MODE0)  /* 
>> uart2_tx.uart2_tx */
>> +>;
>> +};
>> +
>> +uart2_rx_irq_pins: pinmux_uart2_rx_irq_pins {
>> +pinctrl-single,pins = <
>> +/* switch RX to GPIO so that we can get interrupts by 
>> the start bit */
>> +0x14a (PIN_INPUT | MUX_MODE4)   /* 
>> uart2_tx.uart2_rx */
>> +>;
>> +};
>> +
>> +}
>> -- 
>> 1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] arm: dts: Add gta04a5 model

[Dr. H. Nikolaus Schaller]

Hi,

Am 29.07.2014 um 14:41 schrieb Tony Lindgren:

> * Dr. H. Nikolaus Schaller  [140728 13:45]:
>> 
>> Yes,
>> the boot loader either loads gta04a3.dtb gta04.dtb or gta05.dtb
>> depending on board revision.
>> 
>> Therefore we have multiple device tree files to represent such hardware
>> differences. Like Gumstix Overo variants (using a  omap3-overo-base.dtsi)
>> 
>> And, this difference is not the only one. Other are to come later.
>> 
>> Unfortunately we were recommend to submit only DT nodes that already
>> have drivers. Therefore, the "big picture" of the device variants support
>> may not yet be visible from this patch.
>> 
>> I hope this gives a little background.
> 
> So what's the conclusion? Are changes needed to this patch
> series or not?

I advocate that no changes are needed.

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] arm: dts: Add gta04a5 model

[Dr. H. Nikolaus Schaller]

Hi,

Am 28.07.2014 um 22:12 schrieb Michael Trimarchi:

> Hi
> 
> Il 28/lug/2014 22:06 "Belisko Marek"  ha scritto:
> >
> > On Mon, Jul 28, 2014 at 10:04 PM, Michael Trimarchi
> >  wrote:
> > > Hi
> > >
> > > Il 28/lug/2014 22:02 "Belisko Marek"  ha scritto:
> > >
> > >
> > >>
> > >> Hi Michael,
> > >>
> > >> On Mon, Jul 28, 2014 at 9:56 PM, Michael Trimarchi
> > >>  wrote:
> > >> > Hi Marek
> > >> >
> > >> > Il 28/lug/2014 21:54 "Marek Belisko"  ha scritto:
> > >> >
> > >> >
> > >> >>
> > >> >> Add model a5 which have additional jack detection.
> > >> >>
> > >> >> Signed-off-by: Marek Belisko 
> > >> >> ---
> > >> >>  arch/arm/boot/dts/Makefile  |  1 +
> > >> >>  arch/arm/boot/dts/omap3-gta04a5.dts | 17 +
> > >> >>  2 files changed, 18 insertions(+)
> > >> >>  create mode 100644 arch/arm/boot/dts/omap3-gta04a5.dts
> > >> >>
> > >> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > >> >> index 7d45fde..576c595 100644
> > >> >> --- a/arch/arm/boot/dts/Makefile
> > >> >> +++ b/arch/arm/boot/dts/Makefile
> > >> >> @@ -285,6 +285,7 @@ dtb-$(CONFIG_ARCH_OMAP3) += am3517-craneboard.dtb 
> > >> >> \
> > >> >> omap3-evm-37xx.dtb \
> > >> >> omap3-gta04a3.dtb \
> > >> >> omap3-gta04a4.dtb \
> > >> >> +   omap3-gta04a5.dtb \
> > >> >> omap3-igep0020.dtb \
> > >> >> omap3-igep0030.dtb \
> > >> >> omap3-ldp.dtb \
> > >> >> diff --git a/arch/arm/boot/dts/omap3-gta04a5.dts
> > >> >> b/arch/arm/boot/dts/omap3-gta04a5.dts
> > >> >> new file mode 100644
> > >> >> index 000..210317c3
> > >> >> --- /dev/null
> > >> >> +++ b/arch/arm/boot/dts/omap3-gta04a5.dts
> > >> >> @@ -0,0 +1,17 @@
> > >> >> +/*
> > >> >> + * Copyright (C) 2014 H. Nikolaus Schaller 
> > >> >> + *
> > >> >> + * This program is free software; you can redistribute it and/or
> > >> >> modify
> > >> >> + * it under the terms of the GNU General Public License version 2 as
> > >> >> + * published by the Free Software Foundation.
> > >> >> + */
> > >> >> +
> > >> >> +#include "omap3-gta04.dtsi"
> > >> >> +
> > >> >> +/ {
> > >> >> +   model = "Goldelico GTA04A5;
> > >> >> +
> > >> >> +   sound {
> > >> >> +   ti,jack-det-gpio = <&twl_gpio 2 0>;/* GTA04A5 only
> > >> >> */
> > >> >> +   };
> > >> >> +};
> > >> >> --
> > >> >> 1.9.1
> > >> >>
> > >> >
> > >> > Can we have only one dts with status enabled/disabled?
> > >> Do you mean to define this node it gta04.dtsi disabled and enable only
> > >> for a5 model?
> > >> >
> > >
> > > This should be possible in the bootloader
> > Sorry I didn't get that. Can you give me some short example (or
> > existing board which using it)?
> > Thanks.
> > >
> 
> http://www.denx.de/wiki/view/DULG/UBootCmdFDT
> 
> I just suggest to not create a dts for each board revision but let it manage 
> by the bootloader. I don't know if there are examples but make sense to me to 
> avoid to add a lot of dts.
> 
> 

Hm. Why should we dynamically modify the dtb in memory through slow, 
interpreted FDT commands in u-boot, if the DTC can statically compile complete 
.dtb files by using an #include mechanism? It would slow down boot time for no 
other improvement. And debugging is more difficult that looking for compiler 
errors.

And our u-boot does not support the fdt command to avoid bloating it. IMHO boot 
loaders should be as small and fast as possible and contain only what is barely 
necessary. Everything more sophisticated should be done by the kernel and 
device drivers and some init user space process. I.e. boot loader should only 
tell the kernel only which board revision it is but not "manage" the board 
revisions.

We also have a nice feature that a system on an SD card has the uImage in /boot 
and all existing and relevant .dtb files. U-Boot simply picks and loads the 
right one (by file name). This keeps the boot loader stable (people always have 
difficulties flashing new boot loaders). Therefore we need precompiled DTBs 
that completely and consistently describe each board variant. This mechanism 
allows that we can easily swap the SD card between different boards and 
variants. Implementing this feature using U-Boot scripts with FDT commands to 
patch the DT, appears to become a nightmare. If you are interested, the whole 
concept is described here:

http://projects.goldelico.com/p/gta04-main/downloads/55/

Another aspect to consider: in my understanding, the main reason why DT is 
still maintained within the kernel is that the bindings are not yet stable. 
Therefore if bindings are patched, all device trees using them are also to be 
changed. I don't see how this can work if we have part of the board variations 
managed outside the kernel tree. Therefore each board revision is to be treated 
not differently from a completely new board. And similar parts are treated by a 
common .dtsi. This is how the concept IMHO should be. And the consequence is 
that we need a different .dts file for each board 

Re: [PATCH 1/5] arm: dts: omap3-gta04: Add missing nodes to fully describe gta04 board

[Dr. H. Nikolaus Schaller]

Hi all,

Am 18.07.2014 um 11:21 schrieb Javier Martinez Canillas:

> Hello Marek and Dr. H. Nikolaus,
> 
> On Fri, Jul 18, 2014 at 8:55 AM, Joachim Eastwood  wrote:
>> On 16 July 2014 09:17, Dr. H. Nikolaus Schaller  wrote:
>>> Am 15.07.2014 um 14:45 schrieb Joachim Eastwood:
>>> 
>>>> Hi Marek,
>>>> 
>>>> You seem to add some DT nodes for hw that doesn't have drivers in
>>>> mainline. I think you should leave those out until the driver itself
>>>> is upstream and the bindings for it is documented.
>>> is there some policy for only having nodes for existing drivers in DT files?
>> 
> 
> I strongly agree with Joachim on this regard.
> 
>>> If I understand the device tree concept correctly, it should not describe 
>>> drivers
>>> (and hence nothing about the state of them being mainlined), but it should 
>>> statically
>>> describe the given hardware in a way that kernel and drivers can read it 
>>> (or ignore).
>>> And even other kernels can use it (because they run on the same hardware).
>>> 
> 
> Yes, it should describe hardware but that description should be
> previously agreed and properly documented as was mentioned before.

Ok, it is a little a hen and egg problem - who should start with the 
documentation.
The device tree file, more or less describing what the hardware requirements
are. Or the drivers which describe what they support.

And you are right - these two ends must match and that can only be resolved by
discussions and negotiations between both ends.

> 
>>> So unless there is an important reason not to have "currently unused" nodes
>>> in the DT source/binary (loading time is IMHO neglectable), I would ask 
>>> that we
>>> can keep with the complete DT instead of splitting it up artificially and 
>>> getting back
>>> to the same status (because the hardware does not change over time).
>> 
> 
> I understand your motivation since it will allow you to not have to
> maintain a patch-set for your downstream DTS changes but I would like
> to ask you the opposite question. What's the benefit for the kernel
> community to keep in a mainline DTS a bunch of device nodes with DT
> bindings that have not been neither discussed nor documented?

Less patches to review individually. Just a single one instead.
But this might be a weak benefit.

> 
> Developers doing a board bring-up usually use the DTS in mainline as a
> reference for their boards and having non-documented/agreed DT binding
> on the upstream DTS will only bring confusion and frustration to them
> IMHO.

Well, my experience (at least for the current status) is that in most times new
boards need new drivers and DT nodes anyways. But this might change.

> 
> We already have some issues with Device Trees (bindings that broke
> backward compatibility, drivers implementing custom DT binding instead
> of using standard ones, bindings that don't really reflect the actual
> H/W, etc), please don't make this even more complicated to developers.

Ok, then I think we will do the way as suggested and only submit DT nodes
for already existing drivers. Or submit new ones bundled with driver and
documentation patches.

Marek will update the patches anyways, so that they apply without errors.

> 
> Thanks a lot and best regards,
> Javier

BR,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] arm: dts: omap3-gta04: Add missing nodes to fully describe gta04 board

[Dr. H. Nikolaus Schaller]

Hi Joachim,
is there some policy for only having nodes for existing drivers in DT files?

If I understand the device tree concept correctly, it should not describe 
drivers
(and hence nothing about the state of them being mainlined), but it should 
statically
describe the given hardware in a way that kernel and drivers can read it (or 
ignore).
And even other kernels can use it (because they run on the same hardware).

So unless there is an important reason not to have "currently unused" nodes
in the DT source/binary (loading time is IMHO neglectable), I would ask that we
can keep with the complete DT instead of splitting it up artificially and 
getting back
to the same status (because the hardware does not change over time).

Regarding bindings documentation I agree that it is a very necessary part of
each driver, i.e. documenting what the driver supports.

BR,
Nikolaus


Am 15.07.2014 um 14:45 schrieb Joachim Eastwood:

> Hi Marek,
> 
> You seem to add some DT nodes for hw that doesn't have drivers in
> mainline. I think you should leave those out until the driver itself
> is upstream and the bindings for it is documented.
> 
> On 14 July 2014 22:20, Marek Belisko  wrote:
>> Signed-off-by: Marek Belisko 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>> arch/arm/boot/dts/omap3-gta04.dts | 443 
>> +++---
>> 1 file changed, 412 insertions(+), 31 deletions(-)
>> 
>> diff --git a/arch/arm/boot/dts/omap3-gta04.dts 
>> b/arch/arm/boot/dts/omap3-gta04.dts
>> index 215513b..bd6a71d 100644
>> --- a/arch/arm/boot/dts/omap3-gta04.dts
>> +++ b/arch/arm/boot/dts/omap3-gta04.dts
>> @@ -12,7 +12,7 @@
>> #include "omap36xx.dtsi"
>> 
>> / {
>> -   model = "OMAP3 GTA04";
>> +   model = "Goldelico GTA04";
>>compatible = "ti,omap3-gta04", "ti,omap36xx", "ti,omap3";
>> 
>>cpus {
>> @@ -26,6 +26,11 @@
>>reg = <0x8000 0x2000>; /* 512 MB */
>>};
>> 
>> +   aliases {
>> +   display0 = &lcd;
>> +   display1 = &tv0;
>> +   };
>> +
>>gpio-keys {
>>compatible = "gpio-keys";
>> 
>> @@ -37,15 +42,78 @@
>>};
>>};
>> 
>> +   gpio-keys-wwan-wakeup {
>> +   compatible = "gpio-keys";
>> +
>> +   wwan_wakeup_button: wwan-wakeup-button {
>> +   label = "3G_WOE";
>> +   linux,code = <240>;
>> +   gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>;
>> +   gpio-key,wakeup;
>> +   };
>> +   };
>> +
>> +   hsusb2_phy: hsusb2_phy {
>> +   compatible = "usb-nop-xceiv";
>> +   reset-gpios = <&gpio6 14 GPIO_ACTIVE_LOW>; /* gpio_174 = 
>> reset for USB3322 */
>> +   };
>> +
>> +   antenna-detect {
>> +   compatible = "linux,extcon-gpio";
>> +   label = "gps_antenna";
>> +   gpios = <&gpio5 16 0>; /* gpio_144 */
>> +   debounce-delay-ms = <10>;
>> +   irq-flags = ;
>> +   state-on = "external";
>> +   state-off = "internal";
>> +   };
>> +
>>sound {
>>compatible = "ti,omap-twl4030";
>>ti,model = "gta04";
>> 
>>ti,mcbsp = <&mcbsp2>;
>>ti,codec = <&twl_audio>;
>> +
>> +   ti,mcbsp-voice = <&mcbsp4>;
>> +   };
>> +
>> +   sound_card {
>> +   compatible = "goldelico,gta04-audio";
>> +   gta04,cpu-dai = <&mcbsp2>;
>> +   };
>> +
>> +   gtm601_codec: voice_codec {
>> +   compatible = "gtm601-codec";
>> +   };
>> +
>> +   sound_voice {
>> +   compatible = "goldelico,gta04-voice";
>> +   gta04,cpu-dai = <&mcbsp4>;
>> +   gta04,codec = <>m601_codec>;
>>};
>> 
>> -   spi_lcd {
>> +   w2cbw003_codec: headset_codec {
>> +   compatible = "w2cbw003-codec";
>> +   };
>> +
>> +   sound_headset {
>> +   compatible = "goldelico,gta04-headset";
>> +   gta04,cpu-dai = <&mcbsp3>;
>> +   gta04,codec = <&w2cbw003_codec>;
>> +   };
>> +
>> +   sound_fm {
>> +   compatible = "goldelico,gta04-fm";
>> +   gta04,cpu-dai = <&mcbsp1>;
>> +   gta04,codec = <&si4721_codec>;
>> +   };
>> +
>> +   madc-hwmon {
>> +   compatible = "ti,twl4030-madc-hwmon";
>> +   };
>> +
>> +   spi_lcd: spi_lcd {
>>compatible = "spi-gpio";
>>#address-cells = <0x1>;
>>#size-cells = <0x0>;
>> @@ -75,7 +143,7 @@
>>};
>>};
>> 
>> -   battery {
>> +   madc_battery: battery {
>>compatible = "ti,twl4030-madc-battery";
>>capacity = <120>;
>>charging-calibration-data = <4200 100
>> @@ -100,6 +168,83 @@
>>   "ichg",
>>   

Re: [PATCH] net: rfkill-regulator: Add devicetree support.

[Dr. H. Nikolaus Schaller]

Am 10.02.2014 um 09:27 schrieb Johannes Berg:

> On Fri, 2014-02-07 at 20:48 +0100, Marek Belisko wrote:
> 
>> +#define RFKILL_TYPE_ALL (0)
>> +#define RFKILL_TYPE_WLAN(1)
>> +#define RFKILL_TYPE_BLUETOOTH   (2)
>> +#define RFKILL_TYPE_UWB (3)
>> +#define RFKILL_TYPE_WIMAX   (4)
>> +#define RFKILL_TYPE_WWAN(5)
>> +#define RFKILL_TYPE_GPS (6)
>> +#define RFKILL_TYPE_FM  (7)
>> +#define RFKILL_TYPE_NFC (8)
> 
> This seems like a bad idea since there's an enum elsewhere in userspace
> API already.

Yes,
you are right. It is defined in include/uapi/linux/rfkill.h

Tnx,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] misc: bmp085: Add missing platform data.

[Dr. H. Nikolaus Schaller]

Am 15.11.2013 um 14:58 schrieb Arnd Bergmann:

> On Thursday 14 November 2013, Marek Belisko wrote:
>> DT bindings contains more parameters to set so add them to platform data also
>> to have possibility to use on arch where DT isn't available yet.
>> 
>> Signed-off-by: Marek Belisko 
> 
> Can you give an example of a platform that uses this chip and cannot
> yet use DT in the mainline kernel? 

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/omap3-gta04.dts

exists but is far from being complete, because the transition to DT is really 
complex.

We still need some (private) board file for 3.13 and hope to have everything 
ready
for 3.14. But that depends on speed of acceptance of other drivers because
all DT things are still constantly moving.

So we will have to mix DT+board file for a while.

> If it's only for out-of-tree platforms, I'd prefer to leave this
> patch out of tree as well and put the burden on whoever maintains
> a non-DT platform in a private kernel.
> 
> 
>> diff --git a/include/linux/i2c/bmp085.h b/include/linux/i2c/bmp085.h
>> index b66cb98..addb972 100644
>> --- a/include/linux/i2c/bmp085.h
>> +++ b/include/linux/i2c/bmp085.h
> 
> Shouldn't this be in include/linux/platform_data? 
> 
>   Arnd

-- hns--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html