| Message ID | 1583760362-26978-1-git-send-email-kgunda@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add support for WLED5 | expand |
On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote: > Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay > callback functions to prepare the driver for adding WLED5 support. > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> Overall this code would a lot easier to review if > --- > drivers/video/backlight/qcom-wled.c | 196 +++++++++++++++++++++++------------- > 1 file changed, 126 insertions(+), 70 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c > index 3d276b3..b73f273 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -128,6 +128,7 @@ struct wled_config { > bool cs_out_en; > bool ext_gen; > bool cabc; > + bool en_cabc; Does this ever get set to true? > bool external_pfet; > bool auto_detection_enabled; > }; > @@ -147,14 +148,20 @@ struct wled { > u32 max_brightness; > u32 short_count; > u32 auto_detect_count; > + u32 version; > bool disabled_by_short; > bool has_short_detect; > + bool cabc_disabled; > int short_irq; > int ovp_irq; > > struct wled_config cfg; > struct delayed_work ovp_work; > int (*wled_set_brightness)(struct wled *wled, u16 brightness); > + int (*cabc_config)(struct wled *wled, bool enable); > + int (*wled_sync_toggle)(struct wled *wled); > + int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set); > + int (*wled_ovp_delay)(struct wled *wled); Let's get some doc comments explaining what these callbacks do (and which versions they apply to). cabc_config() in particular appears to have a very odd interface for wled4. It looks like it relies on being initially called with enable set a particular way and prevents itself from acting again. Therefore if the comment you end up writing doesn't sound "right" then please also fix the API! Finally, why is everything except cabc_config() prefixed with wled? Daniel.
On 2020-03-10 20:57, Daniel Thompson wrote: > On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote: >> Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay >> callback functions to prepare the driver for adding WLED5 support. >> >> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> > > Overall this code would a lot easier to review if >> --- >> drivers/video/backlight/qcom-wled.c | 196 >> +++++++++++++++++++++++------------- >> 1 file changed, 126 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/video/backlight/qcom-wled.c >> b/drivers/video/backlight/qcom-wled.c >> index 3d276b3..b73f273 100644 >> --- a/drivers/video/backlight/qcom-wled.c >> +++ b/drivers/video/backlight/qcom-wled.c >> @@ -128,6 +128,7 @@ struct wled_config { >> bool cs_out_en; >> bool ext_gen; >> bool cabc; >> + bool en_cabc; > > Does this ever get set to true? > Yes. If user wants use the cabc pin to control the brightness and use the "qcom,cabc" DT property in the device tree. >> bool external_pfet; >> bool auto_detection_enabled; >> }; >> @@ -147,14 +148,20 @@ struct wled { >> u32 max_brightness; >> u32 short_count; >> u32 auto_detect_count; >> + u32 version; >> bool disabled_by_short; >> bool has_short_detect; >> + bool cabc_disabled; >> int short_irq; >> int ovp_irq; >> >> struct wled_config cfg; >> struct delayed_work ovp_work; >> int (*wled_set_brightness)(struct wled *wled, u16 brightness); >> + int (*cabc_config)(struct wled *wled, bool enable); >> + int (*wled_sync_toggle)(struct wled *wled); >> + int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set); >> + int (*wled_ovp_delay)(struct wled *wled); > > Let's get some doc comments explaining what these callbacks do (and > which versions they apply to). > Sure. I will update it in the commit text in next post. > cabc_config() in particular appears to have a very odd interface for > wled4. It looks like it relies on being initially called with enable > set a particular way and prevents itself from acting again. Therefore > if > the comment you end up writing doesn't sound "right" then please also > fix the API! > Actually this variable is useful for WLED5, where the default HW state is CABC1 enabled mode. So, if the user doesn't want to use the CABC we are configuring the HW state to "0" based on the DT property and then setting a flag to not enable it again. This is not needed for WLED4. I will remove it for WLED4 in next post. > Finally, why is everything except cabc_config() prefixed with wled? > It is typo. I will correct it in the next post. > > Daniel.
On Wed, Mar 11, 2020 at 12:11:00PM +0530, kgunda@codeaurora.org wrote: > On 2020-03-10 20:57, Daniel Thompson wrote: > > On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote: > > > Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay > > > callback functions to prepare the driver for adding WLED5 support. > > > > > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> > > > > Overall this code would a lot easier to review if > > > --- > > > drivers/video/backlight/qcom-wled.c | 196 > > > +++++++++++++++++++++++------------- > > > 1 file changed, 126 insertions(+), 70 deletions(-) > > > > > > diff --git a/drivers/video/backlight/qcom-wled.c > > > b/drivers/video/backlight/qcom-wled.c > > > index 3d276b3..b73f273 100644 > > > --- a/drivers/video/backlight/qcom-wled.c > > > +++ b/drivers/video/backlight/qcom-wled.c > > > @@ -128,6 +128,7 @@ struct wled_config { > > > bool cs_out_en; > > > bool ext_gen; > > > bool cabc; > > > + bool en_cabc; > > > > Does this ever get set to true? > > > Yes. If user wants use the cabc pin to control the brightness and > use the "qcom,cabc" DT property in the device tree. That sounds like what you intended the code to do! Is the code that does this present in the patch? I could not find it. Daniel.
On 2020-03-11 16:00, Daniel Thompson wrote: > On Wed, Mar 11, 2020 at 12:11:00PM +0530, kgunda@codeaurora.org wrote: >> On 2020-03-10 20:57, Daniel Thompson wrote: >> > On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote: >> > > Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay >> > > callback functions to prepare the driver for adding WLED5 support. >> > > >> > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> >> > >> > Overall this code would a lot easier to review if >> > > --- >> > > drivers/video/backlight/qcom-wled.c | 196 >> > > +++++++++++++++++++++++------------- >> > > 1 file changed, 126 insertions(+), 70 deletions(-) >> > > >> > > diff --git a/drivers/video/backlight/qcom-wled.c >> > > b/drivers/video/backlight/qcom-wled.c >> > > index 3d276b3..b73f273 100644 >> > > --- a/drivers/video/backlight/qcom-wled.c >> > > +++ b/drivers/video/backlight/qcom-wled.c >> > > @@ -128,6 +128,7 @@ struct wled_config { >> > > bool cs_out_en; >> > > bool ext_gen; >> > > bool cabc; >> > > + bool en_cabc; >> > >> > Does this ever get set to true? >> > >> Yes. If user wants use the cabc pin to control the brightness and >> use the "qcom,cabc" DT property in the device tree. > > That sounds like what you intended the code to do! > > Is the code that does this present in the patch? I could not find > it. > okay... It's my bad. We already have the "cabc" for this. I will remove the en_cabc in next series. > > Daniel.
Currently, WLED driver supports only WLED4 peripherals that is present on pmi8998 and pm660L. This patch series converts the existing WLED4 bindings from .txt to .yaml format and adds the support for WLED5 peripheral that is present on PM8150L. PM8150L WLED supports the following. - Two modulators and each sink can use any of the modulator - Multiple CABC selection options - Multiple brightness width selection (12 bits to 15 bits) Changes from V1: - Rebased on top of the below commit. backlight: qcom-wled: Fix unsigned comparison to zero Changes from V2: - Addressed Bjorn's comments by splitting the WLED4 changes in a seperate patch. - Added WLED5 auto calibration support Kiran Gunda (4): backlight: qcom-wled: convert the wled bindings to .yaml format backlight: qcom-wled: Add callbacks functions backlight: qcom-wled: Add support for WLED5 peripheral in PM8150L backlight: qcom-wled: Update auto calibration support for WLED5 .../bindings/leds/backlight/qcom-wled.txt | 154 ----- .../bindings/leds/backlight/qcom-wled.yaml | 223 ++++++++ drivers/video/backlight/qcom-wled.c | 622 ++++++++++++++++++--- 3 files changed, 772 insertions(+), 227 deletions(-) delete mode 100644 Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt create mode 100644 Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml