From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:43:15
Okay, the title may be a little "aggressive"? However, the qcom-labibb
driver wasn't really .. doing much.
The current form of this driver is only taking care of enabling or
disabling the regulators, which is pretty useless if they were not
pre-set from the bootloader, which sets them only if continuous
splash is enabled.
Moreover, some bootloaders are setting a higher voltage and/or a higher
current limit compared to what's actually required by the attached
hardware (which is, in 99.9% of the cases, a display) and this produces
a higher power consumption, higher heat output and a risk of actually
burning the display if kept up for a very long time: for example, this
is true on at least some Sony Xperia MSM8998 (Yoshino platform) and
especially on some Sony Xperia SDM845 (Tama platform) smartphones.
In any case, the main reason why this change was necessary for us is
that, during the bringup of Sony Xperia MSM8998 phones, we had an issue
with the bootloader not turning on the display and not setting the lab
and ibb regulators before booting the kernel, making it impossible to
powerup the display.
With this said, this patchset enables setting voltage, current limiting,
overcurrent and short-circuit protection.. and others, on the LAB/IBB
regulators.
Each commit in this patch series provides as many informations as
possible about what's going on and testing methodology.
Changes in v2:
- From Mark Brown review:
- Replaced some if branches with switch statements
- Moved irq get and request in probe function
- Changed short conditionals to full ones
- Removed useless check for ocp_irq_requested
- Fixed issues with YAML documentation
AngeloGioacchino Del Regno (7):
regulator: qcom-labibb: Implement voltage selector ops
regulator: qcom-labibb: Implement current limiting
regulator: qcom-labibb: Implement pull-down, softstart, active
discharge
dt-bindings: regulator: qcom-labibb: Document soft start properties
regulator: qcom-labibb: Implement short-circuit and over-current IRQs
dt-bindings: regulator: qcom-labibb: Document SCP/OCP interrupts
arm64: dts: pmi8998: Add the right interrupts for LAB/IBB SCP and OCP
.../regulator/qcom-labibb-regulator.yaml | 30 +-
arch/arm64/boot/dts/qcom/pmi8998.dtsi | 8 +-
drivers/regulator/qcom-labibb-regulator.c | 661 +++++++++++++++++-
3 files changed, 686 insertions(+), 13 deletions(-)
--
2.29.2
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:43:15
Implement {get,set}_voltage_sel, list_voltage, map_voltage with
the useful regulator regmap helpers in order to be able to manage
the voltage of LAB (positive) and IBB (negative) regulators.
In particular, the supported ranges are the following:
- LAB (pos): 4600mV to 6100mV with 100mV stepping,
- IBB (neg): -7700mV to -1400mV with 100mV stepping.
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 28 +++++++++++++++++++++++
1 file changed, 28 insertions(+)
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
Implement {get,set}_voltage_sel, list_voltage, map_voltage with
the useful regulator regmap helpers in order to be able to manage
the voltage of LAB (positive) and IBB (negative) regulators.
In particular, the supported ranges are the following:
- LAB (pos): 4600mV to 6100mV with 100mV stepping,
- IBB (neg): -7700mV to -1400mV with 100mV stepping.
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:43:18
LAB and IBB regulators can be current-limited by setting the
appropriate registers, but this operation is granted only after
sending an unlock code for secure access.
Besides the secure access, it would be possible to use the
regmap helper for get_current_limit, as there is no security
blocking reads, but I chose not to as to avoid having a very
big array containing current limits, especially for IBB.
That said, these regulators support current limiting for:
- LAB (pos): 200-1600mA, with 200mA per step (8 steps),
- IBB (neg): 0-1550mA, with 50mA per step (32 steps).
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
1 file changed, 92 insertions(+)
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
quoted hunk
LAB and IBB regulators can be current-limited by setting the
appropriate registers, but this operation is granted only after
sending an unlock code for secure access.
Besides the secure access, it would be possible to use the
regmap helper for get_current_limit, as there is no security
blocking reads, but I chose not to as to avoid having a very
big array containing current limits, especially for IBB.
That said, these regulators support current limiting for:
- LAB (pos): 200-1600mA, with 200mA per step (8 steps),
- IBB (neg): 0-1550mA, with 50mA per step (32 steps).
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
1 file changed, 92 insertions(+)
I was under the impression that a regulator driver should either
implement set_voltage_* or set_current_limit, depending on which type of
regulator it is - i.e. this API isn't supposed to be setting the current
limit. Perhaps I'm wrong though?
+{
+ struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+ struct regulator_desc *desc = &vreg->desc;
+ struct labibb_current_limits *lim = &vreg->uA_limits;
+ u32 mask, val;
+ int i, ret, sel = -1;
+
+ if (min_uA < lim->uA_min || max_uA < lim->uA_min)
+ return -EINVAL;
+
+ for (i = 0; i < desc->n_current_limits; i++) {
+ int uA_limit = (lim->uA_step * i) + lim->uA_min;
+
+ if (max_uA >= uA_limit && min_uA <= uA_limit)
I presume here you rely on the client passing something like min_uA = 0
and max_uA 500? Because if the client where to
regulator_set_current_limit(475, 475) you will pass through this loop
without finding a match, but 450 would probably be a really good
pick...
But what does it even mean to pass min/max uA for a current limit?
That said, I think this loop would be better expressed as a single
subtract uA_min and then divide by uA_step.
Apart from that, this patch looks good to me.
Regards,
Bjorn
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-17 18:14:39
Il 15/01/21 05:37, Bjorn Andersson ha scritto:
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
quoted
LAB and IBB regulators can be current-limited by setting the
appropriate registers, but this operation is granted only after
sending an unlock code for secure access.
Besides the secure access, it would be possible to use the
regmap helper for get_current_limit, as there is no security
blocking reads, but I chose not to as to avoid having a very
big array containing current limits, especially for IBB.
That said, these regulators support current limiting for:
- LAB (pos): 200-1600mA, with 200mA per step (8 steps),
- IBB (neg): 0-1550mA, with 50mA per step (32 steps).
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
1 file changed, 92 insertions(+)
I was under the impression that a regulator driver should either
implement set_voltage_* or set_current_limit, depending on which type of
regulator it is - i.e. this API isn't supposed to be setting the current
limit. Perhaps I'm wrong though?
As far as I've understood, these are two entirely different things - set
voltage and set current limits - that's how I'm using them, exactly, and
I'm really sure that I'm right on this.
Besides what I think, though, obviously Mark has the last word on this.
quoted
+{
+ struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+ struct regulator_desc *desc = &vreg->desc;
+ struct labibb_current_limits *lim = &vreg->uA_limits;
+ u32 mask, val;
+ int i, ret, sel = -1;
+
+ if (min_uA < lim->uA_min || max_uA < lim->uA_min)
+ return -EINVAL;
+
+ for (i = 0; i < desc->n_current_limits; i++) {
+ int uA_limit = (lim->uA_step * i) + lim->uA_min;
+
+ if (max_uA >= uA_limit && min_uA <= uA_limit)
I presume here you rely on the client passing something like min_uA = 0
and max_uA 500? Because if the client where to
regulator_set_current_limit(475, 475) you will pass through this loop
without finding a match, but 450 would probably be a really good
pick...
This regulator does not support setting a minimum limit, but only a
ceiling limit, and that's used to raise an interrupt for over-current
protection.
As far as I've understood, the Portable Batch System does its job only
in the specific case of *short circuit* detection, but the OCP is
totally left to the driver.
The reason why I am restricting the match in a sort of paranoid way is
that this regulator can set the current limit in steps, but what I
wanted to avoid was a scenario like:
- My display hardware draws a maximum of 475mA
- I set the current limit on LAB/IBB to 475mA
- The driver picks 450mA because it likes that value more
- I get Over Current Protection interrupts and I think that my hardware
will get fried if I keep going on
- This points me to fix the display driver
- Wrong! It was the regulator driver doing something different from
what I asked it to.
So that's what I am avoiding here. I don't want developers to go crazy
over their eventually new driver design for their new HW for "no reason".
But what does it even mean to pass min/max uA for a current limit?
As I explained above, in this specific case, it means setting a limit to
trigger the over current interrupt in order to protect the hardware that
is attached to this regulator.
In other cases, meanings may be *slightly* different (at least, from my
understanding of it).
That said, I think this loop would be better expressed as a single
subtract uA_min and then divide by uA_step.
Yes I can write it shorter... and even more... but I wanted to improve
human readability of this function (and this entire driver) because
regulators may be dangerous, if badly understood and/or badly set.
I just wanted two things:
1. Whoever reviewed my patches couldn't misunderstand what I wrote
as much as possible;
2. Any other developer reading this driver (which may not be really
familiar with this HW) gets the meaning of what I'm doing in less
time, without doing too much time-expensive research.
After all, sometimes, writing shorter code decreases human readability
without improving performance in any way, and I think that this would
be one of these times... so... :))
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:43:53
Soft start is required to avoid inrush current during LAB ramp-up and
IBB ramp-down, protecting connected hardware to which we supply voltage.
Since soft start is configurable on both LAB and IBB regulators, it
was necessary to add two DT properties, respectively "qcom,soft-start-us"
to control LAB ramp-up and "qcom,discharge-resistor-kohms" to control
the discharge resistor for IBB ramp-down, which obviously brought the
need of implementing a of_parse callback for both regulators.
Finally, also implement pull-down mode in order to avoid unpredictable
behavior when the regulators are disabled (random voltage spikes etc).
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 94 +++++++++++++++++++++++
1 file changed, 94 insertions(+)
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
quoted hunk
Soft start is required to avoid inrush current during LAB ramp-up and
IBB ramp-down, protecting connected hardware to which we supply voltage.
Since soft start is configurable on both LAB and IBB regulators, it
was necessary to add two DT properties, respectively "qcom,soft-start-us"
to control LAB ramp-up and "qcom,discharge-resistor-kohms" to control
the discharge resistor for IBB ramp-down, which obviously brought the
need of implementing a of_parse callback for both regulators.
Finally, also implement pull-down mode in order to avoid unpredictable
behavior when the regulators are disabled (random voltage spikes etc).
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 94 +++++++++++++++++++++++
1 file changed, 94 insertions(+)
@@ -120,6 +133,70 @@ static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)return(cur_step*lim->uA_step)+lim->uA_min;}+staticintqcom_labibb_set_soft_start(structregulator_dev*rdev)+{+structlabibb_regulator*vreg=rdev_get_drvdata(rdev);+u32val=0;++if(vreg->type==QCOM_IBB_TYPE)+val=vreg->dischg_sel;+else+val=vreg->soft_start_sel;++returnregmap_write(rdev->regmap,rdev->desc->soft_start_reg,val);+}++staticintqcom_labibb_get_table_sel(constint*table,intsz,u32value)+{+inti;++for(i=0;i<sz;i++)+if(table[i]==value)+returni;+return-EINVAL;+}++/* IBB discharge resistor values in KOhms */+staticconstintdischg_resistor_values[]={300,64,32,16};++/* Soft start time in microseconds */+staticconstintsoft_start_values[]={200,400,600,800};++staticintqcom_labibb_of_parse_cb(structdevice_node*np,+conststructregulator_desc*desc,+structregulator_config*config)+{+structlabibb_regulator*vreg=config->driver_data;+u32dischg_kohms,soft_start_time;+intret;++ret=of_property_read_u32(np,"qcom,discharge-resistor-kohms",+&dischg_kohms);+if(ret)+dischg_kohms=300;
Nit, if you just initialize dischg_kohms to 300 during definition you
can rely on of_property_read_u32() not updating the value on failure...
That said, I think this patch looks good.
Reviewed-by: Bjorn Andersson <redacted>
Regards,
Bjorn
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-17 18:18:45
Il 15/01/21 05:53, Bjorn Andersson ha scritto:
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
quoted
Soft start is required to avoid inrush current during LAB ramp-up and
IBB ramp-down, protecting connected hardware to which we supply voltage.
Since soft start is configurable on both LAB and IBB regulators, it
was necessary to add two DT properties, respectively "qcom,soft-start-us"
to control LAB ramp-up and "qcom,discharge-resistor-kohms" to control
the discharge resistor for IBB ramp-down, which obviously brought the
need of implementing a of_parse callback for both regulators.
Finally, also implement pull-down mode in order to avoid unpredictable
behavior when the regulators are disabled (random voltage spikes etc).
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 94 +++++++++++++++++++++++
1 file changed, 94 insertions(+)
@@ -120,6 +133,70 @@ static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)return(cur_step*lim->uA_step)+lim->uA_min;}+staticintqcom_labibb_set_soft_start(structregulator_dev*rdev)+{+structlabibb_regulator*vreg=rdev_get_drvdata(rdev);+u32val=0;++if(vreg->type==QCOM_IBB_TYPE)+val=vreg->dischg_sel;+else+val=vreg->soft_start_sel;++returnregmap_write(rdev->regmap,rdev->desc->soft_start_reg,val);+}++staticintqcom_labibb_get_table_sel(constint*table,intsz,u32value)+{+inti;++for(i=0;i<sz;i++)+if(table[i]==value)+returni;+return-EINVAL;+}++/* IBB discharge resistor values in KOhms */+staticconstintdischg_resistor_values[]={300,64,32,16};++/* Soft start time in microseconds */+staticconstintsoft_start_values[]={200,400,600,800};++staticintqcom_labibb_of_parse_cb(structdevice_node*np,+conststructregulator_desc*desc,+structregulator_config*config)+{+structlabibb_regulator*vreg=config->driver_data;+u32dischg_kohms,soft_start_time;+intret;++ret=of_property_read_u32(np,"qcom,discharge-resistor-kohms",+&dischg_kohms);+if(ret)+dischg_kohms=300;
Nit, if you just initialize dischg_kohms to 300 during definition you
can rely on of_property_read_u32() not updating the value on failure...
I can change it if that's really necessary, I did it like this in an
attempt of spoon-feed the logic to the reader, but perhaps just
initializing it during definition would achieve the same, anyway.
Should I?
That said, I think this patch looks good.
Reviewed-by: Bjorn Andersson <redacted>
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:43:53
Document properties to configure soft start and discharge resistor
for LAB and IBB respectively.
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
.../bindings/regulator/qcom-labibb-regulator.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
@@ -22,6 +22,11 @@ properties:type:objectproperties:+qcom,soft-start-us:+$ref:/schemas/types.yaml#/definitions/uint32+description:Regulator soft start time in microseconds.+enum:[200,400,600,800]+default:200interrupts:maxItems:1
@@ -35,6 +40,11 @@ properties:type:objectproperties:+qcom,discharge-resistor-kohms:+$ref:/schemas/types.yaml#/definitions/uint32+description:Discharge resistor value in KiloOhms.+enum:[300,64,32,16]+default:300interrupts:maxItems:1
@@ -22,6 +22,11 @@ properties:type:objectproperties:+qcom,soft-start-us:+$ref:/schemas/types.yaml#/definitions/uint32+description:Regulator soft start time in microseconds.+enum:[200,400,600,800]+default:200interrupts:maxItems:1
@@ -35,6 +40,11 @@ properties:type:objectproperties:+qcom,discharge-resistor-kohms:+$ref:/schemas/types.yaml#/definitions/uint32+description:Discharge resistor value in KiloOhms.+enum:[300,64,32,16]+default:300interrupts:maxItems:1
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:43:53
Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
very important for regulators like LAB and IBB, which are designed to
provide from very small to relatively big amounts of current to the
device (normally, a display).
Now that this regulator supports both voltage setting and current
limiting in this driver, to me it looked like being somehow essential
to provide support for SCP and OCP, for two reasons:
1. SCP is a drastic measure to prevent damaging "more" hardware in
the worst situations, if any was damaged, preventing potentially
drastic issues;
2. OCP is a great way to protect the hardware that we're powering
through these regulators as if anything bad happens, the HW will
draw more current than expected: in this case, the OCP interrupt
will fire and the regulators will be immediately shut down,
preventing hardware damage in many cases.
Both interrupts were successfully tested in a "sort-of" controlled
manner, with the following methodology:
Short-Circuit Protection (SCP):
1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA;
2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces
on a FxTec Pro1 smartphone for a very brief time (in short words,
"just a rapid touch with flying wires");
3. The Short-Circuit protection trips: IRQ raises, regulators get
cut. Recovery OK, test repeated without rebooting, OK.
Over-Current Protection (OCP):
1. Set LAB/IBB to the expected voltage to power up the display of
a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set
current limit to LAB 200mA, IBB 50mA (the values that this
display unit needs are 200/800mA);
2. Boot the kernel: OCP fires. Recovery never happens because
the selected current limit is too low, but that's expected.
Test OK.
3. Set LAB/IBB to the expected current limits for XZ Premium
(LAB 200mA, IBB 800mA), but lower than expected voltage,
specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V);
4. Boot the kernel: OCP fires. Recovery never happens because
the selected voltage (still in the working range limits)
is producing a current draw of more than 200mA on LAB.
Test OK.
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++-
1 file changed, 444 insertions(+), 3 deletions(-)
@@ -82,6 +105,379 @@ struct labibb_regulator_data {conststructregulator_desc*desc;};+staticintqcom_labibb_ocp_hw_enable(structregulator_dev*rdev)+{+structlabibb_regulator*vreg=rdev_get_drvdata(rdev);+intret;++/* Clear irq latch status to avoid spurious event */+ret=regmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_LATCHED_CLR,+LABIBB_INT_VREG_OK,1);+if(ret)+returnret;++/* Enable OCP HW interrupt */+returnregmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_EN_SET,+LABIBB_INT_VREG_OK,1);+}++staticintqcom_labibb_ocp_hw_disable(structregulator_dev*rdev)+{+structlabibb_regulator*vreg=rdev_get_drvdata(rdev);++returnregmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_EN_CLR,+LABIBB_INT_VREG_OK,1);+}++/*+*qcom_labibb_check_ocp_status-ChecktheOver-CurrentProtectionstatus+*@rdev:Regulatordevice+*+*ThisfunctioncheckstheSTATUS1registerfortheVREG_OKbit:ifitis+*set,thenthereisnoOver-Currentevent.+*+*Returns:Zeroifthereisnoover-current,1ifinover-currentor+*negativenumberforerror+*/+staticintqcom_labibb_check_ocp_status(structlabibb_regulator*vreg)+{+u32cur_status;+intret;++ret=regmap_read(vreg->rdev->regmap,vreg->base+REG_LABIBB_STATUS1,+&cur_status);+if(ret)+returnret;++return!(cur_status&LABIBB_STATUS1_VREG_OK_BIT);+}++staticvoidqcom_labibb_ocp_recovery_worker(structwork_struct*work)+{+structlabibb_regulator*vreg;+conststructregulator_ops*ops;+intret;++vreg=container_of(work,structlabibb_regulator,+ocp_recovery_work.work);+ops=vreg->rdev->desc->ops;++if(vreg->ocp_irq_count>=LABIBB_MAX_OCP_COUNT){+/*+*Ifwetriedtodisabletheregulatormultipletimesbut+*wekeptfailing,there'sonlyonelasthopetosaveour+*hardwarefromthedeath:raiseakernelbug,rebootand+*hopethatthebootloaderkindlysavesus.This,though+*isdoneonlyasparanoidchecking,becausefailingthe+*regmapwritetodisablethevregisalmostimpossible,+*sincewegothereaftermultipleregmapR/W.+*/+BUG_ON(vreg->fatal_count>LABIBB_MAX_FATAL_COUNT);+dev_err(&vreg->rdev->dev,"LABIBB: CRITICAL: Disabling regulator\n");++/* Disable the regulator immediately to avoid damage */+ret=ops->disable(vreg->rdev);+if(ret){+vreg->fatal_count++;+gotoreschedule;+}+enable_irq(vreg->ocp_irq);+vreg->fatal_count=0;+return;+}++ret=qcom_labibb_check_ocp_status(vreg);+if(ret!=0){+vreg->ocp_irq_count++;+gotoreschedule;+}++ret=qcom_labibb_ocp_hw_enable(vreg->rdev);+if(ret){+/* We cannot trust it without OCP enabled. */+dev_err(vreg->dev,"Cannot enable OCP IRQ\n");+vreg->ocp_irq_count++;+gotoreschedule;+}++enable_irq(vreg->ocp_irq);+/* Everything went fine: reset the OCP count! */+vreg->ocp_irq_count=0;+return;++reschedule:+mod_delayed_work(system_wq,&vreg->ocp_recovery_work,+msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));+}++staticirqreturn_tqcom_labibb_ocp_isr(intirq,void*chip)+{+structlabibb_regulator*vreg=chip;+conststructregulator_ops*ops=vreg->rdev->desc->ops;+intret;++/* If the regulator is not enabled, this is a fake event */+if(!ops->is_enabled(vreg->rdev))+return0;++/* If we tried to recover for too many times it's not getting better */+if(vreg->ocp_irq_count>LABIBB_MAX_OCP_COUNT)+returnIRQ_NONE;++/*+*Ifwe(unlikely)can'treadthisregister,topreventhardware+*damageatallcosts,weassumethattheovercurrenteventwas+*real;Moreover,ifthestatusregisterisnotsignalingOCP,+*itwasaspuriousevent,soit'sallok.+*/+ret=qcom_labibb_check_ocp_status(vreg);+if(ret==0){+vreg->ocp_irq_count=0;+gotoend;+}+vreg->ocp_irq_count++;++/*+*Disabletheinterrupttemporarily,oritwillfirecontinuously;+*wewillre-enableitintherecoveryworkerfunction.+*/+disable_irq(irq);++/* Warn the user for overcurrent */+dev_warn(vreg->dev,"Over-Current interrupt fired!\n");++/* Disable the interrupt to avoid hogging */+ret=qcom_labibb_ocp_hw_disable(vreg->rdev);+if(ret)+gotoend;++/* Signal overcurrent event to drivers */+regulator_notifier_call_chain(vreg->rdev,+REGULATOR_EVENT_OVER_CURRENT,NULL);++end:+/* Schedule the recovery work */+schedule_delayed_work(&vreg->ocp_recovery_work,+msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));+if(ret)+returnIRQ_NONE;++returnIRQ_HANDLED;+}++staticintqcom_labibb_set_ocp(structregulator_dev*rdev)+{+structlabibb_regulator*vreg=rdev_get_drvdata(rdev);+char*ocp_irq_name;+u32irq_flags=IRQF_ONESHOT;+intirq_trig_low,ret;++/* If there is no OCP interrupt, there's nothing to set */+if(vreg->ocp_irq<=0)+return-EINVAL;++ocp_irq_name=devm_kasprintf(vreg->dev,GFP_KERNEL,"%s-over-current",+vreg->desc.name);+if(!ocp_irq_name)+return-ENOMEM;++/* IRQ polarities - LAB: trigger-low, IBB: trigger-high */+switch(vreg->type){+caseQCOM_LAB_TYPE:+irq_flags|=IRQF_TRIGGER_LOW;+irq_trig_low=1;+break;+caseQCOM_IBB_TYPE:+irq_flags|=IRQF_TRIGGER_HIGH;+irq_trig_low=0;+break;+default:+return-EINVAL;+}++/* Activate OCP HW level interrupt */+ret=regmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_SET_TYPE,+LABIBB_INT_VREG_OK,+LABIBB_INT_VREG_TYPE_LEVEL);+if(ret)+returnret;++/* Set OCP interrupt polarity */+ret=regmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_POLARITY_HIGH,+LABIBB_INT_VREG_OK,!irq_trig_low);+if(ret)+returnret;+ret=regmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_POLARITY_LOW,+LABIBB_INT_VREG_OK,irq_trig_low);+if(ret)+returnret;++ret=qcom_labibb_ocp_hw_enable(rdev);+if(ret)+returnret;++returndevm_request_threaded_irq(vreg->dev,vreg->ocp_irq,NULL,+qcom_labibb_ocp_isr,irq_flags,+ocp_irq_name,vreg);+}++/*+*qcom_labibb_check_sc_status-ChecktheShortCircuitProtectionstatus+*@rdev:Regulatordevice+*+*ThisfunctioncheckstheSTATUS1registeronbothLABandIBBregulators+*fortheShortCircuitbit:ifitisseton*any*ofthem,thenwehave+*experiencedashort-circuitevent.+*+*Returns:Zeroifthereisnoshort-circuit,1ifinshort-circuitor+*negativenumberforerror+*/+staticintqcom_labibb_check_sc_status(structlabibb_regulator*vreg)+{+u32ibb_status,ibb_reg,lab_status,lab_reg;+intret;++/* We have to work on both regulators due to PBS... */+lab_reg=ibb_reg=vreg->base;+if(vreg->type==QCOM_LAB_TYPE)+ibb_reg-=PMI8998_IBB_LAB_REG_OFFSET;+else+lab_reg+=PMI8998_IBB_LAB_REG_OFFSET;++ret=regmap_read(vreg->rdev->regmap,lab_reg,&lab_status);+if(ret)+returnret;+ret=regmap_read(vreg->rdev->regmap,ibb_reg,&ibb_status);+if(ret)+returnret;++return!!(lab_status&LABIBB_STATUS1_SC_BIT)||+!!(ibb_status&LABIBB_STATUS1_SC_BIT);+}++staticvoidqcom_labibb_sc_recovery_worker(structwork_struct*work)+{+structlabibb_regulator*vreg;+conststructregulator_ops*ops;+u32lab_reg,ibb_reg,temp,val;+boolpbs_cut=false;+inti,sc,ret;++vreg=container_of(work,structlabibb_regulator,+sc_recovery_work.work);+ops=vreg->rdev->desc->ops;++/*+*Ifwetriedtochecktheregulatorstatusmultipletimesbutwe+*keptfailing,thenjustbailout,asthePortableBatchSystem+*(PBS)willdisablethevregsforus,preventinghardwaredamage.+*/+if(vreg->fatal_count>LABIBB_MAX_FATAL_COUNT)+return;++/* Too many short-circuit events. Throw in the towel. */+if(vreg->sc_count>LABIBB_MAX_SC_COUNT)+return;++/*+*ThePortableBatchSystem(PBS)automaticallydisablesLAB+*andIBBwhenashort-circuiteventisdetected,sowehaveto+*checkandworkonbothofthematthesametime.+*/+lab_reg=ibb_reg=vreg->base;+if(vreg->type==QCOM_LAB_TYPE)+ibb_reg-=PMI8998_IBB_LAB_REG_OFFSET;+else+lab_reg+=PMI8998_IBB_LAB_REG_OFFSET;++sc=qcom_labibb_check_sc_status(vreg);+if(sc)+gotoreschedule;++for(i=0;i<LABIBB_MAX_SC_COUNT;i++){+ret=regmap_read(vreg->regmap,lab_reg,&temp);+if(ret){+vreg->fatal_count++;+gotoreschedule;+}+val=temp;++ret=regmap_read(vreg->regmap,ibb_reg,&temp);+if(ret){+vreg->fatal_count++;+gotoreschedule;+}+val&=temp;++if(val&LABIBB_CONTROL_ENABLE){+usleep_range(5000,6000);+continue;+}+pbs_cut=true;+break;+}+if(pbs_cut)+gotoreschedule;++/*+*Ifwehavereachedthispoint,weeitherhadaspuriousSCIRQ+*orwehavesuccessfullyrecoveredfromtheSCcondition,which+*meansthatwecanre-enabletheregulators,iftheyhaveever+*beendisabledbythePBS.+*/+ret=ops->enable(vreg->rdev);+if(ret)+gotoreschedule;++/* Everything went fine: reset the OCP count! */+vreg->sc_count=0;+enable_irq(vreg->sc_irq);+return;++reschedule:+/*+*Nowthatwehavedonebasichandlingoftheshort-circuit,+*reschedulethisworkerintheregularsystemworkqueue,as+*takingactionisnottrulyurgentanymore.+*/+vreg->sc_count++;+mod_delayed_work(system_wq,&vreg->sc_recovery_work,+msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));+}++staticirqreturn_tqcom_labibb_sc_isr(intirq,void*chip)+{+structlabibb_regulator*vreg=chip;++if(vreg->sc_count>LABIBB_MAX_SC_COUNT)+returnIRQ_NONE;++/* Warn the user for short circuit */+dev_warn(vreg->dev,"Short-Circuit interrupt fired!\n");++/*+*Disabletheinterrupttemporarily,oritwillfirecontinuously;+*wewillre-enableitintherecoveryworkerfunction.+*/+disable_irq(irq);++/* Signal out of regulation event to drivers */+regulator_notifier_call_chain(vreg->rdev,+REGULATOR_EVENT_REGULATION_OUT,NULL);++/* Schedule the short-circuit handling as high-priority work */+mod_delayed_work(system_highpri_wq,&vreg->sc_recovery_work,+msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));+returnIRQ_HANDLED;+}++staticintqcom_labibb_set_current_limit(structregulator_dev*rdev,intmin_uA,intmax_uA){
@@ -291,7 +688,7 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)structlabibb_regulator*vreg;structdevice*dev=&pdev->dev;structregulator_configcfg={};-+structdevice_node*reg_node;conststructof_device_id*match;conststructlabibb_regulator_data*reg_data;structregmap*reg_regmap;
@@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)return-ENODEV;for(reg_data=match->data;reg_data->name;reg_data++){+char*sc_irq_name;+intirq=0;/* Validate if the type of regulator is indeed*what'smentionedinDT.
@@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)if(!vreg)return-ENOMEM;+sc_irq_name=devm_kasprintf(dev,GFP_KERNEL,+"%s-short-circuit",+reg_data->name);+if(!sc_irq_name)+return-ENOMEM;++reg_node=of_get_child_by_name(pdev->dev.of_node,+reg_data->name);+if(!reg_node)+return-EINVAL;++/* The Short Circuit interrupt is critical */+irq=of_irq_get_byname(reg_node,"sc-err");+if(irq<=0){+if(irq==0)+irq=-EINVAL;++returndev_err_probe(vreg->dev,irq,+"Short-circuit irq not found.\n");+}+vreg->sc_irq=irq;++/* OverCurrent Protection IRQ is optional */+irq=of_irq_get_byname(reg_node,"ocp");+vreg->ocp_irq=irq;+vreg->ocp_irq_count=0;+of_node_put(reg_node);+vreg->regmap=reg_regmap;vreg->dev=dev;vreg->base=reg_data->base;vreg->type=reg_data->type;+INIT_DELAYED_WORK(&vreg->sc_recovery_work,+qcom_labibb_sc_recovery_worker);++if(vreg->ocp_irq>0)+INIT_DELAYED_WORK(&vreg->ocp_recovery_work,+qcom_labibb_ocp_recovery_worker);switch(vreg->type){caseQCOM_LAB_TYPE:
@@ -369,6 +802,14 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev)reg_data->name,ret);returnPTR_ERR(vreg->rdev);}++ret=devm_request_threaded_irq(vreg->dev,vreg->sc_irq,NULL,+qcom_labibb_sc_isr,+IRQF_ONESHOT|+IRQF_TRIGGER_RISING,+sc_irq_name,vreg);+if(ret)+returnret;}return0;
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
very important for regulators like LAB and IBB, which are designed to
provide from very small to relatively big amounts of current to the
device (normally, a display).
Now that this regulator supports both voltage setting and current
limiting in this driver, to me it looked like being somehow essential
to provide support for SCP and OCP, for two reasons:
1. SCP is a drastic measure to prevent damaging "more" hardware in
the worst situations, if any was damaged, preventing potentially
drastic issues;
2. OCP is a great way to protect the hardware that we're powering
through these regulators as if anything bad happens, the HW will
draw more current than expected: in this case, the OCP interrupt
will fire and the regulators will be immediately shut down,
preventing hardware damage in many cases.
So when the OCP fires it stops the regulator? Is it automatically
enabled by the re-enabling of the OCP interrupt (if so please mention
this in a comment or so in the code), or does it simply signal the
client driver and it will have to re-enable the regulator?
quoted hunk
Both interrupts were successfully tested in a "sort-of" controlled
manner, with the following methodology:
Short-Circuit Protection (SCP):
1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA;
2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces
on a FxTec Pro1 smartphone for a very brief time (in short words,
"just a rapid touch with flying wires");
3. The Short-Circuit protection trips: IRQ raises, regulators get
cut. Recovery OK, test repeated without rebooting, OK.
Over-Current Protection (OCP):
1. Set LAB/IBB to the expected voltage to power up the display of
a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set
current limit to LAB 200mA, IBB 50mA (the values that this
display unit needs are 200/800mA);
2. Boot the kernel: OCP fires. Recovery never happens because
the selected current limit is too low, but that's expected.
Test OK.
3. Set LAB/IBB to the expected current limits for XZ Premium
(LAB 200mA, IBB 800mA), but lower than expected voltage,
specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V);
4. Boot the kernel: OCP fires. Recovery never happens because
the selected voltage (still in the working range limits)
is producing a current draw of more than 200mA on LAB.
Test OK.
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++-
1 file changed, 444 insertions(+), 3 deletions(-)
@@ -82,6 +105,379 @@ struct labibb_regulator_data {conststructregulator_desc*desc;};+staticintqcom_labibb_ocp_hw_enable(structregulator_dev*rdev)+{+structlabibb_regulator*vreg=rdev_get_drvdata(rdev);+intret;++/* Clear irq latch status to avoid spurious event */+ret=regmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_LATCHED_CLR,+LABIBB_INT_VREG_OK,1);
Either the clear register reads as 0, making the read-modify-write
pointless, or there are 1s in there which we inadvertently will use to
clear unrelated interrupts with.
So a regmap_write() seems more appropriate.
Please add another '*' to complete the /** and () to the function name,
if you intend for this to be valid kerneldoc.
+ * qcom_labibb_check_ocp_status - Check the Over-Current Protection status
+ * @rdev: Regulator device
rdev != vreg
+ *
+ * This function checks the STATUS1 register for the VREG_OK bit: if it is
+ * set, then there is no Over-Current event.
+ *
+ * Returns: Zero if there is no over-current, 1 if in over-current or
+ * negative number for error
+ */
+static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg)
+{
+ u32 cur_status;
+ int ret;
+
+ ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1,
+ &cur_status);
+ if (ret)
+ return ret;
+
+ return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT);
+}
+
+static void qcom_labibb_ocp_recovery_worker(struct work_struct *work)
+{
+ struct labibb_regulator *vreg;
+ const struct regulator_ops *ops;
+ int ret;
+
+ vreg = container_of(work, struct labibb_regulator,
+ ocp_recovery_work.work);
+ ops = vreg->rdev->desc->ops;
+
+ if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) {
+ /*
+ * If we tried to disable the regulator multiple times but
+ * we kept failing, there's only one last hope to save our
+ * hardware from the death: raise a kernel bug, reboot and
+ * hope that the bootloader kindly saves us. This, though
+ * is done only as paranoid checking, because failing the
+ * regmap write to disable the vreg is almost impossible,
+ * since we got here after multiple regmap R/W.
+ */
+ BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT);
+ dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n");
+
+ /* Disable the regulator immediately to avoid damage */
+ ret = ops->disable(vreg->rdev);
+ if (ret) {
+ vreg->fatal_count++;
+ goto reschedule;
+ }
+ enable_irq(vreg->ocp_irq);
+ vreg->fatal_count = 0;
+ return;
+ }
+
+ ret = qcom_labibb_check_ocp_status(vreg);
+ if (ret != 0) {
+ vreg->ocp_irq_count++;
+ goto reschedule;
+ }
+
+ ret = qcom_labibb_ocp_hw_enable(vreg->rdev);
+ if (ret) {
+ /* We cannot trust it without OCP enabled. */
+ dev_err(vreg->dev, "Cannot enable OCP IRQ\n");
+ vreg->ocp_irq_count++;
+ goto reschedule;
+ }
+
+ enable_irq(vreg->ocp_irq);
+ /* Everything went fine: reset the OCP count! */
+ vreg->ocp_irq_count = 0;
+ return;
+
+reschedule:
+ mod_delayed_work(system_wq, &vreg->ocp_recovery_work,
+ msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
+}
+
+static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
+{
+ struct labibb_regulator *vreg = chip;
+ const struct regulator_ops *ops = vreg->rdev->desc->ops;
+ int ret;
+
+ /* If the regulator is not enabled, this is a fake event */
+ if (!ops->is_enabled(vreg->rdev))
+ return 0;
+
+ /* If we tried to recover for too many times it's not getting better */
+ if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT)
+ return IRQ_NONE;
+
+ /*
+ * If we (unlikely) can't read this register, to prevent hardware
+ * damage at all costs, we assume that the overcurrent event was
+ * real; Moreover, if the status register is not signaling OCP,
+ * it was a spurious event, so it's all ok.
+ */
+ ret = qcom_labibb_check_ocp_status(vreg);
+ if (ret == 0) {
+ vreg->ocp_irq_count = 0;
+ goto end;
+ }
+ vreg->ocp_irq_count++;
+
+ /*
+ * Disable the interrupt temporarily, or it will fire continuously;
+ * we will re-enable it in the recovery worker function.
+ */
+ disable_irq(irq);
+
+ /* Warn the user for overcurrent */
+ dev_warn(vreg->dev, "Over-Current interrupt fired!\n");
+
+ /* Disable the interrupt to avoid hogging */
+ ret = qcom_labibb_ocp_hw_disable(vreg->rdev);
+ if (ret)
+ goto end;
+
+ /* Signal overcurrent event to drivers */
+ regulator_notifier_call_chain(vreg->rdev,
+ REGULATOR_EVENT_OVER_CURRENT, NULL);
+
+end:
+ /* Schedule the recovery work */
+ schedule_delayed_work(&vreg->ocp_recovery_work,
+ msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
+ if (ret)
+ return IRQ_NONE;
+
+ return IRQ_HANDLED;
+}
+
+static int qcom_labibb_set_ocp(struct regulator_dev *rdev)
+{
+ struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+ char *ocp_irq_name;
+ u32 irq_flags = IRQF_ONESHOT;
+ int irq_trig_low, ret;
+
+ /* If there is no OCP interrupt, there's nothing to set */
+ if (vreg->ocp_irq <= 0)
+ return -EINVAL;
+
+ ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current",
+ vreg->desc.name);
+ if (!ocp_irq_name)
+ return -ENOMEM;
+
+ /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
+ switch (vreg->type) {
+ case QCOM_LAB_TYPE:
+ irq_flags |= IRQF_TRIGGER_LOW;
+ irq_trig_low = 1;
+ break;
+ case QCOM_IBB_TYPE:
+ irq_flags |= IRQF_TRIGGER_HIGH;
+ irq_trig_low = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* Activate OCP HW level interrupt */
+ ret = regmap_update_bits(rdev->regmap,
+ vreg->base + REG_LABIBB_INT_SET_TYPE,
+ LABIBB_INT_VREG_OK,
+ LABIBB_INT_VREG_TYPE_LEVEL);
+ if (ret)
+ return ret;
+
+ /* Set OCP interrupt polarity */
+ ret = regmap_update_bits(rdev->regmap,
+ vreg->base + REG_LABIBB_INT_POLARITY_HIGH,
+ LABIBB_INT_VREG_OK, !irq_trig_low);
+ if (ret)
+ return ret;
+ ret = regmap_update_bits(rdev->regmap,
+ vreg->base + REG_LABIBB_INT_POLARITY_LOW,
+ LABIBB_INT_VREG_OK, irq_trig_low);
+ if (ret)
+ return ret;
+
+ ret = qcom_labibb_ocp_hw_enable(rdev);
+ if (ret)
+ return ret;
+
+ return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
+ qcom_labibb_ocp_isr, irq_flags,
+ ocp_irq_name, vreg);
+}
+
+/*
+ * qcom_labibb_check_sc_status - Check the Short Circuit Protection status
+ * @rdev: Regulator device
+ *
+ * This function checks the STATUS1 register on both LAB and IBB regulators
+ * for the ShortCircuit bit: if it is set on *any* of them, then we have
+ * experienced a short-circuit event.
+ *
+ * Returns: Zero if there is no short-circuit, 1 if in short-circuit or
+ * negative number for error
+ */
+static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg)
+{
+ u32 ibb_status, ibb_reg, lab_status, lab_reg;
+ int ret;
+
+ /* We have to work on both regulators due to PBS... */
+ lab_reg = ibb_reg = vreg->base;
+ if (vreg->type == QCOM_LAB_TYPE)
+ ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
Minus? The register comes before the base address?
+ else
+ lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
+
+ ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status);
+ if (ret)
+ return ret;
+ ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status);
+ if (ret)
+ return ret;
+
+ return !!(lab_status & LABIBB_STATUS1_SC_BIT) ||
+ !!(ibb_status & LABIBB_STATUS1_SC_BIT);
+}
+
+static void qcom_labibb_sc_recovery_worker(struct work_struct *work)
+{
+ struct labibb_regulator *vreg;
+ const struct regulator_ops *ops;
+ u32 lab_reg, ibb_reg, temp, val;
+ bool pbs_cut = false;
+ int i, sc, ret;
+
+ vreg = container_of(work, struct labibb_regulator,
+ sc_recovery_work.work);
+ ops = vreg->rdev->desc->ops;
+
+ /*
+ * If we tried to check the regulator status multiple times but we
+ * kept failing, then just bail out, as the Portable Batch System
+ * (PBS) will disable the vregs for us, preventing hardware damage.
+ */
+ if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT)
+ return;
+
+ /* Too many short-circuit events. Throw in the towel. */
+ if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
+ return;
+
+ /*
+ * The Portable Batch System (PBS) automatically disables LAB
+ * and IBB when a short-circuit event is detected, so we have to
+ * check and work on both of them at the same time.
+ */
+ lab_reg = ibb_reg = vreg->base;
+ if (vreg->type == QCOM_LAB_TYPE)
+ ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
+ else
+ lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
+
+ sc = qcom_labibb_check_sc_status(vreg);
+ if (sc)
+ goto reschedule;
+
+ for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) {
+ ret = regmap_read(vreg->regmap, lab_reg, &temp);
+ if (ret) {
+ vreg->fatal_count++;
+ goto reschedule;
+ }
+ val = temp;
+
+ ret = regmap_read(vreg->regmap, ibb_reg, &temp);
+ if (ret) {
+ vreg->fatal_count++;
+ goto reschedule;
+ }
+ val &= temp;
Perhaps using two variables, with suitable names makes this more
maintainable?
+
+ if (val & LABIBB_CONTROL_ENABLE) {
Flip this around and do pbs_cut = true; break; in the conditional, to
not end the loop with a conditional continue and an unconditional break.
+ usleep_range(5000, 6000);
+ continue;
+ }
+ pbs_cut = true;
+ break;
+ }
+ if (pbs_cut)
+ goto reschedule;
+
+ /*
+ * If we have reached this point, we either had a spurious SC IRQ
+ * or we have successfully recovered from the SC condition, which
Wouldn't it make sense to put the "recovered from the SC condition"
before the "spurious" part in this sentence - just seems like this is
the scenario we're most likely looking for.
Regards,
Bjorn
quoted hunk
+ * means that we can re-enable the regulators, if they have ever
+ * been disabled by the PBS.
+ */
+ ret = ops->enable(vreg->rdev);
+ if (ret)
+ goto reschedule;
+
+ /* Everything went fine: reset the OCP count! */
+ vreg->sc_count = 0;
+ enable_irq(vreg->sc_irq);
+ return;
+
+reschedule:
+ /*
+ * Now that we have done basic handling of the short-circuit,
+ * reschedule this worker in the regular system workqueue, as
+ * taking action is not truly urgent anymore.
+ */
+ vreg->sc_count++;
+ mod_delayed_work(system_wq, &vreg->sc_recovery_work,
+ msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
+}
+
+static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip)
+{
+ struct labibb_regulator *vreg = chip;
+
+ if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
+ return IRQ_NONE;
+
+ /* Warn the user for short circuit */
+ dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n");
+
+ /*
+ * Disable the interrupt temporarily, or it will fire continuously;
+ * we will re-enable it in the recovery worker function.
+ */
+ disable_irq(irq);
+
+ /* Signal out of regulation event to drivers */
+ regulator_notifier_call_chain(vreg->rdev,
+ REGULATOR_EVENT_REGULATION_OUT, NULL);
+
+ /* Schedule the short-circuit handling as high-priority work */
+ mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work,
+ msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
+ return IRQ_HANDLED;
+}
+
+
static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
int min_uA, int max_uA)
{
@@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) return -ENODEV; for (reg_data = match->data; reg_data->name; reg_data++) {+ char *sc_irq_name;+ int irq = 0; /* Validate if the type of regulator is indeed * what's mentioned in DT.
@@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) if (!vreg) return -ENOMEM;+ sc_irq_name = devm_kasprintf(dev, GFP_KERNEL,+ "%s-short-circuit",+ reg_data->name);+ if (!sc_irq_name)+ return -ENOMEM;++ reg_node = of_get_child_by_name(pdev->dev.of_node,+ reg_data->name);+ if (!reg_node)+ return -EINVAL;++ /* The Short Circuit interrupt is critical */+ irq = of_irq_get_byname(reg_node, "sc-err");+ if (irq <= 0) {+ if (irq == 0)+ irq = -EINVAL;++ return dev_err_probe(vreg->dev, irq,+ "Short-circuit irq not found.\n");+ }+ vreg->sc_irq = irq;++ /* OverCurrent Protection IRQ is optional */+ irq = of_irq_get_byname(reg_node, "ocp");+ vreg->ocp_irq = irq;+ vreg->ocp_irq_count = 0;+ of_node_put(reg_node);+ vreg->regmap = reg_regmap; vreg->dev = dev; vreg->base = reg_data->base; vreg->type = reg_data->type;+ INIT_DELAYED_WORK(&vreg->sc_recovery_work,+ qcom_labibb_sc_recovery_worker);++ if (vreg->ocp_irq > 0)+ INIT_DELAYED_WORK(&vreg->ocp_recovery_work,+ qcom_labibb_ocp_recovery_worker); switch (vreg->type) { case QCOM_LAB_TYPE:
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-17 19:16:36
Il 15/01/21 06:31, Bjorn Andersson ha scritto:
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
quoted
Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
very important for regulators like LAB and IBB, which are designed to
provide from very small to relatively big amounts of current to the
device (normally, a display).
Now that this regulator supports both voltage setting and current
limiting in this driver, to me it looked like being somehow essential
to provide support for SCP and OCP, for two reasons:
1. SCP is a drastic measure to prevent damaging "more" hardware in
the worst situations, if any was damaged, preventing potentially
drastic issues;
2. OCP is a great way to protect the hardware that we're powering
through these regulators as if anything bad happens, the HW will
draw more current than expected: in this case, the OCP interrupt
will fire and the regulators will be immediately shut down,
preventing hardware damage in many cases.
So when the OCP fires it stops the regulator? Is it automatically
enabled by the re-enabling of the OCP interrupt (if so please mention
this in a comment or so in the code), or does it simply signal the
client driver and it will have to re-enable the regulator?
I am sorry if my comments in the driver are not clear enough but,
before giving you an answer, let me explain my comment logic:
In function qcom_labibb_ocp_isr():
/* Signal overcurrent event to drivers */
/* Schedule the recovery work */
In function qcom_labibb_ocp_recovery_worker():
if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT)
/*
* If we tried to disable the regulator multiple times but
* we kept failing, there's only one last hope to save our
* hardware from the death: raise a kernel bug, reboot and
* hope that the bootloader kindly saves us. This, though
* is done only as paranoid checking, because failing the
* regmap write to disable the vreg is almost impossible,
* since we got here after multiple regmap R/W.
*/
ret = qcom_labibb_ocp_hw_enable(vreg->rdev);
if (ret)
/* We cannot trust it without OCP enabled. */
(hence, reschedule worker)
/* Everything went fine: reset the OCP count! */
vreg->ocp_irq_count = 0;
So, schematically:
1. Signals overcurrent event to the driver that is attached;
the driver will have to take action on its own - it's not
our problem, for now;
2. Schedules a recovery worker, which waits until we stop reading
"OUCH! OVERCURRENT!" from the hardware and reschedules itself
for 4 times
3. Did the driver take action? Did the hardware just stabilize its
current draw for whatever reason? Now it may be our problem.
3a. (Yes!) Great, we're done, everything is fine;
3b. (No! The driver didn't care, or the hardware is really at
fault!) Force disabling the regulator to protect HW;
3b-alt. We couldn't disable the regulator, something is seriously
wrong, we are frying the HW. PRESS THE RED BUTTON! Panic
and pray that the bootloader saves us and that it's not
too late. We really can't do anything more than that.
Is the flow clear now?
How would you improve the comments in this commit, if necessary?
quoted
Both interrupts were successfully tested in a "sort-of" controlled
manner, with the following methodology:
Short-Circuit Protection (SCP):
1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA;
2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces
on a FxTec Pro1 smartphone for a very brief time (in short words,
"just a rapid touch with flying wires");
3. The Short-Circuit protection trips: IRQ raises, regulators get
cut. Recovery OK, test repeated without rebooting, OK.
Over-Current Protection (OCP):
1. Set LAB/IBB to the expected voltage to power up the display of
a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set
current limit to LAB 200mA, IBB 50mA (the values that this
display unit needs are 200/800mA);
2. Boot the kernel: OCP fires. Recovery never happens because
the selected current limit is too low, but that's expected.
Test OK.
3. Set LAB/IBB to the expected current limits for XZ Premium
(LAB 200mA, IBB 800mA), but lower than expected voltage,
specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V);
4. Boot the kernel: OCP fires. Recovery never happens because
the selected voltage (still in the working range limits)
is producing a current draw of more than 200mA on LAB.
Test OK.
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++-
1 file changed, 444 insertions(+), 3 deletions(-)
@@ -82,6 +105,379 @@ struct labibb_regulator_data {conststructregulator_desc*desc;};+staticintqcom_labibb_ocp_hw_enable(structregulator_dev*rdev)+{+structlabibb_regulator*vreg=rdev_get_drvdata(rdev);+intret;++/* Clear irq latch status to avoid spurious event */+ret=regmap_update_bits(rdev->regmap,+vreg->base+REG_LABIBB_INT_LATCHED_CLR,+LABIBB_INT_VREG_OK,1);
Either the clear register reads as 0, making the read-modify-write
pointless, or there are 1s in there which we inadvertently will use to
clear unrelated interrupts with.
So a regmap_write() seems more appropriate.
I'm not sure about what the other bits are for in that register, that's
why I chose to use regmap_update_bits...
If you are sure that I can safely use regmap_write here, then I will, so
since it is safety protections that we're talking about, I feel the need
to have a 100% sure answer on this, hence I ask:
Are you sure that it is safe to use regmap_write instead, here?
(P.S.: You should know, I don't want to seem rude in any way, it's just
a paranoid concern about safety)
Please add another '*' to complete the /** and () to the function name,
if you intend for this to be valid kerneldoc.
Oh, thank you, that was a typo. Yes I wanted to write /**.
quoted
+ * qcom_labibb_check_ocp_status - Check the Over-Current Protection status
+ * @rdev: Regulator device
rdev != vreg
Of course, @vreg: Main driver structure, not unexistant rdev. Big oops!
quoted
+ *
+ * This function checks the STATUS1 register for the VREG_OK bit: if it is
+ * set, then there is no Over-Current event.
+ *
+ * Returns: Zero if there is no over-current, 1 if in over-current or
+ * negative number for error
+ */
+static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg)
+{
+ u32 cur_status;
+ int ret;
+
+ ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1,
+ &cur_status);
+ if (ret)
+ return ret;
+
+ return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT);
+}
+
+static void qcom_labibb_ocp_recovery_worker(struct work_struct *work)
+{
+ struct labibb_regulator *vreg;
+ const struct regulator_ops *ops;
+ int ret;
+
+ vreg = container_of(work, struct labibb_regulator,
+ ocp_recovery_work.work);
+ ops = vreg->rdev->desc->ops;
+
+ if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) {
+ /*
+ * If we tried to disable the regulator multiple times but
+ * we kept failing, there's only one last hope to save our
+ * hardware from the death: raise a kernel bug, reboot and
+ * hope that the bootloader kindly saves us. This, though
+ * is done only as paranoid checking, because failing the
+ * regmap write to disable the vreg is almost impossible,
+ * since we got here after multiple regmap R/W.
+ */
+ BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT);
+ dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n");
+
+ /* Disable the regulator immediately to avoid damage */
+ ret = ops->disable(vreg->rdev);
+ if (ret) {
+ vreg->fatal_count++;
+ goto reschedule;
+ }
+ enable_irq(vreg->ocp_irq);
+ vreg->fatal_count = 0;
+ return;
+ }
+
+ ret = qcom_labibb_check_ocp_status(vreg);
+ if (ret != 0) {
+ vreg->ocp_irq_count++;
+ goto reschedule;
+ }
+
+ ret = qcom_labibb_ocp_hw_enable(vreg->rdev);
+ if (ret) {
+ /* We cannot trust it without OCP enabled. */
+ dev_err(vreg->dev, "Cannot enable OCP IRQ\n");
+ vreg->ocp_irq_count++;
+ goto reschedule;
+ }
+
+ enable_irq(vreg->ocp_irq);
+ /* Everything went fine: reset the OCP count! */
+ vreg->ocp_irq_count = 0;
+ return;
+
+reschedule:
+ mod_delayed_work(system_wq, &vreg->ocp_recovery_work,
+ msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
+}
+
+static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
+{
+ struct labibb_regulator *vreg = chip;
+ const struct regulator_ops *ops = vreg->rdev->desc->ops;
+ int ret;
+
+ /* If the regulator is not enabled, this is a fake event */
+ if (!ops->is_enabled(vreg->rdev))
+ return 0;
+
+ /* If we tried to recover for too many times it's not getting better */
+ if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT)
+ return IRQ_NONE;
+
+ /*
+ * If we (unlikely) can't read this register, to prevent hardware
+ * damage at all costs, we assume that the overcurrent event was
+ * real; Moreover, if the status register is not signaling OCP,
+ * it was a spurious event, so it's all ok.
+ */
+ ret = qcom_labibb_check_ocp_status(vreg);
+ if (ret == 0) {
+ vreg->ocp_irq_count = 0;
+ goto end;
+ }
+ vreg->ocp_irq_count++;
+
+ /*
+ * Disable the interrupt temporarily, or it will fire continuously;
+ * we will re-enable it in the recovery worker function.
+ */
+ disable_irq(irq);
+
+ /* Warn the user for overcurrent */
+ dev_warn(vreg->dev, "Over-Current interrupt fired!\n");
+
+ /* Disable the interrupt to avoid hogging */
+ ret = qcom_labibb_ocp_hw_disable(vreg->rdev);
+ if (ret)
+ goto end;
+
+ /* Signal overcurrent event to drivers */
+ regulator_notifier_call_chain(vreg->rdev,
+ REGULATOR_EVENT_OVER_CURRENT, NULL);
+
+end:
+ /* Schedule the recovery work */
+ schedule_delayed_work(&vreg->ocp_recovery_work,
+ msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS));
+ if (ret)
+ return IRQ_NONE;
+
+ return IRQ_HANDLED;
+}
+
+static int qcom_labibb_set_ocp(struct regulator_dev *rdev)
+{
+ struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
+ char *ocp_irq_name;
+ u32 irq_flags = IRQF_ONESHOT;
+ int irq_trig_low, ret;
+
+ /* If there is no OCP interrupt, there's nothing to set */
+ if (vreg->ocp_irq <= 0)
+ return -EINVAL;
+
+ ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current",
+ vreg->desc.name);
+ if (!ocp_irq_name)
+ return -ENOMEM;
+
+ /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */
+ switch (vreg->type) {
+ case QCOM_LAB_TYPE:
+ irq_flags |= IRQF_TRIGGER_LOW;
+ irq_trig_low = 1;
+ break;
+ case QCOM_IBB_TYPE:
+ irq_flags |= IRQF_TRIGGER_HIGH;
+ irq_trig_low = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* Activate OCP HW level interrupt */
+ ret = regmap_update_bits(rdev->regmap,
+ vreg->base + REG_LABIBB_INT_SET_TYPE,
+ LABIBB_INT_VREG_OK,
+ LABIBB_INT_VREG_TYPE_LEVEL);
+ if (ret)
+ return ret;
+
+ /* Set OCP interrupt polarity */
+ ret = regmap_update_bits(rdev->regmap,
+ vreg->base + REG_LABIBB_INT_POLARITY_HIGH,
+ LABIBB_INT_VREG_OK, !irq_trig_low);
+ if (ret)
+ return ret;
+ ret = regmap_update_bits(rdev->regmap,
+ vreg->base + REG_LABIBB_INT_POLARITY_LOW,
+ LABIBB_INT_VREG_OK, irq_trig_low);
+ if (ret)
+ return ret;
+
+ ret = qcom_labibb_ocp_hw_enable(rdev);
+ if (ret)
+ return ret;
+
+ return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL,
+ qcom_labibb_ocp_isr, irq_flags,
+ ocp_irq_name, vreg);
+}
+
+/*
+ * qcom_labibb_check_sc_status - Check the Short Circuit Protection status
+ * @rdev: Regulator device
+ *
+ * This function checks the STATUS1 register on both LAB and IBB regulators
+ * for the ShortCircuit bit: if it is set on *any* of them, then we have
+ * experienced a short-circuit event.
+ *
+ * Returns: Zero if there is no short-circuit, 1 if in short-circuit or
+ * negative number for error
+ */
+static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg)
+{
+ u32 ibb_status, ibb_reg, lab_status, lab_reg;
+ int ret;
+
+ /* We have to work on both regulators due to PBS... */
+ lab_reg = ibb_reg = vreg->base;
+ if (vreg->type == QCOM_LAB_TYPE)
+ ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
Minus? The register comes before the base address?
#define PMI8998_LAB_REG_BASE 0xde00
#define PMI8998_IBB_REG_BASE 0xdc00
#define PMI8998_IBB_LAB_REG_OFFSET 0x200
Yeah, it actually does.. should I write this differently?
I did this to avoid stunts to get the regmap base of the other regulator
by storing both bases in both instances or other worst solutions.
In case, have you got any idea that's cleaner than this?
If so, please tell...
Maybe it's stupid-level simple and my brain doesn't want to cooperate...
quoted
+ else
+ lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
+
+ ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status);
+ if (ret)
+ return ret;
+ ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status);
+ if (ret)
+ return ret;
+
+ return !!(lab_status & LABIBB_STATUS1_SC_BIT) ||
+ !!(ibb_status & LABIBB_STATUS1_SC_BIT);
+}
+
+static void qcom_labibb_sc_recovery_worker(struct work_struct *work)
+{
+ struct labibb_regulator *vreg;
+ const struct regulator_ops *ops;
+ u32 lab_reg, ibb_reg, temp, val;
+ bool pbs_cut = false;
+ int i, sc, ret;
+
+ vreg = container_of(work, struct labibb_regulator,
+ sc_recovery_work.work);
+ ops = vreg->rdev->desc->ops;
+
+ /*
+ * If we tried to check the regulator status multiple times but we
+ * kept failing, then just bail out, as the Portable Batch System
+ * (PBS) will disable the vregs for us, preventing hardware damage.
+ */
+ if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT)
+ return;
+
+ /* Too many short-circuit events. Throw in the towel. */
+ if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
+ return;
+
+ /*
+ * The Portable Batch System (PBS) automatically disables LAB
+ * and IBB when a short-circuit event is detected, so we have to
+ * check and work on both of them at the same time.
+ */
+ lab_reg = ibb_reg = vreg->base;
+ if (vreg->type == QCOM_LAB_TYPE)
+ ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET;
+ else
+ lab_reg += PMI8998_IBB_LAB_REG_OFFSET;
+
+ sc = qcom_labibb_check_sc_status(vreg);
+ if (sc)
+ goto reschedule;
+
+ for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) {
+ ret = regmap_read(vreg->regmap, lab_reg, &temp);
+ if (ret) {
+ vreg->fatal_count++;
+ goto reschedule;
+ }
+ val = temp;
+
+ ret = regmap_read(vreg->regmap, ibb_reg, &temp);
+ if (ret) {
+ vreg->fatal_count++;
+ goto reschedule;
+ }
+ val &= temp;
Perhaps using two variables, with suitable names makes this more
maintainable?
Do you mean like this?
(error checking omitted to be short)
regmap_read(..., &lab_val)
regmap_read(..., &ibb_val)
val = lab_val & ibb_val;
quoted
+
+ if (val & LABIBB_CONTROL_ENABLE) {
Flip this around and do pbs_cut = true; break; in the conditional, to
not end the loop with a conditional continue and an unconditional break.
Yeah. Indeed. It makes no sense to have a loop if I appear to
unconditionally break it. I don't know what I was thinking.
Though, fun story: that's my brain's convoluted logic.
quoted
+ usleep_range(5000, 6000);
+ continue;
+ }
+ pbs_cut = true;
+ break;
+ }
+ if (pbs_cut)
+ goto reschedule;
+
+ /*
+ * If we have reached this point, we either had a spurious SC IRQ
+ * or we have successfully recovered from the SC condition, which
Wouldn't it make sense to put the "recovered from the SC condition"
before the "spurious" part in this sentence - just seems like this is
the scenario we're most likely looking for.
Brain convoluted logic strikes again.
I totally agree with you: let's do it like you suggested.
Regards,
Bjorn
quoted
+ * means that we can re-enable the regulators, if they have ever
+ * been disabled by the PBS.
+ */
+ ret = ops->enable(vreg->rdev);
+ if (ret)
+ goto reschedule;
+
+ /* Everything went fine: reset the OCP count! */
+ vreg->sc_count = 0;
+ enable_irq(vreg->sc_irq);
+ return;
+
+reschedule:
+ /*
+ * Now that we have done basic handling of the short-circuit,
+ * reschedule this worker in the regular system workqueue, as
+ * taking action is not truly urgent anymore.
+ */
+ vreg->sc_count++;
+ mod_delayed_work(system_wq, &vreg->sc_recovery_work,
+ msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
+}
+
+static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip)
+{
+ struct labibb_regulator *vreg = chip;
+
+ if (vreg->sc_count > LABIBB_MAX_SC_COUNT)
+ return IRQ_NONE;
+
+ /* Warn the user for short circuit */
+ dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n");
+
+ /*
+ * Disable the interrupt temporarily, or it will fire continuously;
+ * we will re-enable it in the recovery worker function.
+ */
+ disable_irq(irq);
+
+ /* Signal out of regulation event to drivers */
+ regulator_notifier_call_chain(vreg->rdev,
+ REGULATOR_EVENT_REGULATION_OUT, NULL);
+
+ /* Schedule the short-circuit handling as high-priority work */
+ mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work,
+ msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS));
+ return IRQ_HANDLED;
+}
+
+
static int qcom_labibb_set_current_limit(struct regulator_dev *rdev,
int min_uA, int max_uA)
{
@@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) return -ENODEV; for (reg_data = match->data; reg_data->name; reg_data++) {+ char *sc_irq_name;+ int irq = 0; /* Validate if the type of regulator is indeed * what's mentioned in DT.
@@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) if (!vreg) return -ENOMEM;+ sc_irq_name = devm_kasprintf(dev, GFP_KERNEL,+ "%s-short-circuit",+ reg_data->name);+ if (!sc_irq_name)+ return -ENOMEM;++ reg_node = of_get_child_by_name(pdev->dev.of_node,+ reg_data->name);+ if (!reg_node)+ return -EINVAL;++ /* The Short Circuit interrupt is critical */+ irq = of_irq_get_byname(reg_node, "sc-err");+ if (irq <= 0) {+ if (irq == 0)+ irq = -EINVAL;++ return dev_err_probe(vreg->dev, irq,+ "Short-circuit irq not found.\n");+ }+ vreg->sc_irq = irq;++ /* OverCurrent Protection IRQ is optional */+ irq = of_irq_get_byname(reg_node, "ocp");+ vreg->ocp_irq = irq;+ vreg->ocp_irq_count = 0;+ of_node_put(reg_node);+ vreg->regmap = reg_regmap; vreg->dev = dev; vreg->base = reg_data->base; vreg->type = reg_data->type;+ INIT_DELAYED_WORK(&vreg->sc_recovery_work,+ qcom_labibb_sc_recovery_worker);++ if (vreg->ocp_irq > 0)+ INIT_DELAYED_WORK(&vreg->ocp_recovery_work,+ qcom_labibb_ocp_recovery_worker); switch (vreg->type) { case QCOM_LAB_TYPE:
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:44:16
In commit 208921bae696 ("arm64: dts: qcom: pmi8998: Add nodes for
LAB and IBB regulators") bindings for the lab/ibb regulators were
added to the pmi8998 dt, but the original committer has never
specified what the interrupts were for.
LAB and IBB regulators provide two interrupts, SC-ERR (short
circuit error) and VREG-OK but, in that commit, the regulators
were provided with two different types of interrupts;
specifically, IBB had the SC-ERR interrupt, while LAB had the
VREG-OK one, none of which were (luckily) used, since the driver
didn't actually use these at all.
Assuming that the original intention was to have the SC IRQ in
both LAB and IBB, as per the names appearing in documentation,
fix the SCP interrupt.
While at it, also add the OCP interrupt in order to be able to
enable the Over-Current Protection feature, if requested.
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
arch/arm64/boot/dts/qcom/pmi8998.dtsi | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
In commit 208921bae696 ("arm64: dts: qcom: pmi8998: Add nodes for
LAB and IBB regulators") bindings for the lab/ibb regulators were
added to the pmi8998 dt, but the original committer has never
specified what the interrupts were for.
LAB and IBB regulators provide two interrupts, SC-ERR (short
circuit error) and VREG-OK but, in that commit, the regulators
were provided with two different types of interrupts;
specifically, IBB had the SC-ERR interrupt, while LAB had the
VREG-OK one, none of which were (luckily) used, since the driver
didn't actually use these at all.
Assuming that the original intention was to have the SC IRQ in
both LAB and IBB, as per the names appearing in documentation,
fix the SCP interrupt.
While at it, also add the OCP interrupt in order to be able to
enable the Over-Current Protection feature, if requested.
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-13 19:44:16
Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
now implemented in the driver: document the interrupts.
This also fixes wrong documentation about the SCP interrupt for LAB.
Signed-off-by: AngeloGioacchino Del Regno <redacted>
---
.../regulator/qcom-labibb-regulator.yaml | 20 +++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
@@ -29,9 +29,10 @@ properties:default:200interrupts:-maxItems:1+minItems:1+maxItems:2description:-Short-circuit interrupt for lab.+Short-circuit and over-current interrupts for lab.required:-interrupts
@@ -47,9 +48,10 @@ properties:default:300interrupts:-maxItems:1+minItems:1+maxItems:2description:-Short-circuit interrupt for lab.+Short-circuit and over-current interrupts for ibb.required:-interrupts
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:
Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are
now implemented in the driver: document the interrupts.
This also fixes wrong documentation about the SCP interrupt for LAB.
@@ -29,9 +29,10 @@ properties:default:200interrupts:-maxItems:1+minItems:1+maxItems:2description:-Short-circuit interrupt for lab.+Short-circuit and over-current interrupts for lab.required:-interrupts
@@ -47,9 +48,10 @@ properties:default:300interrupts:-maxItems:1+minItems:1+maxItems:2description:-Short-circuit interrupt for lab.+Short-circuit and over-current interrupts for ibb.required:-interrupts
From: Mark Brown <broonie@kernel.org> Date: 2021-01-15 18:21:32
On Wed, 13 Jan 2021 20:42:07 +0100, AngeloGioacchino Del Regno wrote:
Okay, the title may be a little "aggressive"? However, the qcom-labibb
driver wasn't really .. doing much.
The current form of this driver is only taking care of enabling or
disabling the regulators, which is pretty useless if they were not
pre-set from the bootloader, which sets them only if continuous
splash is enabled.
Moreover, some bootloaders are setting a higher voltage and/or a higher
current limit compared to what's actually required by the attached
hardware (which is, in 99.9% of the cases, a display) and this produces
a higher power consumption, higher heat output and a risk of actually
burning the display if kept up for a very long time: for example, this
is true on at least some Sony Xperia MSM8998 (Yoshino platform) and
especially on some Sony Xperia SDM845 (Tama platform) smartphones.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/7] regulator: qcom-labibb: Implement voltage selector ops
commit: dd582369c6c1f39ec475af6191a934f3e57fda35
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From: AngeloGioacchino Del Regno <hidden> Date: 2021-01-15 19:16:40
Il 15/01/21 19:19, Mark Brown ha scritto:
On Wed, 13 Jan 2021 20:42:07 +0100, AngeloGioacchino Del Regno wrote:
quoted
Okay, the title may be a little "aggressive"? However, the qcom-labibb
driver wasn't really .. doing much.
The current form of this driver is only taking care of enabling or
disabling the regulators, which is pretty useless if they were not
pre-set from the bootloader, which sets them only if continuous
splash is enabled.
Moreover, some bootloaders are setting a higher voltage and/or a higher
current limit compared to what's actually required by the attached
hardware (which is, in 99.9% of the cases, a display) and this produces
a higher power consumption, higher heat output and a risk of actually
burning the display if kept up for a very long time: for example, this
is true on at least some Sony Xperia MSM8998 (Yoshino platform) and
especially on some Sony Xperia SDM845 (Tama platform) smartphones.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/7] regulator: qcom-labibb: Implement voltage selector ops
commit: dd582369c6c1f39ec475af6191a934f3e57fda35
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Hello Mark,
Thanks for applying the commit, however, I feel like reminding you that
setting voltage on this regulator without setting a current limit is
really unsafe (at least, from many, many experiments) so, to ensure the
entire safety of this code and the hardware that's attached to these
regulators you should, please, either apply the entire series, or nothing.
My concerns are only about keeping LAB and IBB safe for everyone's hardware.
Thank you again
-- Angelo