Re: [PATCH v3 2/3] leds: Add driver for Qualcomm LPG

On Sun 19 Nov 13:36 PST 2017, Jacek Anaszewski wrote:

Hi Bjorn,

Thanks for the patch. Please refer to my comments in the code.

On 11/15/2017 08:13 AM, Bjorn Andersson wrote:

[..]

quoted

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 52ea34e337cd..ccc3aa4b2474 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig

@@ -651,6 +651,13 @@ config LEDS_POWERNV
 	  To compile this driver as a module, choose 'm' here: the module
 	  will be called leds-powernv.
 
+config LEDS_QCOM_LPG
+	tristate "LED support for Qualcomm LPG"
+	depends on LEDS_CLASS

You were mentioning that this driver is for a MFD child block,
so we should probably depend on the parent MFD driver?

There's no build time dependency between the two, so it's not strictly
necessary.

Adding a dependency on MFD_SPMI_PMIC would indirectly add a dependency
on ARCH_QCOM, which limit build testing and stop some static code
checkers to check the driver.

So, unless you strongly object I would prefer not to mention the MFD.

quoted

+	help
+	  This option enables support for the Light Pulse Generator found in a
+	  wide variety of Qualcomm PMICs.
+

[..]

quoted

diff --git a/drivers/leds/leds-qcom-lpg.c b/drivers/leds/leds-qcom-lpg.c

[..]

quoted

+#define LPG_PATTERN_CONFIG_REG	0x40
+#define LPG_SIZE_CLK_REG	0x41
+#define LPG_PREDIV_CLK_REG	0x42
+#define PWM_TYPE_CONFIG_REG	0x43
+#define PWM_VALUE_REG		0x44
+#define PWM_ENABLE_CONTROL_REG	0x46
+#define PWM_SYNC_REG		0x47
+#define LPG_RAMP_DURATION_REG	0x50
+#define LPG_HI_PAUSE_REG	0x52
+#define LPG_LO_PAUSE_REG	0x54
+#define LPG_HI_IDX_REG		0x56
+#define LPG_LO_IDX_REG		0x57
+#define PWM_SEC_ACCESS_REG	0xd0
+#define PWM_DTEST_REG(x)	(0xe2 + (x) - 1)
+
+#define TRI_LED_SRC_SEL		0x45
+#define TRI_LED_EN_CTL		0x46
+#define TRI_LED_ATC_CTL		0x47
+
+#define LPG_LUT_REG(x)		(0x40 + (x) * 2)
+#define RAMP_CONTROL_REG	0xc8

Please add QCOM_ namespacing prefix to the macros.
At least PWM prefix is reserved for pwm subsystem.

Will fix.

[..]

quoted

+static void lpg_calc_freq(struct lpg_channel *chan, unsigned int period_us)
+{
+	int             n, m, clk, div;
+	int             best_m, best_div, best_clk;
+	unsigned int    last_err, cur_err, min_err;
+	unsigned int    tmp_p, period_n;
+
+	if (period_us == chan->period_us)
+		return;
+
+	/* PWM Period / N */
+	if (period_us < ((unsigned int)(-1) / NSEC_PER_USEC)) {
+		period_n = (period_us * NSEC_PER_USEC) >> 6;
+		n = 6;
+	} else {
+		period_n = (period_us >> 9) * NSEC_PER_USEC;
+		n = 9;
+	}

Please provide macros for 6 and 9 magic numbers.

They really aren't magic numbers, they represent the number of bits of
resolution, referred to as "size" in the rest of the driver.

I'll replace "n" with "pwm_size" to clarify this. Ok?

[..]

quoted

+static void lpg_apply_freq(struct lpg_channel *chan)
+{
+	unsigned long val;
+	struct lpg *lpg = chan->lpg;
+
+	if (!chan->enabled)
+		return;
+
+	/* Clock register values are off-by-one from lpg_clk_table */
+	val = chan->clk + 1;
+
+	if (chan->pwm_size == 9)
+		val |= lpg->data->pwm_9bit_mask;
+
+	regmap_write(lpg->map, chan->base + LPG_SIZE_CLK_REG, val);
+
+	val = chan->pre_div << 5 | chan->pre_div_exp;
+	regmap_write(lpg->map, chan->base + LPG_PREDIV_CLK_REG, val);

Please provide macros for 5 and 9.

5 definitely deserves a macro.

9 is, as above, the number of bits of resolution (or "size of the pwm").
Is there a name different than "pwm_size" that would make this more
obvious?

[..]

quoted

+static void lpg_brightness_set(struct led_classdev *cdev,
+			      enum led_brightness value)
+{

[..]

quoted

+	/* Trigger start of ramp generator(s) */
+	if (lut_mask)
+		lpg_lut_sync(lpg, lut_mask);

We need some synchronization while changing device state in
few steps, to prevent troubles when we are preempted by other
process in the middle. spin_lock() in this case since it seems
that we are not going to sleep while accessing device registers.

You're right, we need to protect the TRILED during the read-modify-write
of the enable bits and we also need a lock around the allocation of bits
in the LUT block. Will fix this.

As far as I can see the framework is expected to protect me from
concurrent accesses on the same LED though.

[..]

quoted

+static int lpg_add_led(struct lpg *lpg, struct device_node *np)
+{
+	struct lpg_led *led;
+	const char *state;
+	int sources;
+	int size;
+	u32 chan;
+	int ret;
+	int i;
+
+	sources = of_property_count_u32_elems(np, "led-sources");
+	if (sources <= 0) {
+		dev_err(lpg->dev, "invalid led-sources of %s\n",
+			np->name);
+		return -EINVAL;
+	}
+
+	size = sizeof(*led) + sources * sizeof(struct lpg_channel*);

To fix checkpatch.pl complaint:

s/lpg_channel*/lpg_channel */

Sorry, will fix.

quoted

+	led = devm_kzalloc(lpg->dev, size, GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->lpg = lpg;
+	led->num_channels = sources;
+
+	for (i = 0; i < sources; i++) {
+		ret = of_property_read_u32_index(np, "led-sources",
+						 i, &chan);
+		if (ret || !chan || chan > lpg->num_channels) {
+			dev_err(lpg->dev,
+				"invalid led-sources of %s\n",
+				np->name);
+			return -EINVAL;
+		}
+
+		led->channels[i] = &lpg->channels[chan - 1];
+
+		led->channels[i]->in_use = true;
+	}
+
+	/* Use label else node name */
+	led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;

Documentation/leds/leds-class.txt states that LED class device name
pattern is devicename:colour:function.

This is not explicitly stated in the common LED DT bindings, but label
should be prepended with devicename by the driver.

I was under the impression that "devicename" referred to the board name,
but I presume then that this refer to the name of the "LED hardware"?

Not all LED class drivers adhere to this rule and we have some mess in
this area currently, but we will fix it soon I hope.

I presume the default name should be built on the form of
dev_name(dev)::of_node->name then?

Unfortunately I can't find a single driver that does this, so please
let me know the format you would like and I'll update the driver with
this.

quoted

+	led->cdev.default_trigger = of_get_property(np, "linux,default-trigger", NULL);
+	led->cdev.brightness_set = lpg_brightness_set;
+	led->cdev.brightness_get = lpg_brightness_get;
+	led->cdev.blink_set = lpg_blink_set;
+	led->cdev.max_brightness = 255;

You can skip this line, since it will be set to LED_FULL
in case passed 0 to led_classdev_init().

Convenient.


Thank you for the review!

Regards,
Bjorn