Date:   Mon, 11 Dec 2017 14:58:24 +0530
From:   kgunda@...eaurora.org
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     linux-arm-msm@...r.kernel.org, Lee Jones <lee.jones@...aro.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Richard Purdie <rpurdie@...ys.net>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
        linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH V1 2/4] qcom: spmi-wled: Add support for short circuit
 handling

On 2017-12-05 10:05, Bjorn Andersson wrote:
> 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@...eaurora.org>
>> ---
>>  .../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.
> 
Ok. Will address it in next series.
>> +	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".
> 
Ok. Will address it in next series.
>> +
>> +- 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.
> 
Ok. Will address it in next series.
>> +
>>  Example:
>> 
>>  qcom-wled@...0 {
>> @@ -82,6 +102,8 @@ qcom-wled@...0 {
>>  	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.
> 
Ok. Will address it in next series.
>> +	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.
> 
Ok. Will address it in next series.
>> +
>>  /* 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.
> 
Ok. Will address it in next series.
>>  	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.
> 
Ok. Will address it in next series.
>>  };
>> 
>>  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.
> 
Ok. Will address it in next series.
>>  };
>> 
>>  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?
> 
Yes. It avoid the race between the upate_status and sc_irq as the module 
is enabled
at one place and disabled at other place respectively.
>> +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?
> 
As this SC irq gets triggered in very rare conditions, i think it is 
okay
to have a print for the information purpose.
>> +	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)
> 	...
> 
Ok. sure.
>> +		pr_err("SC trigged %d times, disabling WLED forever!\n",
> 
> "forever" as in "until someone turns it on again"?
> 
Yes. It is turned on for the next reboot or some one forcefully enables 
it form the
sysfs.

>> +			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.
> 
Ok. sure.
>> +		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.
> 
Sure. Will remove 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.
> 
Sure. Will correct it.
>> +			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?
> External FET is controlled through the DTEST2 register. External FET is 
> not part of the
WLED IP so it is controlled from the DTEST pins.
>> +			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.
> 
Ok. Sure.
>>  	return 0;
>>  }
>> 
> 
> Regards,
> Bjorn

Powered by blists - more mailing lists