[PATCH v5 0/4] This set of patches adds regulator support for pmbus_core.c and ltc2978.c

[PATCH v5 0/4] This set of patches adds regulator support for pmbus_core.c and ltc2978.c

[atull]

From: Alan Tull <atull@opensource.altera.com>

Each output has individual on/off control.

>From PMBus_Specification_Part_II_Rev_1-3_20140318.pdf:
12.1.1. OPERATION Command Bit [7]
  Bit [7] controls whether the PMBus device output is on or off.
   If bit [7] is cleared (equals 0), then the output is off.
   If bit [7] is set (equals 1), then the output is on.

This patchset now uses "regulator: of: Provide simplified DT parsing method"
I don't see this patch in Linus's tree or for-next yet, so I applied it to my branch.

New in v5:
  clean whitespace in the DT bindings document
  add prototypes for the two new functions in pmbus_core.c
  use "regulator: of: Provide simplified DT parsing method"
   - remove #include <linux/regulator/of_regulator.h>
   - remove of_regulator_match in pmbus_driver_info
   - add of_match and regulators_node for regulator_desc macro
   - remove pmbus_regulator_parse_dt
   - remove unneeded 'else' that set init_data and of_node

Patch 1: document device tree bindings for ltc2978

Patch 2: add two helper functions for byte pmbus byte operations
  * byte write and byte read/modify/write

Patch 3: regulator support added in pmbus_core.c and pmbus.h
  * regulator_ops functions (is_enabled, enable, and disable)
  * gets regulator init data from device tree or platform data
  * registers the regulators
  * header has a macro for chip drivers to build their
    regulator_desc data

Patch 4: changes for ltc2978.c
  * Add Kconfig to enable/disable ltc2978 regulator functionality.
  * Update list of parts supported in Kconfig.
  * add regulator_desc info.
  * use same structs for all parts; set num_regulators appropriately.

Alan Tull (4):
  hwmon: ltc2978: device tree bindings documentation
  pmbus: core: add helpers for byte write and read modify write
  pmbus: add regulator support
  pmbus: ltc2978: add regulator support

 .../devicetree/bindings/hwmon/ltc2978.txt          |   41 +++++++
 drivers/hwmon/pmbus/Kconfig                        |   11 +-
 drivers/hwmon/pmbus/ltc2978.c                      |   24 ++++
 drivers/hwmon/pmbus/pmbus.h                        |   30 +++++
 drivers/hwmon/pmbus/pmbus_core.c                   |  118 ++++++++++++++++++++
 include/linux/i2c/pmbus.h                          |    4 +
 6 files changed, 226 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt

-- 
1.7.9.5

[PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add device tree bindings documentation for ltc2978.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v2: clean whitespace
---
 .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt

diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
new file mode 100644
index 0000000..b2d9c4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
@@ -0,0 +1,41 @@
+ltc2978
+
+Required properties:
+ - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
+ - reg: I2C address
+
+Optional properties:
+ Name of the optional regulator subnode must be "regulators".
+  - #address-cells must be 1.
+  - #size-cells must be 0.
+
+ For each regulator:
+  - reg: regulator number
+  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
+    valid range is:
+      ltc2977, ltc2978 : vout_en0 - vout_en7
+      ltc2974          : vout_en0 - vout_en3
+      ltc3880, ltm4676 : vout_en0 - vout_en1
+      ltc3883          : vout_en0 only
+  - regulator-name: arbitrary name for regulator
+
+Example:
+ltc2978@5e {
+	compatible = "ltc2978";
+	reg = <0x5e>;
+	regulators {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		vdacp0_reg: regulator@0 {
+			reg = <0>;
+			regulator-compatible = "vout_en0";
+			regulator-name = "FPGA-2.5V";
+		};
+		vdacp2_reg: regulator@2 {
+			reg = <2>;
+			regulator-compatible = "vout_en2";
+			regulator-name = "FPGA-1.5V";
+		};
+	};
+};
-- 
1.7.9.5

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Mark Rutland]

On Thu, Oct 02, 2014 at 07:37:48PM +0100, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add device tree bindings documentation for ltc2978.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> v2: clean whitespace
> ---
>  .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> new file mode 100644
> index 0000000..b2d9c4d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> @@ -0,0 +1,41 @@
> +ltc2978
> +
> +Required properties:
> + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676

Could we please format this like:

- compatible: should contain one of:
  * "ltc2974"
  * "ltc2977"
  * "ltc2978"
  * "ltc3880"
  * "ltc3883"
  * "ltm4676"

Given the file name, are these all variants of ltc2978? 

> + - reg: I2C address
> +
> +Optional properties:
> + Name of the optional regulator subnode must be "regulators".
> +  - #address-cells must be 1.

What's the single address cell used for?

> +  - #size-cells must be 0.
> +
> + For each regulator:
> +  - reg: regulator number

Which can be...?

What does this correspond to in the HW?

> +  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
> +    valid range is:
> +      ltc2977, ltc2978 : vout_en0 - vout_en7
> +      ltc2974          : vout_en0 - vout_en3
> +      ltc3880, ltm4676 : vout_en0 - vout_en1
> +      ltc3883          : vout_en0 only

In other bindings I believe I've seen the node name used for this.

Mark, what's the preferred scheme for identifying regulators in a
container node?

> +  - regulator-name: arbitrary name for regulator

Drop the 'aribtarary'. Either this should be the name of the regulator
on the board, or it shouldn't be necessary.

Mark.

> +
> +Example:
> +ltc2978@5e {
> +	compatible = "ltc2978";
> +	reg = <0x5e>;
> +	regulators {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		vdacp0_reg: regulator@0 {
> +			reg = <0>;
> +			regulator-compatible = "vout_en0";
> +			regulator-name = "FPGA-2.5V";
> +		};
> +		vdacp2_reg: regulator@2 {
> +			reg = <2>;
> +			regulator-compatible = "vout_en2";
> +			regulator-name = "FPGA-1.5V";
> +		};
> +	};
> +};
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Guenter Roeck]

