| Message ID | 1573157463-14070-5-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Add LCD panel support to iwg20d | expand |
Hi Fabrizio, (CC'ing Sam) Thank you for the patch. On Thu, Nov 07, 2019 at 08:11:00PM +0000, Fabrizio Castro wrote: > The existing DRM_MODE_CONNECTOR_ definitions don't seem to > describe the connector for RGB/Parallel embedded displays, > hence add DRM_MODE_CONNECTOR_PARALLEL. Please, no. We already have too many connector types for panels, when userspace should really not care. DRM_MODE_CONNECTOR_LVDS, DRM_MODE_CONNECTOR_eDP, DRM_MODE_CONNECTOR_DSI, DRM_MODE_CONNECTOR_DPI and probably DRM_MODE_CONNECTOR_SPI should have been DRM_MODE_CONNECTOR_PANEL. This has been discussed in [1]. Let's instead define a DRM_MODE_CONNECTOR_PANEL, possibly as an alias to one of the existing types, and deprecate the other types. [1] https://www.spinics.net/lists/dri-devel/msg224638.html > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > v2->v3: > * New patch > --- > drivers/gpu/drm/drm_connector.c | 1 + > include/uapi/drm/drm_mode.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 2166000..b233029 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -93,6 +93,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = { > { DRM_MODE_CONNECTOR_DPI, "DPI" }, > { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" }, > { DRM_MODE_CONNECTOR_SPI, "SPI" }, > + { DRM_MODE_CONNECTOR_PARALLEL, "Parallel" }, > }; > > void drm_connector_ida_init(void) > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 735c8cf..5852f47 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -362,6 +362,7 @@ enum drm_mode_subconnector { > #define DRM_MODE_CONNECTOR_DPI 17 > #define DRM_MODE_CONNECTOR_WRITEBACK 18 > #define DRM_MODE_CONNECTOR_SPI 19 > +#define DRM_MODE_CONNECTOR_PARALLEL 20 > > struct drm_mode_get_connector { >
Hi Laurent, Thank you for your feedback! > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart > Sent: 07 November 2019 20:47 > Subject: Re: [PATCH v3 4/7] drm: Define DRM_MODE_CONNECTOR_PARALLEL > > Hi Fabrizio, > > (CC'ing Sam) > > Thank you for the patch. > > On Thu, Nov 07, 2019 at 08:11:00PM +0000, Fabrizio Castro wrote: > > The existing DRM_MODE_CONNECTOR_ definitions don't seem to > > describe the connector for RGB/Parallel embedded displays, > > hence add DRM_MODE_CONNECTOR_PARALLEL. > > Please, no. We already have too many connector types for panels, when > userspace should really not care. DRM_MODE_CONNECTOR_LVDS, > DRM_MODE_CONNECTOR_eDP, DRM_MODE_CONNECTOR_DSI, DRM_MODE_CONNECTOR_DPI > and probably DRM_MODE_CONNECTOR_SPI should have been > DRM_MODE_CONNECTOR_PANEL. > > This has been discussed in [1]. Let's instead define a > DRM_MODE_CONNECTOR_PANEL, possibly as an alias to one of the existing > types, and deprecate the other types. > > [1] https://www.spinics.net/lists/dri-devel/msg224638.html Thank you for the pointer and the for the details. That clarifies things a lot. In my case, as you mentioned in the patch to simple panel, I can use an existing definition, therefore I think it's best if DRM_MODE_CONNECTOR_PANEL gets added when there is a valid use case. Thanks, Fab > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > v2->v3: > > * New patch > > --- > > drivers/gpu/drm/drm_connector.c | 1 + > > include/uapi/drm/drm_mode.h | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index 2166000..b233029 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -93,6 +93,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = { > > { DRM_MODE_CONNECTOR_DPI, "DPI" }, > > { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" }, > > { DRM_MODE_CONNECTOR_SPI, "SPI" }, > > + { DRM_MODE_CONNECTOR_PARALLEL, "Parallel" }, > > }; > > > > void drm_connector_ida_init(void) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > index 735c8cf..5852f47 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -362,6 +362,7 @@ enum drm_mode_subconnector { > > #define DRM_MODE_CONNECTOR_DPI 17 > > #define DRM_MODE_CONNECTOR_WRITEBACK 18 > > #define DRM_MODE_CONNECTOR_SPI 19 > > +#define DRM_MODE_CONNECTOR_PARALLEL 20 > > > > struct drm_mode_get_connector { > > > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 2166000..b233029 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -93,6 +93,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = { { DRM_MODE_CONNECTOR_DPI, "DPI" }, { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" }, { DRM_MODE_CONNECTOR_SPI, "SPI" }, + { DRM_MODE_CONNECTOR_PARALLEL, "Parallel" }, }; void drm_connector_ida_init(void) diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 735c8cf..5852f47 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -362,6 +362,7 @@ enum drm_mode_subconnector { #define DRM_MODE_CONNECTOR_DPI 17 #define DRM_MODE_CONNECTOR_WRITEBACK 18 #define DRM_MODE_CONNECTOR_SPI 19 +#define DRM_MODE_CONNECTOR_PARALLEL 20 struct drm_mode_get_connector {
The existing DRM_MODE_CONNECTOR_ definitions don't seem to describe the connector for RGB/Parallel embedded displays, hence add DRM_MODE_CONNECTOR_PARALLEL. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- v2->v3: * New patch --- drivers/gpu/drm/drm_connector.c | 1 + include/uapi/drm/drm_mode.h | 1 + 2 files changed, 2 insertions(+)