Re: [PATCH v3 05/16] drm/panel-simple-edp: Split eDP panels out of panel-simple
Hi, On Sun, Sep 5, 2021 at 11:42 AM Sam Ravnborg wrote: > > > +++ b/drivers/gpu/drm/panel/panel-simple-edp.c > > @@ -0,0 +1,1298 @@ > > +/* > > + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved. > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the > > "Software"), > > + * to deal in the Software without restriction, including without > > limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sub > > license, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the > > + * next paragraph) shall be included in all copies or substantial portions > > + * of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > > OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > > + * DEALINGS IN THE SOFTWARE. > > + */ > Would be nice if you could use the SPDX thingy for the license. I'm going to leave this alone. I definitely started this driver by copy-pasting the panel-simple.c file and it still shares a lot of lines of code with that driver. It feels like that qualifies for the "substantial portions of the Software" portion which tells me to retain the license. I also kept Thierry as the author since, again, it's really a splitting of the existing driver and not the creation of a new driver. In fact, if I were to assign a new author/license to panel-edp, one could also make the argument that I should assign a new author/license to panel-simple. panel-simple got ~50% of the old panels and panel-edp got the other ~50% of the old panels plus a search-and-replace of "simple" for "edp" and some code deletion. I don't think search-and-replace name change nor code deletion is justification for claiming authorship. ;-) If Thierry wants to chime in and say that I should put down a different license for either of the two files, though, I'd be glad to change it. -Doug
Re: [PATCH v3 05/16] drm/panel-simple-edp: Split eDP panels out of panel-simple
Hi Douglas, On Wed, Sep 01, 2021 at 01:19:23PM -0700, Douglas Anderson wrote: > The panel-simple driver handles way too much. Let's start trying to > get a handle on it by splitting out the eDP panels. This patch does > this: > > 1. Start by copying simple-panel verbatim over to a new driver, >simple-panel-edp. > 2. Rename "panel_simple" to "panel_edp" in the new driver. > 3. Keep only panels marked with `DRM_MODE_CONNECTOR_eDP` in the new >driver. Remove those panels from the old driver. > 4. Remove all recent "DP AUX bus" stuff from the old driver. The DP >AUX bus is only possible on DP panels. > 5. Remove all DSI / MIPI related functions from the new driver. > 6. Remove bus_format / bus_flags from eDP driver. These things don't >seem to make any sense for eDP panels so let's stop filling in made >up stuff. > > In the end we end up with a bunch of duplicated code for now. Future > patches will try to address _some_ of this duplicated code though some > of it will be unavoidable. > > NOTE: This may not actually move all eDP panels over to the new driver > since not all panels were properly marked with > `DRM_MODE_CONNECTOR_eDP`. A future patch will attempt to move wayward > panels I could identify but even so there may be some missed. > > Suggested-by: Sam Ravnborg > Signed-off-by: Douglas Anderson > --- > I believe this is what Sam was looking for when he requested that the > eDP panels split out [1]. Please yell if not. I will yell a big thanks! This was exactly what I hoped to see. And very nice to have panel-simple complexity reduced. The code duplication is worth it. To avoid confusion I would prefer the file named panel-edp.c, as this is not "simple" panels and it then matches the compatible. A few nits in the following. Sam > > [1] https://lore.kernel.org/dri-devel/yrtsfntn%2ft8fl...@ravnborg.org/ > > Changes in v3: > - Split eDP panels patch new for v3. > > drivers/gpu/drm/panel/Kconfig| 16 +- > drivers/gpu/drm/panel/Makefile |1 + > drivers/gpu/drm/panel/panel-simple-edp.c | 1298 ++ > drivers/gpu/drm/panel/panel-simple.c | 575 +- > 4 files changed, 1323 insertions(+), 567 deletions(-) > create mode 100644 drivers/gpu/drm/panel/panel-simple-edp.c > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index 0b3784941312..4b7ff4ebdc34 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -77,14 +77,26 @@ config DRM_PANEL_LVDS > backlight handling if the panel is attached to a backlight controller. > > config DRM_PANEL_SIMPLE > - tristate "support for simple panels" > + tristate "support for simple panels (other than eDP ones)" > + depends on OF > + depends on BACKLIGHT_CLASS_DEVICE > + depends on PM > + select VIDEOMODE_HELPERS > + help > + DRM panel driver for dumb non-eDP panels that need at most a regulator > + and a GPIO to be powered up. Optionally a backlight can be attached so > + that it can be automatically turned off when the panel goes into a > + low power state. > + > +config DRM_PANEL_SIMPLE_EDP So this should also be named DRM_PANEL_EDP to follow the name of the driver. > + tristate "support for simple Embedded DisplayPort panels" > depends on OF > depends on BACKLIGHT_CLASS_DEVICE > depends on PM > select VIDEOMODE_HELPERS > select DRM_DP_AUX_BUS > help > - DRM panel driver for dumb panels that need at most a regulator and > + DRM panel driver for dumb eDP panels that need at most a regulator and > a GPIO to be powered up. Optionally a backlight can be attached so > that it can be automatically turned off when the panel goes into a > low power state. > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > index 60c0149fc54a..640234d4d693 100644 > --- a/drivers/gpu/drm/panel/Makefile > +++ b/drivers/gpu/drm/panel/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_NL6) += > panel-boe-tv101wum-nl6.o > obj-$(CONFIG_DRM_PANEL_DSI_CM) += panel-dsi-cm.o > obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o > obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o > +obj-$(CONFIG_DRM_PANEL_SIMPLE_EDP) += panel-simple-edp.o > obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o > obj-$(CONFIG_DRM_PANEL_FEIXIN_K101_IM2BA02) += panel-feixin-k101-im2ba02.o > obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += > panel-feiyang-fy07024di26a30d.o > diff --git a/drivers/gpu/drm/panel/panel-simple-edp.c > b/drivers/gpu/drm/panel/panel-simple-edp.c > new file mode 100644 > index ..5b47ee4bc338 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-simple-edp.c > @@ -0,0 +1,1298 @@ > +/* > + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy
[PATCH v3 05/16] drm/panel-simple-edp: Split eDP panels out of panel-simple
The panel-simple driver handles way too much. Let's start trying to get a handle on it by splitting out the eDP panels. This patch does this: 1. Start by copying simple-panel verbatim over to a new driver, simple-panel-edp. 2. Rename "panel_simple" to "panel_edp" in the new driver. 3. Keep only panels marked with `DRM_MODE_CONNECTOR_eDP` in the new driver. Remove those panels from the old driver. 4. Remove all recent "DP AUX bus" stuff from the old driver. The DP AUX bus is only possible on DP panels. 5. Remove all DSI / MIPI related functions from the new driver. 6. Remove bus_format / bus_flags from eDP driver. These things don't seem to make any sense for eDP panels so let's stop filling in made up stuff. In the end we end up with a bunch of duplicated code for now. Future patches will try to address _some_ of this duplicated code though some of it will be unavoidable. NOTE: This may not actually move all eDP panels over to the new driver since not all panels were properly marked with `DRM_MODE_CONNECTOR_eDP`. A future patch will attempt to move wayward panels I could identify but even so there may be some missed. Suggested-by: Sam Ravnborg Signed-off-by: Douglas Anderson --- I believe this is what Sam was looking for when he requested that the eDP panels split out [1]. Please yell if not. [1] https://lore.kernel.org/dri-devel/yrtsfntn%2ft8fl...@ravnborg.org/ Changes in v3: - Split eDP panels patch new for v3. drivers/gpu/drm/panel/Kconfig| 16 +- drivers/gpu/drm/panel/Makefile |1 + drivers/gpu/drm/panel/panel-simple-edp.c | 1298 ++ drivers/gpu/drm/panel/panel-simple.c | 575 +- 4 files changed, 1323 insertions(+), 567 deletions(-) create mode 100644 drivers/gpu/drm/panel/panel-simple-edp.c diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 0b3784941312..4b7ff4ebdc34 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -77,14 +77,26 @@ config DRM_PANEL_LVDS backlight handling if the panel is attached to a backlight controller. config DRM_PANEL_SIMPLE - tristate "support for simple panels" + tristate "support for simple panels (other than eDP ones)" + depends on OF + depends on BACKLIGHT_CLASS_DEVICE + depends on PM + select VIDEOMODE_HELPERS + help + DRM panel driver for dumb non-eDP panels that need at most a regulator + and a GPIO to be powered up. Optionally a backlight can be attached so + that it can be automatically turned off when the panel goes into a + low power state. + +config DRM_PANEL_SIMPLE_EDP + tristate "support for simple Embedded DisplayPort panels" depends on OF depends on BACKLIGHT_CLASS_DEVICE depends on PM select VIDEOMODE_HELPERS select DRM_DP_AUX_BUS help - DRM panel driver for dumb panels that need at most a regulator and + DRM panel driver for dumb eDP panels that need at most a regulator and a GPIO to be powered up. Optionally a backlight can be attached so that it can be automatically turned off when the panel goes into a low power state. diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 60c0149fc54a..640234d4d693 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_NL6) += panel-boe-tv101wum-nl6.o obj-$(CONFIG_DRM_PANEL_DSI_CM) += panel-dsi-cm.o obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o +obj-$(CONFIG_DRM_PANEL_SIMPLE_EDP) += panel-simple-edp.o obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o obj-$(CONFIG_DRM_PANEL_FEIXIN_K101_IM2BA02) += panel-feixin-k101-im2ba02.o obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o diff --git a/drivers/gpu/drm/panel/panel-simple-edp.c b/drivers/gpu/drm/panel/panel-simple-edp.c new file mode 100644 index ..5b47ee4bc338 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-simple-edp.c @@ -0,0 +1,1298 @@ +/* + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sub license, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the + * next paragraph) shall be included in all copies or substantial portions + * of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + *
