[PATCH 0/5] drm: ti-tfp410 improvements

[PATCH 0/5] drm: ti-tfp410 improvements

[Laurent Pinchart]

Hello,

This small patch series improves the ti-tfp410 driver with three new features,
in patch 2/5 to 5/5. Patch 1/5 has been previously posted by Stefan, and I've
included it here to support patch 5/5 that needs to store other polarity
information in the bridge timings than the sampling edges.

These changes are meant to support the missing tfp410 features currently
implemented in the omapdrm custom tfp410 driver, in order to move to
drm_bridge.

The series is based on top of the "[PATCH v2 0/2] Clarify display info PIXDATA
bus flags" series [1] previously posted on the mailing list.

[1] https://lists.freedesktop.org/archives/dri-devel/2018-December/199204.html

Laurent Pinchart (4):
  dt-bindings: display: tfp410: Add bus parameters properties
  drm/bridge: ti-tfp410: Set connector type based on DT connector node
  drm/bridge: ti-tfp410: Add support for the powerdown GPIO
  drm/bridge: ti-tfp410: Report input bus config through bridge timings

Stefan Agner (1):
  drm/bridge: use bus flags in bridge timings

 .../bindings/display/bridge/ti,tfp410.txt     |  24 +++-
 drivers/gpu/drm/bridge/dumb-vga-dac.c         |   6 +-
 drivers/gpu/drm/bridge/ti-tfp410.c            | 109 +++++++++++++++++-
 include/drm/drm_bridge.h                      |  12 +-
 4 files changed, 131 insertions(+), 20 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

From: Stefan Agner <stefan@agner.ch>

The DRM bus flags convey additional information on pixel data on
the bus. All current available bus flags might be of interest for
a bridge. Remove the sampling_edge field and use bus_flags.

In the case at hand a dumb VGA bridge needs a specific data enable
polarity (DRM_BUS_FLAG_DE_LOW).

