mbox series

[v2,0/5] PM6125 regulator support

Message ID 20220726181133.3262695-1-iskren.chernev@gmail.com
Headers show
Series PM6125 regulator support | expand

Message

Iskren Chernev July 26, 2022, 6:11 p.m. UTC
This patch series adds SPMI and SMD regulator support for the PM6125 found on
SM4250/SM6115 SoCs from QCom.

This code has been tested on:
* OnePlus Nord N100 (oneplus,billie2)
* Xiaomi 9T (xiaomi,lemon)

The main source used for this change is qpnp pm6125 support patch from caf [1]:

[1]: https://source.codeaurora.org/quic/la/kernel/msm-5.4/commit/?h=kernel.lnx.5.4.r1-rel&id=d1220daeffaa440ffff0a8c47322eb0033bf54f5

v1: https://lkml.org/lkml/2021/8/28/144

Changes from v1:
- add dt-bindings
- split SPMI patch into new reg types and the new PMIC
- add correct supply mapping

Iskren Chernev (5):
  dt-bindings: regulator: Document the PM6125 SPMI PMIC
  dt-bindings: regulator: Document the PM6125 RPM regulators
  regulator: qcom_spmi: Add support for new regulator types
  regulator: qcom_spmi: Add PM6125 PMIC support
  regulator: qcom_smd: Add PM6125 regulators support

 .../regulator/qcom,smd-rpm-regulator.yaml     |   4 +
 .../regulator/qcom,spmi-regulator.yaml        |  19 +++
 drivers/regulator/qcom_smd-regulator.c        |  46 +++++
 drivers/regulator/qcom_spmi-regulator.c       | 160 +++++++++++++++++-
 4 files changed, 227 insertions(+), 2 deletions(-)

--
2.37.1

Comments

Dmitry Baryshkov July 27, 2022, 8:34 a.m. UTC | #1
On Wed, 27 Jul 2022 at 10:49, Iskren Chernev <iskren.chernev@gmail.com> wrote:
>
>
>
> On 7/26/22 23:36, Dmitry Baryshkov wrote:
> > On Tue, 26 Jul 2022 at 21:11, Iskren Chernev <iskren.chernev@gmail.com> wrote:
> >>
> >> Add support for pm6125 compatible string and add relevant supplies in QCom SPMI
> >> regulator documentation.
> >>
> >> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com>
> >> Signed-off-by: Adam Skladowski <a39.skl@gmail.com>
> >
> > The order of sign-offs seems incorrect. The sender's signature should
> > be the last one.
>
> Sure, will do!
>
> >> ---
> >>  .../regulator/qcom,spmi-regulator.yaml        | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> >> index 8b7c4af4b551..d8f18b441484 100644
> >> --- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> >> +++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> >> @@ -12,6 +12,7 @@ maintainers:
> >>  properties:
> >>    compatible:
> >>      enum:
> >> +      - qcom,pm6125-regulators
> >>        - qcom,pm660-regulators
> >>        - qcom,pm660l-regulators
> >>        - qcom,pm8004-regulators
> >> @@ -106,6 +107,24 @@ required:
> >>    - compatible
> >>
> >>  allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >> +              - qcom,pm6125-regulators
> >> +    then:
> >> +      properties:
> >> +        vdd_l1_l7_l17_l18-supply: true
> >> +        vdd_l2_l3_l4-supply: true
> >> +        vdd_l5_l15_l19_l20_l21_l22-supply: true
> >> +        vdd_l6_l8-supply: true
> >> +        vdd_l9_l11-supply: true
> >> +        vdd_l10_l13_l14-supply: true
> >> +        vdd_l12_l16-supply: true
> >> +        vdd_l23_l24-supply: true
> >> +      patternProperties:
> >> +        "^vdd_s[1-8]-supply$": true
> >
> > Add an empty line please.
>
> All other if-then blocks don't have newlines, shall I add one between each as
> well?

Yes, please, in a separate commit.
Mark Brown July 27, 2022, 11:57 a.m. UTC | #2
On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:

> Add support for some regulator types that are missing in this driver, all
> belonging to the FTSMPS426 register layout.  This is done in preparation
> for adding support for the PM6125 PMIC.

> +	.set_mode		= spmi_regulator_ftsmps3_set_mode,
> +	.get_mode		= spmi_regulator_ftsmps426_get_mode,

Why are set and get asymmetric?

> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
>  	SPMI_VREG(LDO,   HT_P600,  0, INF, HFS430, hfs430, ht_p600, 10000),
>  	SPMI_VREG(LDO,   HT_P150,  0, INF, HFS430, hfs430, ht_p150, 10000),
>  	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
> -	SPMI_VREG(BUCK,  HFS430,   0, INF, HFS430, hfs430, hfs430,  10000),
> +	SPMI_VREG(BUCK,  HFS430,   0,   3, HFS430, hfs430, hfs430,  10000),

The changelog said we were adding support for new types but this looks
like changing an existing type.
Iskren Chernev July 27, 2022, 11:14 p.m. UTC | #3
On 7/27/22 14:57, Mark Brown wrote:
> On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:
>
>> Add support for some regulator types that are missing in this driver, all
>> belonging to the FTSMPS426 register layout.  This is done in preparation
>> for adding support for the PM6125 PMIC.
>
>> +	.set_mode		= spmi_regulator_ftsmps3_set_mode,
>> +	.get_mode		= spmi_regulator_ftsmps426_get_mode,
>
> Why are set and get asymmetric?

Because the get method, only uses AUTO and HPM, which have the same value
for ftsmps3 and ftsmps426 (so there is no need for a new function).

>> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
>>  	SPMI_VREG(LDO,   HT_P600,  0, INF, HFS430, hfs430, ht_p600, 10000),
>>  	SPMI_VREG(LDO,   HT_P150,  0, INF, HFS430, hfs430, ht_p150, 10000),
>>  	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
>> -	SPMI_VREG(BUCK,  HFS430,   0, INF, HFS430, hfs430, hfs430,  10000),
>> +	SPMI_VREG(BUCK,  HFS430,   0,   3, HFS430, hfs430, hfs430,  10000),
>
> The changelog said we were adding support for new types but this looks
> like changing an existing type.

The code, as written now does a different thing for BUCK, HFS430 (on
mainline (ML) and downstream (DS) linked in the commit message). Since DS
only supports newer stuff, to be on safe side, I kept existing behavior for
rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
completely.

The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
might be right for the time being (i.e initial revisions had different
behavior). I'm CC-ing Jorge.

Question is is BUCK+HFS430 on common2 (ftsmps426) or common3 (ftsmps3) or
a mix (depending on revision).

[1] 0211f68e626f (regulator: qcom_spmi: add PMS405 SPMI regulator, 2019-06-17)
Mark Brown July 28, 2022, 11:11 a.m. UTC | #4
On Thu, Jul 28, 2022 at 02:14:10AM +0300, Iskren Chernev wrote:
> On 7/27/22 14:57, Mark Brown wrote:
> > On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:

> >> Add support for some regulator types that are missing in this driver, all
> >> belonging to the FTSMPS426 register layout.  This is done in preparation
> >> for adding support for the PM6125 PMIC.

> >> +	.set_mode		= spmi_regulator_ftsmps3_set_mode,
> >> +	.get_mode		= spmi_regulator_ftsmps426_get_mode,

> > Why are set and get asymmetric?

> Because the get method, only uses AUTO and HPM, which have the same value
> for ftsmps3 and ftsmps426 (so there is no need for a new function).

This needs at least a comment.

> >> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
> >>  	SPMI_VREG(LDO,   HT_P600,  0, INF, HFS430, hfs430, ht_p600, 10000),
> >>  	SPMI_VREG(LDO,   HT_P150,  0, INF, HFS430, hfs430, ht_p150, 10000),
> >>  	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
> >> -	SPMI_VREG(BUCK,  HFS430,   0, INF, HFS430, hfs430, hfs430,  10000),
> >> +	SPMI_VREG(BUCK,  HFS430,   0,   3, HFS430, hfs430, hfs430,  10000),

> > The changelog said we were adding support for new types but this looks
> > like changing an existing type.

> The code, as written now does a different thing for BUCK, HFS430 (on
> mainline (ML) and downstream (DS) linked in the commit message). Since DS
> only supports newer stuff, to be on safe side, I kept existing behavior for
> rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
> completely.

This needs describing in the changelog, probably you need multiple
paches here since you are making a number of different changes each of
which needs some explanation.

> The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
> might be right for the time being (i.e initial revisions had different
> behavior). I'm CC-ing Jorge.

