Hi Tomi, On Thu, Aug 01, 2013 at 03:39:27PM +0300, Tomi Valkeinen wrote: > Hi, > > On 01/08/13 11:40, Maxime Ripard wrote: > > From: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx> > > > > Add support for the Himax HX8369 controller as it is quite similar to the > > hx8357. > > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx> > > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> > > Acked-by: Jingoo Han <jg1.han@xxxxxxxxxxx> > > --- > > drivers/video/backlight/hx8357.c | 219 ++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 204 insertions(+), 15 deletions(-) > > I don't have comments about this patch as such, but hx8357.c in general: > > - The commands used look like MIPI DCS commands. We have defined those > in include/video/mipi_display.h. Aaah, yes indeed. Thanks for pointing that out. > - The command arrays could be const True. > - I don't like command arrays. Rather than having array for > "HX8357_SET_COLUMN_ADDRESS, 0x00, 0x00, 0x01, 0x3f,", I'd much more like > a function hx8357_set_column_address(int x1, int x2) or such. Hmm, Ok. > - Looking at the driver makes me think it resembles very much the panel > driver(s) we have for OMAP. If only CDF was ready... ;) > > Those said, I don't see a problem with applying this. We could clean up > later those things mentioned above, but maybe it's better to do that > when moving to CDF. Yes, CDF would be really great for this :) Alexandre and I will try to find some time to do the cleanups you requested. > For this and the two other patches: > > Acked-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > > I can apply these to my 3.12-fbdev branch some day soon, if > Jean-Christophe hasn't come back yet (I'd normally rather deal only with > trivial patches, and leave the rest to Jean-Christophe). Great, thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
