Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties

Em Tue, 18 Aug 2020 11:07:55 -0600
Rob Herring [off-list ref] escreveu:

quoted

quoted

quoted

+  spmi-channel:
+    description: number of the SPMI channel where the PMIC is connected    

This looks like a common (to SPMI), but it's not something defined in 
spmi.txt   

This one is not part of the SPMI core. It is stored inside a private 
structure inside at the HiSilicon spmi controller driver. It is stored 
there as ctrl_dev->channel, and it is used to calculate the register offset
for readl():

	offset  = SPMI_APB_SPMI_STATUS_BASE_ADDR;
	offset += SPMI_CHANNEL_OFFSET * ctrl_dev->channel + SPMI_SLAVE_OFFSET * sid;
	do {
		status = readl(base + offset);
	...

The SPMI bus is somewhat similar to I2C: it is a 2-wire serial bus
with up to 16 devices connected to it.

Now, most modern I2C chipsets provide multiple independent I2C
channels, on different pins. Also, on some chipsets, certain
GPIO pins can be used either as GPIO or as I2C.

I strongly suspect that this is the case here: according with
the Hikey 970 schematics:

	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

The pins used by SPMI clock/data can also be used as GPIO.

While I don't have access to the datasheets for Kirin 970 (or any other
chipsets on this board), for me, it sounds that different GPIO pins
are allowed to use SPMI. The "spmi-channel" property specifies
what pins will be used for SPMI, among the ones that are
compatible with MIPI SPMI specs.  

Based on this, I think it should be called 'hisilicon,spmi-channel' as 
it is vendor specific. 

I'm fine with "hisilicon,spmi-channel".

quoted

quoted

quoted

+
+          vsel-reg:
+            description: Voltage selector register.    

'reg' can have multiple entries if you want.  

Yes, I know. I was in doubt if I should either place vsel-reg on
a separate property or together with reg. I ended keeping it
in separate on the submitted patch series.

What makes more sense?  

Really, not putting it in DT. Like other things, it's fixed for the 
chip.

I agree, but, as I said before, without the datasheet, we can only
hardcode a small subset of the LDO settings.

Due to that, I prefer keeping it at DT - either grouped together at "reg" or 
as two separated properties (reg and vsel-reg).

quoted

quoted

quoted

+description: |
+  The HiSilicon SPMI controller is found on some Kirin-based designs.
+  It is a MIPI System Power Management (SPMI) controller.
+
+  The PMIC part is provided by
+  Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml.
+
+properties:
+  $nodename:
+    pattern: "spmi@[0-9a-f]"
+
+  compatible:
+    const: hisilicon,spmi-controller    

Needs an SoC specific compatible.  

What about:
	hisilicon,kirin970-spmi-controller   

Is 'kirin970' really the SoC name? The older ones are all 'hi[0-9]+'.

This SoC is named Kirin 970. Yet, you can see places where 3670 is
used, like:

	https://en.wikichip.org/wiki/hisilicon/kirin/970

There, it says that Hi3670 is the part number.

Thanks,
Mauro