Re: [PATCH V3 2/4] backlight: qcom-wled: Add callback functions

[kgunda]

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@xxxxxxxxxxxxxx>
>
> 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.