[1/2] dt-bindings: regulator: Add pf8x00 regulator

Add dt-bindings for the pf8x00 driver.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 .../regulator/nxp,pf8x00-regulator.yaml       | 223 ++++++++++++++++++
 MAINTAINERS                                   |   6 +
 2 files changed, 229 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml

On Sun, Dec 06, 2020 at 01:26:28AM +0100, Adrien Grassein wrote:
> Add dt-bindings for the pf8x00 driver.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

> +  compatible:
> +    enum:
> +      - nxp,pf8x00

Compatible strings should be for specific devices not wildcards.

> +          nxp,hw-en:
> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              Only available for ldo2. Used to enable or disable ld02.

I don't understand what this is documenting - what is "hw-en" and how is
it used to enable or disable LDO2?

> +          nxp,vselect-en:
> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              Only available for ldo2. When specified, use the VSELECT pin
> +              of the chip to control the output voltage of the ldo02 regulator.

Shouldn't there be a GPIO specified somewhere or something so that the
VSELECT pin can be controlled?  

> +          nxp,ilim-ma:
> +            $ref: /schemas/types.yaml#definitions/uint32
> +            minimum: 2100
> +            maximum: 4500
> +            default: 2100
> +            enum: [ 2100, 2600, 3000, 4500 ]
> +            description: |
> +              Defines the maximum current delivered by the regulator (in mA).

Is this not a fixed property of the regulator?

> +          nxp,quad-phase:
> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              This allow regulators  sw1 and sw2, or sw3 and sw4 or sw4 and sw5
> +              to work together to deliver a maximum 10A current.

Presumably this must be set on both the regulators being grouped
together?

On Sun, Dec 06, 2020 at 01:26:29AM +0100, Adrien Grassein wrote:

> +/* sw1 to sw6 voltages, from 0.4V to 1.49375V with 0.006250V increments */
> +#define SWV(i)	(6250 * i + 400000)
> +
> +static const int pf8x00_sw1_to_6_voltages[] = {
> +	SWV(0), SWV(1), SWV(2), SWV(3), SWV(4), SWV(5), SWV(6), SWV(7),

There are helpers for linear mappings, please use them directly rather
than open coding - this is clearer and much more efficient at runtime.

> +static const struct i2c_device_id pf8x_device_id[] = {
> +	{.name = "pf8x00",},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, pf8x_device_id);

There should be separate IDs for the individual parts as with the OF
compatible.

> +static int pf8x00_regulator_set_voltage_time_sel(struct regulator_dev *rdev,
> +		unsigned int old_sel, unsigned int new_sel)
> +{
> +	struct pf8x_chip *pf = rdev_get_drvdata(rdev);
> +	struct pf8x_regulator *rdesc = container_of(rdev->desc, struct pf8x_regulator, desc);
> +	const unsigned int *volt_table = rdev->desc->volt_table;
> +	int old_v = volt_table[old_sel];
> +	int new_v = volt_table[new_sel];
> +	int change = (new_v - old_v);
> +	unsigned int index;
> +	unsigned int slew;
> +
> +	index = (rdesc->fast_slew & 1) ? 2 : 0;

Please write normal conditional statements to make things easier to
read.

> +	if ((desc->id != REG_SW1) && quad_phase) {
> +		dev_err(pf->dev, "ignoring, only sw1 can use quad-phase\n");
> +		quad_phase = 0;
> +	}
> +	if ((desc->id != REG_SW1) && (desc->id != REG_SW4)
> +			 && (desc->id != REG_SW5) && dual_phase) {
> +		dev_err(pf->dev, "ignoring, only sw1/sw4/sw5 can use dual-phase\n");
> +		dual_phase = 0;
> +	}

This wasn't in the binding document...

> +static int pf8x_parse_regulators_dt(struct pf8x_chip *pf)
> +{
> +	struct device *dev = pf->dev;
> +	struct device_node *np, *parent;
> +	int ret;
> +
> +	np = of_node_get(dev->of_node);
> +	if (!np)
> +		return -EINVAL;
> +
> +	parent = of_get_child_by_name(np, "regulators");
> +	if (!parent) {
> +		dev_err(dev, "regulators node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_regulator_match(dev, parent, pf8x00_matches,
> +				 ARRAY_SIZE(pf8x00_matches));

Don't open code binding parsing, just specify the relevant names in the
regulator decriptors, register them and and let the core parse things
for you.  

> +			if (ilim <= 3) {
> +				mask |= 3 << 3;
> +				val |= ilim << 3;
> +			}
> +			if (fast_slew <= 1) {
> +				mask |= 1 << 5;
> +				val |= fast_slew << 5;
> +			}

More blank lines between blocks please.

> +static int pf8x_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int pf8x_resume(struct device *dev)
> +{
> +	return 0;
> +}

Remove empty functions.

> +static int __init pf8x_init(void)
> +{
> +	return i2c_add_driver(&pf8x_driver);
> +}
> +subsys_initcall(pf8x_init);

You should be able to use module_i2c_driver() here.

On Sun, Dec 06, 2020 at 01:26:28AM +0100, Adrien Grassein wrote:
> Add dt-bindings for the pf8x00 driver.
> 
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  .../regulator/nxp,pf8x00-regulator.yaml       | 223 ++++++++++++++++++
>  MAINTAINERS                                   |   6 +
>  2 files changed, 229 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> new file mode 100644
> index 000000000000..f4e4f545c5a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> @@ -0,0 +1,223 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/nxp,pf8x00-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP PF8x00 Power Management Integrated Circuit regulators
> +
> +maintainers:
> +  - Adrien Grassein <adrien.grassein@gmail.com>
> +
> +description: |
> +  pf8x00 are a 12-chanel regulator PMIC family. Regulators nodes should
> +  be named to ldo<>, sw<> and vnss. The definition for each of these nodes
> +  is defined using the standard binding for regulators at
> +  Documentation/devicetree/bindings/regulator/regulator.txt.
> +  Datasheet is available at https://www.nxp.com/docs/en/data-sheet/PF8100_PF8200.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,pf8x00
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulators:
> +    type: object
> +    description: |

Don't need '|' if no formatting.

> +      list of regulators provided by this controller
> +
> +    patternProperties:
> +      "^ldo[1-4]$":
> +        type: object
> +        $ref: regulator.yaml#
> +        description: |
> +          Properties for single LDO regulator.
> +
> +        properties:
> +          regulator-name:
> +            pattern: "^ldo[1-4]$"
> +            description: |
> +              should be ldo1", ..., "ldo4"
> +
> +          nxp,hw-en:
> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              Only available for ldo2. Used to enable or disable ld02.
> +
> +          nxp,vselect-en:
> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              Only available for ldo2. When specified, use the VSELECT pin
> +              of the chip to control the output voltage of the ldo02 regulator.
> +
> +        unevaluatedProperties: false
> +
> +      "^sw[1-7]$":
> +        type: object
> +        $ref: regulator.yaml#
> +        description: |
> +          Properties for single SW regulator.
> +
> +        properties:
> +          regulator-name:
> +            pattern: "^sw[1-7]$"
> +            description: |
> +              should be sw1", ..., "sw7"
> +
> +          nxp,ilim-ma:

Use unit suffix defined in property-units.txt.

> +            $ref: /schemas/types.yaml#definitions/uint32
> +            minimum: 2100
> +            maximum: 4500
> +            default: 2100
> +            enum: [ 2100, 2600, 3000, 4500 ]
> +            description: |
> +              Defines the maximum current delivered by the regulator (in mA).
> +
> +          nxp,phase:
> +            $ref: /schemas/types.yaml#definitions/uint32
> +            minimum: 0
> +            maximum: 315
> +            default: 0
> +            enum: [ 0, 45, 90, 135, 180, 225, 270, 315 ]
> +            description: |
> +               This controls the phase shift of the switching frequency.
> +
> +          nxp,fsl,fast-slew:

nxp or fsl?

> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              Controls the DVS ramp of the regulator.
> +
> +          nxp,quad-phase:
> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              This allow regulators  sw1 and sw2, or sw3 and sw4 or sw4 and sw5
> +              to work together to deliver a maximum 10A current.
> +
> +          nxp,dual-phase:
> +            $ref: /schemas/types.yaml#definitions/flag
> +            description: |
> +              This allow regulators  sw1, sw2, sw3 and sw4 to work together
> +              to deliver a maximum 5A current.
> +
> +        unevaluatedProperties: false
> +
> +      "^vsnvs$":

Move to 'properties', not a pattern.

> +        type: object
> +        $ref: regulator.yaml#
> +        description: |
> +          Properties for vsnvs regulator.
> +
> +        unevaluatedProperties: false
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - regulators
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      pmic: pf8100@8 {

pmic@8

> +        compatible = "nxp,pf8x00";
> +        reg = <0x08>;
> +
> +        regulators {
> +            reg_ldo1: ldo1 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <5000000>;
> +              regulator-min-microvolt = <1500000>;
> +            };
> +
> +            reg_ldo2: ldo2 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <5000000>;
> +              regulator-min-microvolt = <1500000>;
> +            };
> +
> +            reg_ldo3: ldo3 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <5000000>;
> +              regulator-min-microvolt = <1500000>;
> +            };
> +
> +            reg_ldo4: ldo4 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <5000000>;
> +              regulator-min-microvolt = <1500000>;
> +            };
> +
> +            reg_sw1: sw1 {
> +              nxp,phase = <0>;
> +              nxp,ilim-ma = <4500>;
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <1800000>;
> +              regulator-min-microvolt =  <400000>;
> +            };
> +
> +            reg_sw2: sw2 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <1800000>;
> +              regulator-min-microvolt =  <400000>;
> +            };
> +
> +            reg_sw3: sw3 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <1800000>;
> +              regulator-min-microvolt =  <400000>;
> +              nxp,fast-slew = <1>;
> +            };
> +
> +            reg_sw4: sw4 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <1800000>;
> +              regulator-min-microvolt =  <400000>;
> +              nxp,fast-slew = <1>;
> +            };
> +
> +            reg_sw5: sw5 {
> +              regulator-always-on;
> +              regulator-boot-on;
> +              regulator-max-microvolt = <1800000>;
> +              regulator-min-microvolt =  <400000>;
> +            };
> +
> +           reg_sw6: sw6 {
> +             regulator-always-on;
> +             regulator-boot-on;
> +             regulator-max-microvolt = <1800000>;
> +             regulator-min-microvolt =  <400000>;
> +           };
> +
> +           reg_sw7: sw7 {
> +             regulator-always-on;
> +             regulator-boot-on;
> +             regulator-max-microvolt = <4100000>;
> +             regulator-min-microvolt = <1000000>;
> +           };
> +
> +          reg_vsnvs: vsnvs {
> +            regulator-always-on;
> +            regulator-boot-on;
> +            regulator-max-microvolt = <3300000>;
> +            regulator-min-microvolt = <1800000>;
> +          };
> +        };
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ebe4829cdd4d..4e266f1e9a2f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13730,6 +13730,12 @@ S:	Maintained
>  F:	include/linux/personality.h
>  F:	include/uapi/linux/personality.h
>  
> +PF8x00 PMIC REGULATORS DRIVERS
> +M:	Adrien Grassein <adrien.grassein@gmail.com>
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> +
>  PHOENIX RC FLIGHT CONTROLLER ADAPTER
>  M:	Marcus Folkesson <marcus.folkesson@gmail.com>
>  L:	linux-input@vger.kernel.org
> -- 
> 2.20.1
>