On 10/03/2014 05:27 AM, Mark Rutland wrote:
> On Thu, Oct 02, 2014 at 07:37:48PM +0100, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add device tree bindings documentation for ltc2978.
>>
>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>> ---
>> v2: clean whitespace
>> ---
>>   .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
>> new file mode 100644
>> index 0000000..b2d9c4d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
>> @@ -0,0 +1,41 @@
>> +ltc2978
>> +
>> +Required properties:
>> + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
>
> Could we please format this like:
>
> - compatible: should contain one of:
>    * "ltc2974"
>    * "ltc2977"
>    * "ltc2978"
>    * "ltc3880"
>    * "ltc3883"
>    * "ltm4676"
>
> Given the file name, are these all variants of ltc2978?
>

All the chips supported by the ltc2978 driver. Some are variants
of the ltc2978, some have different functionality.

[ should those be "linear,ltc2974" and so on ? ]

Guenter

>> + - reg: I2C address
>> +
>> +Optional properties:
>> + Name of the optional regulator subnode must be "regulators".
>> +  - #address-cells must be 1.
>
> What's the single address cell used for?
>
>> +  - #size-cells must be 0.
>> +
>> + For each regulator:
>> +  - reg: regulator number
>
> Which can be...?
>
> What does this correspond to in the HW?
>
>> +  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
>> +    valid range is:
>> +      ltc2977, ltc2978 : vout_en0 - vout_en7
>> +      ltc2974          : vout_en0 - vout_en3
>> +      ltc3880, ltm4676 : vout_en0 - vout_en1
>> +      ltc3883          : vout_en0 only
>
> In other bindings I believe I've seen the node name used for this.
>
> Mark, what's the preferred scheme for identifying regulators in a
> container node?
>
>> +  - regulator-name: arbitrary name for regulator
>
> Drop the 'aribtarary'. Either this should be the name of the regulator
> on the board, or it shouldn't be necessary.
>
> Mark.
>
>> +
>> +Example:
>> +ltc2978@5e {
>> +	compatible = "ltc2978";
>> +	reg = <0x5e>;
>> +	regulators {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		vdacp0_reg: regulator@0 {
>> +			reg = <0>;
>> +			regulator-compatible = "vout_en0";
>> +			regulator-name = "FPGA-2.5V";
>> +		};
>> +		vdacp2_reg: regulator@2 {
>> +			reg = <2>;
>> +			regulator-compatible = "vout_en2";
>> +			regulator-name = "FPGA-1.5V";
>> +		};
>> +	};
>> +};
>> --
>> 1.7.9.5
>>
>>
>

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Mark Rutland]

On Fri, Oct 03, 2014 at 02:03:23PM +0100, Guenter Roeck wrote:
> On 10/03/2014 05:27 AM, Mark Rutland wrote:
> > On Thu, Oct 02, 2014 at 07:37:48PM +0100, atull@opensource.altera.com wrote:
> >> From: Alan Tull <atull@opensource.altera.com>
> >>
> >> Add device tree bindings documentation for ltc2978.
> >>
> >> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> >> ---
> >> v2: clean whitespace
> >> ---
> >>   .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
> >>   1 file changed, 41 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> >> new file mode 100644
> >> index 0000000..b2d9c4d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> >> @@ -0,0 +1,41 @@
> >> +ltc2978
> >> +
> >> +Required properties:
> >> + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
> >
> > Could we please format this like:
> >
> > - compatible: should contain one of:
> >    * "ltc2974"
> >    * "ltc2977"
> >    * "ltc2978"
> >    * "ltc3880"
> >    * "ltc3883"
> >    * "ltm4676"
> >
> > Given the file name, are these all variants of ltc2978?
> >
> 
> All the chips supported by the ltc2978 driver. Some are variants
> of the ltc2978, some have different functionality.

Ok.

> 
> [ should those be "linear,ltc2974" and so on ? ]

Good point; yes they should. That will need a vendor-prefixes.txt
addition.

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Guenter Roeck]

On 10/03/2014 06:05 AM, Mark Rutland wrote:
> On Fri, Oct 03, 2014 at 02:03:23PM +0100, Guenter Roeck wrote:
>> On 10/03/2014 05:27 AM, Mark Rutland wrote:
>>> On Thu, Oct 02, 2014 at 07:37:48PM +0100, atull@opensource.altera.com wrote:
>>>> From: Alan Tull <atull@opensource.altera.com>
>>>>
>>>> Add device tree bindings documentation for ltc2978.
>>>>
>>>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>>>> ---
>>>> v2: clean whitespace
>>>> ---
>>>>    .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
>>>>    1 file changed, 41 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
>>>> new file mode 100644
>>>> index 0000000..b2d9c4d
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
>>>> @@ -0,0 +1,41 @@
>>>> +ltc2978
>>>> +
>>>> +Required properties:
>>>> + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
>>>
>>> Could we please format this like:
>>>
>>> - compatible: should contain one of:
>>>     * "ltc2974"
>>>     * "ltc2977"
>>>     * "ltc2978"
>>>     * "ltc3880"
>>>     * "ltc3883"
>>>     * "ltm4676"
>>>
>>> Given the file name, are these all variants of ltc2978?
>>>
>>
>> All the chips supported by the ltc2978 driver. Some are variants
>> of the ltc2978, some have different functionality.
>
> Ok.
>
>>
>> [ should those be "linear,ltc2974" and so on ? ]
>
> Good point; yes they should. That will need a vendor-prefixes.txt
> addition.
>
Actually, turns out there already is one, only it is not immediately obvious:
lltc for "Linear Technology Corporation".

Guenter

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Guenter Roeck]

