lkml.org 
[lkml]   [2021]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs
From
Date
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.

\
 
 \ /
  Last update: 2021-01-11 20:15    [W:0.074 / U:0.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site