Date:   Mon, 20 Nov 2017 13:10:33 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>
Cc:     Richard Purdie <rpurdie@...ys.net>, Pavel Machek <pavel@....cz>,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, Fenglin Wu <fenglinw@...eaurora.org>
Subject: 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:
[..]
> > 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.

> > +	help
> > +	  This option enables support for the Light Pulse Generator found in a
> > +	  wide variety of Qualcomm PMICs.
> > +
[..]
> > diff --git a/drivers/leds/leds-qcom-lpg.c b/drivers/leds/leds-qcom-lpg.c
[..]
> > +#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.

[..]
> > +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?

[..]
> > +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?

[..]
> > +static void lpg_brightness_set(struct led_classdev *cdev,
> > +			      enum led_brightness value)
> > +{
[..]
> > +	/* 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.

[..]
> > +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.

> > +	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.

> > +	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

Powered by blists - more mailing lists