On 10/03/2014 05:27 AM, Mark Rutland wrote:
> On Thu, Oct 02, 2014 at 07:37:48PM +0100, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add device tree bindings documentation for ltc2978.
>>
>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>> ---
>> v2: clean whitespace
>> ---
>>   .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
>> new file mode 100644
>> index 0000000..b2d9c4d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
>> @@ -0,0 +1,41 @@
>> +ltc2978
>> +
>> +Required properties:
>> + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
>
> Could we please format this like:
>
> - compatible: should contain one of:
>    * "ltc2974"
>    * "ltc2977"
>    * "ltc2978"
>    * "ltc3880"
>    * "ltc3883"
>    * "ltm4676"
>
> Given the file name, are these all variants of ltc2978?
>
>> + - reg: I2C address
>> +
>> +Optional properties:
>> + Name of the optional regulator subnode must be "regulators".
>> +  - #address-cells must be 1.
>
> What's the single address cell used for?
>
I think this is required by the regulator bindings.
Other regulators use the same, though they don't typically
mention it in the properties list but only in the examples.

>> +  - #size-cells must be 0.
>> +
>> + For each regulator:
>> +  - reg: regulator number
>
> Which can be...?
>
> What does this correspond to in the HW?
>
Channel number or index. Pretty much the 0..7 from the name below.

>> +  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
>> +    valid range is:
>> +      ltc2977, ltc2978 : vout_en0 - vout_en7
>> +      ltc2974          : vout_en0 - vout_en3
>> +      ltc3880, ltm4676 : vout_en0 - vout_en1
>> +      ltc3883          : vout_en0 only
>
> In other bindings I believe I've seen the node name used for this.
>
Seems like per-device names are chosen for other regulators.
PMBus commonly uses the term "VOUT" for output voltages,
so vout[0-7] should be ok.

Which reminds me: We had earlier asked to drop the "_en" from the strings
as unnecessary.

> Mark, what's the preferred scheme for identifying regulators in a
> container node?
>
>> +  - regulator-name: arbitrary name for regulator
>
> Drop the 'aribtarary'. Either this should be the name of the regulator
> on the board, or it shouldn't be necessary.
>
Not sure if it even needs to be documented here. It is already documented
in Documentation/devicetree/bindings/regulator/regulator.txt.

> Mark.
>
>> +
>> +Example:
>> +ltc2978@5e {
>> +	compatible = "ltc2978";
>> +	reg = <0x5e>;
>> +	regulators {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		vdacp0_reg: regulator@0 {
>> +			reg = <0>;
>> +			regulator-compatible = "vout_en0";
>> +			regulator-name = "FPGA-2.5V";
>> +		};
>> +		vdacp2_reg: regulator@2 {
>> +			reg = <2>;
>> +			regulator-compatible = "vout_en2";
>> +			regulator-name = "FPGA-1.5V";
>> +		};
>> +	};
>> +};
>> --
>> 1.7.9.5
>>
>>
>

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

On Fri, Oct 03, 2014 at 10:28:59AM -0700, Guenter Roeck wrote:
> On 10/03/2014 05:27 AM, Mark Rutland wrote:

> >>+Optional properties:
> >>+ Name of the optional regulator subnode must be "regulators".
> >>+  - #address-cells must be 1.

> >What's the single address cell used for?

> I think this is required by the regulator bindings.
> Other regulators use the same, though they don't typically
> mention it in the properties list but only in the examples.

Nope, nothing to do with regulator that I'm aware of and I've not seen
any examples (but I don't tend to review users too often).

> >Mark, what's the preferred scheme for identifying regulators in a
> >container node?

> >>+  - regulator-name: arbitrary name for regulator

> >Drop the 'aribtarary'. Either this should be the name of the regulator
> >on the board, or it shouldn't be necessary.

> Not sure if it even needs to be documented here. It is already documented
> in Documentation/devicetree/bindings/regulator/regulator.txt.

Document by reference then?  It really should be the name of the supply
provided by the regulator, that wording is probably good.  Given that
it's a standard regulator property I'd not document it here at all, just
refer to the generic bindings and say any suitable properties there can
be used.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Guenter Roeck]

