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?
Hi,
Thanks for reviewing my patches.
Le lun. 7 déc. 2020 à 14:55, Mark Brown <[email protected]> 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 <[email protected]> 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.