Messages in this thread | | | Date | Thu, 17 May 2018 15:17:34 +0530 | From | kgunda@codeauro ... | Subject | Re: [PATCH V1 2/5] backlight: qcom-wled: Add support for WLED4 peripheral |
| |
On 2018-05-08 15:55, kgunda@codeaurora.org wrote: > On 2018-05-07 21:50, Bjorn Andersson wrote: >> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote: >> >>> WLED4 peripheral is present on some PMICs like pmi8998 >>> and pm660l. It has a different register map and also >>> configurations are different. Add support for it. >>> >> >> Several things are going on in this patch, it needs to be split to >> not hide the functional changes from the structural/renames. >> > Ok. I will split it in the next series. >>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> >>> --- >>> .../bindings/leds/backlight/qcom-wled.txt | 172 ++++- >>> drivers/video/backlight/qcom-wled.c | 749 >>> +++++++++++++++------ >>> 2 files changed, 696 insertions(+), 225 deletions(-) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt >>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt >>> index fb39e32..0ceffa1 100644 >>> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt >>> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt >>> @@ -1,30 +1,129 @@ >>> Binding for Qualcomm Technologies, Inc. WLED driver >>> >>> -Required properties: >>> -- compatible: should be "qcom,pm8941-wled" >>> -- reg: slave address >>> - >>> -Optional properties: >>> -- default-brightness: brightness value on boot, value from: 0-4095 >>> - default: 2048 >>> -- label: The name of the backlight device >>> -- qcom,cs-out: bool; enable current sink output >>> -- qcom,cabc: bool; enable content adaptive backlight control >>> -- qcom,ext-gen: bool; use externally generated modulator signal to >>> dim >>> -- qcom,current-limit: mA; per-string current limit; value from 0 to >>> 25 >>> - default: 20mA >>> -- qcom,current-boost-limit: mA; boost current limit; one of: >>> - 105, 385, 525, 805, 980, 1260, 1400, 1680 >>> - default: 805mA >>> -- qcom,switching-freq: kHz; switching frequency; one of: >>> - 600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371, >>> - 1600, 1920, 2400, 3200, 4800, 9600, >>> - default: 1600kHz >>> -- qcom,ovp: V; Over-voltage protection limit; one of: >>> - 27, 29, 32, 35 >>> - default: 29V >>> -- qcom,num-strings: #; number of led strings attached; value from 1 >>> to 3 >>> - default: 2 >>> +WLED (White Light Emitting Diode) driver is used for controlling >>> display >>> +backlight that is part of PMIC on Qualcomm Technologies, Inc. >>> reference >>> +platforms. The PMIC is connected to the host processor via SPMI bus. >>> + >>> +- compatible >>> + Usage: required >>> + Value type: <string> >>> + Definition: should be "qcom,pm8941-wled" or "qcom,pmi8998-wled". >>> + or "qcom,pm660l-wled". >> >> Better written as >> >> should be one of: >> X >> Y >> Z >> > Will do it in the next series. >>> + >>> +- reg >>> + Usage: required >>> + Value type: <prop encoded array> >>> + Definition: Base address of the WLED modules. >>> + >>> +- interrupts >>> + Usage: optional >>> + Value type: <prop encoded array> >>> + Definition: Interrupts associated with WLED. Interrupts can be >>> + specified as per the encoding listed under >>> + Documentation/devicetree/bindings/spmi/ >>> + qcom,spmi-pmic-arb.txt. >> >> Better to describe that this should be the "short" and "ovp" >> interrupts >> in this property than in the interrupt-names. >> > Ok. I will do it in the next series. >>> + >>> +- interrupt-names >>> + Usage: optional >>> + Value type: <string> >>> + Definition: Interrupt names associated with the interrupts. >>> + Must be "short" and "ovp". The short circuit detection >>> + is not supported for PM8941. >>> + >>> +- 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,current-limit >>> + Usage: optional >>> + Value type: <u32> >>> + Definition: uA; per-string current limit >> >> You can't change unit on an existing property, that breaks any >> existing >> dts using the qcom,pm8941-wled compatible. >> > >>> + value: >>> + For pm8941: from 0 to 25000 with 5000 ua step >>> + Default 20000 uA >>> + For pmi8998: from 0 to 30000 with 5000 ua step >>> + Default 25000 uA. >> >> These values could be described just as well in mA, so keep the >> original >> unit - in particular since the boot-limit is in mA... >> > Ok. Will keep the original as is in the next series. Here, I may have to go with the approach as in "qcom,ovp". Because for pm8941 the current step is 1 mA (I have wrongly mentioned as 5000uA here) and for PMI8998 the current step is 2.5 mA. Hence, I will add another variable "qcom,current-limit-ua" just like "qcom,ovp-mv". >>> + >>> +- qcom,current-boost-limit >>> + Usage: optional >>> + Value type: <u32> >>> + Definition: mA; boost current limit. >>> + For pm8941: one of: 105, 385, 525, 805, 980, 1260, 1400, >>> + 1680. Default: 805 mA >>> + For pmi8998: one of: 105, 280, 450, 620, 970, 1150, 1300, >>> + 1500. Default: 970 mA >>> + >>> +- qcom,switching-freq >>> + Usage: optional >>> + Value type: <u32> >>> + Definition: kHz; switching frequency; one of: 600, 640, 685, 738, >>> + 800, 872, 960, 1066, 1200, 1371, 1600, 1920, 2400, 3200, >>> + 4800, 9600. >>> + Default: for pm8941: 1600 kHz >>> + for pmi8998: 800 kHz >>> + >>> +- qcom,ovp >>> + Usage: optional >>> + Value type: <u32> >>> + Definition: mV; Over-voltage protection limit; >> >> The existing users of qcom,pm8941-wled depends on this being in V, so >> you can't change the unit. I suggest that you add a new "qcom,ovp-mv" >> property and make the driver fall back to looking for qcom,ovp if that >> isn't specified. >> >> PS. This is a very good example of why it is a good idea to not >> restructure and make changes at the same time - I almost missed this. >> > Actually I have checked the current kernel and none of the properties > are > being configured from the device tree node. Hence, i thought it is the > right > time modify the units to mV to support the PMI8998. > > You still want to have the qcom,ovp-mv, even though it is not being > configured from > device tree ? >>> + For pm8941: one of 27000, 29000, 32000, 35000 >>> + Default: 29000 mV >>> + For pmi8998: one of 18100, 19600, 29600, 31100 >>> + Default: 29600 mV >>> + >>> +- qcom,num-strings >>> + Usage: optional >>> + Value type: <u32> >>> + Definition: #; number of led strings attached; >>> + for pm8941: value 3. >>> + for pmi8998: value 4. >> >> This was the number of strings actually used, so you can't turn this >> into max number of strings supported. As we have different compatibles >> for the different pmics this can be hard coded in the driver, based on >> compatible, and qcom,num-strings can be kept as a special case of >> qcom,enabled-strings (enabling string 0 through N). >> > Ok. Actually I don't see the use of this property as the > qcom,enabled-strings > it self is enough to know the strings to be enabled. But for backward > compatibility > i keep it as original property. >>> + >>> +- qcom,enabled-strings >>> + Usage: optional >>> + Value tyoe: <u32> >>> + Definition: Bit mask of the WLED strings. Bit 0 to 3 indicates >>> strings >>> + 0 to 3 respectively. Each string of leds are operated >>> + individually. Specify the strings using the bit mask. Any >>> + combination of led strings can be used. >>> + for pm8941: Default value is 7 (b111). >>> + for pmi8998: Default value is 15 (b1111). >> >> I think it's better if you describe the enabled strings in an array, >> as >> it makes the dts easier to read and write for humans. >> > ok. I will do that in the next series. >> Also I'm uncertain about the validity of these defaults, as it's not >> that the defaults are 7 and 15 it's that if this property is not >> specified all strings will be enabled and the auto calibration will >> kick >> in to figure out the proper configuration. >> >> It would be better to describe this as "absence of this property will >> trigger auto calibration". >> > Ok. I will describe it in the next series. >>> + >>> +- qcom,cs-out >>> + Usage: optional >>> + Value type: <bool> >>> + Definition: enable current sink output. >>> + This property is supported only for PM8941. >>> + >>> +- qcom,cabc >>> + Usage: optional >>> + Value type: <bool> >>> + Definition: enable content adaptive backlight control. >>> + >>> +- qcom,ext-gen >>> + Usage: optional >>> + Value type: <bool> >>> + Definition: use externally generated modulator signal to dim. >>> + This property is supported only for PM8941. >>> + >>> +- qcom,external-pfet >>> + Usage: optional >>> + Value type: <bool> >>> + Definition: Specify if external PFET control for short circuit >>> + protection is used. This property is supported only >>> + for PMI8998. >>> + >>> +- qcom,auto-string-detection >>> + Usage: optional >>> + Value type: <bool> >>> + Definition: Enables auto-detection of the WLED string >>> configuration. >>> + This feature is not supported for PM8941. >> >> I don't like the idea of having the developer specifying >> qcom,enabled-strings and then qcom,auto-string-detection just in case. >> I >> think it's better to trust the developer when qcom,enabled-strings is >> specified and if not use auto detection. >> > Ok. I will modify the description in that way. >>> >>> Example: >>> >>> @@ -34,9 +133,26 @@ pm8941-wled@d800 { >>> label = "backlight"; >>> >>> qcom,cs-out; >>> - qcom,current-limit = <20>; >>> + qcom,current-limit = <20000>; >>> + qcom,current-boost-limit = <805>; >>> + qcom,switching-freq = <1600>; >>> + qcom,ovp = <29000>; >>> + qcom,num-strings = <3>; >>> + qcom,enabled-strings = <7>; >> >> Having to change the existing example shows that you made >> non-backwards >> compatible changes. >> > Ok. I will keep them as is in the next series. >>> +}; >>> + >>> +pmi8998-wled@d800 { >>> + compatible = "qcom,pmi8998-wled"; >>> + reg = <0xd800 0xd900>; >>> + label = "backlight"; >>> + >>> + interrupts = <3 0xd8 2 IRQ_TYPE_EDGE_RISING>, >>> + <3 0xd8 1 IRQ_TYPE_EDGE_RISING>; >>> + interrupt-names = "short", "ovp"; >>> + qcom,current-limit = <25000>; >>> qcom,current-boost-limit = <805>; >>> qcom,switching-freq = <1600>; >>> - qcom,ovp = <29>; >>> - qcom,num-strings = <2>; >>> + qcom,ovp = <29600>; >>> + qcom,num-strings = <4>; >>> + qcom,enabled-strings = <15>; >>> }; >>> diff --git a/drivers/video/backlight/qcom-wled.c >>> b/drivers/video/backlight/qcom-wled.c >>> index 0b6d219..be8b8d3 100644 >>> --- a/drivers/video/backlight/qcom-wled.c >>> +++ b/drivers/video/backlight/qcom-wled.c >>> @@ -15,253 +15,529 @@ >>> #include <linux/module.h> >>> #include <linux/of.h> >>> #include <linux/of_device.h> >>> +#include <linux/of_address.h> >>> #include <linux/regmap.h> >>> >>> /* From DT binding */ >>> -#define PM8941_WLED_DEFAULT_BRIGHTNESS 2048 >>> +#define WLED_DEFAULT_BRIGHTNESS 2048 >> >> These renames are good, but needs to go in a separate commit (either >> patch 1 or a new one), the current approach makes it impossible to >> review the rest of this patch. >> >> >> So, as the DT change is substantial that should be split in one patch >> that change the structure, then one that adds the new properties, one >> patch that renames pm8941 -> wled3 and finally one that adds wled4 >> support. >> > Ok. I will split the patches as you suggested. >> Regards, >> Bjorn
| |