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:53:22 +0200	[thread overview]
Message-ID: <8334772a-6427-352d-4172-ca262267427d@collabora.com> (raw)
In-Reply-To: <YouPdNrBS2xwWwlI@sirena.org.uk>

Il 23/05/22 15:43, Mark Brown ha scritto:
> On Mon, May 23, 2022 at 03:22:39PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 23/05/22 15:00, Mark Brown ha scritto:
> 
>>> 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.
> 
> That seems safer in general if we don't know what those bits are doing -
> it could be some kind of mode control or something.
> 
>> ...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.
> 
> That's still not quite the same thing as a status bit, if the regulator
> disables itself then it won't try to turn itself back on.  Note that the
> core will fall back on using the enable state if there's no status op so
> there's no need for this logic, you can just omit the status op and the
> behaviour will be the same.

Ok then, I'll simply remove that!

Thanks,
Angelo

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:53:22 +0200	[thread overview]
Message-ID: <8334772a-6427-352d-4172-ca262267427d@collabora.com> (raw)
In-Reply-To: <YouPdNrBS2xwWwlI@sirena.org.uk>

Il 23/05/22 15:43, Mark Brown ha scritto:
> On Mon, May 23, 2022 at 03:22:39PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 23/05/22 15:00, Mark Brown ha scritto:
> 
>>> 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.
> 
> That seems safer in general if we don't know what those bits are doing -
> it could be some kind of mode control or something.
> 
>> ...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.
> 
> That's still not quite the same thing as a status bit, if the regulator
> disables itself then it won't try to turn itself back on.  Note that the
> core will fall back on using the enable state if there's no status op so
> there's no need for this logic, you can just omit the status op and the
> behaviour will be the same.

Ok then, I'll simply remove that!

Thanks,
Angelo

_______________________________________________
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:53:22 +0200	[thread overview]
Message-ID: <8334772a-6427-352d-4172-ca262267427d@collabora.com> (raw)
In-Reply-To: <YouPdNrBS2xwWwlI@sirena.org.uk>

Il 23/05/22 15:43, Mark Brown ha scritto:
> On Mon, May 23, 2022 at 03:22:39PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 23/05/22 15:00, Mark Brown ha scritto:
> 
>>> 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.
> 
> That seems safer in general if we don't know what those bits are doing -
> it could be some kind of mode control or something.
> 
>> ...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.
> 
> That's still not quite the same thing as a status bit, if the regulator
> disables itself then it won't try to turn itself back on.  Note that the
> core will fall back on using the enable state if there's no status op so
> there's no need for this logic, you can just omit the status op and the
> behaviour will be the same.

Ok then, I'll simply remove that!

Thanks,
Angelo

_______________________________________________
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:53 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
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 [this message]
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=8334772a-6427-352d-4172-ca262267427d@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.