Signed-off-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Renamed bus_flags to input_bus_flags
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c |  6 +++---
 include/drm/drm_bridge.h              | 12 +++++-------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 7dc14c22f7db..191a4a6e624f 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
  */
 static const struct drm_bridge_timings default_dac_timings = {
 	/* Timing specifications, datasheet page 7 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
 	.setup_time_ps = 500,
 	.hold_time_ps = 1500,
 };
@@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8134_dac_timings = {
 	/* From timing diagram, datasheet page 9 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
 	/* From datasheet, page 12 */
 	.setup_time_ps = 3000,
 	/* I guess this means latched input */
@@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = {
  */
 static const struct drm_bridge_timings ti_ths8135_dac_timings = {
 	/* From timing diagram, datasheet page 14 */
-	.sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
 	/* From datasheet, page 16 */
 	.setup_time_ps = 2000,
 	.hold_time_ps = 500,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bf81fb573a5e..27c17706626c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -244,15 +244,13 @@ struct drm_bridge_funcs {
  */
 struct drm_bridge_timings {
 	/**
-	 * @sampling_edge:
+	 * @input_bus_flags:
 	 *
-	 * Tells whether the bridge samples the digital input signals from the
-	 * display engine on the positive or negative edge of the clock. This
-	 * should use the DRM_BUS_FLAG_PIXDATA_SAMPLE_[POS|NEG]EDGE and
-	 * DRM_BUS_FLAG_SYNC_SAMPLE_[POS|NEG]EDGE bitwise flags from the DRM
-	 * connector (bit 2, 3, 6 and 7 valid).
+	 * Tells what additional settings for the pixel data on the bus
+	 * this bridge requires (like pixel signal polarity). See also
+	 * &drm_display_info->bus_flags.
 	 */
-	u32 sampling_edge;
+	u32 input_bus_flags;
 	/**
 	 * @setup_time_ps:
 	 *
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] dt-bindings: display: tfp410: Add bus parameters properties

[Laurent Pinchart]

The TFP410 supports configuration of several input bus parameters
through either the I2C port or chip pins. In the latter case, we need to
specify those parameters in DT.

Two new properties are added, ti,deskew to specify the data de-skew
configuration (as set through the DK[3:1] pins), and pclk-sample to
specify the pixel clock sampling edge (as set through the EDGE pin).

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../bindings/display/bridge/ti,tfp410.txt     | 24 ++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
index 54d7e31525ec..3f903af93949 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
@@ -6,15 +6,25 @@ Required properties:
 
 Optional properties:
 - powerdown-gpios: power-down gpio
-- reg: I2C address. If and only if present the device node
-       should be placed into the i2c controller node where the
-       tfp410 i2c is connected to.
+- reg: I2C address. If and only if present the device node should be placed
+  into the I2C controller node where the TFP410 I2C is connected to.
+- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured
+  through th DK[3:1] pins. This property shall be present only if the TFP410
+  is not connected through I2C.
 
 Required nodes:
-- Video port 0 for DPI input [1].
-- Video port 1 for DVI output [1].
 
-[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+This device has two video ports. Their connections are modeled using the OF
+graph bindings specified in [1]. Each port node shall have a single endpoint.
+
+- Port 0 is the DPI input port. Its endpoint subnode shall contain a
+  pclk-sample property and a remote-endpoint property as specified in [1].
+
+- Port 1 is the DVI output port. Its endpoint subnode shall contain a
+  remote-endpoint property is specified in [1].
+
+[1] Documentation/devicetree/bindings/media/video-interfaces.txt
+
 
 Example
 -------
@@ -22,6 +32,7 @@ Example
 tfp410: encoder@0 {
 	compatible = "ti,tfp410";
 	powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>;
+	ti,deskew = <4>;
 
 	ports {
 		#address-cells = <1>;
@@ -31,6 +42,7 @@ tfp410: encoder@0 {
 			reg = <0>;
 
 			tfp410_in: endpoint@0 {
+				pclk-sample = <1>;
 				remote-endpoint = <&dpi_out>;
 			};
 		};
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/bridge: ti-tfp410: Set connector type based on DT connector node

[Laurent Pinchart]

The TI TFP410 is a DVI encoder, not a full HDMI encoder. Its output can
be routed to a DVI-D connector, even if in many cases embedded systems
will use an HDMI connector to carry the DVI signals.

Instead of hardcoding the connector type to HDMI, retrieve the connector
type from its DT node.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index c3e32138c6bb..e4280f5af9f5 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -27,6 +27,7 @@
 struct tfp410 {
 	struct drm_bridge	bridge;
 	struct drm_connector	connector;
+	unsigned int		connector_type;
 
 	struct i2c_adapter	*ddc;
 	struct gpio_desc	*hpd;
@@ -126,7 +127,7 @@ static int tfp410_attach(struct drm_bridge *bridge)
 	drm_connector_helper_add(&dvi->connector,
 				 &tfp410_con_helper_funcs);
 	ret = drm_connector_init(bridge->dev, &dvi->connector,
-				 &tfp410_con_funcs, DRM_MODE_CONNECTOR_HDMIA);
+				 &tfp410_con_funcs, dvi->connector_type);
 	if (ret) {
 		dev_err(dvi->dev, "drm_connector_init() failed: %d\n", ret);
 		return ret;
@@ -172,6 +173,11 @@ static int tfp410_get_connector_properties(struct tfp410 *dvi)
 	if (!connector_node)
 		return -ENODEV;
 
+	if (of_device_is_compatible(connector_node, "hdmi-connector"))
+		dvi->connector_type = DRM_MODE_CONNECTOR_HDMIA;
+	else
+		dvi->connector_type = DRM_MODE_CONNECTOR_DVID;
+
 	dvi->hpd = fwnode_get_named_gpiod(&connector_node->fwnode,
 					"hpd-gpios", 0, GPIOD_IN, "hpd");
 	if (IS_ERR(dvi->hpd)) {
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/bridge: ti-tfp410: Add support for the powerdown GPIO

[Laurent Pinchart]

The TFP410 has a powerdown pin that can be connected to a GPIO to
control power saving. The DT bindings define a corresponding property,
but the driver doesn't implement support for it. Fix that.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index e4280f5af9f5..d25d23cfe3f5 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -32,6 +32,7 @@ struct tfp410 {
 	struct i2c_adapter	*ddc;
 	struct gpio_desc	*hpd;
 	struct delayed_work	hpd_work;
+	struct gpio_desc	*powerdown;
 
 	struct device *dev;
 };
@@ -139,8 +140,24 @@ static int tfp410_attach(struct drm_bridge *bridge)
 	return 0;
 }
 
+static void tfp410_enable(struct drm_bridge *bridge)
+{
+	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+
+	gpiod_set_value_cansleep(dvi->powerdown, 0);
+}
+
+static void tfp410_disable(struct drm_bridge *bridge)
+{
+	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+
+	gpiod_set_value_cansleep(dvi->powerdown, 1);
+}
+
 static const struct drm_bridge_funcs tfp410_bridge_funcs = {
 	.attach		= tfp410_attach,
+	.enable		= tfp410_enable,
+	.disable	= tfp410_disable,
 };
 
 static void tfp410_hpd_work_func(struct work_struct *work)
@@ -229,6 +246,13 @@ static int tfp410_init(struct device *dev)
 	if (ret)
 		goto fail;
 
+	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
+						 GPIOD_OUT_HIGH);
+	if (IS_ERR(dvi->powerdown)) {
+		dev_err(dev, "failed to parse powerdown gpio\n");
+		return PTR_ERR(dvi->powerdown);
+	}
+
 	if (dvi->hpd) {
 		INIT_DELAYED_WORK(&dvi->hpd_work, tfp410_hpd_work_func);
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/bridge: ti-tfp410: Report input bus config through bridge timings

[Laurent Pinchart]

The TFP410 supports configurable pixel clock sampling edge and data
de-skew adjustments. The configuration can be set through I2C or
dedicated chip pins.

Report the configuration through the drm_bridge timings. As the
ti-tftp410 driver doesn't support configuring the chip through I2C, we
simply use the default configuration in that case. When the chip is
configured through dedicated pins, we parse the configuration from DT.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 77 ++++++++++++++++++++++++++++--
 1 file changed, 74 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index d25d23cfe3f5..48ec647a7526 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -34,6 +34,8 @@ struct tfp410 {
 	struct delayed_work	hpd_work;
 	struct gpio_desc	*powerdown;
 
+	struct drm_bridge_timings timings;
+
 	struct device *dev;
 };
 
@@ -180,6 +182,70 @@ static irqreturn_t tfp410_hpd_irq_thread(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static const struct drm_bridge_timings tfp410_default_timings = {
+	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
+			 | DRM_BUS_FLAG_DE_HIGH,
+	.setup_time_ps = 1200,
+	.hold_time_ps = 1300,
+};
+
+static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
+{
+	struct drm_bridge_timings *timings = &dvi->timings;
+	struct device_node *ep;
+	u32 pclk_sample = 0;
+	s32 deskew = 0;
+
+	/* Start with defaults. */
+	*timings = tfp410_default_timings;
+
+	if (i2c)
+		/*
+		 * In I2C mode timings are configured through the I2C interface.
+		 * As the driver doesn't support I2C configuration yet, we just
+		 * go with the defaults (BSEL=1, DSEL=1, DKEN=0, EDGE=1).
+		 */
+		return 0;
+
+	/*
+	 * In non-I2C mode, timings are configured through the BSEL, DSEL, DKEN
+	 * and EDGE pins. They are specified in DT through endpoint properties
+	 * and vendor-specific properties.
+	 */
+	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
+	if (!ep)
+		return -EINVAL;
+
+	/* Get the sampling edge from the endpoint. */
+	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
+	of_node_put(ep);
+
+	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
+
+	switch (pclk_sample) {
+	case 0:
+		timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE
+					 |  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE;
+		break;
+	case 1:
+		timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
+					 |  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Get the setup and hold time from vendor-specific properties. */
+	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
+	if (deskew < -4 || deskew > 3)
+		return -EINVAL;
+
+	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
+	timings->hold_time_ps = min(0, 1300 + 350 * deskew);
+
+	return 0;
+}
+
 static int tfp410_get_connector_properties(struct tfp410 *dvi)
 {
 	struct device_node *connector_node, *ddc_phandle;
@@ -223,7 +289,7 @@ static int tfp410_get_connector_properties(struct tfp410 *dvi)
 	return ret;
 }
 
-static int tfp410_init(struct device *dev)
+static int tfp410_init(struct device *dev, bool i2c)
 {
 	struct tfp410 *dvi;
 	int ret;
@@ -240,8 +306,13 @@ static int tfp410_init(struct device *dev)
 
 	dvi->bridge.funcs = &tfp410_bridge_funcs;
 	dvi->bridge.of_node = dev->of_node;
+	dvi->bridge.timings = &dvi->timings;
 	dvi->dev = dev;
 
+	ret = tfp410_parse_timings(dvi, i2c);
+	if (ret)
+		goto fail;
+
 	ret = tfp410_get_connector_properties(dvi);
 	if (ret)
 		goto fail;
@@ -294,7 +365,7 @@ static int tfp410_fini(struct device *dev)
 
 static int tfp410_probe(struct platform_device *pdev)
 {
-	return tfp410_init(&pdev->dev);
+	return tfp410_init(&pdev->dev, false);
 }
 
 static int tfp410_remove(struct platform_device *pdev)
@@ -331,7 +402,7 @@ static int tfp410_i2c_probe(struct i2c_client *client,
 		return -ENXIO;
 	}
 
-	return tfp410_init(&client->dev);
+	return tfp410_init(&client->dev, true);
 }
 
 static int tfp410_i2c_remove(struct i2c_client *client)
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm: ti-tfp410 improvements

[Tomi Valkeinen]

On 06/12/18 22:26, Laurent Pinchart wrote:
> Hello,
> 
> This small patch series improves the ti-tfp410 driver with three new features,
> in patch 2/5 to 5/5. Patch 1/5 has been previously posted by Stefan, and I've
> included it here to support patch 5/5 that needs to store other polarity
> information in the bridge timings than the sampling edges.
> 
> These changes are meant to support the missing tfp410 features currently
> implemented in the omapdrm custom tfp410 driver, in order to move to
> drm_bridge.
> 
> The series is based on top of the "[PATCH v2 0/2] Clarify display info PIXDATA
> bus flags" series [1] previously posted on the mailing list.
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-December/199204.html
> 
> Laurent Pinchart (4):
>   dt-bindings: display: tfp410: Add bus parameters properties
>   drm/bridge: ti-tfp410: Set connector type based on DT connector node
>   drm/bridge: ti-tfp410: Add support for the powerdown GPIO
>   drm/bridge: ti-tfp410: Report input bus config through bridge timings
> 
> Stefan Agner (1):
>   drm/bridge: use bus flags in bridge timings
> 
>  .../bindings/display/bridge/ti,tfp410.txt     |  24 +++-
>  drivers/gpu/drm/bridge/dumb-vga-dac.c         |   6 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c            | 109 +++++++++++++++++-
>  include/drm/drm_bridge.h                      |  12 +-
>  4 files changed, 131 insertions(+), 20 deletions(-)

For the series:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/bridge: ti-tfp410: Set connector type based on DT connector node

[Jyri Sarha]

On 06/12/2018 22:26, Laurent Pinchart wrote:
> The TI TFP410 is a DVI encoder, not a full HDMI encoder. Its output can
> be routed to a DVI-D connector, even if in many cases embedded systems
> will use an HDMI connector to carry the DVI signals.
> 
> Instead of hardcoding the connector type to HDMI, retrieve the connector
> type from its DT node.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

Reviewed-by: Jyri Sarha <jsarha@ti.com>

>  drivers/gpu/drm/bridge/ti-tfp410.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index c3e32138c6bb..e4280f5af9f5 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -27,6 +27,7 @@
>  struct tfp410 {
>  	struct drm_bridge	bridge;
>  	struct drm_connector	connector;
> +	unsigned int		connector_type;
>  
>  	struct i2c_adapter	*ddc;
>  	struct gpio_desc	*hpd;
> @@ -126,7 +127,7 @@ static int tfp410_attach(struct drm_bridge *bridge)
>  	drm_connector_helper_add(&dvi->connector,
>  				 &tfp410_con_helper_funcs);
>  	ret = drm_connector_init(bridge->dev, &dvi->connector,
> -				 &tfp410_con_funcs, DRM_MODE_CONNECTOR_HDMIA);
> +				 &tfp410_con_funcs, dvi->connector_type);
>  	if (ret) {
>  		dev_err(dvi->dev, "drm_connector_init() failed: %d\n", ret);
>  		return ret;
> @@ -172,6 +173,11 @@ static int tfp410_get_connector_properties(struct tfp410 *dvi)
>  	if (!connector_node)
>  		return -ENODEV;
>  
> +	if (of_device_is_compatible(connector_node, "hdmi-connector"))
> +		dvi->connector_type = DRM_MODE_CONNECTOR_HDMIA;
> +	else
> +		dvi->connector_type = DRM_MODE_CONNECTOR_DVID;
> +
>  	dvi->hpd = fwnode_get_named_gpiod(&connector_node->fwnode,
>  					"hpd-gpios", 0, GPIOD_IN, "hpd");
>  	if (IS_ERR(dvi->hpd)) {
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/bridge: ti-tfp410: Add support for the powerdown GPIO

[Jyri Sarha]

On 06/12/2018 22:26, Laurent Pinchart wrote:
> The TFP410 has a powerdown pin that can be connected to a GPIO to
> control power saving. The DT bindings define a corresponding property,
> but the driver doesn't implement support for it. Fix that.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

I find the naming of the the gpio bit backwards, but I guess it si fine
since it is also the name of the pin.

Reviewed-by: Jyri Sarha <jsarha@ti.com>

>  drivers/gpu/drm/bridge/ti-tfp410.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index e4280f5af9f5..d25d23cfe3f5 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -32,6 +32,7 @@ struct tfp410 {
>  	struct i2c_adapter	*ddc;
>  	struct gpio_desc	*hpd;
>  	struct delayed_work	hpd_work;
> +	struct gpio_desc	*powerdown;
>  
>  	struct device *dev;
>  };
> @@ -139,8 +140,24 @@ static int tfp410_attach(struct drm_bridge *bridge)
>  	return 0;
>  }
>  
> +static void tfp410_enable(struct drm_bridge *bridge)
> +{
> +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> +
> +	gpiod_set_value_cansleep(dvi->powerdown, 0);
> +}
> +
> +static void tfp410_disable(struct drm_bridge *bridge)
> +{
> +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> +
> +	gpiod_set_value_cansleep(dvi->powerdown, 1);
> +}
> +
>  static const struct drm_bridge_funcs tfp410_bridge_funcs = {
>  	.attach		= tfp410_attach,
> +	.enable		= tfp410_enable,
> +	.disable	= tfp410_disable,
>  };
>  
>  static void tfp410_hpd_work_func(struct work_struct *work)
> @@ -229,6 +246,13 @@ static int tfp410_init(struct device *dev)
>  	if (ret)
>  		goto fail;
>  
> +	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
> +						 GPIOD_OUT_HIGH);
> +	if (IS_ERR(dvi->powerdown)) {
> +		dev_err(dev, "failed to parse powerdown gpio\n");
> +		return PTR_ERR(dvi->powerdown);
> +	}
> +
>  	if (dvi->hpd) {
>  		INIT_DELAYED_WORK(&dvi->hpd_work, tfp410_hpd_work_func);
>  
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/bridge: ti-tfp410: Add support for the powerdown GPIO

[Laurent Pinchart]

Hi Jyri,

On Tuesday, 11 December 2018 15:01:52 EET Jyri Sarha wrote:
> On 06/12/2018 22:26, Laurent Pinchart wrote:
> > The TFP410 has a powerdown pin that can be connected to a GPIO to
> > control power saving. The DT bindings define a corresponding property,
> > but the driver doesn't implement support for it. Fix that.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> 
> I find the naming of the the gpio bit backwards, but I guess it si fine
> since it is also the name of the pin.

I would have gone for "enable-gpios", but given that we have DTs in the wild 
using the "powerdown-gpios" property (with the omapdrm-specific TFP410 driver 
which uses the same compatible string as the drm_bridge-based driver) I think 
we need to stick to it. As you mentioned this is the pin's name, so it's not 
too big of a deal.

I'll now wait for your review of patch 5/5 before merging these changes :-) 
The plan is to get them in drm-misc for v4.22, please let me know if that 
causes any problem for you. Also, please feel free to review 1/5 and 2/5 if 
you have time.

> Reviewed-by: Jyri Sarha <jsarha@ti.com>
> 
> >  drivers/gpu/drm/bridge/ti-tfp410.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> > b/drivers/gpu/drm/bridge/ti-tfp410.c index e4280f5af9f5..d25d23cfe3f5
> > 100644
> > --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > @@ -32,6 +32,7 @@ struct tfp410 {
> >  	struct i2c_adapter	*ddc;
> >  	struct gpio_desc	*hpd;
> >  	struct delayed_work	hpd_work;
> > +	struct gpio_desc	*powerdown;
> >  	struct device *dev;
> >  };
> > 
> > @@ -139,8 +140,24 @@ static int tfp410_attach(struct drm_bridge *bridge)
> >  	return 0;
> >  }
> > 
> > +static void tfp410_enable(struct drm_bridge *bridge)
> > +{
> > +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> > +
> > +	gpiod_set_value_cansleep(dvi->powerdown, 0);
> > +}
> > +
> > +static void tfp410_disable(struct drm_bridge *bridge)
> > +{
> > +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> > +
> > +	gpiod_set_value_cansleep(dvi->powerdown, 1);
> > +}
> > +
> >  static const struct drm_bridge_funcs tfp410_bridge_funcs = {
> >  	.attach		= tfp410_attach,
> > +	.enable		= tfp410_enable,
> > +	.disable	= tfp410_disable,
> >  };
> >  
> >  static void tfp410_hpd_work_func(struct work_struct *work)
> > @@ -229,6 +246,13 @@ static int tfp410_init(struct device *dev)
> >  	if (ret)
> >  		goto fail;
> > 
> > +	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
> > +						 GPIOD_OUT_HIGH);
> > +	if (IS_ERR(dvi->powerdown)) {
> > +		dev_err(dev, "failed to parse powerdown gpio\n");
> > +		return PTR_ERR(dvi->powerdown);
> > +	}
> > +
> >  	if (dvi->hpd) {
> >  		INIT_DELAYED_WORK(&dvi->hpd_work, tfp410_hpd_work_func);

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/bridge: ti-tfp410: Report input bus config through bridge timings

[Jyri Sarha]

On 06/12/2018 22:26, Laurent Pinchart wrote:
> The TFP410 supports configurable pixel clock sampling edge and data
> de-skew adjustments. The configuration can be set through I2C or
> dedicated chip pins.
> 
> Report the configuration through the drm_bridge timings. As the
> ti-tftp410 driver doesn't support configuring the chip through I2C, we
> simply use the default configuration in that case. When the chip is
> configured through dedicated pins, we parse the configuration from DT.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-tfp410.c | 77 ++++++++++++++++++++++++++++--
>  1 file changed, 74 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index d25d23cfe3f5..48ec647a7526 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -34,6 +34,8 @@ struct tfp410 {
>  	struct delayed_work	hpd_work;
>  	struct gpio_desc	*powerdown;
>  
> +	struct drm_bridge_timings timings;
> +
>  	struct device *dev;
>  };
>  
> @@ -180,6 +182,70 @@ static irqreturn_t tfp410_hpd_irq_thread(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +static const struct drm_bridge_timings tfp410_default_timings = {
> +	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
> +			 | DRM_BUS_FLAG_DE_HIGH,
> +	.setup_time_ps = 1200,
> +	.hold_time_ps = 1300,
> +};
> +
> +static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> +{
> +	struct drm_bridge_timings *timings = &dvi->timings;
> +	struct device_node *ep;
> +	u32 pclk_sample = 0;
> +	s32 deskew = 0;
> +
> +	/* Start with defaults. */
> +	*timings = tfp410_default_timings;
> +
> +	if (i2c)
> +		/*
> +		 * In I2C mode timings are configured through the I2C interface.
> +		 * As the driver doesn't support I2C configuration yet, we just
> +		 * go with the defaults (BSEL=1, DSEL=1, DKEN=0, EDGE=1).
> +		 */
> +		return 0;
> +
> +	/*
> +	 * In non-I2C mode, timings are configured through the BSEL, DSEL, DKEN
> +	 * and EDGE pins. They are specified in DT through endpoint properties
> +	 * and vendor-specific properties.
> +	 */
> +	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	/* Get the sampling edge from the endpoint. */
> +	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
> +	of_node_put(ep);
> +
> +	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
> +
> +	switch (pclk_sample) {
> +	case 0:
> +		timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE
> +					 |  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE;
> +		break;
> +	case 1:
> +		timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
> +					 |  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Get the setup and hold time from vendor-specific properties. */
> +	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
> +	if (deskew < -4 || deskew > 3)
> +		return -EINVAL;

Is "ti,deskew" property documented somewhere? If it is, I could not find
it. Could it be added to the tfp410 binding document, or somewhere else?

Othewise:

Reviewed-by: Jyri Sarha <jsarha@ti.com>

> +
> +	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
> +	timings->hold_time_ps = min(0, 1300 + 350 * deskew);
> +
> +	return 0;
> +}
> +
>  static int tfp410_get_connector_properties(struct tfp410 *dvi)
>  {
>  	struct device_node *connector_node, *ddc_phandle;
> @@ -223,7 +289,7 @@ static int tfp410_get_connector_properties(struct tfp410 *dvi)
>  	return ret;
>  }
>  
> -static int tfp410_init(struct device *dev)
> +static int tfp410_init(struct device *dev, bool i2c)
>  {
>  	struct tfp410 *dvi;
>  	int ret;
> @@ -240,8 +306,13 @@ static int tfp410_init(struct device *dev)
>  
>  	dvi->bridge.funcs = &tfp410_bridge_funcs;
>  	dvi->bridge.of_node = dev->of_node;
> +	dvi->bridge.timings = &dvi->timings;
>  	dvi->dev = dev;
>  
> +	ret = tfp410_parse_timings(dvi, i2c);
> +	if (ret)
> +		goto fail;
> +
>  	ret = tfp410_get_connector_properties(dvi);
>  	if (ret)
>  		goto fail;
> @@ -294,7 +365,7 @@ static int tfp410_fini(struct device *dev)
>  
>  static int tfp410_probe(struct platform_device *pdev)
>  {
> -	return tfp410_init(&pdev->dev);
> +	return tfp410_init(&pdev->dev, false);
>  }
>  
>  static int tfp410_remove(struct platform_device *pdev)
> @@ -331,7 +402,7 @@ static int tfp410_i2c_probe(struct i2c_client *client,
>  		return -ENXIO;
>  	}
>  
> -	return tfp410_init(&client->dev);
> +	return tfp410_init(&client->dev, true);
>  }
>  
>  static int tfp410_i2c_remove(struct i2c_client *client)
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/bridge: ti-tfp410: Report input bus config through bridge timings

[Tomi Valkeinen]

On 11/12/18 16:48, Jyri Sarha wrote:

>> +	/* Get the setup and hold time from vendor-specific properties. */
>> +	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
>> +	if (deskew < -4 || deskew > 3)
>> +		return -EINVAL;
> 
> Is "ti,deskew" property documented somewhere? If it is, I could not find
> it. Could it be added to the tfp410 binding document, or somewhere else?

Patch 2 of this series.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/bridge: ti-tfp410: Report input bus config through bridge timings

[Jyri Sarha]

On 11/12/2018 17:19, Tomi Valkeinen wrote:
> On 11/12/18 16:48, Jyri Sarha wrote:
> 
>>> +	/* Get the setup and hold time from vendor-specific properties. */
>>> +	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
>>> +	if (deskew < -4 || deskew > 3)
>>> +		return -EINVAL;
>>
>> Is "ti,deskew" property documented somewhere? If it is, I could not find
>> it. Could it be added to the tfp410 binding document, or somewhere else?
> 
> Patch 2 of this series.
> 

Hrrm, ok then. Should have applied the patches. Then just:

Reviewed-by: Jyri Sarha <jsarha@ti.com>

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/bridge: ti-tfp410: Report input bus config through bridge timings

[Laurent Pinchart]

Hi Jyri,

On Tuesday, 11 December 2018 16:48:16 EET Jyri Sarha wrote:
> On 06/12/2018 22:26, Laurent Pinchart wrote:
> > The TFP410 supports configurable pixel clock sampling edge and data
> > de-skew adjustments. The configuration can be set through I2C or
> > dedicated chip pins.
> > 
> > Report the configuration through the drm_bridge timings. As the
> > ti-tftp410 driver doesn't support configuring the chip through I2C, we
> > simply use the default configuration in that case. When the chip is
> > configured through dedicated pins, we parse the configuration from DT.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/bridge/ti-tfp410.c | 77 ++++++++++++++++++++++++++++--
> >  1 file changed, 74 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> > b/drivers/gpu/drm/bridge/ti-tfp410.c index d25d23cfe3f5..48ec647a7526
> > 100644
> > --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > @@ -34,6 +34,8 @@ struct tfp410 {
> >  	struct delayed_work	hpd_work;
> >  	struct gpio_desc	*powerdown;
> > 
> > +	struct drm_bridge_timings timings;
> > +
> >  	struct device *dev;
> >  };
> > 
> > @@ -180,6 +182,70 @@ static irqreturn_t tfp410_hpd_irq_thread(int irq,
> > void *arg)
> >  	return IRQ_HANDLED;
> >  }
> > 
> > +static const struct drm_bridge_timings tfp410_default_timings = {
> > +	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
> > +			 | DRM_BUS_FLAG_DE_HIGH,
> > +	.setup_time_ps = 1200,
> > +	.hold_time_ps = 1300,
> > +};
> > +
> > +static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> > +{
> > +	struct drm_bridge_timings *timings = &dvi->timings;
> > +	struct device_node *ep;
> > +	u32 pclk_sample = 0;
> > +	s32 deskew = 0;
> > +
> > +	/* Start with defaults. */
> > +	*timings = tfp410_default_timings;
> > +
> > +	if (i2c)
> > +		/*
> > +		 * In I2C mode timings are configured through the I2C interface.
> > +		 * As the driver doesn't support I2C configuration yet, we just
> > +		 * go with the defaults (BSEL=1, DSEL=1, DKEN=0, EDGE=1).
> > +		 */
> > +		return 0;
> > +
> > +	/*
> > +	 * In non-I2C mode, timings are configured through the BSEL, DSEL, DKEN
> > +	 * and EDGE pins. They are specified in DT through endpoint properties
> > +	 * and vendor-specific properties.
> > +	 */
> > +	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
> > +	if (!ep)
> > +		return -EINVAL;
> > +
> > +	/* Get the sampling edge from the endpoint. */
> > +	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
> > +	of_node_put(ep);
> > +
> > +	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
> > +
> > +	switch (pclk_sample) {
> > +	case 0:
> > +		timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE
> > +					 |  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE;
> > +		break;
> > +	case 1:
> > +		timings->input_bus_flags |= DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE
> > +					 |  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Get the setup and hold time from vendor-specific properties. */
> > +	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
> > +	if (deskew < -4 || deskew > 3)
> > +		return -EINVAL;
> 
> Is "ti,deskew" property documented somewhere? If it is, I could not find
> it. Could it be added to the tfp410 binding document, or somewhere else?

To satisfy your request, I've taken my time machine to rewrite history, and 
this patch series now includes the bindings documentation update in patch 2/5 
;-)

> Othewise:
> 
> Reviewed-by: Jyri Sarha <jsarha@ti.com>
> 
> > +
> > +	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
> > +	timings->hold_time_ps = min(0, 1300 + 350 * deskew);
> > +
> > +	return 0;
> > +}
> > +
> >  static int tfp410_get_connector_properties(struct tfp410 *dvi)
> >  {
 >  	struct device_node *connector_node, *ddc_phandle;
> > @@ -223,7 +289,7 @@ static int tfp410_get_connector_properties(struct
> > tfp410 *dvi)
> >  	return ret;
> >  }
> > 
> > -static int tfp410_init(struct device *dev)
> > +static int tfp410_init(struct device *dev, bool i2c)
> >  {
> >  	struct tfp410 *dvi;
> >  	int ret;
> > @@ -240,8 +306,13 @@ static int tfp410_init(struct device *dev)
> > 
> >  	dvi->bridge.funcs = &tfp410_bridge_funcs;
> >  	dvi->bridge.of_node = dev->of_node;
> > +	dvi->bridge.timings = &dvi->timings;
> >  	dvi->dev = dev;
> > 
> > +	ret = tfp410_parse_timings(dvi, i2c);
> > +	if (ret)
> > +		goto fail;
> > +
> >  	ret = tfp410_get_connector_properties(dvi);
> >  	if (ret)
> >  		goto fail;
> > @@ -294,7 +365,7 @@ static int tfp410_fini(struct device *dev)
> > 
> >  static int tfp410_probe(struct platform_device *pdev)
> >  {
> > -	return tfp410_init(&pdev->dev);
> > +	return tfp410_init(&pdev->dev, false);
> >  }
> >  
> >  static int tfp410_remove(struct platform_device *pdev)
> > @@ -331,7 +402,7 @@ static int tfp410_i2c_probe(struct i2c_client *client,
> >  		return -ENXIO;
> >  	}
> > 
> > -	return tfp410_init(&client->dev);
> > +	return tfp410_init(&client->dev, true);
> >  }
> >  
> >  static int tfp410_i2c_remove(struct i2c_client *client)

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel