From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
To: Mark Brown <broonie@kernel.org>
Cc: linux-arm-msm@vger.kernel.org, agross@kernel.org,
bjorn.andersson@linaro.org, lgirdwood@gmail.com,
robh+dt@kernel.org, sumit.semwal@linaro.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
phone-devel@vger.kernel.org, konrad.dybcio@somainline.org,
marijn.suijten@somainline.org, martin.botka@somainline.org
Subject: Re: [PATCH 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs
Date: Mon, 11 Jan 2021 20:14:08 +0100 [thread overview]
Message-ID: <6dee36e4-fc78-c21b-daf8-120ee44535a3@somainline.org> (raw)
In-Reply-To: <20210111135745.GC4728@sirena.org.uk>
Il 11/01/21 14:57, Mark Brown ha scritto:
> On Sat, Jan 09, 2021 at 02:29:19PM +0100, AngeloGioacchino Del Regno wrote:
>
>> + /* If the regulator is not enabled, this is a fake event */
>> + if (!ops->is_enabled(vreg->rdev))
>> + return 0;
>
> Or handling the interrupt raced with a disable initiated from elsewhere.
> Does the hardware actually have a problem with reporting spurious errors?
>
>> + return ret ? IRQ_NONE : IRQ_HANDLED;
>
> Here and elsewhere please write normal conditional statements to improve
> legibility.
>
No problem. Will do.
>> + /* This function should be called only once, anyway. */
>> + if (unlikely(vreg->ocp_irq_requested))
>> + return 0;
>
> If this is not a fast path it doesn't need an unlikely() annotation;
> indeed it sounds more like there should be a warning printed if this
> isn't supposed to be called multiple times.
>
That was extra-paranoid safety, looking at this one again, that should
be totally unnecessary.
I think that removing this check entirely would be just fine also
because.. anyway.. writing to these registers more than once won't do
any harm, nor break functionality: I mean, even if it happens for
whatever reason, there's *no real need* to avoid it from the hw perspective.
>> + /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
>> + if (vreg->type == QCOM_LAB_TYPE) {
>> + irq_flags |= IRQF_TRIGGER_LOW;
>> + irq_trig_low = 1;
>> + } else {
>> + irq_flags |= IRQF_TRIGGER_HIGH;
>> + irq_trig_low = 0;
>> + }
>
> This would be more clearly written as a switch statement.
>
A switch statement looked like being a bit "too much" for just two cases
where vreg->type cannot be anything else but QCOM_LAB_TYPE or
QCOM_IBB_TYPE... but okay, let's write a switch statement in place of that.
>> + return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
>> + qcom_labibb_ocp_isr, irq_flags,
>> + ocp_irq_name, vreg);
>
> Are you *sure* that devm_ is appropriate here and the interrupt handler
> won't attempt to use things that will be deallocated before devm gets
> round to freeing the interrupt?
>
Yeah, I'm definitely sure.
>> + if (!!(val & LABIBB_CONTROL_ENABLE)) {
>
> The !! is redundant here and makes things less clear.
>
My bad, I forgot to clean this one up before sending.
>> @@ -166,8 +560,37 @@ static int qcom_labibb_of_parse_cb(struct device_node *np,
>> struct regulator_config *config)
>> {
>> struct labibb_regulator *vreg = config->driver_data;
>> + char *sc_irq_name;
>
> I really, really wouldn't expect to see interrupts being requested in
> the DT parsing callback - apart from anything else the device is going
> to have the physical interrupts with or without DT binding information.
> These callbacks are for regulator specific properties, not basic probing.
> Just request the interrupts in the main probe function, this also means
> you can avoid using all the DT specific APIs which are generally a
> warning sign.
>
...And I even wrote a comment saying "The Short Circuit interrupt is
critical: fail if not found"!!! Whoa! That was bad.
Yeah, I'm definitely moving that to the appropriate place.
next prev parent reply other threads:[~2021-01-11 19:15 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-09 13:29 [PATCH 0/7] Really implement Qualcomm LAB/IBB regulators AngeloGioacchino Del Regno
2021-01-09 13:29 ` [PATCH 1/7] regulator: qcom-labibb: Implement voltage selector ops AngeloGioacchino Del Regno
2021-01-11 13:16 ` Mark Brown
2021-01-11 18:43 ` AngeloGioacchino Del Regno
2021-01-09 13:29 ` [PATCH 2/7] regulator: qcom-labibb: Implement current limiting AngeloGioacchino Del Regno
2021-01-09 13:29 ` [PATCH 3/7] regulator: qcom-labibb: Implement pull-down, softstart, active discharge AngeloGioacchino Del Regno
2021-01-09 13:29 ` [PATCH 4/7] dt-bindings: regulator: qcom-labibb: Document soft start properties AngeloGioacchino Del Regno
2021-01-10 17:18 ` Rob Herring
2021-01-10 18:01 ` AngeloGioacchino Del Regno
2021-01-09 13:29 ` [PATCH 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs AngeloGioacchino Del Regno
2021-01-11 13:57 ` Mark Brown
2021-01-11 19:14 ` AngeloGioacchino Del Regno [this message]
2021-01-11 19:23 ` AngeloGioacchino Del Regno
2021-01-11 21:06 ` AngeloGioacchino Del Regno
2021-01-12 17:29 ` Mark Brown
2021-01-12 17:49 ` AngeloGioacchino Del Regno
2021-01-13 17:40 ` Mark Brown
2021-01-09 13:29 ` [PATCH 6/7] dt-bindings: regulator: qcom-labibb: Document SCP/OCP interrupts AngeloGioacchino Del Regno
2021-01-09 13:29 ` [PATCH 7/7] arm64: dts: pmi8998: Add the right interrupts for LAB/IBB SCP and OCP 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=6dee36e4-fc78-c21b-daf8-120ee44535a3@somainline.org \
--to=angelogioacchino.delregno@somainline.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=konrad.dybcio@somainline.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=martin.botka@somainline.org \
--cc=phone-devel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=sumit.semwal@linaro.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).