Re: [PATCH V1 1/4] qcom: spmi-wled: Add support for qcom wled driver

From: kgunda
Date: Mon Dec 11 2017 - 04:11:44 EST

On 2017-12-05 07:31, Bjorn Andersson wrote:
On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote:

WLED driver provides the interface to the display driver to
adjust the brightness of the display backlight.

Signed-off-by: Kiran Gunda <kgunda@xxxxxxxxxxxxxx>
---
.../bindings/leds/backlight/qcom-spmi-wled.txt | 90 ++++
drivers/video/backlight/Kconfig | 9 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/qcom-spmi-wled.c | 504 +++++++++++++++++++++
4 files changed, 604 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
create mode 100644 drivers/video/backlight/qcom-spmi-wled.c

diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
new file mode 100644
index 0000000..f1ea25b
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
@@ -0,0 +1,90 @@
+Binding for Qualcomm WLED driver
+

This binding document quite well describe the pm8941 as well, so please
improve the existing binding (changing to this style is preferable).

Sure. Will do it in the next series, where I will re-use pm8941 driver for
PMI8998 as well.
+WLED (White Light Emitting Diode) driver is used for controlling display
+backlight that is part of PMIC on Qualcomm Technologies reference platforms.
+The PMIC is connected to the host processor via SPMI bus.
+
+- compatible
+ Usage: required
+ Value type: <string>
+ Definition: should be "qcom,pm8998-spmi-wled".

There's no WLED in the pm8998, so please make this pmi8998. This pmic is
SPMI only, so there's no need to keep "spmi" in the compatible.

Sure. Will change it.
+
+- reg
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: Base address and size of the WLED modules.
+
+- reg-names
+ Usage: required
+ Value type: <string>
+ Definition: Names associated with base addresses. should be
+ "qcom-wled-ctrl-base", "qcom-wled-sink-base".
+
+- label
+ Usage: required
+ Value type: <string>
+ Definition: The name of the backlight device.
+
+- default-brightness
+ Usage: optional
+ Value type: <u32>
+ Definition: brightness value on boot, value from: 0-4095
+ default: 2048
+
+- qcom,fs-current-limit
+ Usage: optional
+ Value type: <u32>
+ Definition: per-string full scale current limit in uA. value from
+ 0 to 30000 with 5000 uA resolution. default: 25000 uA

"in steps of 5mA"

Will address it in next series.
+
+- qcom,current-boost-limit
+ Usage: optional
+ Value type: <u32>
+ Definition: ILIM threshold in mA. values are 105, 280, 450, 620, 970,
+ 1150, 1300, 1500. default: 970 mA
+
+- qcom,switching-freq
+ Usage: optional
+ Value type: <u32>
+ Definition: Switching frequency in KHz. values are
+ 600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
+ 1600, 1920, 2400, 3200, 4800, 9600.
+ default: 800 KHz
+
+- qcom,ovp
+ Usage: optional
+ Value type: <u32>
+ Definition: Over-voltage protection limit in mV. values are 31100,
+ 29600, 19600, 18100.
+ default: 29600 mV
+
+- qcom,string-cfg
+ Usage: optional
+ Value type: <u32>
+ Definition: Bit mask of the wled strings. Bit 0 to 3 indicates strings
+ 0 to 3 respectively. Wled module has four strings of leds
+ numbered from 0 to 3. Each string of leds are operated
+ individually. Specify the strings using the bit mask. Any
+ combination of led strings can be used.
+ default value is 15 (b1111).

Please try to avoid expressing things as bitmasks in DT.

The only difference from 8941 here is that there's one additional
string, so please start off by expressing this as the existing binding.

If you really need this flexibility you can follow up with an addition
of a property like this, but name it something like
"qcom,enabled-strings" and make this support available for pm8941 as
well.

Sure. Will address it.
+
+- qcom,en-cabc

No need for the "en", the presence of a bool property means that it's
enabled.

Will address it in next series.
+ Usage: optional
+ Value type: <bool>
+ Definition: Specify if cabc (content adaptive backlight control) is
+ needed.

I presume cabc isn't ever "needed", just make the description "Enable
content adaptive backlight control".

Will address it in next series.
+
+Example:
+
+qcom-wled@d800 {
+ compatible = "qcom,pm8998-spmi-wled";
+ reg = <0xd800 0xd900>;
+ reg-names = "qcom-wled-ctrl-base", "qcom-wled-sink-base";
+ label = "backlight";
+
+ qcom,fs-current-limit = <25000>;
+ qcom,current-boost-limit = <970>;
+ qcom,switching-freq = <800>;
+ qcom,ovp = <29600>;
+ qcom,string-cfg = <15>;
+};
[..]
diff --git a/drivers/video/backlight/qcom-spmi-wled.c b/drivers/video/backlight/qcom-spmi-wled.c

After reviewing your arguments and comparing the drivers I still think
it's beneficial to support both these hardware revisions in the same
driver.

The majority of the register differences relates to the current sink
being split out, but this can easily be handled by a few well places
accessor functions - which depends on this being the case or not.

The addition of OVP handling would benefit 8941 as well.

The short circuit handling in your patches are isolated and not taking
this code path on 8941 should not pose any problems.

[..]
Ok. I will reuse the pm8941-wled.c driver for pmi8998.
+/* General definitions */
+#define QCOM_WLED_DEFAULT_BRIGHTNESS 2048
+#define QCOM_WLED_MAX_BRIGHTNESS 4095
+
+/* WLED control registers */
+#define QCOM_WLED_CTRL_MOD_ENABLE 0x46
+#define QCOM_WLED_CTRL_MOD_EN_MASK BIT(7)
+#define QCOM_WLED_CTRL_MODULE_EN_SHIFT 7
+
+#define QCOM_WLED_CTRL_SWITCH_FREQ 0x4c
+#define QCOM_WLED_CTRL_SWITCH_FREQ_MASK GENMASK(3, 0)
+
+#define QCOM_WLED_CTRL_OVP 0x4d
+#define QCOM_WLED_CTRL_OVP_MASK GENMASK(1, 0)
+
+#define QCOM_WLED_CTRL_ILIM 0x4e
+#define QCOM_WLED_CTRL_ILIM_MASK GENMASK(2, 0)
+
+/* WLED sink registers */
+#define QCOM_WLED_SINK_CURR_SINK_EN 0x46
+#define QCOM_WLED_SINK_CURR_SINK_MASK GENMASK(7, 4)
+#define QCOM_WLED_SINK_CURR_SINK_SHFT 0x04

Shifts are typically not given as hex...

Will address it in next series.
+
+#define QCOM_WLED_SINK_SYNC 0x47
+#define QCOM_WLED_SINK_SYNC_MASK GENMASK(3, 0)
+#define QCOM_WLED_SINK_SYNC_LED1 BIT(0)
+#define QCOM_WLED_SINK_SYNC_LED2 BIT(1)
+#define QCOM_WLED_SINK_SYNC_LED3 BIT(2)
+#define QCOM_WLED_SINK_SYNC_LED4 BIT(3)
+#define QCOM_WLED_SINK_SYNC_CLEAR 0x00
+
+#define QCOM_WLED_SINK_MOD_EN_REG(n) (0x50 + (n * 0x10))
+#define QCOM_WLED_SINK_REG_STR_MOD_MASK BIT(7)
+#define QCOM_WLED_SINK_REG_STR_MOD_EN BIT(7)
+
+#define QCOM_WLED_SINK_SYNC_DLY_REG(n) (0x51 + (n * 0x10))
+#define QCOM_WLED_SINK_FS_CURR_REG(n) (0x52 + (n * 0x10))
+#define QCOM_WLED_SINK_FS_MASK GENMASK(3, 0)
+
+#define QCOM_WLED_SINK_CABC_REG(n) (0x56 + (n * 0x10))
+#define QCOM_WLED_SINK_CABC_MASK BIT(7)
+#define QCOM_WLED_SINK_CABC_EN BIT(7)
+
+#define QCOM_WLED_SINK_BRIGHT_LSB_REG(n) (0x57 + (n * 0x10))
+#define QCOM_WLED_SINK_BRIGHT_MSB_REG(n) (0x58 + (n * 0x10))
+
+struct qcom_wled_config {
+ u32 i_boost_limit;
+ u32 ovp;
+ u32 switch_freq;
+ u32 fs_current;
+ u32 string_cfg;
+ bool en_cabc;
+};
+
+struct qcom_wled {
+ const char *name;
+ struct platform_device *pdev;

Lug around the struct device * instead of the platform_device, and use
this for dev_* prints throughout the code.

Will address it in next series.
+ struct regmap *regmap;
+ u16 sink_addr;
+ u16 ctrl_addr;
+ u32 brightness;
+ bool prev_state;

You can derive prev_state from wled->brightness in
qcom_wled_update_status().

Will remove it in next series.
+
+ struct qcom_wled_config cfg;
+};
+
+static int qcom_wled_module_enable(struct qcom_wled *wled, int val)
+{
+ int rc;
+
+ rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
+ QCOM_WLED_CTRL_MOD_ENABLE, QCOM_WLED_CTRL_MOD_EN_MASK,
+ val << QCOM_WLED_CTRL_MODULE_EN_SHIFT);

