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

[Mauro Carvalho Chehab]

Em Tue, 18 Aug 2020 11:07:55 -0600
Rob Herring <robh@xxxxxxxxxx> escreveu:

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

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

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