Em Fri, 5 Feb 2021 16:19:47 -0600
Rob Herring [off-list ref] escreveu:

On Tue, Jan 19, 2021 at 05:10:43PM +0100, Mauro Carvalho Chehab wrote:

quoted

The Hisilicon 6421v600 SPMI driver is ready for mainstream.

So, move it from staging.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../spmi/hisilicon,hisi-spmi-controller.yaml  |  75 ++++
 MAINTAINERS                                   |   7 +
 drivers/spmi/Kconfig                          |   9 +
 drivers/spmi/Makefile                         |   1 +
 drivers/spmi/hisi-spmi-controller.c           | 358 ++++++++++++++++++
 drivers/staging/hikey9xx/Kconfig              |  11 -
 drivers/staging/hikey9xx/Makefile             |   1 -
 .../staging/hikey9xx/hisi-spmi-controller.c   | 358 ------------------
 .../hisilicon,hisi-spmi-controller.yaml       |  75 ----
 9 files changed, 450 insertions(+), 445 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
 create mode 100644 drivers/spmi/hisi-spmi-controller.c
 delete mode 100644 drivers/staging/hikey9xx/hisi-spmi-controller.c
 delete mode 100644 drivers/staging/hikey9xx/hisilicon,hisi-spmi-controller.yaml

diff --git a/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
new file mode 100644
index 000000000000..21f68a9c2df1
--- /dev/null
+++ b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml

@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spmi/hisilicon,hisi-spmi-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon SPMI controller
+
+maintainers:
+  - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
+
+description: |
+  The HiSilicon SPMI BUS controller is found on some Kirin-based designs.
+  It is a MIPI System Power Management (SPMI) controller.
+
+  The PMIC part is provided by
+  drivers/staging/hikey9xx/hisilicon,hi6421-spmi-pmic.yaml.
+
+properties:
+  $nodename:
+    pattern: "spmi@[0-9a-f]"
+
+  compatible:
+    const: hisilicon,kirin970-spmi-controller  

'-controller' is kind of redundant.

Ok. Will drop it.

quoted

+
+  reg:
+    maxItems: 1
+  

quoted

+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 0  

These 2 are covered by spmi.yaml

Ok.

quoted

+
+  spmi-channel:
+    description: |
+      number of the Kirin 970 SPMI channel where the SPMI devices are connected.  

Common to SPMI? If not, needs a vendor prefix.

That's an interesting question. My understanding is that this is not
vendor-specific, but maybe Stephen can give us more details.

The spmi.h header calls it "nr", and documents it at include/linux/spmi.h
as:

	/**
	 * struct spmi_controller - interface to the SPMI master controller
	 * @dev:	Driver model representation of the device.
	 * @nr:		board-specific number identifier for this controller/bus
	 * @cmd:	sends a non-data command sequence on the SPMI bus.
	 * @read_cmd:	sends a register read command sequence on the SPMI bus.
	 * @write_cmd:	sends a register write command sequence on the SPMI bus.
	 */

There, it says that this is "board-specific number identifier".

Yet, as the SPMI is a serial bus with up to 4 masters (controller), I 
suspect that the idea is to associate it with the master ID.

This is used on boards with multiple SoCs. See, for instance, slide 5 of:

	https://www.mipi.org/sites/default/files/Bangalore-Qualcomm-SPMI-1.0-Multi-master-Verification.pdf

However, it is hard to know for sure, as no drivers use it, except by
Hikey 970 controller:

	$ grep "\b\->nr\b" $(git grep -l spmi.h)
	drivers/spmi/spmi.c:	ida_simple_remove(&ctrl_ida, ctrl->nr);
	drivers/spmi/spmi.c:	dev_set_name(&sdev->dev, "%d-%02x", ctrl->nr, sdev->usid);
	drivers/spmi/spmi.c:	ctrl->nr = id;
	drivers/spmi/spmi.c:		ctrl->nr, &ctrl->dev);
	drivers/staging/hikey9xx/hisi-spmi-controller.c:	ctrl->nr = spmi_controller->channel;

Type? Range of values?

The SPMI core defines it as "unsigned int". So, I would use:

	    $ref: /schemas/types.yaml#/definitions/uint32

as a type. 

At the driver, this is used to calculate the channel offset with:

	static int spmi_write_cmd(...) {
		u32 chnl_ofst = SPMI_CHANNEL_OFFSET * spmi_controller->channel;
...
		writel((u32 __force)cpu_to_be32(data),
		       spmi_controller->base + chnl_ofst +
		       SPMI_APB_SPMI_WDATA0_BASE_ADDR +
		       SPMI_PER_DATAREG_BYTE * i);
...
	}

As on both spmi.h and the Hikey 970 SPMI controller defines it as uint32, 
it doesn't seem to be a good idea to put a range of values, specially 
since we don't have the datasheets for this SoC.

quoted

+
+required:
+  - compatible
+  - reg
+  - spmi-channel  

quoted

+  - "#address-cells"
+  - "#size-cells"  

Covered by spmi.yaml.

quoted

+
+patternProperties:
+  "^pmic@[0-9a-f]$":  

Presumably you could have something besides a PMIC.

Hmm... SPMI means MIPI System Power Management Interface.
The MIPI says that [1]:

	"The MIPI System Power Management Interface is a two-wire serial
	 interface that uses CMOS I/Os for the physical layer. The interface
	 connects the integrated power controller of a system-on-chip (SoC)
	 processor system with one or more power management IC voltage
	 regulation systems."

[1] https://www.mipi.org/specifications/system-power-management-interface

OK, as this is a serial bus, I guess one could abuse the interface
and add non-PMIC devices on it. Also, some future version of SPMI
might extend it to non-PMIC devices, but, IMO, if we ever add a 
non-PMIC device, another patternProperties would be needed in order
to describe the other device types that could be connected to the PM bus.

quoted

+    description: |
+      PMIC properties, which are specific to the used SPMI PMIC device(s).
+      When used in combination with HiSilicon 6421v600, the properties
+      are documented at
+      drivers/staging/hikey9xx/hisilicon,hi6421-spmi-pmic.yaml.
+
+additionalProperties: false
+
+examples:
+  - |
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      spmi: spmi@fff24000 {
+        compatible = "hisilicon,kirin970-spmi-controller";
+        #address-cells = <2>;
+        #size-cells = <0>;
+        status = "ok";  

Drop status.

Ok.

quoted

+        reg = <0x0 0xfff24000 0x0 0x1000>;
+        spmi-channel = <2>;
+
+        pmic@0 {
+          reg = <0 0>;
+          /* pmic properties */
+        };
+      };
+    };

Thanks,
Mauro