[PATCH V3 5/7] backlight: qcom-wled: Add support for WLED4 peripheral
kgunda at codeaurora.org
kgunda at codeaurora.org
Wed Jun 20 11:00:35 UTC 2018
On 2018-06-20 10:44, Bjorn Andersson wrote:
> 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 at codeaurora.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.
>
Sure. I will split it in the next series.
>>
>> 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.
>
There are four different parameters. 1) minimum brightness 2) WLED base
addresses
3) Brightness base addresses 4) the brightness sink registers address
offsets also
different for wled 3 and wled4. (in wled3 0x40, 0x42, 0x44, where as in
wled4 0x57, 0x67, 0x77, 0x87).
Irrelevant to this patch, but wled5 has some more extra registers to set
the brightness.
Keeping this in mind, it is better to have separate functions? Otherwise
we will have to use the
version checks in the wled_set_brightness function, if we have the
common function.
>> +{
>> + 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.
>
Will fix it in the next series.
>> + 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);
>
Will change it in the next series.
>> {
>> 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.
>
Sure. Will do it in the next series.
>> +{
>> + 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.
>
I will separate it out from this patch in next series.
>> +{
>> + 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.
>
Sure. I will do it in the next series.
>> 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.
>
I will modify it in the next series.
>> + 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.
>
I will modify it in the next series.
>> {}
>> };
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-arm-msm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the dri-devel
mailing list