| Message ID | 20171115071345.24331-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | Qualcomm Light Pulse Generator | expand |
On Mon 2017-11-20 13:10:33, Bjorn Andersson wrote: > 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. OTOH, this way even users that can't have qualcom LPG are asked the question. So right way is depends on LEdS_CLASS && (BUILD_TEST || MFD_SPMI_PMIC). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 11/20/2017 10:10 PM, Bjorn Andersson wrote: > 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? 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? pwm_res_bits or maybe you could use directly the register bit field name from the documentation? > [..] >>> +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. Access from sysfs is synchronized but LED subsystem API can be called from atomic context (e.g. from hard/soft irqs), which entails issues for drivers that sleep while setting brightness - proper locking method has to be chosen in the driver (mutex or spin_lock). That's why brightness_set_blocking op was provided for drivers sleeping on brightness setting - they can safely use mutex_lock and the rest using spin_lock implement old brightness_set. Before introduction of brightness_set_blocking op drivers had to use workqueue on their own, now this is handled by the LED core. The mutex protection in LED class sysfs interface accessors was introduced rather to allow for locking LED subsystem sysfs interface when v4l2-flash device controls a flash LED. > [..] >>> +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"? Right. > >> 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. Currently the best approach is shown in the drivers/leds/leds-as3645a.c I believe, i.e. in case label is absent, devicename is taken from the parent DT node and function from per-LED child DT node. Please also refer to the patch [0], which improves the code present in mainline but has not been applied yet due to some flash LED controller related decisions to be taken yet. [0] https://patchwork.linuxtv.org/patch/44275/ -- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 22 Nov 12:42 PST 2017, Jacek Anaszewski wrote: > On 11/20/2017 10:45 PM, Bjorn Andersson wrote: > > On Mon 20 Nov 12:35 PST 2017, Jacek Anaszewski wrote: > > > >> On 11/20/2017 08:58 PM, Bjorn Andersson wrote: > >>> On Sun 19 Nov 13:35 PST 2017, Jacek Anaszewski wrote: > >>> > >>>> Hi Bjorn, > >>>> > >>>> Thanks for the update. > >>>> > >>>> On 11/15/2017 08:13 AM, Bjorn Andersson wrote: > >>>>> This adds the binding document describing the three hardware blocks > >>>>> related to the Light Pulse Generator found in a wide range of Qualcomm > >>>>> PMICs. > >>>>> > >>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > >>>>> --- > >>>>> > >>>>> Changes since v2: > >>>>> - Squashed all things into one node > >>>>> - Removed quirks from the binding, compatible implies number of channels, their > >>>>> configuration etc. > >>>>> - Binding describes LEDs connected as child nodes > >>>>> - Support describing multi-channel LEDs > >>>>> - Change style of the binding document, to match other LED bindings > >>>>> > >>>>> Changes since v1: > >>>>> - Dropped custom pattern properties > >>>>> - Renamed cell-index to qcom,lpg-channel to clarify its purpose > >>>>> > >>>>> .../devicetree/bindings/leds/leds-qcom-lpg.txt | 66 ++++++++++++++++++++++ > >>>>> 1 file changed, 66 insertions(+) > >>>>> create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt > >>>>> new file mode 100644 > >>>>> index 000000000000..9cee6f9f543c > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt > >>>>> @@ -0,0 +1,66 @@ > >>>>> +Binding for Qualcomm Light Pulse Generator > >>>>> + > >>>>> +The Qualcomm Light Pulse Generator consists of three different hardware blocks; > >>>>> +a ramp generator with lookup table, the light pulse generator and a three > >>>>> +channel current sink. These blocks are found in a wide range of Qualcomm PMICs. > >>>>> + > >>>>> +Required properties: > >>>>> +- compatible: one of: > >>>>> + "qcom,pm8916-pwm", > >>>>> + "qcom,pm8941-lpg", > >>>>> + "qcom,pm8994-lpg", > >>>>> + "qcom,pmi8994-lpg", > >>>>> + "qcom,pmi8998-lpg", > >>>>> + > >>>>> +Optional properties: > >>>>> +- qcom,power-source: power-source used to drive the output, as defined in the > >>>>> + datasheet. Should be specified if the TRILED block is > >>>>> + present > >>>> > >>>> Range of possible values is missing here. > >>>> > >>> > >>> There seems to be a 4-way mux in all variants, but the wiring is > >>> different in the different products. E.g. in pm8941 1 represents VPH_PWR > >>> while in pmi8994 this is pulled from a dedicated pin named VIN_RGB. > >>> > >>> Would you like me to list the 4 options for each compatible? > >> > >> Could you please explain why user would prefer one power source > >> over the other? Is it that they have different max current limit? > >> > > > > The mux in pm8941 is connected to ground (0V), vph_pwr (3.6V), internal > > 5V and min(5V, charger). In pmi8994 it's ground, vdd_rgb (a dedicated > > pin) and 4.2V. PMI8998 is a slight variation of PMI8994 and I expect > > there to be more variants. > > > > So it's different voltage level and, potentially, current limit. > > I'd replace this property with led-max-microamp > (see Documentation/devicetree/bindings/leds/common.txt) and let > the driver to decide which power source to choose, basing on that limit. > Unfortunately I don't think specifying this in uA is possible, at least some of these inputs does not have a knows (at least not platform defined) current limit. Like in the case on PM8941, the knobs this property tweaks is: "should we output 3.6V or 5V" and both values depend on external power-supplies. For most cases specifying this as led-microvolt would be possible, but then there are the levels that are min(5V, charger). One thing we've done in some other drivers with similar "enum" like types is to provide an dt-bindings include file with these constants. This makes the dts self documented and doesn't require additional translation of the values. > > [..] > >> One more question regarding TRILED - in your design it will be > >> exposed as a single LED class device with one brightness file, > >> right? Does it mean that all three LEDs will be applied the > >> same brightness after writing it to the sysfs file? > >> > > > > Correct, each LED described in DT will become one LED and can have more > > than one (any number of) physical channel associated. The current > > implementation applies the same brightness (and pattern) to all channels > > associated with a LED. > > The rgb DT node name would be a bit misleading in this case, since > RGB usually implies the possibility of having different intensity > of each color. > In the sense of the devicetree this is exactly what it describes, the fact that we haven't figured out a way to implement this is, in my view, a separate topic. > > The open question is still how to pass a color from user space, the > > brightness_set and pattern_set would need to be modified to map a list > > of brightnesses to the individual channels or to adapt the brightness by > > some color-modifier(?). > > Pavel made and attempt of reworking Heiner Kallweit's HSV approach > few months ago [0]. You can take a look and share your opinion > or even continue this effort. > I did not consider using HSV to get around the problem of everything operating on "brightness", but this seems like a quite nice solution. In the case of lpg_brightness_set() this would map nicely into the case where a LED is either a single channel or three channels, and until we land those patches the driver would just implement H = S = 0. And for the pattern setting, we can retain the proposed interface of pattern being a sequence of brightness/delay values and then map this in the driver as we apply the patterns. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html