Hi,

Thanks for reviewing my patches.

Le lun. 7 déc. 2020 à 14:55, Mark Brown <broonie@kernel.org> a écrit :
>
> On Sun, Dec 06, 2020 at 01:26:28AM +0100, Adrien Grassein wrote:
> > Add dt-bindings for the pf8x00 driver.
>
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.
>

For v2 I just copy-paste another commit message to be sure to be conform.

> > +  compatible:
> > +    enum:
> > +      - nxp,pf8x00
>
> Compatible strings should be for specific devices not wildcards.
>
> > +          nxp,hw-en:
> > +            $ref: /schemas/types.yaml#definitions/flag
> > +            description: |
> > +              Only available for ldo2. Used to enable or disable ld02.
>
> I don't understand what this is documenting - what is "hw-en" and how is
> it used to enable or disable LDO2?
I think I read better documentation for this point. Sorry, it was very unclear.
>
> > +          nxp,vselect-en:
> > +            $ref: /schemas/types.yaml#definitions/flag
> > +            description: |
> > +              Only available for ldo2. When specified, use the VSELECT pin
> > +              of the chip to control the output voltage of the ldo02 regulator.
>
> Shouldn't there be a GPIO specified somewhere or something so that the
> VSELECT pin can be controlled?

I think I read better documentation for this point. Sorry, it was very unclear.
VSELECT is in fact an input pin of the chip. The configuration just enabled it.
>
> > +          nxp,ilim-ma:
> > +            $ref: /schemas/types.yaml#definitions/uint32
> > +            minimum: 2100
> > +            maximum: 4500
> > +            default: 2100
> > +            enum: [ 2100, 2600, 3000, 4500 ]
> > +            description: |
> > +              Defines the maximum current delivered by the regulator (in mA).
>
> Is this not a fixed property of the regulator?
It's not. It's configurable by software.
>
> > +          nxp,quad-phase:
> > +            $ref: /schemas/types.yaml#definitions/flag
> > +            description: |
> > +              This allow regulators  sw1 and sw2, or sw3 and sw4 or sw4 and sw5
> > +              to work together to deliver a maximum 10A current.
>
> Presumably this must be set on both the regulators being grouped
> together?
Not. Only the sw1 configuration will be taken in account.

