[linux,dev-4.10,v5,1/4] dt-bindings: i2c: Document the IBM CCF power supply version 1

Message ID 1502474205-1557-2-git-send-email-eajames@linux.vnet.ibm.com
State Accepted, archived
Headers show

Commit Message

Eddie James Aug. 11, 2017, 5:56 p.m.
From: "Edward A. James" <eajames@us.ibm.com>

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 .../devicetree/bindings/i2c/ibm,cffps1.txt          | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/ibm,cffps1.txt

Comments

Andrew Jeffery Aug. 14, 2017, 7:33 a.m. | #1
On Fri, 2017-08-11 at 12:56 -0500, Eddie James wrote:
> > From: "Edward A. James" <eajames@us.ibm.com>

> > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  .../devicetree/bindings/i2c/ibm,cffps1.txt          | 21
+++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644
Documentation/devicetree/bindings/i2c/ibm,cffps1.txt

> diff --git a/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
> new file mode 100644
> index 0000000..f68a0a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
> @@ -0,0 +1,21 @@
> +Device-tree bindings for IBM Common Form Factor Power Supply Version
1
> +------------------------------------------------------------------
----
> +
> +Required properties:
> + - compatible = "ibm,cffps1";
> > + - reg = < I2C bus address >;		: Address of the
power supply on the
> > +					  I2C bus.

Should we define properties for these registers?

* OPERATION
* ON_OFF_CONFIG
* SYS_CONFIG

Cheers,

Andrew

> +
> +Example:
> +
> > +    i2c-bus@100 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        #interrupt-cells = <1>;
> +        < more properties >
> +
> > +        power-supply@68 {
> +            compatible = "ibm,cffps1";
> +            reg = <0x68>;
> +        };
> +    };
Eddie James Aug. 14, 2017, 3:09 p.m. | #2
On 08/14/2017 02:33 AM, Andrew Jeffery wrote:
> On Fri, 2017-08-11 at 12:56 -0500, Eddie James wrote:
>>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   .../devicetree/bindings/i2c/ibm,cffps1.txt          | 21
> +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>   create mode 100644
> Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
>>   
>> diff --git a/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
> b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
>> new file mode 100644
>> index 0000000..f68a0a6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
>> @@ -0,0 +1,21 @@
>> +Device-tree bindings for IBM Common Form Factor Power Supply Version
> 1
>> +------------------------------------------------------------------
> ----
>> +
>> +Required properties:
>> + - compatible = "ibm,cffps1";
>>> + - reg = < I2C bus address >;		: Address of the
> power supply on the
>>> +					  I2C bus.
> Should we define properties for these registers?
>
> * OPERATION
> * ON_OFF_CONFIG
> * SYS_CONFIG

We could I suppose. I don't see why though; is it normal to document 
specific registers on devices? And why just those ones? The driver 
doesn't use them. Presumably someone who wants to use them can look at 
the spec.

Thanks for the review!
Eddie

>
> Cheers,
>
> Andrew
>
>> +
>> +Example:
>> +
>>> +    i2c-bus@100 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        #interrupt-cells = <1>;
>> +        < more properties >
>> +
>>> +        power-supply@68 {
>> +            compatible = "ibm,cffps1";
>> +            reg = <0x68>;
>> +        };
>> +    };
Andrew Jeffery Aug. 16, 2017, 5:26 a.m. | #3
On Mon, 2017-08-14 at 10:09 -0500, Eddie James wrote:

> On 08/14/2017 02:33 AM, Andrew Jeffery wrote:
> > On Fri, 2017-08-11 at 12:56 -0500, Eddie James wrote:
> > > From: "Edward A. James" <eajames@us.ibm.com>
> > > 
> > >   
> > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > 
> > > ---
> > >   .../devicetree/bindings/i2c/ibm,cffps1.txt          | 21
> > 
> > +++++++++++++++++++++
> > >   1 file changed, 21 insertions(+)
> > >   create mode 100644
> > 
> > Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
> > >   
> > > diff --git a/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
> > 
> > b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
> > > new file mode 100644
> > > index 0000000..f68a0a6
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
> > > @@ -0,0 +1,21 @@
> > > +Device-tree bindings for IBM Common Form Factor Power Supply Version
> > 
> > 1
> > > +------------------------------------------------------------------
> > 
> > ----
> > > +
> > > +Required properties:
> > > + - compatible = "ibm,cffps1";
> > > + - reg = < I2C bus address >;		: Address of the
> > 
> > power supply on the
> > > +					  I2C bus.
> > 
> > Should we define properties for these registers?
> > 
> > * OPERATION
> > * ON_OFF_CONFIG
> > * SYS_CONFIG

> We could I suppose. I don't see why though; is it normal to document 
> specific registers on devices?

It's not normal to document specific registers, no. However, it is normal to
expose the device's configuration fields as properties. Reading back I chose my
words poorly, but my query was about the properties of the device that can be
configured through these registers, not really about the registers themselves.

> And why just those ones?

Because from a quick scan of the datasheet, these seem to be the remaining
registers that contained fields that could be configured.

> The driver doesn't use them. Presumably someone who wants to use them can
> look at the spec.

Upstream's stance is that the bindings describe the hardware, and the
strong preferences is that the description is complete at the time of
submission.

Though maybe if there are optional properties it's not so significant.

Cheers,

Andrew

> Thanks for the review!
> Eddie

> > 
> > Cheers,
> > 
> > Andrew
> > 
> > > +
> > > +Example:
> > > +
> > > > +    i2c-bus@100 {
> > > 
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +        #interrupt-cells = <1>;
> > > +        < more properties >
> > > +
> > > > +        power-supply@68 {
> > > 
> > > +            compatible = "ibm,cffps1";
> > > +            reg = <0x68>;
> > > +        };
> > > +    };

>

Patch

diff --git a/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
new file mode 100644
index 0000000..f68a0a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/ibm,cffps1.txt
@@ -0,0 +1,21 @@ 
+Device-tree bindings for IBM Common Form Factor Power Supply Version 1
+----------------------------------------------------------------------
+
+Required properties:
+ - compatible = "ibm,cffps1";
+ - reg = < I2C bus address >;		: Address of the power supply on the
+					  I2C bus.
+
+Example:
+
+    i2c-bus@100 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        #interrupt-cells = <1>;
+        < more properties >
+
+        power-supply@68 {
+            compatible = "ibm,cffps1";
+            reg = <0x68>;
+        };
+    };