Date:   Mon, 11 Jan 2021 22:06:18 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     linux-arm-msm@...r.kernel.org, agross@...nel.org,
        bjorn.andersson@...aro.org, lgirdwood@...il.com,
        robh+dt@...nel.org, sumit.semwal@...aro.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        phone-devel@...r.kernel.org, konrad.dybcio@...ainline.org,
        marijn.suijten@...ainline.org, martin.botka@...ainline.org
Subject: Re: [PATCH 5/7] regulator: qcom-labibb: Implement short-circuit and
 over-current IRQs

Il 11/01/21 20:23, AngeloGioacchino Del Regno ha scritto:
> Il 11/01/21 20:14, AngeloGioacchino Del Regno ha scritto:
>> 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?
>>>
> Sorry, I forgot to answer to this one in the previous email.
> 
> Yes, apparently the hardware has this issue: when the current draw is 
> very high and you disable the regulator while the attached device is 
> still drawing a lot of current (like on the Xperia XZ2 smartphone, but I 
> don't want to comment on that phone's HW quirks...) then the OCP 
> interrupt fires *after* disabling the LAB/IBB regulators.
> 
> This doesn't seem to happen if the current draw is low in the exact 
> moment the regulator gets disabled, but that's not always possible since 
> it depends on external HW design / board design sometimes...
> 
> 
>>>> +    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.
> 

I'm sorry for the triple e-mail... but I've just acknowledged that using 
platform_get_irq is actually impossible with the current schema.
As you can see in the dt-bindings documentation, the driver is supposed 
to be declared in DT as

		labibb {

			compatible = "qcom,pmi8998-lab-ibb";



			ibb: ibb {

				interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>,

					     <0x3 0xdc 0x0 IRQ_TYPE_LEVEL_HIGH>;

				interrupt-names = "sc-err", "ocp";

			};



			lab: lab {

				interrupts = <0x3 0xde 0x1 IRQ_TYPE_EDGE_RISING>,

					     <0x3 0xde 0x0 IRQ_TYPE_LEVEL_LOW>;

				interrupt-names = "sc-err", "ocp";

			};

		};

...which was already a requirement before I touched it.
Now, this leaves two options here:
1. Keep the of_get_irq way, or
2. Move the interrupts, change the documentation (currently, only 
pmi8998.dtsi) and also fix pmi8998.dtsi to reflect the new changes.

I am asking before proceeding because I know that changing a schema that 
is already set sometimes gets "negated".

How should I proceed?

-- Angelo

Powered by blists - more mailing lists