| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 30 Mar 2020 20:15:27 +0530
From: kgunda@...eaurora.org
To: Daniel Thompson <daniel.thompson@...aro.org>
Cc: bjorn.andersson@...aro.org, jingoohan1@...il.com,
lee.jones@...aro.org, b.zolnierkie@...sung.com,
dri-devel@...ts.freedesktop.org, jacek.anaszewski@...il.com,
pavel@....cz, robh+dt@...nel.org, mark.rutland@....com,
linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Andy Gross <agross@...nel.org>,
linux-arm-msm@...r.kernel.org, linux-fbdev@...r.kernel.org
Subject: 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 <kgunda@...eaurora.org>
>
> 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,
>> + &int_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,
>> + &fault_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_US USEC_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 sink_config = 0, int_sts;
>> + int rc = 0, i, delay_time_us;
>> + u32 sink_config = 0;
>> u8 sink_test = 0, sink_valid = 0, val;
>> + bool fault_set;
>>
>> /* Read configured sink configuration */
>> rc = regmap_read(wled->regmap, wled->sink_addr +
>> @@ -376,14 +441,9 @@ static void wled_auto_string_detection(struct
>> wled *wled)
>> }
>>
>> if (wled->cfg.cabc) {
>> - for (i = 0; i < wled->cfg.num_strings; i++) {
>> - rc = regmap_update_bits(wled->regmap, wled->sink_addr +
>> - WLED4_SINK_REG_STR_CABC(i),
>> - WLED4_SINK_REG_STR_CABC_MASK,
>> - 0);
>> - if (rc < 0)
>> - goto failed_detect;
>> - }
>> + rc = wled->wled_cabc_config(wled, 0);
>
> Be consistent with the argument (elsewhere it is true/false).
>
Will address in the next post.
>
>> + if (rc < 0)
>> + goto failed_detect;
>> }
>>
>> /* Disable all sinks */
>> @@ -427,18 +487,17 @@ static void wled_auto_string_detection(struct
>> wled *wled)
>> goto failed_detect;
>> }
>>
>> - usleep_range(WLED_SOFT_START_DLY_US,
>> - WLED_SOFT_START_DLY_US + 1000);
>> + delay_time_us = wled->wled_ovp_delay(wled);
>> + usleep_range(delay_time_us, delay_time_us + 1000);
>>
>> - rc = regmap_read(wled->regmap, wled->ctrl_addr +
>> - WLED3_CTRL_REG_INT_RT_STS, &int_sts);
>> + rc = wled->wled_ovp_fault_status(wled, &fault_set);
>> if (rc < 0) {
>> - dev_err(wled->dev, "Error in reading WLED3_CTRL_INT_RT_STS
>> rc=%d\n",
>> + dev_err(wled->dev, "Error in getting OVP fault_sts, rc=%d\n",
>> rc);
>> goto failed_detect;
>> }
>>
>
>
>> - if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
>> + if (fault_set)
>> dev_dbg(wled->dev, "WLED OVP fault detected with SINK %d\n",
>> i + 1);
>> else
>> @@ -478,30 +537,30 @@ static void wled_auto_string_detection(struct
>> wled *wled)
>> }
>>
>> /* Enable valid sinks */
>> - for (i = 0; i < wled->cfg.num_strings; i++) {
>> - if (wled->cfg.cabc) {
>> - rc = regmap_update_bits(wled->regmap, wled->sink_addr +
>> - WLED4_SINK_REG_STR_CABC(i),
>> - WLED4_SINK_REG_STR_CABC_MASK,
>> - WLED4_SINK_REG_STR_CABC_MASK);
>> - if (rc < 0)
>> + if (wled->version == 4) {
>> + for (i = 0; i < wled->cfg.num_strings; i++) {
>> + if (sink_config &
>> + BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i))
>> + val = WLED4_SINK_REG_STR_MOD_MASK;
>> + else
>> + /* Disable modulator_en for unused sink */
>> + val = 0x0;
>> +
>> + rc = regmap_write(wled->regmap, wled->sink_addr +
>> + WLED4_SINK_REG_STR_MOD_EN(i), val);
>> + if (rc < 0) {
>> + dev_err(wled->dev, "Failed to configure MODULATOR_EN rc=%d\n",
>> + rc);
>> goto failed_detect;
>> - }
>> -
>> - if (sink_config & BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i))
>> - val = WLED4_SINK_REG_STR_MOD_MASK;
>> - else
>> - val = 0x0; /* Disable modulator_en for unused sink */
>> -
>> - rc = regmap_write(wled->regmap, wled->sink_addr +
>> - WLED4_SINK_REG_STR_MOD_EN(i), val);
>> - if (rc < 0) {
>> - dev_err(wled->dev, "Failed to configure MODULATOR_EN rc=%d\n",
>> - rc);
>> - goto failed_detect;
>> + }
>> }
>> }
>>
>> + /* Enable CABC if it needs to be enabled */
>
> If it needs to be enabled?
>
Ok. Actually Enable CABC should be enough. I will remove the latter part
in the next post.
>
>> + rc = wled->wled_cabc_config(wled, true);
>> + if (rc < 0)
>> + goto failed_detect;
>> +
>> /* Restore the feedback setting */
>> rc = regmap_write(wled->regmap,
>> wled->ctrl_addr + WLED3_CTRL_REG_FEEDBACK_CONTROL, 0);
>> @@ -570,29 +629,19 @@ static bool wled_auto_detection_required(struct
>> wled *wled)
>> static int wled_auto_detection_at_init(struct wled *wled)
>> {
>> int rc;
>> - u32 fault_status, rt_status;
>> + bool fault_set;
>>
>> if (!wled->cfg.auto_detection_enabled)
>> return 0;
>>
>> - rc = regmap_read(wled->regmap,
>> - wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS,
>> - &rt_status);
>> + rc = wled->wled_ovp_fault_status(wled, &fault_set);
>> if (rc < 0) {
>> - dev_err(wled->dev, "Failed to read RT status rc=%d\n", rc);
>> - return rc;
>> - }
>> -
>> - rc = regmap_read(wled->regmap,
>> - wled->ctrl_addr + WLED3_CTRL_REG_FAULT_STATUS,
>> - &fault_status);
>> - if (rc < 0) {
>> - dev_err(wled->dev, "Failed to read fault status rc=%d\n", rc);
>> + dev_err(wled->dev, "Error in getting OVP fault_sts, rc=%d\n",
>> + rc);
>> return rc;
>> }
>>
>> - if ((rt_status & WLED3_CTRL_REG_OVP_FAULT_STATUS) ||
>> - (fault_status & WLED3_CTRL_REG_OVP_FAULT_BIT)) {
>> + if (fault_set) {
>> mutex_lock(&wled->lock);
>> wled_auto_string_detection(wled);
>> mutex_unlock(&wled->lock);
>> @@ -811,17 +860,12 @@ static int wled4_setup(struct wled *wled)
>> wled->cfg.string_i_limit);
>> if (rc < 0)
>> return rc;
>> -
>> - addr = wled->sink_addr +
>> - WLED4_SINK_REG_STR_CABC(j);
>> - rc = regmap_update_bits(wled->regmap, addr,
>> - WLED4_SINK_REG_STR_CABC_MASK,
>> - wled->cfg.cabc ?
>> - WLED4_SINK_REG_STR_CABC_MASK : 0);
>> - if (rc < 0)
>> - return rc;
>> }
>>
>> + rc = wled4_cabc_config(wled, wled->cfg.en_cabc);
>> + if (rc < 0)
>> + return rc;
>> +
>> rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
>> WLED3_CTRL_REG_MOD_EN,
>> WLED3_CTRL_REG_MOD_EN_MASK,
>> @@ -835,7 +879,7 @@ static int wled4_setup(struct wled *wled)
>> if (rc < 0)
>> return rc;
>>
>> - rc = wled_sync_toggle(wled);
>> + rc = wled->wled_sync_toggle(wled);
>> if (rc < 0) {
>> dev_err(wled->dev, "Failed to toggle sync reg rc:%d\n", rc);
>> return rc;
>> @@ -951,7 +995,7 @@ static u32 wled_values(const struct wled_var_cfg
>> *cfg, u32 idx)
>> return idx;
>> }
>>
>> -static int wled_configure(struct wled *wled, int version)
>> +static int wled_configure(struct wled *wled)
>> {
>> struct wled_config *cfg = &wled->cfg;
>> struct device *dev = wled->dev;
>> @@ -1035,12 +1079,13 @@ static int wled_configure(struct wled *wled,
>> int version)
>> if (rc)
>> wled->name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn",
>> dev->of_node);
>>
>> - switch (version) {
>> + switch (wled->version) {
>> case 3:
>> u32_opts = wled3_opts;
>> size = ARRAY_SIZE(wled3_opts);
>> *cfg = wled3_config_defaults;
>> wled->wled_set_brightness = wled3_set_brightness;
>> + wled->wled_sync_toggle = wled3_sync_toggle;
>> wled->max_string_count = 3;
>> wled->sink_addr = wled->ctrl_addr;
>> break;
>> @@ -1050,6 +1095,10 @@ static int wled_configure(struct wled *wled,
>> int version)
>> size = ARRAY_SIZE(wled4_opts);
>> *cfg = wled4_config_defaults;
>> wled->wled_set_brightness = wled4_set_brightness;
>> + wled->wled_sync_toggle = wled3_sync_toggle;
>> + wled->wled_cabc_config = wled4_cabc_config;
>> + wled->wled_ovp_fault_status = wled4_ovp_fault_status;
>> + wled->wled_ovp_delay = wled4_ovp_delay;
>> wled->max_string_count = 4;
>>
>> prop_addr = of_get_address(dev->of_node, 1, NULL, NULL);
>> @@ -1186,7 +1235,6 @@ static int wled_probe(struct platform_device
>> *pdev)
>> struct backlight_device *bl;
>> struct wled *wled;
>> struct regmap *regmap;
>> - int version;
>> u32 val;
>> int rc;
>>
>> @@ -1203,18 +1251,18 @@ static int wled_probe(struct platform_device
>> *pdev)
>> wled->regmap = regmap;
>> wled->dev = &pdev->dev;
>>
>> - version = (uintptr_t)of_device_get_match_data(&pdev->dev);
>> - if (!version) {
>> + wled->version = (uintptr_t)of_device_get_match_data(&pdev->dev);
>> + if (!wled->version) {
>> dev_err(&pdev->dev, "Unknown device version\n");
>> return -ENODEV;
>> }
>>
>> mutex_init(&wled->lock);
>> - rc = wled_configure(wled, version);
>> + rc = wled_configure(wled);
>> if (rc)
>> return rc;
>>
>> - switch (version) {
>> + switch (wled->version) {
>> case 3:
>> wled->cfg.auto_detection_enabled = false;
>> rc = wled3_setup(wled);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
Powered by blists - more mailing lists