All of lore.kernel.org
 help / color / mirror / Atom feed
From: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
To: Mark Brown <broonie@kernel.org>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators
Date: Mon, 23 May 2022 15:22:39 +0200	[thread overview]
Message-ID: <a6606891-d55f-dbce-7c5a-86390694e1c4@collabora.com> (raw)
In-Reply-To: <YouFcSapkVC7ZfuP@sirena.org.uk>

Il 23/05/22 15:00, Mark Brown ha scritto:
> On Mon, May 23, 2022 at 02:49:19PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 20/05/22 16:45, Mark Brown ha scritto:
>>> On Fri, May 20, 2022 at 03:33:03PM +0200, AngeloGioacchino Del Regno wrote:
> 
>>>> +static const unsigned int ldo_volt_table10[] = {
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +};
> 
>>> So the top bits of the voltate selection field just get ignored?  Might
>>> be easier to just write the code to not include the top bits.
>>>
> 
>> No, they're all valid values for real... but I guess that I can simplify
>> this voltage table by simply modifying the bitmask that we use for the
>> regulators that are using this table....
> 
> Right, my point here is that it looks awfully like the documentation
> (this came from documentation I guess?) is including some extra bits
> that get ignored in the voltage selection field here.  That seems like a
> weird choice somewhere along the line.
> 

I wish I had a datasheet for these parts...

All of this comes from analyzing a running device on the downstream 3.4 kernel
and on understanding the (not really readable) downstream kernel code...
..but yes, I agree on the fact that this seems to be a weird choice.

Ah, besides, I hooked up an oscilloscope to the VCAM_IO and I can see that the
vreg really does react as expected upon setting the upper bits.. but since it
still works without, we can safely ignore them, which makes us able to simplify
the driver (as no custom code for that will be required) and, at the same time,
avoid seeing a table of values repeated 4 times in a row.

>>>> +	if (info->qi > 0) {
>>>> +		reg = info->desc.enable_reg;
>>>> +		en_mask = info->qi;
> 
>>> If the regulator doesn't have status readback it shouldn't provide a
>>> get_status() operation.
> 
>> What I've understood is that when there's no "QI" flag, the enable register
>> will provide the regulator status (EN/DIS) acting like QI, that's why I've
>> added that if branch...
> 
>> Anyway, I'll recheck this part before sending the next version!
> 
> That would be fairly unusual, often a regulator won't even detect when
> it's gone out of regulation.

Actually, there *is* support for this kind of detection... luckily the registers
and masks/bits are all dumped in a "upmu_hw.h" header downstream, regardless of
whether they're used or not in the code, so at least there's that reference to
look at... and I can see that there are bits to manage the overcurrent protection
(OVP) and configurable (OCPLVL) overcurrent protection (OCP_EN / OC_EN)...

...so, the regulator will indeed shut itself off and clear either/both the QI_EN
and/or its relative bit in the enable register... I've also just found hints of
the latter (enable register being set to 0) downstream, so I'm sure that this is
indeed right.

And finally... I would really like to test the OCP/OVP features to write some
code managing that, but I'm using a production smartphone (a Xperia M5, like
mentioned in the cover letter) for research and testing and you surely understand
that it's not time yet to take this risk... I will, later - but I have to finish
the upstreaming of this SoC and platform before chasing the green smoke... :-)



WARNING: multiple messages have this Message-ID
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Mark Brown <broonie@kernel.org>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators
Date: Mon, 23 May 2022 15:22:39 +0200	[thread overview]
Message-ID: <a6606891-d55f-dbce-7c5a-86390694e1c4@collabora.com> (raw)
In-Reply-To: <YouFcSapkVC7ZfuP@sirena.org.uk>