On 10/03/2014 04:13 PM, Mark Brown wrote:
> On Fri, Oct 03, 2014 at 10:28:59AM -0700, Guenter Roeck wrote:
>> On 10/03/2014 05:27 AM, Mark Rutland wrote:
>
>>>> +Optional properties:
>>>> + Name of the optional regulator subnode must be "regulators".
>>>> +  - #address-cells must be 1.
>
>>> What's the single address cell used for?
>
>> I think this is required by the regulator bindings.
>> Other regulators use the same, though they don't typically
>> mention it in the properties list but only in the examples.
>
> Nope, nothing to do with regulator that I'm aware of and I've not seen
> any examples (but I don't tend to review users too often).
>

Many examples, really. Just picking one.

Documentation/devicetree/bindings/mfd/tps6507x.txt

	regulators {
                 #address-cells = <1>;
                 #size-cells = <0>;
		...

This also uses the reg property for the regulator index
(without explaining it further). Seems to be pretty common.

Guenter

>>> Mark, what's the preferred scheme for identifying regulators in a
>>> container node?
>
>>>> +  - regulator-name: arbitrary name for regulator
>
>>> Drop the 'aribtarary'. Either this should be the name of the regulator
>>> on the board, or it shouldn't be necessary.
>
>> Not sure if it even needs to be documented here. It is already documented
>> in Documentation/devicetree/bindings/regulator/regulator.txt.
>
> Document by reference then?  It really should be the name of the supply
> provided by the regulator, that wording is probably good.  Given that
> it's a standard regulator property I'd not document it here at all, just
> refer to the generic bindings and say any suitable properties there can
> be used.
>

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Fri, Oct 03, 2014 at 04:23:15PM -0700, Guenter Roeck wrote:
> On 10/03/2014 04:13 PM, Mark Brown wrote:

> >Nope, nothing to do with regulator that I'm aware of and I've not seen
> >any examples (but I don't tend to review users too often).

> Many examples, really. Just picking one.

> Documentation/devicetree/bindings/mfd/tps6507x.txt

OK, that's not an actual DTS...  I'm not seeing anything in the code.

> 	regulators {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 		...

> This also uses the reg property for the regulator index
> (without explaining it further). Seems to be pretty common.

If it's doing that it's nothing to do with the regulator API.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Guenter Roeck]

On 10/04/2014 02:53 AM, Mark Brown wrote:
> On Fri, Oct 03, 2014 at 04:23:15PM -0700, Guenter Roeck wrote:
>> On 10/03/2014 04:13 PM, Mark Brown wrote:
>
>>> Nope, nothing to do with regulator that I'm aware of and I've not seen
>>> any examples (but I don't tend to review users too often).
>
>> Many examples, really. Just picking one.
>
>> Documentation/devicetree/bindings/mfd/tps6507x.txt
>
> OK, that's not an actual DTS...  I'm not seeing anything in the code.
>
>> 	regulators {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>> 		...
>
>> This also uses the reg property for the regulator index
>> (without explaining it further). Seems to be pretty common.
>
> If it's doing that it's nothing to do with the regulator API.
>
Since the code does not use those values either, presumably they don't have
to be specified in the first place. I'll test that with a real system
next week.

Guenter

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Guenter Roeck]

On Thu, Oct 02, 2014 at 01:37:48PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add device tree bindings documentation for ltc2978.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> v2: clean whitespace
> ---
>  .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> new file mode 100644
> index 0000000..b2d9c4d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> @@ -0,0 +1,41 @@
> +ltc2978
> +
> +Required properties:
> + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
> + - reg: I2C address
> +
> +Optional properties:
> + Name of the optional regulator subnode must be "regulators".

This is currently a problem. The regulator core trats it as mandatory,
meaning I get error messages such as

ltc2978 5-005e: Failed to find regulator container node

if not specified. We'll have to sort out with the regulator core how this should
be handled.

> +  - #address-cells must be 1.
> +  - #size-cells must be 0.
> +

Checking this out, those do not seem to be necessary.

> + For each regulator:
> +  - reg: regulator number

This does not seem to be necessary either.

> +  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
> +    valid range is:
> +      ltc2977, ltc2978 : vout_en0 - vout_en7
> +      ltc2974          : vout_en0 - vout_en3
> +      ltc3880, ltm4676 : vout_en0 - vout_en1
> +      ltc3883          : vout_en0 only

Besides the unnecessary _en, this is a problem if there is more than one
supported chip in the system, if DEBUG_FS is enabled, and if names are not
specified in the devicetree file. I get a lot of error messages in a
system with a large number of LTC2978 chips.

vout3: Failed to create debugfs directory
vout4: Failed to create debugfs directory
vout5: Failed to create debugfs directory
vout6: Failed to create debugfs directory
vout7: Failed to create debugfs directory
vout2: Failed to create debugfs directory
vout3: Failed to create debugfs directory

and so on (40+ times in my system). We will have to find a solution for this
problem.

I also get

vout7: no parameters

for each regulator which is a bit annoying with 50+ of those regulators
in the system.

Guenter

> +  - regulator-name: arbitrary name for regulator
> +
> +Example:
> +ltc2978@5e {
> +	compatible = "ltc2978";
> +	reg = <0x5e>;
> +	regulators {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		vdacp0_reg: regulator@0 {
> +			reg = <0>;
> +			regulator-compatible = "vout_en0";
> +			regulator-name = "FPGA-2.5V";
> +		};
> +		vdacp2_reg: regulator@2 {
> +			reg = <2>;
> +			regulator-compatible = "vout_en2";
> +			regulator-name = "FPGA-1.5V";
> +		};
> +	};
> +};
> -- 
> 1.7.9.5
> 

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[atull]

On Mon, 6 Oct 2014, Guenter Roeck wrote:

> On Thu, Oct 02, 2014 at 01:37:48PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add device tree bindings documentation for ltc2978.
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> > v2: clean whitespace
> > ---
> >  .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> > new file mode 100644
> > index 0000000..b2d9c4d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> > @@ -0,0 +1,41 @@
> > +ltc2978
> > +
> > +Required properties:
> > + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
> > + - reg: I2C address
> > +
> > +Optional properties:
> > + Name of the optional regulator subnode must be "regulators".
> 
> This is currently a problem. The regulator core trats it as mandatory,
> meaning I get error messages such as
> 
> ltc2978 5-005e: Failed to find regulator container node
> 
> if not specified. We'll have to sort out with the regulator core how this should
> be handled.
> 
> > +  - #address-cells must be 1.
> > +  - #size-cells must be 0.
> > +
> 
> Checking this out, those do not seem to be necessary.

I just tried it, and don't see a need for it myself.
	 
> 
> > + For each regulator:
> > +  - reg: regulator number
> 
> This does not seem to be necessary either.

Same here.  For some reason I thought they were required.  
I'll take them out of the bindings doc.

> 
> > +  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
> > +    valid range is:
> > +      ltc2977, ltc2978 : vout_en0 - vout_en7
> > +      ltc2974          : vout_en0 - vout_en3
> > +      ltc3880, ltm4676 : vout_en0 - vout_en1
> > +      ltc3883          : vout_en0 only
> 
> Besides the unnecessary _en,

I don't mind taking out the '_en'.  I was trying to name these
after pin names on the device.  I thought that was the norm.
If someone adds voltage support later, that will look even
weirder, so I agree that should change.

> this is a problem if there is more than one
> supported chip in the system, if DEBUG_FS is enabled, and if names are not
> specified in the devicetree file. I get a lot of error messages in a
> system with a large number of LTC2978 chips.
> 
> vout3: Failed to create debugfs directory
> vout4: Failed to create debugfs directory
> vout5: Failed to create debugfs directory
> vout6: Failed to create debugfs directory
> vout7: Failed to create debugfs directory
> vout2: Failed to create debugfs directory
> vout3: Failed to create debugfs directory
> 
> and so on (40+ times in my system). We will have to find a solution for this
> problem.

Note that whatever name is here is going to be the compatible
string for this particular regulator output in the DT.

It seems like this can't be the only case of this in the kernel.
I imagine there are lots of boards with multiple regulators but
no regulator info specified. I'll have to dig a bit to see why
this isn't an issue for other regulator drivers.

> 
> I also get
> 
> vout7: no parameters
> 
> for each regulator which is a bit annoying with 50+ of those regulators
> in the system.

Yes I see that and tried to make it stop (but couldn't).  It is not
really helpful information.

Alan

> 
> Guenter
> 
> > +  - regulator-name: arbitrary name for regulator
> > +
> > +Example:
> > +ltc2978@5e {
> > +	compatible = "ltc2978";
> > +	reg = <0x5e>;
> > +	regulators {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		vdacp0_reg: regulator@0 {
> > +			reg = <0>;
> > +			regulator-compatible = "vout_en0";
> > +			regulator-name = "FPGA-2.5V";
> > +		};
> > +		vdacp2_reg: regulator@2 {
> > +			reg = <2>;
> > +			regulator-compatible = "vout_en2";
> > +			regulator-name = "FPGA-1.5V";
> > +		};
> > +	};
> > +};
> > -- 
> > 1.7.9.5
> > 
> 

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Guenter Roeck]

On Wed, Oct 08, 2014 at 11:12:29AM -0500, atull wrote:
> On Mon, 6 Oct 2014, Guenter Roeck wrote:
> 
> > On Thu, Oct 02, 2014 at 01:37:48PM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > Add device tree bindings documentation for ltc2978.
> > > 
> > > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > > ---
> > > v2: clean whitespace
> > > ---
> > >  .../devicetree/bindings/hwmon/ltc2978.txt          |   41 ++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> > > new file mode 100644
> > > index 0000000..b2d9c4d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> > > @@ -0,0 +1,41 @@
> > > +ltc2978
> > > +
> > > +Required properties:
> > > + - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
> > > + - reg: I2C address
> > > +
> > > +Optional properties:
> > > + Name of the optional regulator subnode must be "regulators".
> > 
> > This is currently a problem. The regulator core trats it as mandatory,
> > meaning I get error messages such as
> > 
> > ltc2978 5-005e: Failed to find regulator container node
> > 
> > if not specified. We'll have to sort out with the regulator core how this should
> > be handled.
> > 
Followup on this: Since the regulator core considers the property to be
mandatory (which I guess makes some sense for a presumed regulator),
you'll have to check in the calling code and ensure that a 'regulators'
entry is there prior to calling the regulator code.

> > > +  - #address-cells must be 1.
> > > +  - #size-cells must be 0.
> > > +
> > 
> > Checking this out, those do not seem to be necessary.
> 
> I just tried it, and don't see a need for it myself.
> 	 
> > 
> > > + For each regulator:
> > > +  - reg: regulator number
> > 
> > This does not seem to be necessary either.
> 
> Same here.  For some reason I thought they were required.  
> I'll take them out of the bindings doc.
> 
> > 
> > > +  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
> > > +    valid range is:
> > > +      ltc2977, ltc2978 : vout_en0 - vout_en7
> > > +      ltc2974          : vout_en0 - vout_en3
> > > +      ltc3880, ltm4676 : vout_en0 - vout_en1
> > > +      ltc3883          : vout_en0 only
> > 
> > Besides the unnecessary _en,
> 
> I don't mind taking out the '_en'.  I was trying to name these
> after pin names on the device.  I thought that was the norm.
> If someone adds voltage support later, that will look even
> weirder, so I agree that should change.
> 
> > this is a problem if there is more than one
> > supported chip in the system, if DEBUG_FS is enabled, and if names are not
> > specified in the devicetree file. I get a lot of error messages in a
> > system with a large number of LTC2978 chips.
> > 
> > vout3: Failed to create debugfs directory
> > vout4: Failed to create debugfs directory
> > vout5: Failed to create debugfs directory
> > vout6: Failed to create debugfs directory
> > vout7: Failed to create debugfs directory
> > vout2: Failed to create debugfs directory
> > vout3: Failed to create debugfs directory
> > 
> > and so on (40+ times in my system). We will have to find a solution for this
> > problem.
> 
> Note that whatever name is here is going to be the compatible
> string for this particular regulator output in the DT.
> 
Yes, but I don't want to have to specify dummy names even for unused
regulator channels. There are lots of those in our systems (see below).

> It seems like this can't be the only case of this in the kernel.
> I imagine there are lots of boards with multiple regulators but
> no regulator info specified. I'll have to dig a bit to see why
> this isn't an issue for other regulator drivers.
> 
My wild guess is that it is quite atypical to have multiple regulators
of the same type in a system, so maybe it is as simple as no one hitting
the problem before.

Problem is that we can not add bus numbers and/or the device address
into the name either. Bus numbers can change across reboots, and
there can be multiple chips with the same i2c address on different
busses.

Example:

ltc2978-i2c-2-5c
ltc2978-i2c-5-5d
ltc2978-i2c-5-5e
ltc2978-i2c-5-5f
ltc2978-i2c-5-60
ltc2978-i2c-5-61
ltc2978-i2c-5-62
ltc2978-i2c-11-5c
ltc2978-i2c-12-5c

>From another system:

ltc2978-i2c-37-5d
ltc2978-i2c-37-5e
ltc2978-i2c-37-5f
ltc2978-i2c-37-60
ltc2978-i2c-37-61
ltc2978-i2c-37-62
ltc2978-i2c-45-5d
ltc2978-i2c-45-5e
ltc2978-i2c-45-5f
ltc2978-i2c-45-60
ltc2978-i2c-45-61
ltc2978-i2c-45-62
ltc2978-i2c-53-5d
ltc2978-i2c-53-5f
ltc2978-i2c-53-60
ltc2978-i2c-53-61
ltc2978-i2c-53-62
ltc2978-i2c-61-5d
ltc2978-i2c-61-5e
ltc2978-i2c-69-5d
ltc2978-i2c-69-5e
ltc2978-i2c-21-5c
ltc2978-i2c-22-5c

Yes, that may be a bit excessive, but it is from real systems.

> > 
> > I also get
> > 
> > vout7: no parameters
> > 
> > for each regulator which is a bit annoying with 50+ of those regulators
> > in the system.
> 
> Yes I see that and tried to make it stop (but couldn't).  It is not
> really helpful information.
> 
I think this will have to be fixed in the infrastructure. The message
should probably be a debug message.

Thanks,
Guenter

Re: [PATCH v5 1/4] hwmon: ltc2978: device tree bindings documentation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

On Wed, Oct 08, 2014 at 01:12:54PM -0700, Guenter Roeck wrote:
> On Wed, Oct 08, 2014 at 11:12:29AM -0500, atull wrote:

> > > if not specified. We'll have to sort out with the regulator core how this should
> > > be handled.

> Followup on this: Since the regulator core considers the property to be
> mandatory (which I guess makes some sense for a presumed regulator),
> you'll have to check in the calling code and ensure that a 'regulators'
> entry is there prior to calling the regulator code.

We could just lower the severity of the message, it's not mandatory at
all but rather there so that users can figure out what's going wrong if
we fail to parse.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v5 2/4] pmbus: core: add helpers for byte write and read modify write

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add two helper functions:
 * pmbus_write_byte_data  = paged byte write
 * pmbus_update_byte_data = paged byte read/modify/write

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v2: add prototypes for the two new functions
---
 drivers/hwmon/pmbus/pmbus.h      |    4 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   31 +++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..3ae79a7 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -375,6 +375,10 @@ int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg);
 int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16 word);
 int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg);
 int pmbus_write_byte(struct i2c_client *client, int page, u8 value);
+int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg,
+			  u8 value);
+int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
+			   u8 mask, u8 value);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 291d11f..d6c3701 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -253,6 +253,37 @@ int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg)
 }
 EXPORT_SYMBOL_GPL(pmbus_read_byte_data);
 
