Re: [RFC][PATCH 0/4] lcd: platform-lcd: Add lcd panel and device tree support

[Thomas Abraham]

On 3 January 2012 17:56, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 01/03/2012 12:54 PM, Thomas Abraham wrote:
>> Hi Lars,
>>
>> On 3 January 2012 14:36, Lars-Peter Clausen <lars@xxxxxxxxxx> 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.

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.

>
> As said before, writing a plain LCD driver which implements the GPIO
> handling and leaving the platform-lcd driver as it is, is in my opinion a
> better approach.

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