Hello,
this new version moves the driver and its bindings to use semi-standard
names for powerdown and output enable GPIOs, as result of the discussion with
Laurent, Vladimir and Rob. I kept the actual pin names in the bindings
description for reference, even if there are no huge ambiguities on which
chip pin is actually an enable and which one a power down.
I have reworked the regulator management, making the 'vcc' supply the only
requested one, and all other optional supplies have been removed as suggested
by Laurent. It is unlikely a dedicated regulator is to be installed for each
power supply, and in case some HW design requires this, it's an easy add to be
implemented in future.
Contrary to what discussed on v6, the 'vcc' supply is still described
as optional in dt bindings, and the driver is now using
'regulator_get(NORMAL_GET)' in place of the _optional() version that was used
before. With the 'NORMAL_GET' version the regulator core provides a dummy
regulator in case an actual one is not available. This simplifies integration
in designs where the chip power supplies are directly connected to some power
rail. At the same time it makes easier to forget to add a regulator if there's
actually one there, and someone could find herself wondering why the chip does
not work even if probe completes properly.
I removed the Eagle display enablement patch from the series, I'll send it
separately squashed on top of Niklas' series that addresses the issue.
Thanks
j
v6 -> v7:
- Use semi-standard names for powerdown and output enable GPIOs as suggested
by Rob and Vladimir
- Use 'regulator_get()' not the optional version, and list only 'vcc' as
requested supply
- Addressed Laurent's review comments and removed Eagle display enablement patch
to be sent separately
v5 -> v6:
- Drop check for CONFIG_OF as it is a Kconfig dependency
- Add Niklas Reviewed-by tags
- List [3/3] depenencies below commit message to ease integration
v4 -> v5:
- Fix punctuation in bindings documentation
- Add small statement to bindings document to clarify the chip has no
control bus
- Print regulator name in enable/disable routines error path
- Add Andrzej Reviewed-by tag
v3 -> v4:
- Rename permutations of "pdwn" to just "pdwn" everywhere in the series
- Improve power enable/disable routines as suggested by Andrzej and Sergei
- Change "pdwn" gpio initialization to use the logical output level
- Change Kconfig description
v2 -> v3:
- Drop support for "lvds-decoder" and make the driver THC63LVD1024 specific
-- Rework bindings to describe multiple input/output ports
-- Rename driver and remove "lvds-decoder" references
-- Rework Eagle DTS to use new bindings
v1 -> v2:
- Drop support for THC63LVD1024
Jacopo Mondi (2):
dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
drm: bridge: Add thc63lvd1024 LVDS decoder driver
.../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++
drivers/gpu/drm/bridge/Kconfig | 6 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/thc63lvd1024.c | 212 +++++++++++++++++++++
4 files changed, 279 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
--
2.7.4
Document Thine THC63LVD1024 LVDS decoder device tree bindings.
Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Reviewed-by: Niklas Söderlund <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
.../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
new file mode 100644
index 0000000..1191f17
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
@@ -0,0 +1,60 @@
+Thine Electronics THC63LVD1024 LVDS decoder
+-------------------------------------------
+
+The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
+to parallel data outputs. The chip supports single/dual input/output modes,
+handling up to two two input LVDS stream and up to two digital CMOS/TTL outputs.
+
+Single or dual operation modes, output data mapping and DDR output modes are
+configured through input signals and the chip does not expose any control bus.
+
+Required properties:
+- compatible: Shall be "thine,thc63lvd1024"
+
+Optional properties:
+- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
+ PPL and digital circuitry
+- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
+- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
+
+The THC63LVD1024 video port connections are modeled according
+to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
+
+Required video port nodes:
+- port@0: First LVDS input port
+- port@2: First digital CMOS/TTL parallel output
+
+Optional video port nodes:
+- port@1: Second LVDS input port
+- port@3: Second digital CMOS/TTL parallel output
+
+Example:
+--------
+
+ thc63lvd1024: lvds-decoder {
+ compatible = "thine,thc63lvd1024";
+
+ vcc-supply = <®_lvds_vcc>;
+ powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ lvds_dec_in_0: endpoint {
+ remote-endpoint = <&lvds_out>;
+ };
+ };
+
+ port@2{
+ reg = <2>;
+
+ lvds_dec_out_2: endpoint {
+ remote-endpoint = <&adv7511_in>;
+ };
+ };
+ };
+ };
--
2.7.4
Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
output converter.
Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Reviewed-by: Niklas Söderlund <[email protected]>
---
drivers/gpu/drm/bridge/Kconfig | 6 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/thc63lvd1024.c | 212 ++++++++++++++++++++++++++++++++++
3 files changed, 219 insertions(+)
create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3aa65bd..42c9c2d 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -93,6 +93,12 @@ config DRM_SII9234
It is an I2C driver, that detects connection of MHL bridge
and starts encapsulation of HDMI signal.
+config DRM_THINE_THC63LVD1024
+ tristate "Thine THC63LVD1024 LVDS decoder bridge"
+ depends on OF
+ ---help---
+ Thine THC63LVD1024 LVDS/parallel converter driver.
+
config DRM_TOSHIBA_TC358767
tristate "Toshiba TC358767 eDP bridge"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..fd90b16 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
obj-$(CONFIG_DRM_SII902X) += sii902x.o
obj-$(CONFIG_DRM_SII9234) += sii9234.o
+obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
new file mode 100644
index 0000000..c8fad9c
--- /dev/null
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * THC63LVD1024 LVDS to parallel data DRM bridge driver.
+ *
+ * Copyright (C) 2018 Jacopo Mondi <[email protected]>
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+
+enum thc63_ports {
+ THC63_LVDS_IN0,
+ THC63_LVDS_IN1,
+ THC63_RGB_OUT0,
+ THC63_RGB_OUT1,
+};
+
+struct thc63_dev {
+ struct device *dev;
+
+ struct regulator *vcc;
+
+ struct gpio_desc *pdwn;
+ struct gpio_desc *oe;
+
+ struct drm_bridge bridge;
+ struct drm_bridge *next;
+};
+
+static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
+{
+ return container_of(bridge, struct thc63_dev, bridge);
+}
+
+static int thc63_attach(struct drm_bridge *bridge)
+{
+ struct thc63_dev *thc63 = to_thc63(bridge);
+
+ return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
+}
+
+static void thc63_enable(struct drm_bridge *bridge)
+{
+ struct thc63_dev *thc63 = to_thc63(bridge);
+ int ret;
+
+ ret = regulator_enable(thc63->vcc);
+ if (ret) {
+ dev_err(thc63->dev,
+ "Failed to enable regulator \"vcc\": %d\n", ret);
+ return;
+ }
+
+ if (thc63->pdwn)
+ gpiod_set_value(thc63->pdwn, 0);
+
+ if (thc63->oe)
+ gpiod_set_value(thc63->oe, 1);
+}
+
+static void thc63_disable(struct drm_bridge *bridge)
+{
+ struct thc63_dev *thc63 = to_thc63(bridge);
+ int ret;
+
+ if (thc63->oe)
+ gpiod_set_value(thc63->oe, 0);
+
+ if (thc63->pdwn)
+ gpiod_set_value(thc63->pdwn, 1);
+
+ ret = regulator_disable(thc63->vcc);
+ if (ret)
+ dev_err(thc63->dev,
+ "Failed to disable regulator \"vcc\": %d\n", ret);
+}
+
+static const struct drm_bridge_funcs thc63_bridge_func = {
+ .attach = thc63_attach,
+ .enable = thc63_enable,
+ .disable = thc63_disable,
+};
+
+static int thc63_parse_dt(struct thc63_dev *thc63)
+{
+ struct device_node *thc63_out;
+ struct device_node *remote;
+
+ thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
+ THC63_RGB_OUT0, -1);
+ if (!thc63_out) {
+ dev_err(thc63->dev, "Missing endpoint in port@%u\n",
+ THC63_RGB_OUT0);
+ return -ENODEV;
+ }
+
+ remote = of_graph_get_remote_port_parent(thc63_out);
+ of_node_put(thc63_out);
+ if (!remote) {
+ dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
+ THC63_RGB_OUT0);
+ return -ENODEV;
+ }
+
+ if (!of_device_is_available(remote)) {
+ dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
+ THC63_RGB_OUT0);
+ of_node_put(remote);
+ return -ENODEV;
+ }
+
+ thc63->next = of_drm_find_bridge(remote);
+ if (!thc63->next) {
+ of_node_put(remote);
+ return -EPROBE_DEFER;
+ }
+
+ return 0;
+}
+
+static int thc63_gpio_init(struct thc63_dev *thc63)
+{
+ thc63->oe = devm_gpiod_get_optional(thc63->dev, "enable", GPIOD_OUT_LOW);
+ if (IS_ERR(thc63->oe)) {
+ dev_err(thc63->dev, "Unable to get \"enable-gpios\": %ld\n",
+ PTR_ERR(thc63->oe));
+ return PTR_ERR(thc63->oe);
+ }
+
+ thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "powerdown",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(thc63->pdwn)) {
+ dev_err(thc63->dev, "Unable to get \"powerdown-gpios\": %ld\n",
+ PTR_ERR(thc63->pdwn));
+ return PTR_ERR(thc63->pdwn);
+ }
+
+ return 0;
+}
+
+static int thc63_probe(struct platform_device *pdev)
+{
+ struct thc63_dev *thc63;
+ int ret;
+
+ thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
+ if (!thc63)
+ return -ENOMEM;
+
+ thc63->dev = &pdev->dev;
+ platform_set_drvdata(pdev, thc63);
+
+ thc63->vcc = devm_regulator_get(thc63->dev, "vcc");
+ if (IS_ERR(thc63->vcc)) {
+ if (PTR_ERR(thc63->vcc) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ dev_err(thc63->dev, "Unable to get \"vcc\" supply: %ld\n",
+ PTR_ERR(thc63->vcc));
+ return PTR_ERR(thc63->vcc);
+ }
+
+ ret = thc63_gpio_init(thc63);
+ if (ret)
+ return ret;
+
+ ret = thc63_parse_dt(thc63);
+ if (ret)
+ return ret;
+
+ thc63->bridge.driver_private = thc63;
+ thc63->bridge.of_node = pdev->dev.of_node;
+ thc63->bridge.funcs = &thc63_bridge_func;
+
+ drm_bridge_add(&thc63->bridge);
+
+ return 0;
+}
+
+static int thc63_remove(struct platform_device *pdev)
+{
+ struct thc63_dev *thc63 = platform_get_drvdata(pdev);
+
+ drm_bridge_remove(&thc63->bridge);
+
+ return 0;
+}
+
+static const struct of_device_id thc63_match[] = {
+ { .compatible = "thine,thc63lvd1024", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, thc63_match);
+
+static struct platform_driver thc63_driver = {
+ .probe = thc63_probe,
+ .remove = thc63_remove,
+ .driver = {
+ .name = "thc63lvd1024",
+ .of_match_table = thc63_match,
+ },
+};
+module_platform_driver(thc63_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <[email protected]>");
+MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
Hi Jacopo,
Thank you for the patch.
On Friday, 6 April 2018 15:41:56 EEST Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> Reviewed-by: Andrzej Hajda <[email protected]>
> Reviewed-by: Niklas S?derlund <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 +++++++++++++++++++
> 1 file changed, 60 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 0000000..1191f17
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,60 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +-------------------------------------------
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
s/to two two/to two/
s/stream/streams/
> outputs.
> +
> +Single or dual operation modes, output data mapping and DDR output modes
> are
> +configured through input signals and the chip does not expose any control
> bus.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024"
> +
> +Optional properties:
> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> + PPL and digital circuitry
> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
As Rob mentioned in a reply to v6, we currently use "enable" as the inverse of
"powerdown". I would call this one oe-gpios instead. Quoting Rob:
"Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar with
h/w design should recognize OE."
> +
> +The THC63LVD1024 video port connections are modeled according
> +to OF graph bindings specified by Documentation/devicetree/bindings/
> graph.txt
> +
> +Required video port nodes:
> +- port@0: First LVDS input port
> +- port@2: First digital CMOS/TTL parallel output
> +
> +Optional video port nodes:
> +- port@1: Second LVDS input port
> +- port@3: Second digital CMOS/TTL parallel output
> +
> +Example:
> +--------
> +
> + thc63lvd1024: lvds-decoder {
> + compatible = "thine,thc63lvd1024";
> +
> + vcc-supply = <®_lvds_vcc>;
> + powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + lvds_dec_in_0: endpoint {
> + remote-endpoint = <&lvds_out>;
> + };
> + };
> +
> + port@2{
> + reg = <2>;
> +
> + lvds_dec_out_2: endpoint {
> + remote-endpoint = <&adv7511_in>;
> + };
> + };
> + };
> + };
--
Regards,
Laurent Pinchart
Hi Jacopo,
Thank you for the patch.
On Friday, 6 April 2018 15:41:57 EEST Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> Reviewed-by: Andrzej Hajda <[email protected]>
> Reviewed-by: Niklas S?derlund <[email protected]>
> ---
> drivers/gpu/drm/bridge/Kconfig | 6 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/thc63lvd1024.c | 212 +++++++++++++++++++++++++++++++
> 3 files changed, 219 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3aa65bd..42c9c2d 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -93,6 +93,12 @@ config DRM_SII9234
> It is an I2C driver, that detects connection of MHL bridge
> and starts encapsulation of HDMI signal.
>
> +config DRM_THINE_THC63LVD1024
> + tristate "Thine THC63LVD1024 LVDS decoder bridge"
> + depends on OF
> + ---help---
> + Thine THC63LVD1024 LVDS/parallel converter driver.
> +
> config DRM_TOSHIBA_TC358767
> tristate "Toshiba TC358767 eDP bridge"
> depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile
> b/drivers/gpu/drm/bridge/Makefile index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
> obj-$(CONFIG_DRM_SII902X) += sii902x.o
> obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
> obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
> obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c new file mode 100644
> index 0000000..c8fad9c
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <[email protected]>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
You should include slab.h as you're using devm_kzalloc().
> +
> +enum thc63_ports {
> + THC63_LVDS_IN0,
> + THC63_LVDS_IN1,
> + THC63_RGB_OUT0,
> + THC63_RGB_OUT1,
> +};
> +
> +struct thc63_dev {
> + struct device *dev;
> +
> + struct regulator *vcc;
> +
> + struct gpio_desc *pdwn;
pwdn ?
> + struct gpio_desc *oe;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + int ret;
> +
> + ret = regulator_enable(thc63->vcc);
> + if (ret) {
> + dev_err(thc63->dev,
> + "Failed to enable regulator \"vcc\": %d\n", ret);
> + return;
> + }
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 0);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 1);
You don't need to check the gpio_desc pointers first, gpiod_set_value() is a
no-op if the pointer is NULL.
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + int ret;
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 0);
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 1);
Same here.
> + ret = regulator_disable(thc63->vcc);
> + if (ret)
> + dev_err(thc63->dev,
> + "Failed to disable regulator \"vcc\": %d\n", ret);
> +}
> +
> +static const struct drm_bridge_funcs thc63_bridge_func = {
> + .attach = thc63_attach,
> + .enable = thc63_enable,
> + .disable = thc63_disable,
> +};
> +
> +static int thc63_parse_dt(struct thc63_dev *thc63)
> +{
> + struct device_node *thc63_out;
> + struct device_node *remote;
> +
> + thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> + THC63_RGB_OUT0, -1);
> + if (!thc63_out) {
> + dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> + THC63_RGB_OUT0);
> + return -ENODEV;
> + }
> +
> + remote = of_graph_get_remote_port_parent(thc63_out);
> + of_node_put(thc63_out);
> + if (!remote) {
> + dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
> + THC63_RGB_OUT0);
> + return -ENODEV;
> + }
> +
> + if (!of_device_is_available(remote)) {
> + dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
> + THC63_RGB_OUT0);
> + of_node_put(remote);
> + return -ENODEV;
> + }
> +
> + thc63->next = of_drm_find_bridge(remote);
> + if (!thc63->next) {
> + of_node_put(remote);
> + return -EPROBE_DEFER;
> + }
> +
> + return 0;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "enable", GPIOD_OUT_LOW);
If you agree with my comment regarding the DT bindings, this should be "oe".
> + if (IS_ERR(thc63->oe)) {
> + dev_err(thc63->dev, "Unable to get \"enable-gpios\": %ld\n",
> + PTR_ERR(thc63->oe));
And the message should be update here too.
With those small issues fixed,
Reviewed-by: Laurent Pinchart <[email protected]>
> + return PTR_ERR(thc63->oe);
> + }
> +
> + thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "powerdown",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(thc63->pdwn)) {
> + dev_err(thc63->dev, "Unable to get \"powerdown-gpios\": %ld\n",
> + PTR_ERR(thc63->pdwn));
> + return PTR_ERR(thc63->pdwn);
> + }
> +
> + return 0;
> +}
> +
> +static int thc63_probe(struct platform_device *pdev)
> +{
> + struct thc63_dev *thc63;
> + int ret;
> +
> + thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
> + if (!thc63)
> + return -ENOMEM;
> +
> + thc63->dev = &pdev->dev;
> + platform_set_drvdata(pdev, thc63);
> +
> + thc63->vcc = devm_regulator_get(thc63->dev, "vcc");
> + if (IS_ERR(thc63->vcc)) {
> + if (PTR_ERR(thc63->vcc) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + dev_err(thc63->dev, "Unable to get \"vcc\" supply: %ld\n",
> + PTR_ERR(thc63->vcc));
> + return PTR_ERR(thc63->vcc);
> + }
> +
> + ret = thc63_gpio_init(thc63);
> + if (ret)
> + return ret;
> +
> + ret = thc63_parse_dt(thc63);
> + if (ret)
> + return ret;
> +
> + thc63->bridge.driver_private = thc63;
> + thc63->bridge.of_node = pdev->dev.of_node;
> + thc63->bridge.funcs = &thc63_bridge_func;
> +
> + drm_bridge_add(&thc63->bridge);
> +
> + return 0;
> +}
> +
> +static int thc63_remove(struct platform_device *pdev)
> +{
> + struct thc63_dev *thc63 = platform_get_drvdata(pdev);
> +
> + drm_bridge_remove(&thc63->bridge);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id thc63_match[] = {
> + { .compatible = "thine,thc63lvd1024", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +
> +static struct platform_driver thc63_driver = {
> + .probe = thc63_probe,
> + .remove = thc63_remove,
> + .driver = {
> + .name = "thc63lvd1024",
> + .of_match_table = thc63_match,
> + },
> +};
> +module_platform_driver(thc63_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <[email protected]>");
> +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
> +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
Hi Laurent,
On Fri, Apr 06, 2018 at 04:15:35PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Friday, 6 April 2018 15:41:56 EEST Jacopo Mondi wrote:
> > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > Reviewed-by: Andrzej Hajda <[email protected]>
> > Reviewed-by: Niklas Söderlund <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > ---
> > .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 +++++++++++++++++++
> > 1 file changed, 60 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > new file mode 100644
> > index 0000000..1191f17
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -0,0 +1,60 @@
> > +Thine Electronics THC63LVD1024 LVDS decoder
> > +-------------------------------------------
> > +
> > +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> > streams
> > +to parallel data outputs. The chip supports single/dual input/output modes,
> > +handling up to two two input LVDS stream and up to two digital CMOS/TTL
>
> s/to two two/to two/
> s/stream/streams/
>
> > outputs.
> > +
> > +Single or dual operation modes, output data mapping and DDR output modes
> > are
> > +configured through input signals and the chip does not expose any control
> > bus.
> > +
> > +Required properties:
> > +- compatible: Shall be "thine,thc63lvd1024"
> > +
> > +Optional properties:
> > +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> > + PPL and digital circuitry
> > +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> > +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
>
> As Rob mentioned in a reply to v6, we currently use "enable" as the inverse of
> "powerdown". I would call this one oe-gpios instead. Quoting Rob:
>
> "Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar with
> h/w design should recognize OE."
>
I got a different understanding of what Rob meant. I thought "anyone
familiar with h/w design should recognize OE" as that nobody would get
confused if a pin named OE in the chip manual is descibed by an
'enable' property.
But as discussed offline, enable has probably to be used as the
opposite of powerdown for complete chip sleep, not just for output
pad.
Anyway, we spent enough time on naming issues, starting from my first
stupid 'pdwn' permutations then on this semi-standard names.
I'll send next version with 'powerdown-gpios' and 'oe-gpios'
properties hoping that would be finally accepted by everyone.
Same on the mandatory/optional VCC supply thing. Let's try to make
next version the final one. If the optional property with the dummy
regulator doesn't satisfy you and it is preferred to have a fixed-regulator
anyhow in DT I'll do in next version, othewise let's try not to change
it again. I'll just remark here that in the current Eagle design vcc is
connected to a power rail with no regulator at all :)
Thanks
j
> > +
> > +The THC63LVD1024 video port connections are modeled according
> > +to OF graph bindings specified by Documentation/devicetree/bindings/
> > graph.txt
> > +
> > +Required video port nodes:
> > +- port@0: First LVDS input port
> > +- port@2: First digital CMOS/TTL parallel output
> > +
> > +Optional video port nodes:
> > +- port@1: Second LVDS input port
> > +- port@3: Second digital CMOS/TTL parallel output
> > +
> > +Example:
> > +--------
> > +
> > + thc63lvd1024: lvds-decoder {
> > + compatible = "thine,thc63lvd1024";
> > +
> > + vcc-supply = <®_lvds_vcc>;
> > + powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + lvds_dec_in_0: endpoint {
> > + remote-endpoint = <&lvds_out>;
> > + };
> > + };
> > +
> > + port@2{
> > + reg = <2>;
> > +
> > + lvds_dec_out_2: endpoint {
> > + remote-endpoint = <&adv7511_in>;
> > + };
> > + };
> > + };
> > + };
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
Hi Jacopo,
(CC'ing Mark Brown)
On Friday, 6 April 2018 17:25:58 EEST jacopo mondi wrote:
> On Fri, Apr 06, 2018 at 04:15:35PM +0300, Laurent Pinchart wrote:
> > On Friday, 6 April 2018 15:41:56 EEST Jacopo Mondi wrote:
> >> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >>
> >> Signed-off-by: Jacopo Mondi <[email protected]>
> >> Reviewed-by: Andrzej Hajda <[email protected]>
> >> Reviewed-by: Niklas S?derlund <[email protected]>
> >> Reviewed-by: Laurent Pinchart <[email protected]>
> >> ---
> >>
> >> .../bindings/display/bridge/thine,thc63lvd1024.txt | 60
> >> +++++++++++++++++++
> >> 1 file changed, 60 insertions(+)
> >> create mode 100644
> >>
> >> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> new file mode 100644
> >> index 0000000..1191f17
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> @@ -0,0 +1,60 @@
> >> +Thine Electronics THC63LVD1024 LVDS decoder
> >> +-------------------------------------------
> >> +
> >> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> >> streams
> >> +to parallel data outputs. The chip supports single/dual input/output
> >> modes,
> >> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
> >
> > s/to two two/to two/
> > s/stream/streams/
> >
> >> outputs.
> >> +
> >> +Single or dual operation modes, output data mapping and DDR output
> >> modes are
> >> +configured through input signals and the chip does not expose any
> >> control bus.
> >> +
> >> +Required properties:
> >> +- compatible: Shall be "thine,thc63lvd1024"
> >> +
> >> +Optional properties:
> >> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS
> >> input,
> >> + PPL and digital circuitry
> >> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> >> +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
> >
> > As Rob mentioned in a reply to v6, we currently use "enable" as the
> > inverse of "powerdown". I would call this one oe-gpios instead. Quoting
> > Rob:
> >
> > "Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar
> > with h/w design should recognize OE."
>
> I got a different understanding of what Rob meant. I thought "anyone
> familiar with h/w design should recognize OE" as that nobody would get
> confused if a pin named OE in the chip manual is descibed by an
> 'enable' property.
>
> But as discussed offline, enable has probably to be used as the
> opposite of powerdown for complete chip sleep, not just for output
> pad.
>
> Anyway, we spent enough time on naming issues, starting from my first
> stupid 'pdwn' permutations then on this semi-standard names.
>
> I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> properties hoping that would be finally accepted by everyone.
I certainly won't complain (as long as you write pwdn instead of pdwn in the
driver :-)).
> Same on the mandatory/optional VCC supply thing. Let's try to make
> next version the final one. If the optional property with the dummy
> regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> anyhow in DT I'll do in next version, othewise let's try not to change
> it again. I'll just remark here that in the current Eagle design vcc is
> connected to a power rail with no regulator at all :)
I don't like the dummy regulator much, as it generates a dev_warn(), which
makes me believe that it's a hack rather than a proper solution. You might
want to ask Mark Brown for his opinion.
> >> +
> >> +The THC63LVD1024 video port connections are modeled according
> >> +to OF graph bindings specified by Documentation/devicetree/bindings/
> >> graph.txt
> >> +
> >> +Required video port nodes:
> >> +- port@0: First LVDS input port
> >> +- port@2: First digital CMOS/TTL parallel output
> >> +
> >> +Optional video port nodes:
> >> +- port@1: Second LVDS input port
> >> +- port@3: Second digital CMOS/TTL parallel output
> >> +
> >> +Example:
> >> +--------
> >> +
> >> + thc63lvd1024: lvds-decoder {
> >> + compatible = "thine,thc63lvd1024";
> >> +
> >> + vcc-supply = <®_lvds_vcc>;
> >> + powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> >> +
> >> + ports {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + port@0 {
> >> + reg = <0>;
> >> +
> >> + lvds_dec_in_0: endpoint {
> >> + remote-endpoint = <&lvds_out>;
> >> + };
> >> + };
> >> +
> >> + port@2{
> >> + reg = <2>;
> >> +
> >> + lvds_dec_out_2: endpoint {
> >> + remote-endpoint = <&adv7511_in>;
> >> + };
> >> + };
> >> + };
> >> + };
--
Regards,
Laurent Pinchart
Hi Laurent
On Fri, Apr 06, 2018 at 06:40:14PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> (CC'ing Mark Brown)
Hi Mark
[snip]
> >
> > Anyway, we spent enough time on naming issues, starting from my first
> > stupid 'pdwn' permutations then on this semi-standard names.
> >
> > I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> > properties hoping that would be finally accepted by everyone.
>
> I certainly won't complain (as long as you write pwdn instead of pdwn in the
> driver :-)).
>
Oh, so you won't complain if I address your comments? Thank you! :D
By the way, the dumb pdwn name comes, again, from the chip name. I can
change it to a saner name for sure...
> > Same on the mandatory/optional VCC supply thing. Let's try to make
> > next version the final one. If the optional property with the dummy
> > regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> > anyhow in DT I'll do in next version, othewise let's try not to change
> > it again. I'll just remark here that in the current Eagle design vcc is
> > connected to a power rail with no regulator at all :)
>
> I don't like the dummy regulator much, as it generates a dev_warn(), which
> makes me believe that it's a hack rather than a proper solution. You might
> want to ask Mark Brown for his opinion.
>
Sure: Hi Mark, a bit of context here to save you a long(er) reading.
Unsurprisingly, the chip for which I'm writing this small driver needs
a power supply to be properly functional :) In the development board
it is installed on, the power supply is connected to a power rail,
with no regulator. At the same time, some other designs may instead
include a regulator. I assume that's a very common situation. I
started by describing the regulator as optional in DT bindings, and
use regulator_get_optional(). If that function returns PTR_ERR, the
regulator is then ignored in driver's power management routines.
In this last version I thought I was acting smart and copied what happens
in other DRM bridges like adv7511, which use 'regulator_get()' and work
with a dummy if no regulator is provided in DT. Laurent has the above
doubts on using a dummy, and I actually share some of his concerns
(that dev_warn() is how I found out adv7511 was using a dummy, actually).
To sum up: when a regulator is described as optional in DT, do you
suggest to work with a dummy if it's not there, or use the _optional()
version of regulator_get() and manually set it to NULL and ignore it
in the enable/disable driver's routines?
Bonus question: Laurent likes to have the regulator described as required,
and thus require it to be described in DT also in cases where it is not
actually there using a 'fixed-regulator' and refuse to probe if the regulator
is not available. Do you encourage this approach, or prefer to have it
optional and handle it in the driver in one of the above described
ways?
Thank you
j
Hi Jacopo,
On Saturday, 7 April 2018 12:45:56 EEST jacopo mondi wrote:
> On Fri, Apr 06, 2018 at 06:40:14PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > (CC'ing Mark Brown)
>
> Hi Mark
>
> [snip]
>
> >> Anyway, we spent enough time on naming issues, starting from my first
> >> stupid 'pdwn' permutations then on this semi-standard names.
> >>
> >> I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> >> properties hoping that would be finally accepted by everyone.
> >
> > I certainly won't complain (as long as you write pwdn instead of pdwn in
> > the driver :-)).
>
> Oh, so you won't complain if I address your comments? Thank you! :D
> By the way, the dumb pdwn name comes, again, from the chip name. I can
> change it to a saner name for sure...
And I've just realized that, I thought it was a typo :-/ If it comes with the
datasheet I'm fine with either.
> >> Same on the mandatory/optional VCC supply thing. Let's try to make
> >> next version the final one. If the optional property with the dummy
> >> regulator doesn't satisfy you and it is preferred to have a
> >> fixed-regulator anyhow in DT I'll do in next version, othewise let's try
> >> not to change it again. I'll just remark here that in the current Eagle
> >> design vcc is connected to a power rail with no regulator at all :)
> >
> > I don't like the dummy regulator much, as it generates a dev_warn(), which
> > makes me believe that it's a hack rather than a proper solution. You might
> > want to ask Mark Brown for his opinion.
>
> Sure: Hi Mark, a bit of context here to save you a long(er) reading.
>
> Unsurprisingly, the chip for which I'm writing this small driver needs
> a power supply to be properly functional :) In the development board
> it is installed on, the power supply is connected to a power rail,
> with no regulator. At the same time, some other designs may instead
> include a regulator.
To be precise, with an always-on regulator that can't be software-controlled.
> I assume that's a very common situation. I started by describing the
> regulator as optional in DT bindings, and use regulator_get_optional(). If
> that function returns PTR_ERR, the regulator is then ignored in driver's
> power management routines.
>
> In this last version I thought I was acting smart and copied what happens
> in other DRM bridges like adv7511, which use 'regulator_get()' and work
> with a dummy if no regulator is provided in DT. Laurent has the above
> doubts on using a dummy, and I actually share some of his concerns
> (that dev_warn() is how I found out adv7511 was using a dummy, actually).
>
> To sum up: when a regulator is described as optional in DT, do you
> suggest to work with a dummy if it's not there, or use the _optional()
> version of regulator_get() and manually set it to NULL and ignore it
> in the enable/disable driver's routines?
>
> Bonus question: Laurent likes to have the regulator described as required,
> and thus require it to be described in DT also in cases where it is not
> actually there using a 'fixed-regulator' and refuse to probe if the
> regulator is not available. Do you encourage this approach, or prefer to
> have it optional and handle it in the driver in one of the above described
> ways?
--
Regards,
Laurent Pinchart
On Fri, Apr 06, 2018 at 06:40:14PM +0300, Laurent Pinchart wrote:
> On Friday, 6 April 2018 17:25:58 EEST jacopo mondi wrote:
> > Same on the mandatory/optional VCC supply thing. Let's try to make
> > next version the final one. If the optional property with the dummy
> > regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> > anyhow in DT I'll do in next version, othewise let's try not to change
> > it again. I'll just remark here that in the current Eagle design vcc is
> > connected to a power rail with no regulator at all :)
> I don't like the dummy regulator much, as it generates a dev_warn(), which
> makes me believe that it's a hack rather than a proper solution. You might
> want to ask Mark Brown for his opinion.
The DT should describe the physical supplies the device needs, if that's
a fixed regulator with no software control then the DT should have a
fixed regulator with no software control.