Re: [PATCH V1 2/4] qcom: spmi-wled: Add support for short circuit handling

From: Bjorn Andersson
Date: Mon Dec 04 2017 - 23:35:31 EST

Next message: Nicolin Chen: "Re: [PATCH 04/10] ASoC: fsl_ssi: Refine all comments"
Previous message: Pravin Shedge: "[PATCH 05/45] drivers: hwtracing: remove duplicate includes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote:

> Handle the short circuit(SC) interrupt and check if the SC interrupt
> is valid. Re-enable the module to check if it goes away. Disable the
> module altogether if the SC event persists.
>
> Signed-off-by: Kiran Gunda <kgunda@xxxxxxxxxxxxxx>
> ---
> .../bindings/leds/backlight/qcom-spmi-wled.txt | 22 ++++
> drivers/video/backlight/qcom-spmi-wled.c | 126 ++++++++++++++++++++-
> 2 files changed, 142 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> index f1ea25b..768608c 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> @@ -74,6 +74,26 @@ The PMIC is connected to the host processor via SPMI bus.
> Definition: Specify if cabc (content adaptive backlight control) is
> needed.
>
> +- qcom,ext-pfet-sc-pro-en

Please use readable names, rather than a bunch of abbreviations.

> + Usage: optional
> + Value type: <bool>
> + Definition: Specify if external PFET control for short circuit
> + protection is needed.

What does this mean? At least change the wording to "...protection is
used".

> +
> +- interrupts
> + Usage: optional
> + Value type: <prop encoded array>
> + Definition: Interrupts associated with WLED. Interrupts can be
> + specified as per the encoding listed under
> + Documentation/devicetree/bindings/spmi/
> + qcom,spmi-pmic-arb.txt.
> +
> +- interrupt-names
> + Usage: optional
> + Value type: <string>
> + Definition: Interrupt names associated with the interrupts.
> + Must be "sc-irq".

This is obviously an irq, so no need to include that in the name. I
would also prefer if you use the name "short" to make this easier to
read.