If that's the case perhaps part of this needs to be sent as a fix.
Iskren Chernev July 28, 2022, 8:59 p.m. UTC | #5
On 7/28/22 14:11, Mark Brown wrote:
> On Thu, Jul 28, 2022 at 02:14:10AM +0300, Iskren Chernev wrote:
>> On 7/27/22 14:57, Mark Brown wrote:
>>> On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:
>
>>>> Add support for some regulator types that are missing in this driver, all
>>>> belonging to the FTSMPS426 register layout.  This is done in preparation
>>>> for adding support for the PM6125 PMIC.
>
>>>> +	.set_mode		= spmi_regulator_ftsmps3_set_mode,
>>>> +	.get_mode		= spmi_regulator_ftsmps426_get_mode,
>
>>> Why are set and get asymmetric?
>
>> Because the get method, only uses AUTO and HPM, which have the same value
>> for ftsmps3 and ftsmps426 (so there is no need for a new function).
>
> This needs at least a comment.

I agree, I think to add the function with the right macros, and comment
that it is the same now but might change in the future if support for mode
modes is added.

>>>> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
>>>>  	SPMI_VREG(LDO,   HT_P600,  0, INF, HFS430, hfs430, ht_p600, 10000),
>>>>  	SPMI_VREG(LDO,   HT_P150,  0, INF, HFS430, hfs430, ht_p150, 10000),
>>>>  	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
>>>> -	SPMI_VREG(BUCK,  HFS430,   0, INF, HFS430, hfs430, hfs430,  10000),
>>>> +	SPMI_VREG(BUCK,  HFS430,   0,   3, HFS430, hfs430, hfs430,  10000),
>
>>> The changelog said we were adding support for new types but this looks
>>> like changing an existing type.
>
>> The code, as written now does a different thing for BUCK, HFS430 (on
>> mainline (ML) and downstream (DS) linked in the commit message). Since DS
>> only supports newer stuff, to be on safe side, I kept existing behavior for
>> rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
>> completely.
>
> This needs describing in the changelog, probably you need multiple
> paches here since you are making a number of different changes each of
> which needs some explanation.
>
>> The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
>> might be right for the time being (i.e initial revisions had different
>> behavior). I'm CC-ing Jorge.
>
> If that's the case perhaps part of this needs to be sent as a fix.

The Downstream patch is adding 3 logical types:
- LDO_510 -- these have new subtypes, so no existing PMICs are affected
- FTSMPS3 -- this has a new subtype (0xb), so no existing PMICs are
  affected
- HFSMPS -- this has the same type and subtype (BUCK+HFS430) as an existing
  mainline logical type (HFS430), both declaring 0-INF revisions.

So if we fully trust the downstream patch, I can make a fix for the
existing BUCK+HFS430+0-INF, so it uses the slighly modified mode values.

Currently the set mode fn differs in LPM mode (5 in the common2 case and
4 in the common3 case), so if indeed downstream is correct it would mean
this regulator (when turned off) was set to an invalid mode (5 has
undefined meaning in common3 map) from 2019 onward.

On the other hand, if we assume downstream is wrong, then their code sets
4, which actually means RETENTION (not LPM). I really don't know how this
could cause trouble. In fact downstream does a bunch of weird stuff, it
doesn't "just" set to LPM (like mainline), instead there is complex logic
per logical type and "initial mode". Or they're just masking this mistake
;-)

TL;DR Jorge's mail is gone, so we can't get info from the original author.

Another issue is I can't really test any other PMIC (and even my PMIC
I can't turn off most of the regs without loosing critical functionality,
and the BUCKs are kinda important :)).

So we can:
1. politely ask for somebody with access to the secret sauce to say what is
   correct, at least according to the docs (with a timeout)
2. assume downstream patch is right, and fix the existing HFS430 regulator
3. maintain the current (patch) behavior, which likely won't affect older
   PMICs, but is still adhering to DS patch, because it adds support for
   this particular PMIC, so presumably it was tested and works with it
4. drop the pmic patch and rely on SMD

Please advice.

In any case if we go with 2 or 3, I can split out this particular (BUCK)
part in a separate patch with more information/comments.
Mark Brown July 29, 2022, 12:04 p.m. UTC | #6
On Thu, Jul 28, 2022 at 11:59:03PM +0300, Iskren Chernev wrote:
> 
> 
> On 7/28/22 14:11, Mark Brown wrote:
> > On Thu, Jul 28, 2022 at 02:14:10AM +0300, Iskren Chernev wrote:
> >> On 7/27/22 14:57, Mark Brown wrote:
> >>> On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:
> >
> >>>> Add support for some regulator types that are missing in this driver, all
> >>>> belonging to the FTSMPS426 register layout.  This is done in preparation
> >>>> for adding support for the PM6125 PMIC.
> >
> >>>> +	.set_mode		= spmi_regulator_ftsmps3_set_mode,
> >>>> +	.get_mode		= spmi_regulator_ftsmps426_get_mode,
> >
> >>> Why are set and get asymmetric?
> >
> >> Because the get method, only uses AUTO and HPM, which have the same value
> >> for ftsmps3 and ftsmps426 (so there is no need for a new function).
> >
> > This needs at least a comment.
> 
> I agree, I think to add the function with the right macros, and comment
> that it is the same now but might change in the future if support for mode
> modes is added.
> 
> >>>> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
> >>>>  	SPMI_VREG(LDO,   HT_P600,  0, INF, HFS430, hfs430, ht_p600, 10000),
> >>>>  	SPMI_VREG(LDO,   HT_P150,  0, INF, HFS430, hfs430, ht_p150, 10000),
> >>>>  	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
> >>>> -	SPMI_VREG(BUCK,  HFS430,   0, INF, HFS430, hfs430, hfs430,  10000),
> >>>> +	SPMI_VREG(BUCK,  HFS430,   0,   3, HFS430, hfs430, hfs430,  10000),
> >
> >>> The changelog said we were adding support for new types but this looks
> >>> like changing an existing type.
> >
> >> The code, as written now does a different thing for BUCK, HFS430 (on
> >> mainline (ML) and downstream (DS) linked in the commit message). Since DS
> >> only supports newer stuff, to be on safe side, I kept existing behavior for
> >> rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
> >> completely.
> >
> > This needs describing in the changelog, probably you need multiple
> > paches here since you are making a number of different changes each of
> > which needs some explanation.
> >
> >> The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
> >> might be right for the time being (i.e initial revisions had different
> >> behavior). I'm CC-ing Jorge.
> >
> > If that's the case perhaps part of this needs to be sent as a fix.
> 
> The Downstream patch is adding 3 logical types:
> - LDO_510 -- these have new subtypes, so no existing PMICs are affected
> - FTSMPS3 -- this has a new subtype (0xb), so no existing PMICs are
>   affected
> - HFSMPS -- this has the same type and subtype (BUCK+HFS430) as an existing
>   mainline logical type (HFS430), both declaring 0-INF revisions.
> 
> So if we fully trust the downstream patch, I can make a fix for the
> existing BUCK+HFS430+0-INF, so it uses the slighly modified mode values.
> 
> Currently the set mode fn differs in LPM mode (5 in the common2 case and
> 4 in the common3 case), so if indeed downstream is correct it would mean
> this regulator (when turned off) was set to an invalid mode (5 has
> undefined meaning in common3 map) from 2019 onward.
> 
> On the other hand, if we assume downstream is wrong, then their code sets
> 4, which actually means RETENTION (not LPM). I really don't know how this
> could cause trouble. In fact downstream does a bunch of weird stuff, it
> doesn't "just" set to LPM (like mainline), instead there is complex logic
> per logical type and "initial mode". Or they're just masking this mistake
> ;-)
> 
> TL;DR Jorge's mail is gone, so we can't get info from the original author.

Jorge moved to foundries.io, copying him in in case he remembers
anything about this.

