[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
On 03/06/15 11:58, Russell King - ARM Linux wrote: > On Fri, Mar 06, 2015 at 10:33:27AM +0200, Jyri Sarha wrote: >> Would it be Ok to add a check that master->ops->add_components is defined, >> before calling it in find_componets() (drivers/base/component.c:120) and >> return 0 if it is not? > > No: > > http://ftp.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=8c4e8764a7e3 > > also: > > http://ftp.arm.linux.org.uk/cgit/linux-arm.git/log/?h=11eda5aaf41e > > is what's planned to be merged when I can get a round tuit, and people > stop using the old methods. > Ok, but could it still be allowed to add a master without any components (match == NULL)? Or do I have to handle the configurations without any components separately? Best regards, Jyri
[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
On Fri, Mar 06, 2015 at 12:21:42PM +0200, Jyri Sarha wrote: > On 03/06/15 11:58, Russell King - ARM Linux wrote: > >On Fri, Mar 06, 2015 at 10:33:27AM +0200, Jyri Sarha wrote: > >>Would it be Ok to add a check that master->ops->add_components is defined, > >>before calling it in find_componets() (drivers/base/component.c:120) and > >>return 0 if it is not? > > > >No: > > > >http://ftp.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=8c4e8764a7e3 > > > >also: > > > >http://ftp.arm.linux.org.uk/cgit/linux-arm.git/log/?h=11eda5aaf41e > > > >is what's planned to be merged when I can get a round tuit, and people > >stop using the old methods. > > > > Ok, but could it still be allowed to add a master without any components > (match == NULL)? > > Or do I have to handle the configurations without any components separately? That's not a decision I want to make in my current state. Give me a couple of week or two and re-ping me. http://archive.arm.linux.org.uk/lurker/message/20150306.102749.fcabd2bf.en.html (and the reason becomes self-evident when you realise that message did not go to the right list on Tuesday evening when it was meant to.) -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
On 03/02/15 18:01, Russell King - ARM Linux wrote:
> On Thu, Feb 26, 2015 at 04:55:32PM +0200, Jyri Sarha wrote:
>> +ret = component_bind_all(dev->dev, dev);
>> +if (ret < 0) {
>> +dev_err(dev->dev, "Binding subcomponents failed: %d\n", ret);
>
> Do you need to print this? The component helper is already fairly
> verbose about what succeeds and fails.
>
Will remove.
>> +static const struct component_master_ops tilcdc_comp_ops = {
>> +.add_components = tilcdc_add_external_components,
>
> I'd much rather you used the new matching support rather than using the
> old .add_components. The new matching support is more efficient as you
> only have to scan DT once, rather than each time we try to probe. That
> will mean...
>
That is otherwise fine, but with the match code it not possible to
implement a master that may not have any components.
Would it be Ok to add a check that master->ops->add_components is
defined, before calling it in find_componets()
(drivers/base/component.c:120) and return 0 if it is not?
Best regards,
Jyri
>> @@ -613,12 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device
>> *pdev)
>> return -ENXIO;
>> }
>
> You need to build a struct component_match array here using
> component_match_add()...
>
>>
>> -return drm_platform_init(_driver, pdev);
>> +return component_master_add(>dev, _comp_ops);
>
> and then finally register the ops with component_master_add_with_match().
>
> Thanks.
>
[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
On Fri, Mar 06, 2015 at 10:33:27AM +0200, Jyri Sarha wrote: > Would it be Ok to add a check that master->ops->add_components is defined, > before calling it in find_componets() (drivers/base/component.c:120) and > return 0 if it is not? No: http://ftp.arm.linux.org.uk/cgit/linux-arm.git/commit/?h=8c4e8764a7e3 also: http://ftp.arm.linux.org.uk/cgit/linux-arm.git/log/?h=11eda5aaf41e is what's planned to be merged when I can get a round tuit, and people stop using the old methods. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
On Thu, Feb 26, 2015 at 04:55:32PM +0200, Jyri Sarha wrote:
> + ret = component_bind_all(dev->dev, dev);
> + if (ret < 0) {
> + dev_err(dev->dev, "Binding subcomponents failed: %d\n", ret);
Do you need to print this? The component helper is already fairly
verbose about what succeeds and fails.
> +static const struct component_master_ops tilcdc_comp_ops = {
> + .add_components = tilcdc_add_external_components,
I'd much rather you used the new matching support rather than using the
old .add_components. The new matching support is more efficient as you
only have to scan DT once, rather than each time we try to probe. That
will mean...
> @@ -613,12 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device
> *pdev)
> return -ENXIO;
> }
You need to build a struct component_match array here using
component_match_add()...
>
> - return drm_platform_init(_driver, pdev);
> + return component_master_add(>dev, _comp_ops);
and then finally register the ops with component_master_add_with_match().
Thanks.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
On 26/02/15 16:55, Jyri Sarha wrote:
> Add support for an external compontised DRM encoder. The external
> encoder can be connected to tilcdc trough device tree graph binding.
> The binding document for tilcdc has been updated. The support has only
> been tested with tda998x encoder, but other encoders should work too
> with a little tweaking.
>
> I got the idea and some lines of code from Jean-Francois Moine's
> "drm/tilcdc: Change the interface with the tda998x driver"-patch.
>
> Signed-off-by: Jyri Sarha
> ---
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c
> b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> new file mode 100644
> index 000..7254151
> --- /dev/null
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright (C) 2015 Texas Instruments
> + * Author: Jyri Sarha
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + *
> + */
> +#define DEBUG 1
You probably didn't mean to include this.
> +
> +#include
> +#include
> +
> +#include "tilcdc_drv.h"
> +#include "tilcdc_external.h"
> +
> +static const struct tilcdc_panel_info panel_info_defaults = {
> + .ac_bias= 255,
> + .ac_bias_intrpt = 0,
> + .dma_burst_sz = 16,
> + .bpp= 16,
> + .fdd= 0x80,
> + .tft_alt_mode = 0,
> + .invert_pxl_clk = 1,
> + .sync_edge = 1,
> + .sync_ctrl = 1,
> + .raster_order = 0,
> +};
> +
> +static int tilcdc_add_external_encoder(struct drm_device *dev, int *bpp,
> +struct drm_connector *connector)
> +{
> + struct tilcdc_drm_private *priv = dev->dev_private;
> +
> + priv->connectors[priv->num_connectors++] = connector;
> + priv->encoders[priv->num_encoders++] = connector->encoder;
> +
> + tilcdc_crtc_set_simulate_vesa_sync(priv->crtc, true);
> + tilcdc_crtc_set_panel_info(priv->crtc, _info_defaults);
Setting of the simulate vesa sync and the panel info here look a bit
like a hack to me. Both of them are for tda998x, not "defaults".
So... I don't know. You could state that at the moment tilcdc only
supports tda998x as an external encoder. Then the above would be ok, but
still it would be good to clearly state this in the desc, comments and
variable names.
Doing this properly may be more difficult. Some parameters should be
defined in the .dts, some should probably come from tda998x driver, and
some should be deduced by tilcdc driver internally.
> + *bpp = panel_info_defaults.bpp;
> +
> + dev_info(dev->dev, "External encoder '%s' connected\n",
> + connector->encoder->name);
This and the other dev_info in this patch look more like dev_dbg to me.
> +
> + return 0;
> +}
> +
> +
> +int tilcdc_add_external_encoders(struct drm_device *dev, int *bpp)
> +{
> + struct tilcdc_drm_private *priv = dev->dev_private;
> + struct drm_connector *connector;
> + int num_internal_connectors = priv->num_connectors;
> +
> + list_for_each_entry(connector, >mode_config.connector_list, head) {
> + bool found = false;
> + int i, ret;
> +
> + for (i = 0; i < num_internal_connectors; i++)
> + if (connector == priv->connectors[i])
> + found = true;
> + if (!found) {
> + ret = tilcdc_add_external_encoder(dev, bpp, connector);
> + if (ret)
> + return ret;
> + }
> + }
> + if (priv->num_connectors - num_internal_connectors > 1) {
> + dev_err(dev->dev, "Only one external encoder is supported.");
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +int tilcdc_add_external_components(struct device *master,
> +struct master *m)
> +{
> + struct device_node *ep = NULL;
> +
> + while ((ep = of_graph_get_next_endpoint(master->of_node, ep))) {
> + struct device_node *node;
> + int ret;
> +
> + node = of_graph_get_remote_port_parent(ep);
> + of_node_put(ep);
Note that there's an unmerged series "Add of-graph helpers to loop over
endpoints and find ports by id" from Philipp which changes the behavior
of of_graph_get_next_endpoint.
> + if (!node || !of_device_is_available(node))
> + continue;
Should you of_node_put(node) if node != NULL above?
> +
> + dev_info(master, "Subdevice node '%s' found\n", node->name);
> + ret = component_master_add_child(m,
[PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
Add support for an external compontised DRM encoder. The external
encoder can be connected to tilcdc trough device tree graph binding.
The binding document for tilcdc has been updated. The support has only
been tested with tda998x encoder, but other encoders should work too
with a little tweaking.
I got the idea and some lines of code from Jean-Francois Moine's
"drm/tilcdc: Change the interface with the tda998x driver"-patch.
Signed-off-by: Jyri Sarha
---
.../devicetree/bindings/drm/tilcdc/tilcdc.txt | 27 ++
drivers/gpu/drm/tilcdc/Makefile| 1 +
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +++
drivers/gpu/drm/tilcdc/tilcdc_drv.c| 56 ---
drivers/gpu/drm/tilcdc/tilcdc_drv.h| 2 +
drivers/gpu/drm/tilcdc/tilcdc_external.c | 105 +
drivers/gpu/drm/tilcdc/tilcdc_external.h | 24 +
7 files changed, 235 insertions(+), 13 deletions(-)
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_external.h
diff --git a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
index fff10da..2136ee8 100644
--- a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
+++ b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
@@ -18,6 +18,12 @@ Optional properties:
- max-pixelclock: The maximum pixel clock that can be supported
by the lcd controller in KHz.
+Optional nodes:
+
+ - port/ports: to describe a connection to an external encoder. The
+ binding follows Documentation/devicetree/bindings/graph.txt and
+ suppors a single port with a single endpoint.
+
Example:
fb: fb at 4830e000 {
@@ -26,4 +32,25 @@ Example:
interrupt-parent = <>;
interrupts = <36>;
ti,hwmods = "lcdc";
+
+ port {
+ lcdc_0: endpoint at 0 {
+ remote-endpoint = <_0>;
+ };
+ };
+ };
+
+ tda19988: tda19988 {
+ compatible = "nxp,tda998x";
+ reg = <0x70>;
+
+ pinctrl-names = "default", "off";
+ pinctrl-0 = <_hdmi_bonelt_pins>;
+ pinctrl-1 = <_hdmi_bonelt_off_pins>;
+
+ port {
+ hdmi_0: endpoint at 0 {
+ remote-endpoint = <_0>;
+ };
+ };
};
diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
index 44485f9..e1f738b 100644
--- a/drivers/gpu/drm/tilcdc/Makefile
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -7,6 +7,7 @@ tilcdc-y := \
tilcdc_crtc.o \
tilcdc_tfp410.o \
tilcdc_panel.o \
+ tilcdc_external.o \
tilcdc_drv.o
obj-$(CONFIG_DRM_TILCDC) += tilcdc.o
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index c2d5980..7d07733 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -37,6 +37,9 @@ struct tilcdc_crtc {
/* for deferred fb unref's: */
struct drm_flip_work unref_work;
+
+ /* Only set if an external encoder is connected */
+ bool simulate_vesa_sync;
};
#define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
@@ -214,6 +217,28 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
const struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode)
{
+ struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+
+ if (!tilcdc_crtc->simulate_vesa_sync)
+ return true;
+
+ /*
+* tilcdc does not generate VESA-compliant sync but aligns
+* VS on the second edge of HS instead of first edge.
+* We use adjusted_mode, to fixup sync by aligning both rising
+* edges and add HSKEW offset to fix the sync.
+*/
+ adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
+ adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
+
+ if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
+ adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
+ adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
+ } else {
+ adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
+ adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
+ }
+
return true;
}
@@ -534,6 +559,14 @@ void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
tilcdc_crtc->info = info;
}
+void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
+ bool simulate_vesa_sync)
+{
+ struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+
+ tilcdc_crtc->simulate_vesa_sync = simulate_vesa_sync;
+}
+
void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
{
struct tilcdc_crtc *tilcdc_crtc =
