Re: [PATCH V1 2/2] backlight: qcom-wled: Add support for WLED5 peripheral in PM8150L

[kgunda]

On 2020-03-09 03:17, Bjorn Andersson wrote:
> On Mon 02 Mar 04:55 PST 2020, Kiran Gunda wrote:
> > diff --git a/drivers/video/backlight/qcom-wled.c 
> > b/drivers/video/backlight/qcom-wled.c
> [..]
> > @@ -147,14 +187,39 @@ 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);
>
> Please split this patch in one that adds these and breaks out the wled3
> support, and then a second patch that adds wled5.
Sure. Will make this change in the next post.
> > +};
> > +
> [..]
> > +static int wled5_set_brightness(struct wled *wled, u16 brightness)
> > +{
> > +	int rc, offset;
> > +	u16 low_limit = wled->max_brightness * 1 / 1000;
> > +	u8 v[2], brightness_msb_mask;
> > +
> > +	/* WLED5's lower limit is 0.1% */
> > +	if (brightness > 0 && brightness < low_limit)
> > +		brightness = low_limit;
> > +
> > +	brightness_msb_mask = 0xf;
> > +	if (wled->max_brightness == WLED5_SINK_REG_BRIGHT_MAX_15B)
> > +		brightness_msb_mask = 0x7f;
>
> Why not just brightness &= wled->max_brightness? But given that it 
> seems
> like the framework ensures that brightness <= max_brightness, why not
> skip this altogether?
Okay. I will modify the code to remove the min/max, low_limit checks in 
next post.
> > +
> > +	v[0] = brightness & 0xff;
> > +	v[1] = (brightness >> 8) & brightness_msb_mask;
> > +
> > +	offset = wled5_brightness_reg[wled->cfg.mod_sel];
> > +	rc = regmap_bulk_write(wled->regmap, wled->sink_addr + offset,
> > +			v, 2);
> > +	return rc;
> > +}
> > +
> >  static int wled4_set_brightness(struct wled *wled, u16 brightness)
> >  {
> >  	int rc, i;
> > @@ -237,7 +325,28 @@ static int wled_module_enable(struct wled *wled, 
> > int val)
> >  	return 0;
> >  }
> >
> > -static int wled_sync_toggle(struct wled *wled)
> > +static int wled5_sync_toggle(struct wled *wled)
> > +{
> > +	int rc;
> > +	u8 val;
> > +
> > +	val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
> > +					     WLED5_SINK_REG_SYNC_MOD_B_BIT;
> > +	rc = regmap_update_bits(wled->regmap,
> > +				wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
> > +				WLED5_SINK_REG_SYNC_MASK, val);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	val = 0;
>
> Just plug 0 in the function call.
Sure. Will do it in next post.
> > +	rc = regmap_update_bits(wled->regmap,
> > +				wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
> > +				WLED5_SINK_REG_SYNC_MASK, val);
> > +
> > +	return rc;
>
> And return regmap_update_bits(...);
Sure. Will do it in next post.
> > +}
> > +
>
> Regards,
> Bjorn