Il 23/05/22 15:00, Mark Brown ha scritto:
> On Mon, May 23, 2022 at 02:49:19PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 20/05/22 16:45, Mark Brown ha scritto:
>>> On Fri, May 20, 2022 at 03:33:03PM +0200, AngeloGioacchino Del Regno wrote:
> 
>>>> +static const unsigned int ldo_volt_table10[] = {
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +};
> 
>>> So the top bits of the voltate selection field just get ignored?  Might
>>> be easier to just write the code to not include the top bits.
>>>
> 
>> No, they're all valid values for real... but I guess that I can simplify
>> this voltage table by simply modifying the bitmask that we use for the
>> regulators that are using this table....
> 
> Right, my point here is that it looks awfully like the documentation
> (this came from documentation I guess?) is including some extra bits
> that get ignored in the voltage selection field here.  That seems like a
> weird choice somewhere along the line.
> 

I wish I had a datasheet for these parts...

All of this comes from analyzing a running device on the downstream 3.4 kernel
and on understanding the (not really readable) downstream kernel code...
..but yes, I agree on the fact that this seems to be a weird choice.

Ah, besides, I hooked up an oscilloscope to the VCAM_IO and I can see that the
vreg really does react as expected upon setting the upper bits.. but since it
still works without, we can safely ignore them, which makes us able to simplify
the driver (as no custom code for that will be required) and, at the same time,
avoid seeing a table of values repeated 4 times in a row.

>>>> +	if (info->qi > 0) {
>>>> +		reg = info->desc.enable_reg;
>>>> +		en_mask = info->qi;
> 
>>> If the regulator doesn't have status readback it shouldn't provide a
>>> get_status() operation.
> 
>> What I've understood is that when there's no "QI" flag, the enable register
>> will provide the regulator status (EN/DIS) acting like QI, that's why I've
>> added that if branch...
> 
>> Anyway, I'll recheck this part before sending the next version!
> 
> That would be fairly unusual, often a regulator won't even detect when
> it's gone out of regulation.

Actually, there *is* support for this kind of detection... luckily the registers
and masks/bits are all dumped in a "upmu_hw.h" header downstream, regardless of
whether they're used or not in the code, so at least there's that reference to
look at... and I can see that there are bits to manage the overcurrent protection
(OVP) and configurable (OCPLVL) overcurrent protection (OCP_EN / OC_EN)...

...so, the regulator will indeed shut itself off and clear either/both the QI_EN
and/or its relative bit in the enable register... I've also just found hints of
the latter (enable register being set to 0) downstream, so I'm sure that this is
indeed right.

And finally... I would really like to test the OCP/OVP features to write some
code managing that, but I'm using a production smartphone (a Xperia M5, like
mentioned in the cover letter) for research and testing and you surely understand
that it's not time yet to take this risk... I will, later - but I have to finish
the upstreaming of this SoC and platform before chasing the green smoke... :-)



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

WARNING: multiple messages have this Message-ID
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Mark Brown <broonie@kernel.org>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators
Date: Mon, 23 May 2022 15:22:39 +0200	[thread overview]
Message-ID: <a6606891-d55f-dbce-7c5a-86390694e1c4@collabora.com> (raw)
In-Reply-To: <YouFcSapkVC7ZfuP@sirena.org.uk>

Il 23/05/22 15:00, Mark Brown ha scritto:
> On Mon, May 23, 2022 at 02:49:19PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 20/05/22 16:45, Mark Brown ha scritto:
>>> On Fri, May 20, 2022 at 03:33:03PM +0200, AngeloGioacchino Del Regno wrote:
> 
>>>> +static const unsigned int ldo_volt_table10[] = {
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +	1200000, 1300000, 1500000, 1800000,
>>>> +};
> 
>>> So the top bits of the voltate selection field just get ignored?  Might
>>> be easier to just write the code to not include the top bits.
>>>
> 
>> No, they're all valid values for real... but I guess that I can simplify
>> this voltage table by simply modifying the bitmask that we use for the
>> regulators that are using this table....
> 
> Right, my point here is that it looks awfully like the documentation
> (this came from documentation I guess?) is including some extra bits
> that get ignored in the voltage selection field here.  That seems like a
> weird choice somewhere along the line.
> 

