Re: [PATCH V4 2/4] backlight: qcom-wled: Add callback functions
On 2020-03-25 21:00, Daniel Thompson wrote:
On Mon, Mar 23, 2020 at 11:16:56PM +0530, Kiran Gunda wrote:
Add wled_cabc_config, wled_sync_toggle, wled_ovp_fault_status
and wled_ovp_delay callback functions to prepare the driver for
adding WLED5 support.
wled_cabc_config() ===> Used to configure the cabc register.
It is applicable for wled4 and wled5.
wled_sync_toggle() ===> used to toggle the Sync register bit for the
brightness update to take place.
It is applicable for WLED3, WLED4 and WLED5.
wled_ovp_fault_status() ===> Used to determine if the OVP fault is
triggered.
It is applicable for WLED4 and WLED5.
wled_ovp_delay() ===> Provides the time to wait before checking the
OVP status
after wled module enable.
It is applicable for WLED4 and WLED5.
These look like comments to me. Move them out of the patch header and
make them into real comments!
Sure. Will do it in next post.
Signed-off-by: Kiran Gunda
This patch does not compile. Please fix this.
Sorry for that. Some how it didn't show up in my compilation.
Will fix it in next post.
---
drivers/video/backlight/qcom-wled.c | 188
++--
1 file changed, 118 insertions(+), 70 deletions(-)
diff --git a/drivers/video/backlight/qcom-wled.c
b/drivers/video/backlight/qcom-wled.c
index 3d276b3..a3daf9e 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -147,6 +147,7 @@ struct wled {
u32 max_brightness;
u32 short_count;
u32 auto_detect_count;
+ u32 version;
Why does some of the changes here use function pointers and other parts
use if/else networks (wled->version == X) ?
There is one place (in auto_string_detection) where we have to add the
if (version == 4) because the per string modulator_enable is present
only
for WLED4 and there is no equivalent functionality available for WLED5.
There is another place, that i can convert it to the function pointer
(auto_detection_required).
Overall I almost wonder if the reduced clarify that comes from function
pointers is actually adding much value?
I believe adding the function pointer makes it cleaner and the same
approach is used for
WLED4/WLED3 also.
bool disabled_by_short;
bool has_short_detect;
int short_irq;
@@ -155,6 +156,10 @@ struct wled {
struct wled_config cfg;
struct delayed_work ovp_work;
int (*wled_set_brightness)(struct wled *wled, u16 brightness);
+ int (*wled_cabc_config)(struct wled *wled, bool enable);
+ int (*wled_sync_toggle)(struct wled *wled);
+ int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set);
+ int (*wled_ovp_delay)(struct wled *wled);
};
static int wled3_set_brightness(struct wled *wled, u16 brightness)
@@ -237,7 +242,7 @@ static int wled_module_enable(struct wled *wled,
int val)
return 0;
}
-static int wled_sync_toggle(struct wled *wled)
+static int wled3_sync_toggle(struct wled *wled)
{
int rc;
unsigned int mask = GENMASK(wled->max_string_count - 1, 0);
@@ -255,6 +260,46 @@ static int wled_sync_toggle(struct wled *wled)
return rc;
}
+static int wled4_ovp_fault_status(struct wled *wled, bool *fault_set)
+{
+ int rc;
+ u32 int_rt_sts, fault_sts;
+
+ *fault_set = false;
+ rc = regmap_read(wled->regmap,
+ wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS,
+ _rt_sts);
+ if (rc < 0) {
+ dev_err(wled->dev, "Failed to read INT_RT_STS rc=%d\n", rc);
+ return rc;
+ }
+
+ rc = regmap_read(wled->regmap,
+ wled->ctrl_addr + WLED3_CTRL_REG_FAULT_STATUS,
+ _sts);
+ if (rc < 0) {
+ dev_err(wled->dev, "Failed to read FAULT_STATUS rc=%d\n", rc);
+ return rc;
+ }
+
+ if (int_rt_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
+ *fault_set = true;
+
+ if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT)
+ *fault_set = true;
+
+ if (*fault_set)
+ dev_dbg(wled->dev, "WLED OVP fault detected, int_rt_sts=0x%x
fault_sts=0x%x\n",
+ int_rt_sts, fault_sts);
+
+ return rc;
+}
+
+static int wled4_ovp_delay(struct wled *wled)
+{
+ return WLED_SOFT_START_DLY_US;
+}
+
static int wled_update_status(struct backlight_device *bl)
{
struct wled *wled = bl_get_data(bl);
@@ -275,7 +320,7 @@ static int wled_update_status(struct
backlight_device *bl)
goto unlock_mutex;
}
- rc = wled_sync_toggle(wled);
+ rc = wled->wled_sync_toggle(wled);
if (rc < 0) {
dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
goto
Re: [PATCH V4 2/4] backlight: qcom-wled: Add callback functions
On Mon, Mar 23, 2020 at 11:16:56PM +0530, Kiran Gunda wrote:
> Add wled_cabc_config, wled_sync_toggle, wled_ovp_fault_status
> and wled_ovp_delay callback functions to prepare the driver for
> adding WLED5 support.
>
> wled_cabc_config() ===> Used to configure the cabc register.
> It is applicable for wled4 and wled5.
>
> wled_sync_toggle() ===> used to toggle the Sync register bit for the
> brightness update to take place.
> It is applicable for WLED3, WLED4 and WLED5.
>
> wled_ovp_fault_status() ===> Used to determine if the OVP fault is triggered.
> It is applicable for WLED4 and WLED5.
>
> wled_ovp_delay() ===> Provides the time to wait before checking the OVP status
> after wled module enable.
> It is applicable for WLED4 and WLED5.
These look like comments to me. Move them out of the patch header and
make them into real comments!
> Signed-off-by: Kiran Gunda
This patch does not compile. Please fix this.
> ---
> drivers/video/backlight/qcom-wled.c | 188
> ++--
> 1 file changed, 118 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/video/backlight/qcom-wled.c
> b/drivers/video/backlight/qcom-wled.c
> index 3d276b3..a3daf9e 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -147,6 +147,7 @@ struct wled {
> u32 max_brightness;
> u32 short_count;
> u32 auto_detect_count;
> + u32 version;
Why does some of the changes here use function pointers and other parts
use if/else networks (wled->version == X) ?
Overall I almost wonder if the reduced clarify that comes from function
pointers is actually adding much value?
> bool disabled_by_short;
> bool has_short_detect;
> int short_irq;
> @@ -155,6 +156,10 @@ struct wled {
> struct wled_config cfg;
> struct delayed_work ovp_work;
> int (*wled_set_brightness)(struct wled *wled, u16 brightness);
> + int (*wled_cabc_config)(struct wled *wled, bool enable);
> + int (*wled_sync_toggle)(struct wled *wled);
> + int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set);
> + int (*wled_ovp_delay)(struct wled *wled);
> };
>
> static int wled3_set_brightness(struct wled *wled, u16 brightness)
> @@ -237,7 +242,7 @@ static int wled_module_enable(struct wled *wled, int val)
> return 0;
> }
>
> -static int wled_sync_toggle(struct wled *wled)
> +static int wled3_sync_toggle(struct wled *wled)
> {
> int rc;
> unsigned int mask = GENMASK(wled->max_string_count - 1, 0);
> @@ -255,6 +260,46 @@ static int wled_sync_toggle(struct wled *wled)
> return rc;
> }
>
> +static int wled4_ovp_fault_status(struct wled *wled, bool *fault_set)
> +{
> + int rc;
> + u32 int_rt_sts, fault_sts;
> +
> + *fault_set = false;
> + rc = regmap_read(wled->regmap,
> + wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS,
> + _rt_sts);
> + if (rc < 0) {
> + dev_err(wled->dev, "Failed to read INT_RT_STS rc=%d\n", rc);
> + return rc;
> + }
> +
> + rc = regmap_read(wled->regmap,
> + wled->ctrl_addr + WLED3_CTRL_REG_FAULT_STATUS,
> + _sts);
> + if (rc < 0) {
> + dev_err(wled->dev, "Failed to read FAULT_STATUS rc=%d\n", rc);
> + return rc;
> + }
> +
> + if (int_rt_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
> + *fault_set = true;
> +
> + if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT)
> + *fault_set = true;
> +
> + if (*fault_set)
> + dev_dbg(wled->dev, "WLED OVP fault detected, int_rt_sts=0x%x
> fault_sts=0x%x\n",
> + int_rt_sts, fault_sts);
> +
> + return rc;
> +}
> +
> +static int wled4_ovp_delay(struct wled *wled)
> +{
> + return WLED_SOFT_START_DLY_US;
> +}
> +
> static int wled_update_status(struct backlight_device *bl)
> {
> struct wled *wled = bl_get_data(bl);
> @@ -275,7 +320,7 @@ static int wled_update_status(struct backlight_device *bl)
> goto unlock_mutex;
> }
>
> - rc = wled_sync_toggle(wled);
> + rc = wled->wled_sync_toggle(wled);
> if (rc < 0) {
> dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
> goto unlock_mutex;
> @@ -298,6 +343,25 @@ static int wled_update_status(struct backlight_device
> *bl)
> return rc;
> }
>
> +static int wled4_cabc_config(struct wled *wled, bool enable)
> +{
> + int i, j, rc;
> + u8 val;
> +
> + for (i = 0; i < wled->cfg.num_strings; i++) {
> + j = wled->cfg.enabled_strings[i];
> +
> + val = enable ? WLED4_SINK_REG_STR_CABC_MASK : 0;
> + rc = regmap_update_bits(wled->regmap, wled->sink_addr +
> +
[PATCH V4 2/4] backlight: qcom-wled: Add callback functions
Add wled_cabc_config, wled_sync_toggle, wled_ovp_fault_status
and wled_ovp_delay callback functions to prepare the driver for
adding WLED5 support.
wled_cabc_config() ===> Used to configure the cabc register.
It is applicable for wled4 and wled5.
wled_sync_toggle() ===> used to toggle the Sync register bit for the
brightness update to take place.
It is applicable for WLED3, WLED4 and WLED5.
wled_ovp_fault_status() ===> Used to determine if the OVP fault is triggered.
It is applicable for WLED4 and WLED5.
wled_ovp_delay() ===> Provides the time to wait before checking the OVP status
after wled module enable.
It is applicable for WLED4 and WLED5.
Signed-off-by: Kiran Gunda
---
drivers/video/backlight/qcom-wled.c | 188 ++--
1 file changed, 118 insertions(+), 70 deletions(-)
diff --git a/drivers/video/backlight/qcom-wled.c
b/drivers/video/backlight/qcom-wled.c
index 3d276b3..a3daf9e 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -147,6 +147,7 @@ struct wled {
u32 max_brightness;
u32 short_count;
u32 auto_detect_count;
+ u32 version;
bool disabled_by_short;
bool has_short_detect;
int short_irq;
@@ -155,6 +156,10 @@ struct wled {
struct wled_config cfg;
struct delayed_work ovp_work;
int (*wled_set_brightness)(struct wled *wled, u16 brightness);
+ int (*wled_cabc_config)(struct wled *wled, bool enable);
+ int (*wled_sync_toggle)(struct wled *wled);
+ int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set);
+ int (*wled_ovp_delay)(struct wled *wled);
};
static int wled3_set_brightness(struct wled *wled, u16 brightness)
@@ -237,7 +242,7 @@ static int wled_module_enable(struct wled *wled, int val)
return 0;
}
-static int wled_sync_toggle(struct wled *wled)
+static int wled3_sync_toggle(struct wled *wled)
{
int rc;
unsigned int mask = GENMASK(wled->max_string_count - 1, 0);
@@ -255,6 +260,46 @@ static int wled_sync_toggle(struct wled *wled)
return rc;
}
+static int wled4_ovp_fault_status(struct wled *wled, bool *fault_set)
+{
+ int rc;
+ u32 int_rt_sts, fault_sts;
+
+ *fault_set = false;
+ rc = regmap_read(wled->regmap,
+ wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS,
+ _rt_sts);
+ if (rc < 0) {
+ dev_err(wled->dev, "Failed to read INT_RT_STS rc=%d\n", rc);
+ return rc;
+ }
+
+ rc = regmap_read(wled->regmap,
+ wled->ctrl_addr + WLED3_CTRL_REG_FAULT_STATUS,
+ _sts);
+ if (rc < 0) {
+ dev_err(wled->dev, "Failed to read FAULT_STATUS rc=%d\n", rc);
+ return rc;
+ }
+
+ if (int_rt_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
+ *fault_set = true;
+
+ if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT)
+ *fault_set = true;
+
+ if (*fault_set)
+ dev_dbg(wled->dev, "WLED OVP fault detected, int_rt_sts=0x%x
fault_sts=0x%x\n",
+ int_rt_sts, fault_sts);
+
+ return rc;
+}
+
+static int wled4_ovp_delay(struct wled *wled)
+{
+ return WLED_SOFT_START_DLY_US;
+}
+
static int wled_update_status(struct backlight_device *bl)
{
struct wled *wled = bl_get_data(bl);
@@ -275,7 +320,7 @@ static int wled_update_status(struct backlight_device *bl)
goto unlock_mutex;
}
- rc = wled_sync_toggle(wled);
+ rc = wled->wled_sync_toggle(wled);
if (rc < 0) {
dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
goto unlock_mutex;
@@ -298,6 +343,25 @@ static int wled_update_status(struct backlight_device *bl)
return rc;
}
+static int wled4_cabc_config(struct wled *wled, bool enable)
+{
+ int i, j, rc;
+ u8 val;
+
+ for (i = 0; i < wled->cfg.num_strings; i++) {
+ j = wled->cfg.enabled_strings[i];
+
+ val = enable ? WLED4_SINK_REG_STR_CABC_MASK : 0;
+ rc = regmap_update_bits(wled->regmap, wled->sink_addr +
+ WLED4_SINK_REG_STR_CABC(j),
+ WLED4_SINK_REG_STR_CABC_MASK, val);
+ if (rc < 0)
+ return rc;
+ }
+
+ return 0;
+}
+
#define WLED_SHORT_DLY_MS 20
#define WLED_SHORT_CNT_MAX 5
#define WLED_SHORT_RESET_CNT_DLY_USUSEC_PER_SEC
@@ -345,9 +409,10 @@ static irqreturn_t wled_short_irq_handler(int irq, void
*_wled)
static void wled_auto_string_detection(struct wled *wled)
{
- int rc = 0, i;
- u32
