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 | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index a32f70bc68ea..5fab0fabcd15 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; @@ -647,6 +649,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; }
@@ -690,7 +698,11 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
- return 0; + ret = regulator_enable(ctx->vcc); + if (ret) + dev_err(dev, "Failed to enable vcc\n"); + + return ret; }
static int sn65dsi83_remove(struct i2c_client *client) @@ -701,6 +713,7 @@ static int sn65dsi83_remove(struct i2c_client *client) mipi_dsi_device_unregister(ctx->dsi); drm_bridge_remove(&ctx->bridge); of_node_put(ctx->host_node); + regulator_disable(ctx->vcc);
return 0; }
Add a VCC regulator which needs to be enabled before the EN pin is released.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index 07b20383cbca..149cff3233c2 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
@@ -94,6 +97,7 @@ required: - compatible - reg - enable-gpios + - vcc-supply - ports
allOf: @@ -135,6 +139,7 @@ examples: reg = <0x2d>;
enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>; + vcc-supply = <®_sn65dsi83_1v8>;
ports { #address-cells = <1>;
On Wed, Oct 6, 2021 at 2:47 AM Alexander Stein alexander.stein@ew.tq-group.com wrote:
Add a VCC regulator which needs to be enabled before the EN pin is released.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
.../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index 07b20383cbca..149cff3233c2 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
@@ -94,6 +97,7 @@ required:
- compatible
- reg
- enable-gpios
- vcc-supply
You generally can't make added properties required unless all DT files already had this property. It breaks compatibility.
Rob
On Wed, Oct 06, 2021 at 09:47:12AM +0200, Alexander Stein wrote:
Add a VCC regulator which needs to be enabled before the EN pin is released.
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
Looks good, Reviewed-by: Sam Ravnborg sam@ravnborg.org
When you resend please put bindings patches first, we should not commit code changes that uses undocumented bindings. On principle.
Sam
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")
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 5fab0fabcd15..101da29ba698 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -679,7 +679,7 @@ static int sn65dsi83_probe(struct i2c_client *client, model = id->driver_data; }
- 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);
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 --- .../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 07b20383cbca..a5779bf17849 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -93,7 +93,6 @@ properties: required: - compatible - reg - - enable-gpios - ports
allOf:
base-commit: 1e3944578b749449bd7fa6bf0bae4c3d3f5f1733
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
.../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 07b20383cbca..a5779bf17849 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -93,7 +93,6 @@ properties: required:
- compatible
- reg
- enable-gpios
- ports
allOf:
base-commit: 1e3944578b749449bd7fa6bf0bae4c3d3f5f1733
Reviewed-by: Alexander Stein alexander.stein@ew.tq-group.com
Best regards, Alexander
Hi Alexander,
Thank you for the patch.
On Wed, Oct 06, 2021 at 09:47:13AM +0200, Alexander Stein wrote:
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")
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 5fab0fabcd15..101da29ba698 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -679,7 +679,7 @@ static int sn65dsi83_probe(struct i2c_client *client, model = id->driver_data; }
- 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);
You can wrap this line as
ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable", GPIOD_OUT_LOW);
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Corresponding DT bindings changes are needed, I've sent a patch in this mail thread.
if (IS_ERR(ctx->enable_gpio)) return PTR_ERR(ctx->enable_gpio);
On Wed, Oct 06, 2021 at 12:18:02PM +0300, Laurent Pinchart wrote:
Hi Alexander,
Thank you for the patch.
One more thing, the subject line has a typo, it should read ti-sn65dsi83.
On Wed, Oct 06, 2021 at 09:47:13AM +0200, Alexander Stein wrote:
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")
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 5fab0fabcd15..101da29ba698 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -679,7 +679,7 @@ static int sn65dsi83_probe(struct i2c_client *client, model = id->driver_data; }
- 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);
You can wrap this line as
ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable", GPIOD_OUT_LOW);
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Corresponding DT bindings changes are needed, I've sent a patch in this mail thread.
if (IS_ERR(ctx->enable_gpio)) return PTR_ERR(ctx->enable_gpio);
Hi Alexander,
On Wed, Oct 06, 2021 at 09:47:13AM +0200, Alexander Stein wrote:
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")
Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com
Looks good. Reviewed-by: Sam Ravnborg sam@ravnborg.org Please include the dt-bindings patch from Laurent *before* this patch when you repost.
Sam
Hi Alexander, On Wed, Oct 06, 2021 at 09:47:11AM +0200, 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 | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index a32f70bc68ea..5fab0fabcd15 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;
@@ -647,6 +649,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));
The use of ERR_PTR(ret) is wrong here as ret do not include the return result. Just use ctx->vcc instead as this is what %pe expects.
- return 0;
}
@@ -690,7 +698,11 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
- return 0;
- ret = regulator_enable(ctx->vcc);
- if (ret)
dev_err(dev, "Failed to enable vcc\n");
Print the return code too?
With the two things fixed you can add my: Reviewed-by: Sam Ravnborg sam@ravnborg.org
- return ret;
}
static int sn65dsi83_remove(struct i2c_client *client) @@ -701,6 +713,7 @@ static int sn65dsi83_remove(struct i2c_client *client) mipi_dsi_device_unregister(ctx->dsi); drm_bridge_remove(&ctx->bridge); of_node_put(ctx->host_node);
regulator_disable(ctx->vcc);
return 0;
}
2.25.1
Hi Alexander,
On Wed, Oct 06, 2021 at 09:47:11AM +0200, 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 | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index a32f70bc68ea..5fab0fabcd15 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;
@@ -647,6 +649,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");
In the binding the vcc regulator is required, but devm_regulator_get() will create a dummy regulator if not found. Maybe this is on purpose and all is good.
Sam
dri-devel@lists.freedesktop.org
-
Alexander Stein -
Alexander Stein
-
Laurent Pinchart -
Laurent Pinchart -
Rob Herring -
Sam Ravnborg