| 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
| ||
|
Message-ID: <20180620051417.GI15126@tuxbook-pro>
Date: Tue, 19 Jun 2018 22:14:17 -0700
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Kiran Gunda <kgunda@...eaurora.org>
Cc: jingoohan1@...il.com, lee.jones@...aro.org,
b.zolnierkie@...sung.com, dri-devel@...ts.freedesktop.org,
Daniel Thompson <daniel.thompson@...aro.org>,
linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH V3 5/7] backlight: qcom-wled: Add support for WLED4
peripheral
On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
> WLED4 peripheral is present on some PMICs like pmi8998 and
> pm660l. It has a different register map and configurations
> are also different. Add support for it.
>
> Signed-off-by: Kiran Gunda <kgunda@...eaurora.org>
> ---
> drivers/video/backlight/qcom-wled.c | 635 ++++++++++++++++++++++++++++--------
> 1 file changed, 503 insertions(+), 132 deletions(-)
Split this further into a patch that does structural preparation of
WLED3 support and then an addition of WLED4, the mixture makes parts of
this patch almost impossible to review. Please find some comments below.
>
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
[..]
>
> /* WLED3 sink registers */
> #define WLED3_SINK_REG_SYNC 0x47
Drop the 3 from this if it's common between the two.
> -#define WLED3_SINK_REG_SYNC_MASK 0x07
> +#define WLED3_SINK_REG_SYNC_MASK GENMASK(2, 0)
> +#define WLED4_SINK_REG_SYNC_MASK GENMASK(3, 0)
> #define WLED3_SINK_REG_SYNC_LED1 BIT(0)
> #define WLED3_SINK_REG_SYNC_LED2 BIT(1)
> #define WLED3_SINK_REG_SYNC_LED3 BIT(2)
> +#define WLED4_SINK_REG_SYNC_LED4 BIT(3)
> #define WLED3_SINK_REG_SYNC_ALL 0x07
> +#define WLED4_SINK_REG_SYNC_ALL 0x0f
> #define WLED3_SINK_REG_SYNC_CLEAR 0x00
>
[..]
> +static int wled4_set_brightness(struct wled *wled, u16 brightness)
Afaict this is identical to wled3_set_brightness() with the exception
that there's a minimum brightness and the base address for the
brightness registers are different.
I would suggest that you add a min_brightness to wled and that you
figure out a way to carry the brightness base register address; and by
that you squash these two functions.
> +{
> + int rc, i;
> + u16 low_limit = wled->max_brightness * 4 / 1000;
> + u8 v[2];
>
> - rc = regmap_bulk_write(wled->regmap,
> - wled->addr + WLED3_CTRL_REG_VAL_BASE + 2 * i,
> - v, 2);
> - if (rc)
> + /* WLED4's lower limit of operation is 0.4% */
> + if (brightness > 0 && brightness < low_limit)
> + brightness = low_limit;
> +
> + v[0] = brightness & 0xff;
> + v[1] = (brightness >> 8) & 0xf;
> +
> + for (i = 0; i < wled->cfg.num_strings; ++i) {
> + rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
> + WLED4_SINK_REG_BRIGHT(i), v, 2);
> + if (rc < 0)
> return rc;
> }
>
> + return 0;
> +}
> +
> +static int wled_module_enable(struct wled *wled, int val)
> +{
> + int rc;
> +
> + rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
> + WLED3_CTRL_REG_MOD_EN,
> + WLED3_CTRL_REG_MOD_EN_MASK,
> + WLED3_CTRL_REG_MOD_EN_MASK);
This should depend on val.
> + return rc;
> +}
> +
> +static int wled3_sync_toggle(struct wled *wled)
> +{
> + int rc;
> +
> rc = regmap_update_bits(wled->regmap,
> - wled->addr + WLED3_SINK_REG_SYNC,
> - WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_ALL);
> - if (rc)
> + wled->ctrl_addr + WLED3_SINK_REG_SYNC,
> + WLED3_SINK_REG_SYNC_MASK,
> + WLED3_SINK_REG_SYNC_MASK);
> + if (rc < 0)
> return rc;
>
> rc = regmap_update_bits(wled->regmap,
> - wled->addr + WLED3_SINK_REG_SYNC,
> - WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_CLEAR);
> + wled->ctrl_addr + WLED3_SINK_REG_SYNC,
> + WLED3_SINK_REG_SYNC_MASK,
> + WLED3_SINK_REG_SYNC_CLEAR);
> +
> return rc;
> }
>
> -static int wled_setup(struct wled *wled)
> +static int wled4_sync_toggle(struct wled *wled)
This is identical to wled3_sync_toggle() if you express the SYNC_MASK as
GENMASK(max-string-count, 0);
> {
> int rc;
> - int i;
>
> rc = regmap_update_bits(wled->regmap,
> - wled->addr + WLED3_CTRL_REG_OVP,
> - WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
> - if (rc)
> + wled->sink_addr + WLED3_SINK_REG_SYNC,
> + WLED4_SINK_REG_SYNC_MASK,
> + WLED4_SINK_REG_SYNC_MASK);
> + if (rc < 0)
> return rc;
>
> rc = regmap_update_bits(wled->regmap,
> - wled->addr + WLED3_CTRL_REG_ILIMIT,
> - WLED3_CTRL_REG_ILIMIT_MASK, wled->cfg.boost_i_limit);
> + wled->sink_addr + WLED3_SINK_REG_SYNC,
> + WLED4_SINK_REG_SYNC_MASK,
> + WLED3_SINK_REG_SYNC_CLEAR);
> +
> + return rc;
> +}
> +
> +static int wled_update_status(struct backlight_device *bl)
You should be able to do the structural changes of this in one patch,
then follow up with the additions of WLED4 in a separate patch.
> +{
> + struct wled *wled = bl_get_data(bl);
> + u16 brightness = bl->props.brightness;
> + int rc = 0;
> +
> + if (bl->props.power != FB_BLANK_UNBLANK ||
> + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> + bl->props.state & BL_CORE_FBBLANK)
> + brightness = 0;
> +
> + if (brightness) {
> + rc = wled->wled_set_brightness(wled, brightness);
> + if (rc < 0) {
> + dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
> + rc);
> + return rc;
> + }
> +
> + rc = wled->wled_sync_toggle(wled);
> + if (rc < 0) {
> + dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
> + return rc;
> + }
> + }
> +
> + if (!!brightness != !!wled->brightness) {
> + rc = wled_module_enable(wled, !!brightness);
> + if (rc < 0) {
> + dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
> + return rc;
> + }
> + }
> +
> + wled->brightness = brightness;
> +
> + return rc;
> +}
> +
> +static int wled3_setup(struct wled *wled)
Changes to this function should be unrelated to the addition of WLED4
support.
> +{
> + u16 addr;
> + u8 sink_en = 0;
> + int rc, i, j;
> +
> + rc = regmap_update_bits(wled->regmap,
> + wled->ctrl_addr + WLED3_CTRL_REG_OVP,
> + WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
> if (rc)
> return rc;
>
> rc = regmap_update_bits(wled->regmap,
> - wled->addr + WLED3_CTRL_REG_FREQ,
> - WLED3_CTRL_REG_FREQ_MASK, wled->cfg.switch_freq);
> + wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
> + WLED3_CTRL_REG_ILIMIT_MASK,
> + wled->cfg.boost_i_limit);
[..]
> @@ -392,15 +743,33 @@ static int wled_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> wled->regmap = regmap;
> + wled->dev = &pdev->dev;
>
> - rc = wled_configure(wled, &pdev->dev);
> - if (rc)
> - return rc;
> + wled->version = of_device_get_match_data(&pdev->dev);
> + if (!wled->version) {
> + dev_err(&pdev->dev, "Unknown device version\n");
> + return -ENODEV;
> + }
>
> - rc = wled_setup(wled);
> + rc = wled_configure(wled);
Pass version as a parameter to wled_configure to make "version" a local
variable.
> if (rc)
> return rc;
>
> + if (*wled->version == WLED_PM8941) {
This would be better expressed with a select statement. And rather than
keeping track of every single version just stick to 3 and 4, if there
are further differences within version 4 let's try to encode these with
some sort of quirks or feature flags.
> + rc = wled3_setup(wled);
> + if (rc) {
> + dev_err(&pdev->dev, "wled3_setup failed\n");
> + return rc;
> + }
> + } else if (*wled->version == WLED_PMI8998 ||
> + *wled->version == WLED_PM660L) {
> + rc = wled4_setup(wled);
> + if (rc) {
> + dev_err(&pdev->dev, "wled4_setup failed\n");
> + return rc;
> + }
> + }
> +
> val = WLED_DEFAULT_BRIGHTNESS;
> of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
>
> @@ -415,7 +784,9 @@ static int wled_probe(struct platform_device *pdev)
> };
>
> static const struct of_device_id wled_match_table[] = {
> - { .compatible = "qcom,pm8941-wled" },
> + { .compatible = "qcom,pm8941-wled", .data = &version_table[0] },
> + { .compatible = "qcom,pmi8998-wled", .data = &version_table[1] },
> + { .compatible = "qcom,pm660l-wled", .data = &version_table[2] },
You can simply do .data = (void *)3 and .data = (void *)4 to pass the
WLED version to probe.
> {}
> };
Regards,
Bjorn
Powered by blists - more mailing lists