On 2018-06-23 04:39, Bjorn Andersson wrote:

On Wed 20 Jun 04:00 PDT 2018, kgunda@codeaurora.org wrote:

quoted

On 2018-06-20 10:44, Bjorn Andersson wrote:

quoted

On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:

quoted

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 <redacted>
---
 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!

quoted

quoted

quoted

diff --git a/drivers/video/backlight/qcom-wled.c

b/drivers/video/backlight/qcom-wled.c

[..]

quoted

 /* WLED3 sink registers */
 #define WLED3_SINK_REG_SYNC				0x47

Drop the 3 from this if it's common between the two.

quoted

-#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

[..]

quoted

+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.

quoted

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html