> Another issue is I can't really test any other PMIC (and even my PMIC
> I can't turn off most of the regs without loosing critical functionality,
> and the BUCKs are kinda important :)).
> 
> So we can:
> 1. politely ask for somebody with access to the secret sauce to say what is
>    correct, at least according to the docs (with a timeout)
> 2. assume downstream patch is right, and fix the existing HFS430 regulator
> 3. maintain the current (patch) behavior, which likely won't affect older
>    PMICs, but is still adhering to DS patch, because it adds support for
>    this particular PMIC, so presumably it was tested and works with it
> 4. drop the pmic patch and rely on SMD
> 
> Please advice.
> 
> In any case if we go with 2 or 3, I can split out this particular (BUCK)
> part in a separate patch with more information/comments.
Jorge Ramirez-Ortiz, Gmail July 29, 2022, 9:07 p.m. UTC | #7
On 29/07/22 13:04:57, Mark Brown wrote:
> On Thu, Jul 28, 2022 at 11:59:03PM +0300, Iskren Chernev wrote:
> > 
> > 
> > On 7/28/22 14:11, Mark Brown wrote:
> > > On Thu, Jul 28, 2022 at 02:14:10AM +0300, Iskren Chernev wrote:
> > >> On 7/27/22 14:57, Mark Brown wrote:
> > >>> On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:
> > >
> > >>>> Add support for some regulator types that are missing in this driver, all
> > >>>> belonging to the FTSMPS426 register layout.  This is done in preparation
> > >>>> for adding support for the PM6125 PMIC.
> > >
> > >>>> +	.set_mode		= spmi_regulator_ftsmps3_set_mode,
> > >>>> +	.get_mode		= spmi_regulator_ftsmps426_get_mode,
> > >
> > >>> Why are set and get asymmetric?
> > >
> > >> Because the get method, only uses AUTO and HPM, which have the same value
> > >> for ftsmps3 and ftsmps426 (so there is no need for a new function).
> > >
> > > This needs at least a comment.
> > 
> > I agree, I think to add the function with the right macros, and comment
> > that it is the same now but might change in the future if support for mode
> > modes is added.
> > 
> > >>>> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
> > >>>>  	SPMI_VREG(LDO,   HT_P600,  0, INF, HFS430, hfs430, ht_p600, 10000),
> > >>>>  	SPMI_VREG(LDO,   HT_P150,  0, INF, HFS430, hfs430, ht_p150, 10000),
> > >>>>  	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
> > >>>> -	SPMI_VREG(BUCK,  HFS430,   0, INF, HFS430, hfs430, hfs430,  10000),
> > >>>> +	SPMI_VREG(BUCK,  HFS430,   0,   3, HFS430, hfs430, hfs430,  10000),
> > >
> > >>> The changelog said we were adding support for new types but this looks
> > >>> like changing an existing type.
> > >
> > >> The code, as written now does a different thing for BUCK, HFS430 (on
> > >> mainline (ML) and downstream (DS) linked in the commit message). Since DS
> > >> only supports newer stuff, to be on safe side, I kept existing behavior for
> > >> rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
> > >> completely.
> > >
> > > This needs describing in the changelog, probably you need multiple
> > > paches here since you are making a number of different changes each of
> > > which needs some explanation.
> > >
> > >> The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
> > >> might be right for the time being (i.e initial revisions had different
> > >> behavior). I'm CC-ing Jorge.
> > >
> > > If that's the case perhaps part of this needs to be sent as a fix.
> > 
> > The Downstream patch is adding 3 logical types:
> > - LDO_510 -- these have new subtypes, so no existing PMICs are affected
> > - FTSMPS3 -- this has a new subtype (0xb), so no existing PMICs are
> >   affected
> > - HFSMPS -- this has the same type and subtype (BUCK+HFS430) as an existing
> >   mainline logical type (HFS430), both declaring 0-INF revisions.
> > 
> > So if we fully trust the downstream patch, I can make a fix for the
> > existing BUCK+HFS430+0-INF, so it uses the slighly modified mode values.
> > 
> > Currently the set mode fn differs in LPM mode (5 in the common2 case and
> > 4 in the common3 case), so if indeed downstream is correct it would mean
> > this regulator (when turned off) was set to an invalid mode (5 has
> > undefined meaning in common3 map) from 2019 onward.
> > 
> > On the other hand, if we assume downstream is wrong, then their code sets
> > 4, which actually means RETENTION (not LPM). I really don't know how this
> > could cause trouble. In fact downstream does a bunch of weird stuff, it
> > doesn't "just" set to LPM (like mainline), instead there is complex logic
> > per logical type and "initial mode". Or they're just masking this mistake
> > ;-)
> > 
> > TL;DR Jorge's mail is gone, so we can't get info from the original author.
> 
> Jorge moved to foundries.io, copying him in in case he remembers
> anything about this.

I am sorry, I really dont remember the details. I believe this was part of the
QCS404 upstreaming work so it would have been tested not just by us but also by
the release team working on the final product. Sorry I cant be of much help, it
has been a while.

> 
> > Another issue is I can't really test any other PMIC (and even my PMIC
> > I can't turn off most of the regs without loosing critical functionality,
> > and the BUCKs are kinda important :)).
> > 
> > So we can:
> > 1. politely ask for somebody with access to the secret sauce to say what is
> >    correct, at least according to the docs (with a timeout)
> > 2. assume downstream patch is right, and fix the existing HFS430 regulator
> > 3. maintain the current (patch) behavior, which likely won't affect older
> >    PMICs, but is still adhering to DS patch, because it adds support for
> >    this particular PMIC, so presumably it was tested and works with it
> > 4. drop the pmic patch and rely on SMD
> > 
> > Please advice.
> > 
> > In any case if we go with 2 or 3, I can split out this particular (BUCK)
> > part in a separate patch with more information/comments.