I wish I had a datasheet for these parts...

All of this comes from analyzing a running device on the downstream 3.4 kernel
and on understanding the (not really readable) downstream kernel code...
..but yes, I agree on the fact that this seems to be a weird choice.

Ah, besides, I hooked up an oscilloscope to the VCAM_IO and I can see that the
vreg really does react as expected upon setting the upper bits.. but since it
still works without, we can safely ignore them, which makes us able to simplify
the driver (as no custom code for that will be required) and, at the same time,
avoid seeing a table of values repeated 4 times in a row.

>>>> +	if (info->qi > 0) {
>>>> +		reg = info->desc.enable_reg;
>>>> +		en_mask = info->qi;
> 
>>> If the regulator doesn't have status readback it shouldn't provide a
>>> get_status() operation.
> 
>> What I've understood is that when there's no "QI" flag, the enable register
>> will provide the regulator status (EN/DIS) acting like QI, that's why I've
>> added that if branch...
> 
>> Anyway, I'll recheck this part before sending the next version!
> 
> That would be fairly unusual, often a regulator won't even detect when
> it's gone out of regulation.

Actually, there *is* support for this kind of detection... luckily the registers
and masks/bits are all dumped in a "upmu_hw.h" header downstream, regardless of
whether they're used or not in the code, so at least there's that reference to
look at... and I can see that there are bits to manage the overcurrent protection
(OVP) and configurable (OCPLVL) overcurrent protection (OCP_EN / OC_EN)...

...so, the regulator will indeed shut itself off and clear either/both the QI_EN
and/or its relative bit in the enable register... I've also just found hints of
the latter (enable register being set to 0) downstream, so I'm sure that this is
indeed right.

And finally... I would really like to test the OCP/OVP features to write some
code managing that, but I'm using a production smartphone (a Xperia M5, like
mentioned in the cover letter) for research and testing and you surely understand
that it's not time yet to take this risk... I will, later - but I have to finish
the upstreaming of this SoC and platform before chasing the green smoke... :-)



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2022-05-23 13:22 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-20 13:33 [PATCH 0/4] MediaTek Helio X10 MT6795 - MT6331/6332 Regulators AngeloGioacchino Del Regno
2022-05-20 13:33 ` AngeloGioacchino Del Regno
2022-05-20 13:33 ` AngeloGioacchino Del Regno
2022-05-20 13:33 ` [PATCH 1/4] dt-bindings: regulator: Add bindings for MT6331 regulator AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-21 14:40   ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-20 13:33 ` [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 14:45   ` Mark Brown
2022-05-20 14:45     ` Mark Brown
2022-05-20 14:45     ` Mark Brown
2022-05-23 12:49     ` AngeloGioacchino Del Regno
2022-05-23 12:49       ` AngeloGioacchino Del Regno
2022-05-23 12:49       ` AngeloGioacchino Del Regno
2022-05-23 13:00       ` Mark Brown
2022-05-23 13:00         ` Mark Brown
2022-05-23 13:00         ` Mark Brown
2022-05-23 13:22         ` AngeloGioacchino Del Regno [this message]
2022-05-23 13:22           ` AngeloGioacchino Del Regno
2022-05-23 13:22           ` AngeloGioacchino Del Regno
2022-05-23 13:43           ` Mark Brown
2022-05-23 13:43             ` Mark Brown
2022-05-23 13:43             ` Mark Brown
2022-05-23 13:53             ` AngeloGioacchino Del Regno
2022-05-23 13:53               ` AngeloGioacchino Del Regno
2022-05-23 13:53               ` AngeloGioacchino Del Regno
2022-05-20 13:33 ` [PATCH 3/4] dt-bindings: regulator: Add bindings for MT6332 regulator AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-21 14:40   ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-20 13:33 ` [PATCH 4/4] regulator: Add driver for MT6332 PMIC regulators AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a6606891-d55f-dbce-7c5a-86390694e1c4@collabora.com \
    --to=angelogioacchino.delregno@collabora.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.