+int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
+{
+	int rv;
+
+	rv = pmbus_set_page(client, page);
+	if (rv < 0)
+		return rv;
+
+	return i2c_smbus_write_byte_data(client, reg, value);
+}
+EXPORT_SYMBOL_GPL(pmbus_write_byte_data);
+
+int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
+			   u8 mask, u8 value)
+{
+	unsigned int tmp;
+	int rv;
+
+	rv = pmbus_read_byte_data(client, page, reg);
+	if (rv < 0)
+		return rv;
+
+	tmp = (rv & ~mask) | (value & mask);
+
+	if (tmp != rv)
+		rv = pmbus_write_byte_data(client, page, reg, tmp);
+
+	return rv;
+}
+EXPORT_SYMBOL_GPL(pmbus_update_byte_data);
+
 /*
  * _pmbus_read_byte_data() is similar to pmbus_read_byte_data(), but checks if
  * a device specific mapping function exists and calls it if necessary.
-- 
1.7.9.5

[PATCH v5 3/4] pmbus: add regulator support

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add support for simple on/off control of each channel.

To add regulator support, the pmbus part driver needs to add
regulator_desc information and number of regulators to its
pmbus_driver_info struct.

regulator_desc can be declared using default macro for a
regulator (PMBUS_REGULATOR) that is in pmbus.h

The regulator_init_data can be intialized from either
platform data or the device tree.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v2: Remove '#include <linux/regulator/machine.h>'
    Only one regulator per pmbus device
    Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
    Move most code to pmbus_core.c
    fixed values for on/off

v4: rename _pmbus_regulator_enable to _pmbus_regulator_on_off
    simplify _pmbus_regulator_on_off code
    s/regulator_regulator/regulator/
    fix break when !CONFIG_REGULATOR
    remove unused #define PB_OPERATION_CONTROL_SEQ_OFF

v5: use "regulator: of: Provide simplified DT parsing method"
    remove #include <linux/regulator/of_regulator.h>
    remove of_regulator_match structure in pmbus_driver_info
    add of_match and regulators_node for regulator_desc macro
    remove pmbus_regulator_parse_dt
    remove unneeded 'else' that set init_data and of_node
---
 drivers/hwmon/pmbus/pmbus.h      |   26 ++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c |   87 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 117 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 3ae79a7..89a23ff 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -19,6 +19,8 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 
@@ -186,6 +188,11 @@
 #define PMBUS_VIRT_STATUS_VMON		(PMBUS_VIRT_BASE + 35)
 
 /*
+ * OPERATION
+ */
+#define PB_OPERATION_CONTROL_ON		(1<<7)
+
+/*
  * CAPABILITY
  */
 #define PB_CAPABILITY_SMBALERT		(1<<4)