> +
> Example:
>
> qcom-wled@d800 {
> @@ -82,6 +102,8 @@ qcom-wled@d800 {
> reg-names = "qcom-wled-ctrl-base", "qcom-wled-sink-base";
> label = "backlight";
>
> + interrupts = <0x3 0xd8 0x2 IRQ_TYPE_EDGE_RISING>;

We tend to write these on the form <decimal, hex, decimal, enum>, please
follow this.

> + interrupt-names = "sc-irq";
> qcom,fs-current-limit = <25000>;
> qcom,current-boost-limit = <970>;
> qcom,switching-freq = <800>;
> diff --git a/drivers/video/backlight/qcom-spmi-wled.c b/drivers/video/backlight/qcom-spmi-wled.c
> index 14c3adc..7dbaaa7 100644
> --- a/drivers/video/backlight/qcom-spmi-wled.c
> +++ b/drivers/video/backlight/qcom-spmi-wled.c
> @@ -11,6 +11,9 @@
> * GNU General Public License for more details.
> */
>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/ktime.h>
> #include <linux/kernel.h>
> #include <linux/backlight.h>
> #include <linux/module.h>
> @@ -23,7 +26,13 @@
> #define QCOM_WLED_DEFAULT_BRIGHTNESS 2048
> #define QCOM_WLED_MAX_BRIGHTNESS 4095
>
> +#define QCOM_WLED_SC_DLY_MS 20
> +#define QCOM_WLED_SC_CNT_MAX 5
> +#define QCOM_WLED_SC_RESET_CNT_DLY_US 1000000

With times of this ballpark you can just use jiffies, with this just
being HZ.

> +
> /* WLED control registers */
> +#define QCOM_WLED_CTRL_FAULT_STATUS 0x08
> +
> #define QCOM_WLED_CTRL_MOD_ENABLE 0x46
> #define QCOM_WLED_CTRL_MOD_EN_MASK BIT(7)
> #define QCOM_WLED_CTRL_MODULE_EN_SHIFT 7
> @@ -37,6 +46,15 @@
> #define QCOM_WLED_CTRL_ILIM 0x4e
> #define QCOM_WLED_CTRL_ILIM_MASK GENMASK(2, 0)
>
> +#define QCOM_WLED_CTRL_SHORT_PROTECT 0x5e
> +#define QCOM_WLED_CTRL_SHORT_EN_MASK BIT(7)
> +
> +#define QCOM_WLED_CTRL_SEC_ACCESS 0xd0
> +#define QCOM_WLED_CTRL_SEC_UNLOCK 0xa5
> +
> +#define QCOM_WLED_CTRL_TEST1 0xe2
> +#define QCOM_WLED_EXT_FET_DTEST2 0x09
> +
> /* WLED sink registers */
> #define QCOM_WLED_SINK_CURR_SINK_EN 0x46
> #define QCOM_WLED_SINK_CURR_SINK_MASK GENMASK(7, 4)
> @@ -71,19 +89,23 @@ struct qcom_wled_config {
> u32 switch_freq;
> u32 fs_current;
> u32 string_cfg;
> + int sc_irq;

Keep data parsed directly from DT in the config and move this to
qcom_wled.

> bool en_cabc;
> + bool ext_pfet_sc_pro_en;

This name is long and hard to parse. "external_pfet" would be much
easier to read.

> };
>
> struct qcom_wled {
> const char *name;
> struct platform_device *pdev;
> struct regmap *regmap;
> + struct mutex lock;
> + struct qcom_wled_config cfg;
> + ktime_t last_sc_event_time;
> u16 sink_addr;
> u16 ctrl_addr;
> u32 brightness;
> + u32 sc_count;
> bool prev_state;
> -
> - struct qcom_wled_config cfg;

Moving this seems unnecessary.

> };
>
> static int qcom_wled_module_enable(struct qcom_wled *wled, int val)
> @@ -157,25 +179,26 @@ static int qcom_wled_update_status(struct backlight_device *bl)
> bl->props.state & BL_CORE_FBBLANK)
> brightness = 0;
>
> + mutex_lock(&wled->lock);

Is this lock necessary?

> +static irqreturn_t qcom_wled_sc_irq_handler(int irq, void *_wled)
> +{
> + struct qcom_wled *wled = _wled;
> + int rc;
> + u32 val;
> + s64 elapsed_time;
> +
> + rc = regmap_read(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_FAULT_STATUS, &val);
> + if (rc < 0) {
> + pr_err("Error in reading WLED_FAULT_STATUS rc=%d\n", rc);
> + return IRQ_HANDLED;
> + }
> +
> + wled->sc_count++;
> + pr_err("WLED short circuit detected %d times fault_status=%x\n",
> + wled->sc_count, val);

Who will read this and is it worth the extra read of FAULT_STATUS just
to produce this print?

> + mutex_lock(&wled->lock);
> + rc = qcom_wled_module_enable(wled, false);
> + if (rc < 0) {
> + pr_err("wled disable failed rc:%d\n", rc);
> + goto unlock_mutex;
> + }
> +
> + elapsed_time = ktime_us_delta(ktime_get(),
> + wled->last_sc_event_time);
> + if (elapsed_time > QCOM_WLED_SC_RESET_CNT_DLY_US) {
> + wled->sc_count = 0;
> + } else if (wled->sc_count > QCOM_WLED_SC_CNT_MAX) {

This isn't really "else elapsed_time was more than DLY_US". Split this
into:

if (elapsed_time > xyz)
wled->sc_count = 0;

if (wled->sc_count > QCOM_WLED_SC_CNT_MAX)
...

> + pr_err("SC trigged %d times, disabling WLED forever!\n",

"forever" as in "until someone turns it on again"?

> + wled->sc_count);
> + goto unlock_mutex;
> + }
> +
> + wled->last_sc_event_time = ktime_get();
> +
> + msleep(QCOM_WLED_SC_DLY_MS);
> + rc = qcom_wled_module_enable(wled, true);
> + if (rc < 0)
> + pr_err("wled enable failed rc:%d\n", rc);
> +
> +unlock_mutex:
> + mutex_unlock(&wled->lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int qcom_wled_setup(struct qcom_wled *wled)
> {
> int rc, temp, i;
> u8 sink_en = 0;
> u8 string_cfg = wled->cfg.string_cfg;
> + int sc_irq = wled->cfg.sc_irq;
>
> rc = regmap_update_bits(wled->regmap,
> wled->ctrl_addr + QCOM_WLED_CTRL_OVP,
> @@ -261,6 +334,39 @@ static int qcom_wled_setup(struct qcom_wled *wled)
> return rc;
> }
>
> + if (sc_irq >= 0) {

I think things will be cleaner if you let qcom_wled_setup() configure
the hardware based on the wled->cfg (as is done to this point) and then
deal with the interrupts in a separate code path from the probe
function.

> + rc = devm_request_threaded_irq(&wled->pdev->dev, sc_irq,
> + NULL, qcom_wled_sc_irq_handler, IRQF_ONESHOT,
> + "qcom_wled_sc_irq", wled);
> + if (rc < 0) {
> + pr_err("Unable to request sc(%d) IRQ(err:%d)\n",
> + sc_irq, rc);

sc_irq is just a number without meaning, no need to print it.

> + return rc;
> + }
> +
> + rc = regmap_update_bits(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_SHORT_PROTECT,
> + QCOM_WLED_CTRL_SHORT_EN_MASK,
> + QCOM_WLED_CTRL_SHORT_EN_MASK);
> + if (rc < 0)
> + return rc;
> +
> + if (wled->cfg.ext_pfet_sc_pro_en) {
> + /* unlock the secure access regisetr */

Spelling of register, and this operation does "Unlock the secure
register access" it doesn't unlock the secure access register.

> + rc = regmap_write(wled->regmap, wled->ctrl_addr +
> + QCOM_WLED_CTRL_SEC_ACCESS,
> + QCOM_WLED_CTRL_SEC_UNLOCK);
> + if (rc < 0)
> + return rc;
> +
> + rc = regmap_write(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_TEST1,
> + QCOM_WLED_EXT_FET_DTEST2);

What is the relationship between DTEST2 and the external FET?

> + if (rc < 0)
> + return rc;
> + }
> + }
> +
> return 0;
> }
>
> @@ -271,6 +377,7 @@ static int qcom_wled_setup(struct qcom_wled *wled)
> .switch_freq = 11,
> .string_cfg = 0xf,
> .en_cabc = 0,
> + .ext_pfet_sc_pro_en = 1,
> };
>
> struct qcom_wled_var_cfg {
> @@ -376,6 +483,7 @@ static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev)
> bool *val_ptr;
> } bool_opts[] = {
> { "qcom,en-cabc", &cfg->en_cabc, },
> + { "qcom,ext-pfet-sc-pro", &cfg->ext_pfet_sc_pro_en, },
> };
>
> prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);
> @@ -427,6 +535,10 @@ static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev)
> *bool_opts[i].val_ptr = true;
> }
>
> + wled->cfg.sc_irq = platform_get_irq_byname(wled->pdev, "sc-irq");
> + if (wled->cfg.sc_irq < 0)
> + dev_dbg(&wled->pdev->dev, "sc irq is not used\n");
> +

Move this to qcom_wled_probe() or into its own code path, together with
the rest of the sc_irq initialization.

And as you're not enabling or disabling it you can store it in a local
variable.

> return 0;
> }
>

Regards,
Bjorn

Next message: Nicolin Chen: "Re: [PATCH 04/10] ASoC: fsl_ssi: Refine all comments"
Previous message: Pravin Shedge: "[PATCH 05/45] drivers: hwtracing: remove duplicate includes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]