Messages in this thread | | | Subject | Re: [PATCH 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs | From | AngeloGioacchino Del Regno <> | Date | Mon, 11 Jan 2021 22:06:18 +0100 |
| |
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
| |