Changes in V4 of this set: * Rebased to next-20211118 (due to merge-conflict in linux-next) * Added Rob Herring's Ack on Patch 1 & 3 * Reworked patch 4 due to other changes in linux-next * Removed Sam Ravnborg's Reviewed-by for patch4 due to rework
Changes in V3 of this set: * Do not require vcc-supply in bindings, making it purely optional * Move regulator enable/disable to sn65dsi83_atomic_pre_enable and sn65dsi83_atomic_disable
Changes in V2 of this set: * Add patch from Laurent for fixing the binding regarding optional GPIO * Reorder patches so bindings are changed beforehand * Add small fixes from Sam's review
Alexander Stein (3): drm/bridge: ti-sn65dsi83: Make enable GPIO optional dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
Laurent Pinchart (1): dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional
.../bindings/display/bridge/ti,sn65dsi83.yaml | 5 ++++- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 22 ++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-)
From: Laurent Pinchart laurent.pinchart@ideasonboard.com
The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Rob Herring robh@kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 1 - 1 file changed, 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index b446d0f0f1b4..c3f3e73f740a 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -91,7 +91,6 @@ properties: required: - compatible - reg - - enable-gpios - ports
allOf:
On Thu, Nov 18, 2021 at 2:50 PM Alexander Stein alexander.stein@ew.tq-group.com wrote:
From: Laurent Pinchart laurent.pinchart@ideasonboard.com
The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional.
Sorry, I couldn't understand what it means. Does it mean VCC enabled designs no need to enable GPIO? I've a design that do support both EN and VCC.
Jagan.
Am Donnerstag, dem 09.12.2021 um 12:37 +0530 schrieb Jagan Teki:
On Thu, Nov 18, 2021 at 2:50 PM Alexander Stein < alexander.stein@ew.tq-group.com
wrote: From: Laurent Pinchart < laurent.pinchart@ideasonboard.com
The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional.
Sorry, I couldn't understand what it means. Does it mean VCC enabled designs no need to enable GPIO? I've a design that do support both EN and VCC.
The patches 1 & 2 are only about the "enable" gpio for the bridge, it's unrelated to the VCC regulator introduced in patch 3 & 4. Maybe the commit message should say:
The SN65DSI8x EN signal may be hard-wired to VCC, or otherwise
controlled[...] But I copied the message from bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make enable GPIO optional").
This is for use-cases where there is no GPIO the kernel can use, to control the EN pad of the bridge. Thus make this gpio optional in bindings and driver.
HTH Alexander
The enable signal may not be controllable by the kernel. Make it optional. This is a similar to commit bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make enable GPIO optional")
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Sam Ravnborg sam@ravnborg.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 945f08de45f1..065610edc37a 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -662,7 +662,8 @@ static int sn65dsi83_probe(struct i2c_client *client, }
/* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */ - ctx->enable_gpio = devm_gpiod_get(ctx->dev, "enable", GPIOD_OUT_LOW); + ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable", + GPIOD_OUT_LOW); if (IS_ERR(ctx->enable_gpio)) return PTR_ERR(ctx->enable_gpio);
Add a VCC regulator which needs to be enabled before the EN pin is released.
Reviewed-by: Sam Ravnborg sam@ravnborg.org Acked-by: Rob Herring robh@kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index c3f3e73f740a..48a97bb3e2e0 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -32,6 +32,9 @@ properties: maxItems: 1 description: GPIO specifier for bridge_en pin (active high).
+ vcc-supply: + description: A 1.8V power supply (see regulator/regulator.yaml). + ports: $ref: /schemas/graph.yaml#/properties/ports
@@ -132,6 +135,7 @@ examples: reg = <0x2d>;
enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>; + vcc-supply = <®_sn65dsi83_1v8>;
ports { #address-cells = <1>;
On Thu, Nov 18, 2021 at 2:50 PM Alexander Stein alexander.stein@ew.tq-group.com wrote:
Add a VCC regulator which needs to be enabled before the EN pin is released.
Reviewed-by: Sam Ravnborg sam@ravnborg.org Acked-by: Rob Herring robh@kernel.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
.../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index c3f3e73f740a..48a97bb3e2e0 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -32,6 +32,9 @@ properties: maxItems: 1 description: GPIO specifier for bridge_en pin (active high).
- vcc-supply:
- description: A 1.8V power supply (see regulator/regulator.yaml).
Reviewed-by: Jagan Teki jagan@amarulasolutions.com
VCC needs to be enabled before releasing the enable GPIO.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 065610edc37a..54d18e82ed74 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -33,6 +33,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h>
#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@ -143,6 +144,7 @@ struct sn65dsi83 { struct mipi_dsi_device *dsi; struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio; + struct regulator *vcc; int dsi_lanes; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap; @@ -337,6 +339,12 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, u16 val; int ret;
+ ret = regulator_enable(ctx->vcc); + if (ret) { + dev_err(ctx->dev, "Failed to enable vcc\n"); + return; + } + /* Deassert reset */ gpiod_set_value(ctx->enable_gpio, 1); usleep_range(1000, 1100); @@ -486,11 +494,16 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge); + int ret;
/* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */ gpiod_set_value(ctx->enable_gpio, 0); usleep_range(10000, 11000);
+ ret = regulator_disable(ctx->vcc); + if (ret) + dev_err(ctx->dev, "Failed to disable vcc: %i\n", ret); + regcache_mark_dirty(ctx->regmap); }
@@ -599,6 +612,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
ctx->panel_bridge = panel_bridge;
+ ctx->vcc = devm_regulator_get(dev, "vcc"); + if (IS_ERR(ctx->vcc)) + return dev_err_probe(dev, PTR_ERR(ctx->vcc), + "Failed to get supply 'vcc': %pe\n", + ERR_PTR(ret)); + return 0; }
On Thu, Nov 18, 2021 at 2:50 PM Alexander Stein alexander.stein@ew.tq-group.com wrote:
VCC needs to be enabled before releasing the enable GPIO.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 065610edc37a..54d18e82ed74 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -33,6 +33,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h>
#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@ -143,6 +144,7 @@ struct sn65dsi83 { struct mipi_dsi_device *dsi; struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio;
struct regulator *vcc; int dsi_lanes; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap;@@ -337,6 +339,12 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, u16 val; int ret;
ret = regulator_enable(ctx->vcc);if (ret) {dev_err(ctx->dev, "Failed to enable vcc\n");return;}
Better check the vcc and enable it since it is an optional one.
Jagan.
Hi Jagan,
On Thu, Dec 09, 2021 at 12:34:49PM +0530, Jagan Teki wrote:
On Thu, Nov 18, 2021 at 2:50 PM Alexander Stein wrote:
VCC needs to be enabled before releasing the enable GPIO.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 065610edc37a..54d18e82ed74 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -33,6 +33,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h>
#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@ -143,6 +144,7 @@ struct sn65dsi83 { struct mipi_dsi_device *dsi; struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio;
struct regulator *vcc; int dsi_lanes; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap;@@ -337,6 +339,12 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, u16 val; int ret;
ret = regulator_enable(ctx->vcc);if (ret) {dev_err(ctx->dev, "Failed to enable vcc\n");return;}Better check the vcc and enable it since it is an optional one.
Won't the regulator core create a dummy regulator if none is specified in DT ?
Hi Laurent,
On Thu, Dec 9, 2021 at 10:09 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Jagan,
On Thu, Dec 09, 2021 at 12:34:49PM +0530, Jagan Teki wrote:
On Thu, Nov 18, 2021 at 2:50 PM Alexander Stein wrote:
VCC needs to be enabled before releasing the enable GPIO.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 065610edc37a..54d18e82ed74 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -33,6 +33,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h>
#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@ -143,6 +144,7 @@ struct sn65dsi83 { struct mipi_dsi_device *dsi; struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio;
struct regulator *vcc; int dsi_lanes; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap;@@ -337,6 +339,12 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, u16 val; int ret;
ret = regulator_enable(ctx->vcc);if (ret) {dev_err(ctx->dev, "Failed to enable vcc\n");return;}Better check the vcc and enable it since it is an optional one.
Won't the regulator core create a dummy regulator if none is specified in DT ?
Agreed, thanks (Usually I do check to avoid NULL pointer if any).
Jagan.
Hi Alexander,
Thank you for the patch.
On Thu, Nov 18, 2021 at 10:19:55AM +0100, Alexander Stein wrote:
VCC needs to be enabled before releasing the enable GPIO.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 065610edc37a..54d18e82ed74 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -33,6 +33,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h>
#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@ -143,6 +144,7 @@ struct sn65dsi83 { struct mipi_dsi_device *dsi; struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio;
- struct regulator *vcc; int dsi_lanes; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap;
@@ -337,6 +339,12 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, u16 val; int ret;
- ret = regulator_enable(ctx->vcc);
- if (ret) {
dev_err(ctx->dev, "Failed to enable vcc\n");
I'd print the error code here as you do so in sn65dsi83_atomic_disable().
return;- }
- /* Deassert reset */ gpiod_set_value(ctx->enable_gpio, 1); usleep_range(1000, 1100);
@@ -486,11 +494,16 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
int ret;
/* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */ gpiod_set_value(ctx->enable_gpio, 0); usleep_range(10000, 11000);
ret = regulator_disable(ctx->vcc);
if (ret)
dev_err(ctx->dev, "Failed to disable vcc: %i\n", ret);
I wish printf didn't have identical %i and %d specifiers :-)
- regcache_mark_dirty(ctx->regmap);
}
@@ -599,6 +612,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
ctx->panel_bridge = panel_bridge;
- ctx->vcc = devm_regulator_get(dev, "vcc");
- if (IS_ERR(ctx->vcc))
return dev_err_probe(dev, PTR_ERR(ctx->vcc),"Failed to get supply 'vcc': %pe\n",ERR_PTR(ret));
This doesn't seem right, ret doesn't contain any useful error code at this point.
return dev_err_probe(dev, PTR_ERR(ctx->vcc), "Failed to get supply 'vcc'\n");
should be enough, as dev_err_probe() adds the error to the message internally.
With those small fixes,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- return 0;
}
dri-devel@lists.freedesktop.org
-
Alexander Stein -
Jagan Teki -
Laurent Pinchart