Hi Lars, On 4 January 2012 00:06, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: > On 01/03/2012 06:07 PM, Thomas Abraham wrote: >> 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. > > 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. Ok, I understand your point now. A new lcd driver will be written mostly derived from platform-lcd driver (minus the platform specific callbacks in platform data) that can manage power control for lcd panels which can be controlled by a single gpio. Also, regulator and device tree support will be added to this driver. Thanks for your comments. Regards, Thomas. > > - Lars > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |