Antwort: [PATCH v2 13/39] acpi: Add a binding for ACPI settings in the device tree

[Wolfgang Wallner <wolfgang.wallner@br-automation.com>]

Hi Simon,

-----"Simon Glass" <sjg@chromium.org> schrieb: -----
> 
> Devices need to report various identifiers in the ACPI tables. Rather than
> hard-coding these in drivers it is typically better to put them in the
> device tree.
> 
> Add a binding file to describe this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Fix definition of HID
> - Infer hid-over-i2c CID value
> - Add the hid-over-i2c binding document
> 
>  doc/device-tree-bindings/device.txt           | 36 +++++++++++++++
>  .../input/hid-over-i2c.txt                    | 44 +++++++++++++++++++
>  2 files changed, 80 insertions(+)
>  create mode 100644 doc/device-tree-bindings/device.txt
>  create mode 100644 doc/device-tree-bindings/input/hid-over-i2c.txt
> 
> diff --git a/doc/device-tree-bindings/device.txt b/doc/device-tree-bindings/device.txt
> new file mode 100644
> index 0000000000..31ec2fa31b
> --- /dev/null
> +++ b/doc/device-tree-bindings/device.txt
> @@ -0,0 +1,36 @@
> +Devices
> +=======
> +
> +Device bindings are described by their own individual binding files.
> +
> +U-Boot provides for some optional properties which are documented here. See
> +also hid-over-i2c.txt which describes HID devices.
> +
> + - acpi,has-power-resource : (boolean) true if this device has a power resource.
> +    This causes a PRIC (ACPI PowerResource) to be written containing the

What is the meaning of PRIC? I can't find a defition for this term.

> +    properties provided by this binding, to describe how to handle powering the
> +    device up and down using GPIOs
> + - acpi,compatible : compatible string to report

What does "compatible string" mean in this context? Does it refer to the 
"Compatible ID" (_CID) of ACPI? As stated in the previous mail thread [1],
I think we could infer many of the ACPI properties from existing device tree
properties. Especially I suspect that ACPI's _CID could have a 1:1 mapping
to the compatible property in device tree. E.g. a driver which states that
it is compatible with "hid-over-i2c" in its device tree description would
know (implement internally) that it is also compatible with ACPI's _CID
"PNP0C50", thus we would not have to add this information in the device tree.

[1] https://lists.denx.de/pipermail/u-boot/2020-February/398856.html

> + - acpi,desc : Contains the string to use as the _DDN (DOS (Disk Operating
> +    System) Device Name)

Nit: I assume "desc" stands for "description". But strictly speaking it is
not a description, it is a device name. I would prefer something like
"acpi,devname" or even "acpi,ddn".

> + - acpi,hid : Contains the string to use as the HID (Hardware ID)
> +    identifier _HID
> + - hid-descr-addr : HID register offset (for Human Interface Devices)

This property is already described in the file hid-over-i2c.txt (with a similar
but slightly different text), thus I would not describe it here also.

> + - acpi,probed : Tells U-Boot to add 'linux,probed' to the ACPI tables so that
> +    Linux will not re-init the device

I can't find 'linux,probed' in the Linux kernel.
I only find it in patches specific for chromium-os[2], but the description
there does not match the description given here (there it is described as
being probed before insertion vs. here it is described as not being probed
any more).

[2] https://groups.google.com/a/chromium.org/forum/#!msg/chromium-os-reviews/4HTHl78IGHw/oz82uImnBgAJ

> + - acpi,uid : _UID value for device
> +
> +
> +Example
> +-------
> +
> +synaptics_touchpad: synaptics-touchpad at 2c {
> +	compatible = "hid-over-i2c";
> +	reg = <0x2c>;
> +	acpi,hid = "PNP0C50";
> +	acpi,desc = "Synaptics Touchpad";
> +	interrupts-extended = <&acpi_gpe GPIO_18_IRQ
> +			IRQ_TYPE_EDGE_FALLING>;
> +	acpi,probed;
> +	hid-descr-addr = <0x20>;
> +};
> diff --git a/doc/device-tree-bindings/input/hid-over-i2c.txt b/doc/device-tree-bindings/input/hid-over-i2c.txt
> new file mode 100644

Adding the file hid-over-i2c.txt is straight forward, while getting the ACPI
bindings correct is probably more tricky. Would it make sense to add this file
in an individual patch?

> index 0000000000..c76bafaf98
> --- /dev/null
> +++ b/doc/device-tree-bindings/input/hid-over-i2c.txt

This file is taken from the Linux kernel, should this be stated at the
beginning? (I often see "Taken from ..." in source code files, I'm not sure
how this is handled for documentation).

> @@ -0,0 +1,44 @@
> +* HID over I2C Device-Tree bindings
> +
> +HID over I2C provides support for various Human Interface Devices over the
> +I2C bus. These devices can be for example touchpads, keyboards, touch screens
> +or sensors.
> +
> +The specification has been written by Microsoft and is currently available here:
> +http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
> +
> +If this binding is used, the kernel module i2c-hid will handle the communication
> +with the device and the generic hid core layer will handle the protocol.

This sentence is specific to the Linux kernel, and does not really apply to
U-Boot. I would propose one of the following:

* Point out specifically that the Linux kernel is talked about
* Drop the sentence
* State at the beginning of the file that the file is taken unmodified from the
  Linux kernel

> +
> +Required properties:
> +- compatible: must be "hid-over-i2c"
> +- reg: i2c slave address
> +- hid-descr-addr: HID descriptor address
> +- interrupts: interrupt line
> +
> +Additional optional properties:
> +
> +Some devices may support additional optional properties to help with, e.g.,
> +power sequencing. The following properties can be supported by one or more
> +device-specific compatible properties, which should be used in addition to the
> +"hid-over-i2c" string.
> +
> +- compatible:
> +  * "wacom,w9013" (Wacom W9013 digitizer). Supports:
> +    - vdd-supply (3.3V)
> +    - vddl-supply (1.8V)
> +    - post-power-on-delay-ms
> +
> +- vdd-supply: phandle of the regulator that provides the supply voltage.
> +- post-power-on-delay-ms: time required by the device after enabling its regulators
> +  or powering it on, before it is ready for communication.
> +
> +Example:
> +
> +	i2c-hid-dev at 2c {
> +		compatible = "hid-over-i2c";
> +		reg = <0x2c>;
> +		hid-descr-addr = <0x0020>;
> +		interrupt-parent = <&gpx3>;
> +		interrupts = <3 2>;
> +	};
> -- 
> 2.25.1.481.gfbce0eb801-goog
> 

regards, Wolfgang