This shift obfuscate the fact that val is only 0 or 1, make val a bool
and make the macro for the enabled state be BIT(7).

Will address it in next series.
+ return rc;
+}
+
+static int qcom_wled_get_brightness(struct backlight_device *bl)
+{
+ struct qcom_wled *wled = bl_get_data(bl);
+
+ return wled->brightness;
+}
+
+static int qcom_wled_sync_toggle(struct qcom_wled *wled)
+{
+ int rc;
+
+ rc = regmap_update_bits(wled->regmap,
+ wled->sink_addr + QCOM_WLED_SINK_SYNC,
+ QCOM_WLED_SINK_SYNC_MASK, QCOM_WLED_SINK_SYNC_MASK);
+ if (rc < 0)
+ return rc;
+
+ rc = regmap_update_bits(wled->regmap,
+ wled->sink_addr + QCOM_WLED_SINK_SYNC,
+ QCOM_WLED_SINK_SYNC_MASK, QCOM_WLED_SINK_SYNC_CLEAR);
+
+ return rc;
+}
+
+static int qcom_wled_set_brightness(struct qcom_wled *wled, u16 brightness)
+{
+ int rc, i;
+ u16 low_limit = QCOM_WLED_MAX_BRIGHTNESS * 4 / 1000;
+ u8 string_cfg = wled->cfg.string_cfg;
+ u8 v[2];
+
+ /* WLED's lower limit of operation is 0.4% */
+ if (brightness > 0 && brightness < low_limit)
+ brightness = low_limit;

What happens between 0 and 0.4%? Is this policy or is this related to
some hardware issue?

This is related to a HW bug and if the brightness goes below 0.4% when
the module is enabled, we see the continuous OVP interrupts.
Also, this function will not be called with brightness = 0, so you don't
need to check that case.

We are disabling the module when the brightness is '0'. We update the brightness
and enable the module for the next update request. So it is not needed to call this
function for '0' brightness.
+
+ v[0] = brightness & 0xff;
+ v[1] = (brightness >> 8) & 0xf;
+
+ for (i = 0; (string_cfg >> i) != 0; i++) {

The condition looks optimal... Just loop from 0 to 3 and it will be
easier to read without any measurable losses.

sure. will address it in next series.
+ if (string_cfg & BIT(i)) {

Flip this condition around and use "continue" to reduce the indentation
level of the rest of the block.

sure. will address it in next series.
+ rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
+ QCOM_WLED_SINK_BRIGHT_LSB_REG(i), v, 2);
+ if (rc < 0)
+ return rc;
+ }
+ }
+
+ return 0;
+}
+
+static int qcom_wled_update_status(struct backlight_device *bl)
+{
+ struct qcom_wled *wled = bl_get_data(bl);
+ u16 brightness = bl->props.brightness;
+ int rc;
+
+ 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 = qcom_wled_set_brightness(wled, brightness);
+ if (rc < 0) {
+ pr_err("wled failed to set brightness rc:%d\n", rc);

Use dev_err() and dev_dbg() throughout the driver.

sure. will address it in next series.
+ return rc;
+ }
+
+ if (!!brightness != wled->prev_state) {
+ rc = qcom_wled_module_enable(wled, !!brightness);
+ if (rc < 0) {
+ pr_err("wled enable failed rc:%d\n", rc);
+ return rc;
+ }
+ }

This block is exactly the same as the else statement, there's no need to
repeat yourself.

This else is for the "if (brightness) {" not for the just above it.
+ } else {
+ rc = qcom_wled_module_enable(wled, brightness);
+ if (rc < 0) {
+ pr_err("wled disable failed rc:%d\n", rc);
+ return rc;
+ }
+ }
+
+ wled->prev_state = !!brightness;
+
+ rc = qcom_wled_sync_toggle(wled);
+ if (rc < 0) {
+ pr_err("wled sync failed rc:%d\n", rc);
+ return rc;
+ }
+
+ wled->brightness = brightness;
+
+ return rc;
+}
+
+static int qcom_wled_setup(struct qcom_wled *wled)
+{
+ int rc, temp, i;
+ u8 sink_en = 0;
+ u8 string_cfg = wled->cfg.string_cfg;
+
+ rc = regmap_update_bits(wled->regmap,
+ wled->ctrl_addr + QCOM_WLED_CTRL_OVP,
+ QCOM_WLED_CTRL_OVP_MASK, wled->cfg.ovp);
+ if (rc < 0)
+ return rc;
+
+ rc = regmap_update_bits(wled->regmap,
+ wled->ctrl_addr + QCOM_WLED_CTRL_ILIM,
+ QCOM_WLED_CTRL_ILIM_MASK, wled->cfg.i_boost_limit);
+ if (rc < 0)
+ return rc;
+
+ rc = regmap_update_bits(wled->regmap,
+ wled->ctrl_addr + QCOM_WLED_CTRL_SWITCH_FREQ,
+ QCOM_WLED_CTRL_SWITCH_FREQ_MASK, wled->cfg.switch_freq);
+ if (rc < 0)
+ return rc;
+
+ for (i = 0; (string_cfg >> i) != 0; i++) {
+ if (string_cfg & BIT(i)) {

Same as above.

Ok. Will address it in next series.
+ u16 addr = wled->sink_addr +
+ QCOM_WLED_SINK_MOD_EN_REG(i);
+
+ rc = regmap_update_bits(wled->regmap, addr,
+ QCOM_WLED_SINK_REG_STR_MOD_MASK,
+ QCOM_WLED_SINK_REG_STR_MOD_EN);
+ if (rc < 0)
+ return rc;
+
+ addr = wled->sink_addr +
+ QCOM_WLED_SINK_FS_CURR_REG(i);
+ rc = regmap_update_bits(wled->regmap, addr,
+ QCOM_WLED_SINK_FS_MASK,
+ wled->cfg.fs_current);
+ if (rc < 0)
+ return rc;
+
+ addr = wled->sink_addr +
+ QCOM_WLED_SINK_CABC_REG(i);
+ rc = regmap_update_bits(wled->regmap, addr,
+ QCOM_WLED_SINK_CABC_MASK,
+ wled->cfg.en_cabc ?
+ QCOM_WLED_SINK_CABC_EN : 0);
+ if (rc)
+ return rc;
+
+ temp = i + QCOM_WLED_SINK_CURR_SINK_SHFT;
+ sink_en |= 1 << temp;

I'm failing to see the reason for the "temp" variable here. Please do:

sink_en |= BIT(i + QCOM_WLED_SINK_CURR_SINK_SHFT)

Ok. Will address it in next series.
+ }
+ }
+
+ rc = regmap_update_bits(wled->regmap,
+ wled->sink_addr + QCOM_WLED_SINK_CURR_SINK_EN,
+ QCOM_WLED_SINK_CURR_SINK_MASK, sink_en);
+ if (rc < 0)
+ return rc;
+
+ rc = qcom_wled_sync_toggle(wled);
+ if (rc < 0) {
+ pr_err("Failed to toggle sync reg rc:%d\n", rc);
+ return rc;
+ }
+
+ return 0;
+}
+
[..]
+static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev)
+{
+ struct qcom_wled_config *cfg = &wled->cfg;
+ const __be32 *prop_addr;
+ u32 val, c;
+ int rc, i, j;
+
+ const struct {
+ const char *name;
+ u32 *val_ptr;
+ const struct qcom_wled_var_cfg *cfg;
+ } u32_opts[] = {

I suggest that you tie this list of options to the compatible (through
of_device_id->data) and pass it as a parameter to this function. That
way you can handle variation in properties and their values between
different compatibles.

Ok. Will address it in next series.
[..]
+ *cfg = wled_config_defaults;
+ for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) {
+ rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val);

of_property_read_u32() returns -ENODATA when there's no associated data,
you can probably use this to implement support for the boolean types in
the same list of opts.

[..]
+ }
+
+ for (i = 0; i < ARRAY_SIZE(bool_opts); ++i) {
+ if (of_property_read_bool(dev->of_node, bool_opts[i].name))
+ *bool_opts[i].val_ptr = true;
+ }
+
+ return 0;
+}
+
+static const struct backlight_ops qcom_wled_ops = {
+ .update_status = qcom_wled_update_status,
+ .get_brightness = qcom_wled_get_brightness,
+};
+
+static int qcom_wled_probe(struct platform_device *pdev)
+{
+ struct backlight_properties props;
+ struct backlight_device *bl;
+ struct qcom_wled *wled;
+ struct regmap *regmap;
+ u32 val;
+ int rc;
+
+ regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!regmap) {
+ pr_err("Unable to get regmap\n");
+ return -EINVAL;
+ }
+
+ wled = devm_kzalloc(&pdev->dev, sizeof(*wled), GFP_KERNEL);
+ if (!wled)
+ return -ENOMEM;
+
+ wled->regmap = regmap;
+ wled->pdev = pdev;
+
+ rc = qcom_wled_configure(wled, &pdev->dev);
+ if (rc < 0) {
+ pr_err("wled configure failed rc:%d\n", rc);

qcom_wled_configure() already printed an error message for you, no need
to repeat this.

Will address it in next series.
+ return rc;
+ }
+

Please also run checkpatch.pl with the --strict option and fix the
indentation issues reported.

Sure.
Regards,
Bjorn