@@ -365,8 +372,27 @@ struct pmbus_driver_info {
 	 */
 	int (*identify)(struct i2c_client *client,
 			struct pmbus_driver_info *info);
+
+	/* Regulator functionality, if supported by this chip driver. */
+	int num_regulators;
+	const struct regulator_desc *reg_desc;
 };
 
+/* Regulator ops */
+
+extern struct regulator_ops pmbus_regulator_ops;
+
+/* Macro for filling in array of struct regulator_desc */
+#define PMBUS_REGULATOR(_name, _id)				\
+	[_id] = {						\
+		.name = (_name # _id),				\
+		.id = (_id),					\
+		.of_match = of_match_ptr(_name # _id),		\
+		.regulators_node = of_match_ptr("regulators"),	\
+		.ops = &pmbus_regulator_ops,			\
+		.owner = THIS_MODULE,				\
+	}
+
 /* Function declarations */
 
 void pmbus_clear_cache(struct i2c_client *client);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index d6c3701..f2e47c7 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -29,6 +29,8 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*
@@ -1758,6 +1760,84 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 page = rdev_get_id(rdev);
+	int ret;
+
+	ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & PB_OPERATION_CONTROL_ON);
+}
+
+static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 page = rdev_get_id(rdev);
+
+	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+				      PB_OPERATION_CONTROL_ON,
+				      enable ? PB_OPERATION_CONTROL_ON : 0);
+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_on_off(rdev, 1);
+}
+
+static int pmbus_regulator_disable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_on_off(rdev, 0);
+}
+
+struct regulator_ops pmbus_regulator_ops = {
+	.enable = pmbus_regulator_enable,
+	.disable = pmbus_regulator_disable,
+	.is_enabled = pmbus_regulator_is_enabled,
+};
+EXPORT_SYMBOL_GPL(pmbus_regulator_ops);
+
+static int pmbus_regulator_register(struct pmbus_data *data)
+{
+	struct device *dev = data->dev;
+	const struct pmbus_driver_info *info = data->info;
+	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
+	struct regulator_dev *rdev;
+	int i;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		struct regulator_config config = { };
+
+		config.dev = dev;
+		config.driver_data = data;
+
+		if (pdata && pdata->reg_init_data)
+			config.init_data = &pdata->reg_init_data[i];
+
+		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(dev, "Failed to register %s regulator\n",
+				info->reg_desc[i].name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_regulator_register(struct pmbus_data *data)
+{
+	return 0;
+}
+#endif
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {
@@ -1812,8 +1892,15 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		dev_err(dev, "Failed to register hwmon device\n");
 		goto out_kfree;
 	}
+
+	ret = pmbus_regulator_register(data);
+	if (ret)
+		goto out_unregister;
+
 	return 0;
 
+out_unregister:
+	hwmon_device_unregister(data->hwmon_dev);
 out_kfree:
 	kfree(data->group.attrs);
 	return ret;
diff --git a/include/linux/i2c/pmbus.h b/include/linux/i2c/pmbus.h
index 69280db..ee3c2ab 100644
--- a/include/linux/i2c/pmbus.h
+++ b/include/linux/i2c/pmbus.h
@@ -40,6 +40,10 @@
 
 struct pmbus_platform_data {
 	u32 flags;		/* Device specific flags */
+
+	/* regulator support */
+	int num_regulators;
+	struct regulator_init_data *reg_init_data;
 };
 
 #endif /* _PMBUS_H_ */
-- 
1.7.9.5

Re: [PATCH v5 3/4] pmbus: add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

On Thu, Oct 02, 2014 at 01:37:50PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add support for simple on/off control of each channel.
> 
> To add regulator support, the pmbus part driver needs to add
> regulator_desc information and number of regulators to its
> pmbus_driver_info struct.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 3/4] pmbus: add regulator support

[Guenter Roeck]

On 10/03/2014 07:27 AM, Mark Brown wrote:
> On Thu, Oct 02, 2014 at 01:37:50PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add support for simple on/off control of each channel.
>>
>> To add regulator support, the pmbus part driver needs to add
>> regulator_desc information and number of regulators to its
>> pmbus_driver_info struct.
>
> Reviewed-by: Mark Brown <broonie@kernel.org>
>

Mark,

do you have an immutable branch or tag I could merge to get
the necessary regulator patches ?

Thanks,
Guenter

Re: [PATCH v5 3/4] pmbus: add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

On Fri, Oct 03, 2014 at 08:23:02AM -0700, Guenter Roeck wrote:

> do you have an immutable branch or tag I could merge to get
> the necessary regulator patches ?

The following changes since commit 7d1311b93e58ed55f3a31cc8f94c4b8fe988a2b9:

  Linux 3.17-rc1 (2014-08-16 10:40:26 -0600)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git tags/regulator-simple-of

for you to fetch changes up to ef126a4a8112998a9d4dcea166781331f1020f76:

  regulator: of: Add stub OF match function for !OF case (2014-09-10 16:40:07 +0100)

----------------------------------------------------------------
regulator: Simplified OF matching

Provide a simplified way of matching regulators in DT in order to save
some fiddly code in drivers.

----------------------------------------------------------------
Mark Brown (2):
      regulator: of: Provide simplified DT parsing method
      regulator: of: Add stub OF match function for !OF case

 drivers/regulator/core.c         | 10 +++++---
 drivers/regulator/internal.h     | 14 +++++++++++
 drivers/regulator/of_regulator.c | 51 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |  4 ++++
 4 files changed, 76 insertions(+), 3 deletions(-)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 3/4] pmbus: add regulator support

