On 2018-06-23 04:39, Bjorn Andersson wrote:
On Wed 20 Jun 04:00 PDT 2018, kgunda@xxxxxxxxxxxxxx wrote:
On 2018-06-20 10:44, Bjorn Andersson wrote:
> On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
>
> > WLED4 peripheral is present on some PMICs like pmi8998 and
> > pm660l. It has a different register map and configurations
> > are also different. Add support for it.
> >
> > Signed-off-by: Kiran Gunda <kgunda@xxxxxxxxxxxxxx>
> > ---
> > drivers/video/backlight/qcom-wled.c | 635
> > ++++++++++++++++++++++++++++--------
> > 1 file changed, 503 insertions(+), 132 deletions(-)
>
> Split this further into a patch that does structural preparation of
> WLED3 support and then an addition of WLED4, the mixture makes parts of
> this patch almost impossible to review. Please find some comments below.
>
Sure. I will split it in the next series.
Thanks!
> >
> > diff --git a/drivers/video/backlight/qcom-wled.c
> > b/drivers/video/backlight/qcom-wled.c
> [..]
> >
> > /* WLED3 sink registers */
> > #define WLED3_SINK_REG_SYNC 0x47
>
> Drop the 3 from this if it's common between the two.
>
> > -#define WLED3_SINK_REG_SYNC_MASK 0x07
> > +#define WLED3_SINK_REG_SYNC_MASK GENMASK(2, 0)
> > +#define WLED4_SINK_REG_SYNC_MASK GENMASK(3, 0)
> > #define WLED3_SINK_REG_SYNC_LED1 BIT(0)
> > #define WLED3_SINK_REG_SYNC_LED2 BIT(1)
> > #define WLED3_SINK_REG_SYNC_LED3 BIT(2)
> > +#define WLED4_SINK_REG_SYNC_LED4 BIT(3)
> > #define WLED3_SINK_REG_SYNC_ALL 0x07
> > +#define WLED4_SINK_REG_SYNC_ALL 0x0f
> > #define WLED3_SINK_REG_SYNC_CLEAR 0x00
> >
> [..]
> > +static int wled4_set_brightness(struct wled *wled, u16 brightness)
>
> Afaict this is identical to wled3_set_brightness() with the exception
> that there's a minimum brightness and the base address for the
> brightness registers are different.
>
> I would suggest that you add a min_brightness to wled and that you
> figure out a way to carry the brightness base register address; and by
> that you squash these two functions.
>
There are four different parameters. 1) minimum brightness 2) WLED
base
addresses
3) Brightness base addresses 4) the brightness sink registers address
offsets also
different for wled 3 and wled4. (in wled3 0x40, 0x42, 0x44, where as
in
wled4 0x57, 0x67, 0x77, 0x87).
Sorry, I must have gotten lost in the defines, I see the difference
between the two register layouts now. If you retain the old mechanism
of
doing the math openly in the function this would have been obvious.
Irrelevant to this patch, but wled5 has some more extra registers to
set the brightness. Keeping this in mind, it is better to have
separate functions? Otherwise we will have to use the version checks
in the wled_set_brightness function, if we have the common function.
Okay, so it sounds reasonable to split this out to some degree.
Regards,
Bjorn
--
Thanks for that !
To unsubscribe from this list: send the line "unsubscribe
linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
