Re: [PATCH V5 5/8] backlight: qcom-wled: Restructure the driver for WLED3

[kgunda]

On 2018-08-27 15:39, Pavel Machek wrote:
> On Fri 2018-08-24 15:57:44, Kiran Gunda wrote:
> > Restructure the driver to add the support for new WLED
> > peripherals.
> >
> > Signed-off-by: Kiran Gunda <kgunda@xxxxxxxxxxxxxx>
> > Acked-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> > ---
> > Changes from V3:
> >     - This is the new patch after splitting the
> >       "backlight: qcom-wled: Add support for WLED4 peripheral" patch
> >       to seperate the WLED3 specific restructure.
> >
> > Changes from V4:
> >     - Initialize wled->cfg.enabled_strings to 0,1,2,3.
> >     - Replaced the WLED3 macro with 3.
> >
> >  drivers/video/backlight/qcom-wled.c | 395 
> > ++++++++++++++++++++++--------------
> >  1 file changed, 245 insertions(+), 150 deletions(-)
> >
> > diff --git a/drivers/video/backlight/qcom-wled.c 
> > b/drivers/video/backlight/qcom-wled.c
> > index 3cd6e75..a746bec 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -15,59 +15,71 @@
> >  #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 WLED_MAX_STRINGS				4
> > +
> >  #define WLED_DEFAULT_BRIGHTNESS				2048
> >
> > -#define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
> > -#define WLED3_CTRL_REG_VAL_BASE				0x40
> > +#define WLED_SINK_REG_BRIGHT_MAX			0xFFF
>
> Stop this, no. In previous patch you renamed from ABC123_ to WLED3_,
> now you are renaming back to WLED?
>
> Stop messing with the names. I'd actually prefer you to stick with
> original driver name, and just add note that this now supports more
> hardware.
>
> But yes, _one_ rename is okay. I guess. But renaming it twice in one
> series is not acceptable.
ok. I will stop renaming from WLED3 to WLED in this patch. I did it 
because these registers
are common for both WLED3 and WLED4 and Bjorn also suggested the same. 
Anyways I will stop
this renaming in this patch from WLED3 to WLED.

> > @@ -365,6 +433,15 @@ static int wled_configure(struct wled *wled, 
> > struct device *dev)
> >
> >  	cfg->num_strings = cfg->num_strings + 1;
> >
> > +	string_len = of_property_count_elems_of_size(dev->of_node,
> > +						     "qcom,enabled-strings",
> > +						     sizeof(u32));
> > +	if (string_len > 0)
> > +		rc = of_property_read_u32_array(dev->of_node,
> > +						"qcom,enabled-strings",
> > +						wled->cfg.enabled_strings,
> > +						sizeof(u32));
> > +
> >  	return 0;
> >  }
>
> rc is assigned but never used.
Will address it next series.
> 									Pavel