[atull]

On Fri, 3 Oct 2014, Mark Brown wrote:

> On Thu, Oct 02, 2014 at 01:37:50PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add support for simple on/off control of each channel.
> > 
> > To add regulator support, the pmbus part driver needs to add
> > regulator_desc information and number of regulators to its
> > pmbus_driver_info struct.
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>
>

Thanks!

Alan

[PATCH v5 4/4] pmbus: ltc2978: add regulator support

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add simple on/off regulator support for ltc2978 and
other pmbus parts supported by ltc2978.c

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v2: Remove '#include <linux/regulator/machine.h>'
    Only one regulator per pmbus device
    Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
    Move most code to pmbus_core.c
    fixed values for on/off

v4: fix a #endif comment
    simplify probe code, remove added switch statement
    remove BUG_ON(), add error message and fix num_regulators

v5: Kconfig: update list of supported chips
    use "regulator: of: Provide simplified DT parsing method"
    remove #include <linux/regulator/of_regulator.h>
    remove of_regulator_match
---
 drivers/hwmon/pmbus/Kconfig   |   11 +++++++++--
 drivers/hwmon/pmbus/ltc2978.c |   24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 6e1e493..a674cd8 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -47,15 +47,22 @@ config SENSORS_LM25066
 	  be called lm25066.
 
 config SENSORS_LTC2978
-	tristate "Linear Technologies LTC2974, LTC2978, LTC3880, and LTC3883"
+	tristate "Linear Technologies LTC2978 and compatibles"
 	default n
 	help
 	  If you say yes here you get hardware monitoring support for Linear
-	  Technology LTC2974, LTC2978, LTC3880, and LTC3883.
+	  Technology LTC2974, LTC2977, LTC2978, LTC3880, LTC3883, and LTM4676.
 
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2978.
 
+config SENSORS_LTC2978_REGULATOR
+	boolean "Regulator support for LTC2978 and compatibles"
+	depends on SENSORS_LTC2978 && REGULATOR
+	help
+	  If you say yes here you get regulator support for Linear
+	  Technology LTC2974, LTC2977, LTC2978, LTC3880, LTC3883, and LTM4676.
+
 config SENSORS_MAX16064
 	tristate "Maxim MAX16064"
 	default n
diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index e24ed52..efac4bf 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -22,6 +22,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/regulator/driver.h>
 #include "pmbus.h"
 
 enum chips { ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676 };
