Message-ID: <4F034AC0.5080900@metafoo.de>
Date:	Tue, 03 Jan 2012 19:36:48 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Thomas Abraham <thomas.abraham@...aro.org>
CC:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	rpurdie@...ys.net, linux-samsung-soc@...r.kernel.org,
	grant.likely@...retlab.ca, rob.herring@...xeda.com,
	kgene.kim@...sung.com, jg1.han@...sung.com,
	broonie@...nsource.wolfsonmicro.com, kyungmin.park@...sung.com,
	cbou@...l.ru, kwangwoo.lee@...il.com, augulis.darius@...il.com,
	ben-linux@...ff.org, patches@...aro.org
Subject: Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device
 tree support

On 01/03/2012 06:07 PM, Thomas Abraham wrote:
> On 3 January 2012 17:56, Lars-Peter Clausen <lars@...afoo.de> wrote:
>> On 01/03/2012 12:54 PM, Thomas Abraham wrote:
>>> Hi Lars,
>>>
>>> On 3 January 2012 14:36, Lars-Peter Clausen <lars@...afoo.de> wrote:
>>>> On 01/02/2012 06:54 AM, Thomas Abraham wrote:
>>>>> The platform-lcd driver depends on platform-specific callbacks to setup the
>>>>> lcd panel. These callbacks are supplied using driver's platform data. But
>>>>> for adding device tree support for platform-lcd driver, providing such
>>>>> callbacks is not possible (without using auxdata).
>>>>>
>>>>> Since the callbacks are usually lcd panel specific, it is possible to include
>>>>> the lcd panel specific setup and control functionality in the platform-lcd
>>>>> driver itself, thereby eliminating the need for supplying platform specific
>>>>> callbacks to the driver. The platform-lcd driver can include support for
>>>>> multiple lcd panels.
>>>>>
>>>>> This patchset removes the need for platform data for platform-lcd driver and
>>>>> adds support which can be used to implement lcd panel specific functionality
>>>>> in the driver. As an example, the support for Hydis hv070wsa lcd panel is added
>>>>> to the platform-lcd driver which is then used on the Exynos4 based Origen board.
>>>>> This currently breaks build for other users of platform-lcd driver. Those can be
>>>>> fixed if this approach is acceptable.
>>>>
>>>> The whole approach looks rather backwards to me. The exact purpose of the
>>>> platform_lcd driver is to redirect the lcd driver callbacks to board code.
>>>> So by removing this support you not only break all the existing driver but
>>>> also create a driver which does nothing. Then you add another layer of
>>>> abstraction to implement custom drivers in this driver. A better approach in
>>>> my opinion is to simply implement these drivers as first level LCD drivers.
>>>> So leave the platform-lcd driver as it is and just add a gpio (or maybe
>>>> regulator) lcd driver instead.
>>>
>>> The existing platform-lcd driver mostly depends on the platform
>>> callbacks for lcd panel power controls. Looking at the current users
>>> of platform-lcd driver, a majority of the users of platform-lcd driver
>>> use a gpio to enable/disable the display/power. The gpio is controlled
>>> by the platform callbacks which the platform-lcd driver calls.
>>>
>>> Hence, it is possible to extend the platform-lcd driver to include the
>>> functionality of managing the gpio for lcd control. The platform code
>>> is only expected to provide a gpio number to the platform-lcd driver.
>>> This allows consolidation of the all the different platform callbacks
>>> that use a gpio for simple enable/disable of the lcd display.
>>>
>>> But there are other users of platform-lcd that do lot more than just
>>> control a single gpio. For such cases, it is possible to reuse the
>>> existing infrastructure of platform-lcd driver and extend it to handle
>>> such lcd panel specific functionality.
>>>
>>> The main advantages that I see here is the consolidation of platform
>>> specific callbacks into the driver which inturn allows adding device
>>> tree support without depending on platform data which have pointers to
>>> platform specific functions. In the next version of this patchset, it
>>> will be ensured that no platform breaks due to this change.
>>
>> Consolidation of the different implementations which use a GPIO to control
>> the LCD state is a good idea. But as I wrote above in this series you added
>> more or less another layer of abstraction. Namely introducing the
>> platform-lcd driver as a intermediate layer between the actual driver and
>> the LCD framework. But the layer is so thin that all it does is to call the
>> plat_lcd_driver_data callback from the lcd_ops callback. There is really no
>> point in doing this since you can setup the lcd_ops callbacks directly. Also
>> following your approach we would end up with a bunch of unrelated LCD
>> drivers in the platform-lcd driver module. The platform-lcd driver is so
>> generic that basically any LCD driver could be implemented on-top of it, but
>> this does not mean it has to.
> 
> Yes, this will lead to including support for multiple lcd panels in
> the platform-lcd driver. But, we get to reuse most of the
> infrastruture in the platform-lcd driver such as module init/cleanup,
> suspend/resume, probe and lcd driver registration. There is a plan to
> include regulator support in the platform-lcd driver as well. If we go
> for independent drivers for all existing users of platform-lcd driver,
> then this common code in platform-lcd driver will have to be
> duplicated in multiple new drivers.

Most of the infrastructure code is driver boiler-plate code. And you'll add
about the same amount of code due to your additional layer redirection. You'll
still have a probe and remove function per driver. You'll have to define a set
of plat_lcd_driver_data per driver. You'll have all these ugly ifdefs.

An exception is the suspend/resume handling. But this does not look like it is
something which is specific to simple displays, more "complex" ones or
non-platform driver lcd drivers might want to use a similar scheme. A good idea
would be to add support for this to the LCD framework. Similar to the backlight
frameworks BL_CORE_SUSPENDRESUME.

> 
> What would be your suggestion considering this. Actually, I don't
> clearly understand the technical/maintainability reasons which do not
> favour including support for multiple types of simple lcd panels in
> the platform-lcd driver.

To me it seems very unidiomatic to put multiple drivers into the same driver.
E.g. where will we draw the line, which kind of device is simple enough so
support for this device should be embedded in the plaform-lcd driver. Once you
have support for multiple types of simple lcd panels in one driver things will
start to get messy. Image how the driver will look like if it has support for
say 5 or 10 different types of simple lcd panels.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists