Date:   Mon, 11 Dec 2017 14:41:34 +0530
From:   kgunda@...eaurora.org
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     linux-arm-msm@...r.kernel.org, Lee Jones <lee.jones@...aro.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Richard Purdie <rpurdie@...ys.net>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
        linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH V1 1/4] qcom: spmi-wled: Add support for qcom wled driver

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@...eaurora.org>
>> ---
>>  .../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@...0 {
>> +	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

Powered by blists - more mailing lists