Thanks again,
Adrien

On Thu, Dec 10, 2020 at 11:24:17PM +0100, Adrien Grassein wrote:
> Le lun. 7 déc. 2020 à 14:55, Mark Brown <broonie@kernel.org> a écrit :
> > On Sun, Dec 06, 2020 at 01:26:28AM +0100, Adrien Grassein wrote:

> > > Add dt-bindings for the pf8x00 driver.

> > Please submit patches using subject lines reflecting the style for the
> > subsystem, this makes it easier for people to identify relevant patches.
> > Look at what existing commits in the area you're changing are doing and
> > make sure your subject lines visually resemble what they're doing.
> > There's no need to resubmit to fix this alone.

> For v2 I just copy-paste another commit message to be sure to be conform.

For patches for a given subsystem you should use the prefix the
subsystem uses, for regulator that's "regulator: ".

> > > +            $ref: /schemas/types.yaml#definitions/flag
> > > +            description: |
> > > +              Only available for ldo2. When specified, use the VSELECT pin
> > > +              of the chip to control the output voltage of the ldo02 regulator.

> > Shouldn't there be a GPIO specified somewhere or something so that the
> > VSELECT pin can be controlled?

> I think I read better documentation for this point. Sorry, it was very unclear.
> VSELECT is in fact an input pin of the chip. The configuration just enabled it.

Then presumably you need some binding to specify how to control this
input too?

> > > +          nxp,quad-phase:
> > > +            $ref: /schemas/types.yaml#definitions/flag
> > > +            description: |
> > > +              This allow regulators  sw1 and sw2, or sw3 and sw4 or sw4 and sw5
> > > +              to work together to deliver a maximum 10A current.

> > Presumably this must be set on both the regulators being grouped
> > together?

> Not. Only the sw1 configuration will be taken in account.

That needs to be documented then.