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 <[email protected]>
> ---
> .../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
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 <[email protected]>
>> ---
>> .../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@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.
>
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