@@ -374,6 +375,19 @@ static const struct i2c_device_id ltc2978_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+static const struct regulator_desc ltc2978_reg_desc[] = {
+	PMBUS_REGULATOR("vout_en", 0),
+	PMBUS_REGULATOR("vout_en", 1),
+	PMBUS_REGULATOR("vout_en", 2),
+	PMBUS_REGULATOR("vout_en", 3),
+	PMBUS_REGULATOR("vout_en", 4),
+	PMBUS_REGULATOR("vout_en", 5),
+	PMBUS_REGULATOR("vout_en", 6),
+	PMBUS_REGULATOR("vout_en", 7),
+};
+#endif /* CONFIG_SENSORS_LTC2978_REGULATOR */
+
 static int ltc2978_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -487,6 +501,16 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->num_regulators = info->pages;
+	info->reg_desc = ltc2978_reg_desc;
+	if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
+		dev_err(&client->dev, "num_regulators too large!");
+		info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
+	}
+#endif
+
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
1.7.9.5

Re: [PATCH v5 4/4] pmbus: ltc2978: add regulator support

[Guenter Roeck]

On 10/02/2014 11:37 AM, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> Add simple on/off regulator support for ltc2978 and
> other pmbus parts supported by ltc2978.c
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> v2: Remove '#include <linux/regulator/machine.h>'
>      Only one regulator per pmbus device
>      Get regulator_init_data from pdata or device tree
>
> v3: Support multiple regulators for each chip
>      Move most code to pmbus_core.c
>      fixed values for on/off
>
> v4: fix a #endif comment
>      simplify probe code, remove added switch statement
>      remove BUG_ON(), add error message and fix num_regulators
>
> v5: Kconfig: update list of supported chips
>      use "regulator: of: Provide simplified DT parsing method"
>      remove #include <linux/regulator/of_regulator.h>
>      remove of_regulator_match
> ---
>   drivers/hwmon/pmbus/Kconfig   |   11 +++++++++--
>   drivers/hwmon/pmbus/ltc2978.c |   24 ++++++++++++++++++++++++
>   2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 6e1e493..a674cd8 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -47,15 +47,22 @@ config SENSORS_LM25066
>   	  be called lm25066.
>
>   config SENSORS_LTC2978
> -	tristate "Linear Technologies LTC2974, LTC2978, LTC3880, and LTC3883"
> +	tristate "Linear Technologies LTC2978 and compatibles"
>   	default n
>   	help
>   	  If you say yes here you get hardware monitoring support for Linear
> -	  Technology LTC2974, LTC2978, LTC3880, and LTC3883.
> +	  Technology LTC2974, LTC2977, LTC2978, LTC3880, LTC3883, and LTM4676.
>
>   	  This driver can also be built as a module. If so, the module will
>   	  be called ltc2978.
>
> +config SENSORS_LTC2978_REGULATOR
> +	boolean "Regulator support for LTC2978 and compatibles"
> +	depends on SENSORS_LTC2978 && REGULATOR
> +	help
> +	  If you say yes here you get regulator support for Linear
> +	  Technology LTC2974, LTC2977, LTC2978, LTC3880, LTC3883, and LTM4676.
> +
>   config SENSORS_MAX16064
>   	tristate "Maxim MAX16064"
>   	default n
> diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> index e24ed52..efac4bf 100644
> --- a/drivers/hwmon/pmbus/ltc2978.c
> +++ b/drivers/hwmon/pmbus/ltc2978.c
> @@ -22,6 +22,7 @@
>   #include <linux/err.h>
>   #include <linux/slab.h>
>   #include <linux/i2c.h>
> +#include <linux/regulator/driver.h>
>   #include "pmbus.h"
>
>   enum chips { ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676 };
> @@ -374,6 +375,19 @@ static const struct i2c_device_id ltc2978_id[] = {
>   };
>   MODULE_DEVICE_TABLE(i2c, ltc2978_id);
>
> +#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> +static const struct regulator_desc ltc2978_reg_desc[] = {
> +	PMBUS_REGULATOR("vout_en", 0),
> +	PMBUS_REGULATOR("vout_en", 1),
> +	PMBUS_REGULATOR("vout_en", 2),
> +	PMBUS_REGULATOR("vout_en", 3),
> +	PMBUS_REGULATOR("vout_en", 4),
> +	PMBUS_REGULATOR("vout_en", 5),
> +	PMBUS_REGULATOR("vout_en", 6),
> +	PMBUS_REGULATOR("vout_en", 7),
> +};
> +#endif /* CONFIG_SENSORS_LTC2978_REGULATOR */
> +
>   static int ltc2978_probe(struct i2c_client *client,
>   			 const struct i2c_device_id *id)
>   {
> @@ -487,6 +501,16 @@ static int ltc2978_probe(struct i2c_client *client,
>   	default:
>   		return -ENODEV;
>   	}
> +
> +#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> +	info->num_regulators = info->pages;
> +	info->reg_desc = ltc2978_reg_desc;
> +	if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
> +		dev_err(&client->dev, "num_regulators too large!");
> +		info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
> +	}
> +#endif
> +
>   	return pmbus_do_probe(client, id, info);
>   }
>
>

Unfortunately the code now only compiles in -next, since the following
fields are only defined there.

drivers/hwmon/pmbus/ltc2978.c:380:2: error: unknown field 'of_match' specified in initializer
drivers/hwmon/pmbus/ltc2978.c:380:2: error: unknown field 'regulators_node' specified in initializer

This means I can not apply the code unless I pull in the relevant changes.
Mark, do you have an immutable branch / tag for me to merge ?

If not this patch series will have to wait until the relevant regulator
changes are available in mainline.

On a side note, I am not too happy with the following comment in the
"Provide simplified DT parsing method" patch: "The current code leaks
the phandles for the child nodes, this will be addressed incrementally
and makes no practical difference for FDT anyway as the DT data
structures are never freed".

My use case includes instantiating the LTC chips through devicetree overlays,
which obviously does not work well with leaking DT nodes.

Thanks,
Guenter