[PATCH v2 0/2] drm/bridge: sii902x: add optional power supplies

[PATCH v2 0/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

This patchset adds optional power supplies to the sii902x
drm bridge driver.

Version 2:
- merge avcc12 & cvcc12 to a single vcc12 supply as suggested by
  Laurent Pinchart (see discussion details in
  https://patchwork.freedesktop.org/patch/216058/)
- improve error messages following Laurent Pinchart comments.
- note about power consumption: as sii902x input IOs are not "io safe",
  it is important to enable/disable voltage regulators during
  probe/remove phases to avoid damages. Then, the only way to improve
  the power consumption is to add the sii902x standy mode. My actual
  platform is not an handheld device so power consumption is not
  critical. But in the future, it could be nice to add the standby
  mode, paying attention to wake-up events (hdmi cable plug)...

Version 1:
- Initial commit

Philippe Cornu (2):
  dt-bindings/display/bridge: sii902x: add optional power supplies
  drm/bridge: sii902x: add optional power supplies

 .../devicetree/bindings/display/bridge/sii902x.txt |  2 ++
 drivers/gpu/drm/bridge/sii902x.c                   | 38 +++++++++++++++++++---
 2 files changed, 36 insertions(+), 4 deletions(-)

-- 
2.15.1

[PATCH v2 0/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

This patchset adds optional power supplies to the sii902x
drm bridge driver.

Version 2:
- merge avcc12 & cvcc12 to a single vcc12 supply as suggested by
  Laurent Pinchart (see discussion details in
  https://patchwork.freedesktop.org/patch/216058/)
- improve error messages following Laurent Pinchart comments.
- note about power consumption: as sii902x input IOs are not "io safe",
  it is important to enable/disable voltage regulators during
  probe/remove phases to avoid damages. Then, the only way to improve
  the power consumption is to add the sii902x standy mode. My actual
  platform is not an handheld device so power consumption is not
  critical. But in the future, it could be nice to add the standby
  mode, paying attention to wake-up events (hdmi cable plug)...

Version 1:
- Initial commit

Philippe Cornu (2):
  dt-bindings/display/bridge: sii902x: add optional power supplies
  drm/bridge: sii902x: add optional power supplies

 .../devicetree/bindings/display/bridge/sii902x.txt |  2 ++
 drivers/gpu/drm/bridge/sii902x.c                   | 38 +++++++++++++++++++---
 2 files changed, 36 insertions(+), 4 deletions(-)

-- 
2.15.1

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

[PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

There is a single 1v2 supply voltage named vcc12 from which cvcc12
(digital core) and avcc12 (TMDS analog) are derived because according
to this data sheet:
"cvcc12 and avcc12 can be derived from the same power source"

Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
---
 Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index 56a3e68ccb80..9fb41fc9af51 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -8,6 +8,8 @@ Optional properties:
 	- interrupts-extended or interrupt-parent + interrupts: describe
 	  the interrupt line used to inform the host about hotplug events.
 	- reset-gpios: OF device-tree gpio specification for RST_N pin.
+	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
+	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
 
 Optional subnodes:
 	- video input: this subnode can contain a video input port node
-- 
2.15.1

[PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

There is a single 1v2 supply voltage named vcc12 from which cvcc12
(digital core) and avcc12 (TMDS analog) are derived because according
to this data sheet:
"cvcc12 and avcc12 can be derived from the same power source"

Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
---
 Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index 56a3e68ccb80..9fb41fc9af51 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -8,6 +8,8 @@ Optional properties:
 	- interrupts-extended or interrupt-parent + interrupts: describe
 	  the interrupt line used to inform the host about hotplug events.
 	- reset-gpios: OF device-tree gpio specification for RST_N pin.
+	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
+	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
 
 Optional subnodes:
 	- video input: this subnode can contain a video input port node
-- 
2.15.1

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

[PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

The sii902x input IOs are not "io safe" so it is important to
enable/disable voltage regulators during probe/remove phases to
avoid damages.

Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..c367d7b91ade 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -24,6 +24,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
@@ -86,6 +87,7 @@ struct sii902x {
 	struct drm_bridge bridge;
 	struct drm_connector connector;
 	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[2];
 };
 
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
@@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
 		return PTR_ERR(sii902x->reset_gpio);
 	}
 
+	sii902x->supplies[0].supply = "iovcc";
+	sii902x->supplies[1].supply = "vcc12";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
+				      sii902x->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to get power supplies: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
+				    sii902x->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
+		return ret;
+	}
+
+	usleep_range(10000, 20000);
+
 	sii902x_reset(sii902x);
 
 	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
 	if (ret)
-		return ret;
+		goto err_disable_regulator;
 
 	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
 			       &chipid, 4);
 	if (ret) {
 		dev_err(dev, "regmap_read failed %d\n", ret);
-		return ret;
+		goto err_disable_regulator;
 	}
 
 	if (chipid[0] != 0xb0) {
 		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
 			chipid[0]);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_disable_regulator;
 	}
 
 	/* Clear all pending interrupts */
@@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
 						IRQF_ONESHOT, dev_name(dev),
 						sii902x);
 		if (ret)
-			return ret;
+			goto err_disable_regulator;
 	}
 
 	sii902x->bridge.funcs = &sii902x_bridge_funcs;
@@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, sii902x);
 
 	return 0;
+
+err_disable_regulator:
+	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+			       sii902x->supplies);
+
+	return ret;
 }
 
 static int sii902x_remove(struct i2c_client *client)
@@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&sii902x->bridge);
 
+	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+			       sii902x->supplies);
+
 	return 0;
 }
 
-- 
2.15.1

[PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe Cornu]

Add the optional power supplies using the description found in
"SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".

The sii902x input IOs are not "io safe" so it is important to
enable/disable voltage regulators during probe/remove phases to
avoid damages.

Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..c367d7b91ade 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -24,6 +24,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
@@ -86,6 +87,7 @@ struct sii902x {
 	struct drm_bridge bridge;
 	struct drm_connector connector;
 	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[2];
 };
 
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
@@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
 		return PTR_ERR(sii902x->reset_gpio);
 	}
 
+	sii902x->supplies[0].supply = "iovcc";
+	sii902x->supplies[1].supply = "vcc12";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
+				      sii902x->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to get power supplies: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
+				    sii902x->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
+		return ret;
+	}
+
+	usleep_range(10000, 20000);
+
 	sii902x_reset(sii902x);
 
 	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
 	if (ret)
-		return ret;
+		goto err_disable_regulator;
 
 	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
 			       &chipid, 4);
 	if (ret) {
 		dev_err(dev, "regmap_read failed %d\n", ret);
-		return ret;
+		goto err_disable_regulator;
 	}
 
 	if (chipid[0] != 0xb0) {
 		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
 			chipid[0]);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_disable_regulator;
 	}
 
 	/* Clear all pending interrupts */
@@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
 						IRQF_ONESHOT, dev_name(dev),
 						sii902x);
 		if (ret)
-			return ret;
+			goto err_disable_regulator;
 	}
 
 	sii902x->bridge.funcs = &sii902x_bridge_funcs;
@@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, sii902x);
 
 	return 0;
+
+err_disable_regulator:
+	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+			       sii902x->supplies);
+
+	return ret;
 }
 
 static int sii902x_remove(struct i2c_client *client)
@@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&sii902x->bridge);
 
+	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
+			       sii902x->supplies);
+
 	return 0;
 }
 
-- 
2.15.1

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

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

Thank you for the patch.

On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> Add optional power supplies using the description found in
> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> 
> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> (digital core) and avcc12 (TMDS analog) are derived because according
> to this data sheet:
> "cvcc12 and avcc12 can be derived from the same power source"

Shouldn't the power supplies be mandatory, as explained by Mark in https://
lists.freedesktop.org/archives/dri-devel/2018-April/172400.html ?

> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>  Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
> 56a3e68ccb80..9fb41fc9af51 100644
> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> @@ -8,6 +8,8 @@ Optional properties:
>  	- interrupts-extended or interrupt-parent + interrupts: describe
>  	  the interrupt line used to inform the host about hotplug events.
>  	- reset-gpios: OF device-tree gpio specification for RST_N pin.
> +	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
> +	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
> 
>  Optional subnodes:
>  	- video input: this subnode can contain a video input port node

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

Thank you for the patch.

On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> Add optional power supplies using the description found in
> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> 
> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> (digital core) and avcc12 (TMDS analog) are derived because according
> to this data sheet:
> "cvcc12 and avcc12 can be derived from the same power source"

Shouldn't the power supplies be mandatory, as explained by Mark in https://
lists.freedesktop.org/archives/dri-devel/2018-April/172400.html ?

> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>  Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
> 56a3e68ccb80..9fb41fc9af51 100644
> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> @@ -8,6 +8,8 @@ Optional properties:
>  	- interrupts-extended or interrupt-parent + interrupts: describe
>  	  the interrupt line used to inform the host about hotplug events.
>  	- reset-gpios: OF device-tree gpio specification for RST_N pin.
> +	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
> +	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
> 
>  Optional subnodes:
>  	- video input: this subnode can contain a video input port node

-- 
Regards,

Laurent Pinchart



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

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent & Rob :-)

On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>> Add optional power supplies using the description found in
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>> (digital core) and avcc12 (TMDS analog) are derived because according
>> to this data sheet:
>> "cvcc12 and avcc12 can be derived from the same power source"
> 
> Shouldn't the power supplies be mandatory, as explained by Mark in https://
> lists.freedesktop.org/archives/dri-devel/2018-April/172400.html ?
> 

Laurent,
Many thanks Laurent for your comment, I understood the merge of the two 
1v2 power supplies but missed the "mandatory" part... maybe because this 
patch (with optional power supplies) already got the reviewed-by from 
Rob, I thought the discussion thread you pointed out was applicable 
"only" to totally new driver documentation.

So, on my side, as a "new user" of sii902x IC, no problem to put these 
power supplies as mandatory instead of optional properties but I would 
like to be sure this is applicable to both old and new bindings doc : )

Rob,
could you please confirm these power supply properties should be 
"mandatory"?
if yes, should we then modify other optional properties like the 
reset-gpios too in the future?

Big thanks to both of you,
Philippe :-)

>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>> 56a3e68ccb80..9fb41fc9af51 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> @@ -8,6 +8,8 @@ Optional properties:
>>   	- interrupts-extended or interrupt-parent + interrupts: describe
>>   	  the interrupt line used to inform the host about hotplug events.
>>   	- reset-gpios: OF device-tree gpio specification for RST_N pin.
>> +	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>> +	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
>>
>>   Optional subnodes:
>>   	- video input: this subnode can contain a video input port node
> 

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent & Rob :-)

On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> Hi Philippe,
> 
> Thank you for the patch.
> 
> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>> Add optional power supplies using the description found in
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>> (digital core) and avcc12 (TMDS analog) are derived because according
>> to this data sheet:
>> "cvcc12 and avcc12 can be derived from the same power source"
> 
> Shouldn't the power supplies be mandatory, as explained by Mark in https://
> lists.freedesktop.org/archives/dri-devel/2018-April/172400.html ?
> 

Laurent,
Many thanks Laurent for your comment, I understood the merge of the two 
1v2 power supplies but missed the "mandatory" part... maybe because this 
patch (with optional power supplies) already got the reviewed-by from 
Rob, I thought the discussion thread you pointed out was applicable 
"only" to totally new driver documentation.

So, on my side, as a "new user" of sii902x IC, no problem to put these 
power supplies as mandatory instead of optional properties but I would 
like to be sure this is applicable to both old and new bindings doc : )

Rob,
could you please confirm these power supply properties should be 
"mandatory"?
if yes, should we then modify other optional properties like the 
reset-gpios too in the future?

Big thanks to both of you,
Philippe :-)

>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
>> 56a3e68ccb80..9fb41fc9af51 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
>> @@ -8,6 +8,8 @@ Optional properties:
>>   	- interrupts-extended or interrupt-parent + interrupts: describe
>>   	  the interrupt line used to inform the host about hotplug events.
>>   	- reset-gpios: OF device-tree gpio specification for RST_N pin.
>> +	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
>> +	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
>>
>>   Optional subnodes:
>>   	- video input: this subnode can contain a video input port node
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> > On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> >> Add optional power supplies using the description found in
> >> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>
> >> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> >> (digital core) and avcc12 (TMDS analog) are derived because according
> >> to this data sheet:
> >> "cvcc12 and avcc12 can be derived from the same power source"
> > 
> > Shouldn't the power supplies be mandatory, as explained by Mark in
> > https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
> > ? 
> 
> Laurent,
> Many thanks Laurent for your comment, I understood the merge of the two 
> 1v2 power supplies but missed the "mandatory" part... maybe because this 
> patch (with optional power supplies) already got the reviewed-by from 
> Rob, I thought the discussion thread you pointed out was applicable 
> "only" to totally new driver documentation.
> 
> So, on my side, as a "new user" of sii902x IC, no problem to put these 
> power supplies as mandatory instead of optional properties but I would 
> like to be sure this is applicable to both old and new bindings doc : )

We obviously need to retain backward compatibility, so on the driver side you 
need to treat those power supplies as optional. From a DT bindings point of 
view, however, I think they should be mandatory for new DT.

> Rob,
> could you please confirm these power supply properties should be 
> "mandatory"?
> if yes, should we then modify other optional properties like the 
> reset-gpios too in the future?

The GPIOs properties are different in my opinion, as there's no requirement to 
connect for instance the reset pin to a GPIO controllable by the SoC. The pin 
could be hardwired to VCC, or connected to a system reset that is 
automatically managed without SoC intervention. The power supplies, however, 
are mandatory, in the sense that the chip will not work if you leave the power 
supplies unconnected.

> Big thanks to both of you,

You're welcome !

> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> >> ---
> >> 
> >>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
> >> 56a3e68ccb80..9fb41fc9af51 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >> @@ -8,6 +8,8 @@ Optional properties:
> >>   	- interrupts-extended or interrupt-parent + interrupts: describe
> >>   	  the interrupt line used to inform the host about hotplug events.
> >>   	- reset-gpios: OF device-tree gpio specification for RST_N pin.
> >> +	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
> >> +	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
> >>
> >>   Optional subnodes:
> >>   	- video input: this subnode can contain a video input port node

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> > On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> >> Add optional power supplies using the description found in
> >> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>
> >> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> >> (digital core) and avcc12 (TMDS analog) are derived because according
> >> to this data sheet:
> >> "cvcc12 and avcc12 can be derived from the same power source"
> > 
> > Shouldn't the power supplies be mandatory, as explained by Mark in
> > https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
> > ? 
> 
> Laurent,
> Many thanks Laurent for your comment, I understood the merge of the two 
> 1v2 power supplies but missed the "mandatory" part... maybe because this 
> patch (with optional power supplies) already got the reviewed-by from 
> Rob, I thought the discussion thread you pointed out was applicable 
> "only" to totally new driver documentation.
> 
> So, on my side, as a "new user" of sii902x IC, no problem to put these 
> power supplies as mandatory instead of optional properties but I would 
> like to be sure this is applicable to both old and new bindings doc : )

We obviously need to retain backward compatibility, so on the driver side you 
need to treat those power supplies as optional. From a DT bindings point of 
view, however, I think they should be mandatory for new DT.

> Rob,
> could you please confirm these power supply properties should be 
> "mandatory"?
> if yes, should we then modify other optional properties like the 
> reset-gpios too in the future?

The GPIOs properties are different in my opinion, as there's no requirement to 
connect for instance the reset pin to a GPIO controllable by the SoC. The pin 
could be hardwired to VCC, or connected to a system reset that is 
automatically managed without SoC intervention. The power supplies, however, 
are mandatory, in the sense that the chip will not work if you leave the power 
supplies unconnected.

> Big thanks to both of you,

You're welcome !

> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> >> ---
> >> 
> >>   Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >> b/Documentation/devicetree/bindings/display/bridge/sii902x.txt index
> >> 56a3e68ccb80..9fb41fc9af51 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >> @@ -8,6 +8,8 @@ Optional properties:
> >>   	- interrupts-extended or interrupt-parent + interrupts: describe
> >>   	  the interrupt line used to inform the host about hotplug events.
> >>   	- reset-gpios: OF device-tree gpio specification for RST_N pin.
> >> +	- iovcc-supply: I/O supply voltage (1.8V or 3.3V, host-dependent).
> >> +	- vcc12-supply: TMDS analog & digital core supply voltage (1.2V).
> >>
> >>   Optional subnodes:
> >>   	- video input: this subnode can contain a video input port node

-- 
Regards,

Laurent Pinchart



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

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Rob Herring]

On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
> Hi Philippe,
> 
> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> > On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> > > On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> > >> Add optional power supplies using the description found in
> > >> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> > >>
> > >> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> > >> (digital core) and avcc12 (TMDS analog) are derived because according
> > >> to this data sheet:
> > >> "cvcc12 and avcc12 can be derived from the same power source"
> > > 
> > > Shouldn't the power supplies be mandatory, as explained by Mark in
> > > https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
> > > ? 
> > 
> > Laurent,
> > Many thanks Laurent for your comment, I understood the merge of the two 
> > 1v2 power supplies but missed the "mandatory" part... maybe because this 
> > patch (with optional power supplies) already got the reviewed-by from 
> > Rob, I thought the discussion thread you pointed out was applicable 
> > "only" to totally new driver documentation.
> > 
> > So, on my side, as a "new user" of sii902x IC, no problem to put these 
> > power supplies as mandatory instead of optional properties but I would 
> > like to be sure this is applicable to both old and new bindings doc : )
> 
> We obviously need to retain backward compatibility, so on the driver side you 
> need to treat those power supplies as optional. From a DT bindings point of 
> view, however, I think they should be mandatory for new DT.

We don't really have a way to describe these 3 conditions (required for 
all, optional for all, and required for new). So generally we make 
additions optional. The exception sometimes is if we update all the dts 
files.

> > Rob,
> > could you please confirm these power supply properties should be 
> > "mandatory"?
> > if yes, should we then modify other optional properties like the 
> > reset-gpios too in the future?
> 
> The GPIOs properties are different in my opinion, as there's no requirement to 
> connect for instance the reset pin to a GPIO controllable by the SoC. The pin 
> could be hardwired to VCC, or connected to a system reset that is 
> automatically managed without SoC intervention. The power supplies, however, 
> are mandatory, in the sense that the chip will not work if you leave the power 
> supplies unconnected.

DT only needs to describe what matters to s/w. If a regulator is 
fixed and you don't need to know its voltage (or other read-only 
parameters), then there's not much point in putting it in DT. 

I'd probably base this more at a platform level and you either use 
regulator binding or you don't. It's perfectly valid that you want to do 
things like regulator setup, pin ctrl and muxing setup, etc. all in 
firmware and the OS doesn't touch any of that.

That's all a big can of worms which we shouldn't solve on this 2 line 
change. I think this change is fine as-is, so:

Reviewed-by: Rob Herring <robh@kernel.org>

Rob

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Rob Herring]

On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
> Hi Philippe,
> 
> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> > On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> > > On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> > >> Add optional power supplies using the description found in
> > >> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> > >>
> > >> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> > >> (digital core) and avcc12 (TMDS analog) are derived because according
> > >> to this data sheet:
> > >> "cvcc12 and avcc12 can be derived from the same power source"
> > > 
> > > Shouldn't the power supplies be mandatory, as explained by Mark in
> > > https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
> > > ? 
> > 
> > Laurent,
> > Many thanks Laurent for your comment, I understood the merge of the two 
> > 1v2 power supplies but missed the "mandatory" part... maybe because this 
> > patch (with optional power supplies) already got the reviewed-by from 
> > Rob, I thought the discussion thread you pointed out was applicable 
> > "only" to totally new driver documentation.
> > 
> > So, on my side, as a "new user" of sii902x IC, no problem to put these 
> > power supplies as mandatory instead of optional properties but I would 
> > like to be sure this is applicable to both old and new bindings doc : )
> 
> We obviously need to retain backward compatibility, so on the driver side you 
> need to treat those power supplies as optional. From a DT bindings point of 
> view, however, I think they should be mandatory for new DT.

We don't really have a way to describe these 3 conditions (required for 
all, optional for all, and required for new). So generally we make 
additions optional. The exception sometimes is if we update all the dts 
files.

> > Rob,
> > could you please confirm these power supply properties should be 
> > "mandatory"?
> > if yes, should we then modify other optional properties like the 
> > reset-gpios too in the future?
> 
> The GPIOs properties are different in my opinion, as there's no requirement to 
> connect for instance the reset pin to a GPIO controllable by the SoC. The pin 
> could be hardwired to VCC, or connected to a system reset that is 
> automatically managed without SoC intervention. The power supplies, however, 
> are mandatory, in the sense that the chip will not work if you leave the power 
> supplies unconnected.

DT only needs to describe what matters to s/w. If a regulator is 
fixed and you don't need to know its voltage (or other read-only 
parameters), then there's not much point in putting it in DT. 

I'd probably base this more at a platform level and you either use 
regulator binding or you don't. It's perfectly valid that you want to do 
things like regulator setup, pin ctrl and muxing setup, etc. all in 
firmware and the OS doesn't touch any of that.

That's all a big can of worms which we shouldn't solve on this 2 line 
change. I think this change is fine as-is, so:

Reviewed-by: Rob Herring <robh@kernel.org>

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

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Rob,

On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
> > On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> >> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> >>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> >>>> Add optional power supplies using the description found in
> >>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>>> 
> >>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> >>>> (digital core) and avcc12 (TMDS analog) are derived because according
> >>>> to this data sheet:
> >>>> "cvcc12 and avcc12 can be derived from the same power source"
> >>> 
> >>> Shouldn't the power supplies be mandatory, as explained by Mark in
> >>> https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
> >>> ?
> >> 
> >> Laurent,
> >> Many thanks Laurent for your comment, I understood the merge of the two
> >> 1v2 power supplies but missed the "mandatory" part... maybe because this
> >> patch (with optional power supplies) already got the reviewed-by from
> >> Rob, I thought the discussion thread you pointed out was applicable
> >> "only" to totally new driver documentation.
> >> 
> >> So, on my side, as a "new user" of sii902x IC, no problem to put these
> >> power supplies as mandatory instead of optional properties but I would
> >> like to be sure this is applicable to both old and new bindings doc : )
> > 
> > We obviously need to retain backward compatibility, so on the driver side
> > you need to treat those power supplies as optional. From a DT bindings
> > point of view, however, I think they should be mandatory for new DT.
> 
> We don't really have a way to describe these 3 conditions (required for
> all, optional for all, and required for new). So generally we make
> additions optional. The exception sometimes is if we update all the dts
> files.

Can't we just make it mandatory in the bindings, as long as we treat it as 
optional in drivers ?

> >> Rob,
> >> could you please confirm these power supply properties should be
> >> "mandatory"? if yes, should we then modify other optional properties like
> >> the reset-gpios too in the future?
> > 
> > The GPIOs properties are different in my opinion, as there's no
> > requirement to connect for instance the reset pin to a GPIO controllable
> > by the SoC. The pin could be hardwired to VCC, or connected to a system
> > reset that is automatically managed without SoC intervention. The power
> > supplies, however, are mandatory, in the sense that the chip will not work
> > if you leave the power supplies unconnected.
> 
> DT only needs to describe what matters to s/w. If a regulator is
> fixed and you don't need to know its voltage (or other read-only
> parameters), then there's not much point in putting it in DT.
> 
> I'd probably base this more at a platform level and you either use
> regulator binding or you don't. It's perfectly valid that you want to do
> things like regulator setup, pin ctrl and muxing setup, etc. all in
> firmware and the OS doesn't touch any of that.
> 
> That's all a big can of worms which we shouldn't solve on this 2 line
> change. I think this change is fine as-is, so:
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Rob,

On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
> > On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> >> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> >>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> >>>> Add optional power supplies using the description found in
> >>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>>> 
> >>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> >>>> (digital core) and avcc12 (TMDS analog) are derived because according
> >>>> to this data sheet:
> >>>> "cvcc12 and avcc12 can be derived from the same power source"
> >>> 
> >>> Shouldn't the power supplies be mandatory, as explained by Mark in
> >>> https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
> >>> ?
> >> 
> >> Laurent,
> >> Many thanks Laurent for your comment, I understood the merge of the two
> >> 1v2 power supplies but missed the "mandatory" part... maybe because this
> >> patch (with optional power supplies) already got the reviewed-by from
> >> Rob, I thought the discussion thread you pointed out was applicable
> >> "only" to totally new driver documentation.
> >> 
> >> So, on my side, as a "new user" of sii902x IC, no problem to put these
> >> power supplies as mandatory instead of optional properties but I would
> >> like to be sure this is applicable to both old and new bindings doc : )
> > 
> > We obviously need to retain backward compatibility, so on the driver side
> > you need to treat those power supplies as optional. From a DT bindings
> > point of view, however, I think they should be mandatory for new DT.
> 
> We don't really have a way to describe these 3 conditions (required for
> all, optional for all, and required for new). So generally we make
> additions optional. The exception sometimes is if we update all the dts
> files.

Can't we just make it mandatory in the bindings, as long as we treat it as 
optional in drivers ?

> >> Rob,
> >> could you please confirm these power supply properties should be
> >> "mandatory"? if yes, should we then modify other optional properties like
> >> the reset-gpios too in the future?
> > 
> > The GPIOs properties are different in my opinion, as there's no
> > requirement to connect for instance the reset pin to a GPIO controllable
> > by the SoC. The pin could be hardwired to VCC, or connected to a system
> > reset that is automatically managed without SoC intervention. The power
> > supplies, however, are mandatory, in the sense that the chip will not work
> > if you leave the power supplies unconnected.
> 
> DT only needs to describe what matters to s/w. If a regulator is
> fixed and you don't need to know its voltage (or other read-only
> parameters), then there's not much point in putting it in DT.
> 
> I'd probably base this more at a platform level and you either use
> regulator binding or you don't. It's perfectly valid that you want to do
> things like regulator setup, pin ctrl and muxing setup, etc. all in
> firmware and the OS doesn't touch any of that.
> 
> That's all a big can of worms which we shouldn't solve on this 2 line
> change. I think this change is fine as-is, so:
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

-- 
Regards,

Laurent Pinchart



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

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Rob & Laurent :)

On 04/26/2018 12:05 AM, Laurent Pinchart wrote:
> Hi Rob,
> 
> On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
>> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
>>> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
>>>> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
>>>>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>>>>>> Add optional power supplies using the description found in
>>>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>>>>
>>>>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>>>>>> (digital core) and avcc12 (TMDS analog) are derived because according
>>>>>> to this data sheet:
>>>>>> "cvcc12 and avcc12 can be derived from the same power source"
>>>>>
>>>>> Shouldn't the power supplies be mandatory, as explained by Mark in
>>>>> https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
>>>>> ?
>>>>
>>>> Laurent,
>>>> Many thanks Laurent for your comment, I understood the merge of the two
>>>> 1v2 power supplies but missed the "mandatory" part... maybe because this
>>>> patch (with optional power supplies) already got the reviewed-by from
>>>> Rob, I thought the discussion thread you pointed out was applicable
>>>> "only" to totally new driver documentation.
>>>>
>>>> So, on my side, as a "new user" of sii902x IC, no problem to put these
>>>> power supplies as mandatory instead of optional properties but I would
>>>> like to be sure this is applicable to both old and new bindings doc : )
>>>
>>> We obviously need to retain backward compatibility, so on the driver side
>>> you need to treat those power supplies as optional. From a DT bindings
>>> point of view, however, I think they should be mandatory for new DT.
>>
>> We don't really have a way to describe these 3 conditions (required for
>> all, optional for all, and required for new). So generally we make
>> additions optional. The exception sometimes is if we update all the dts
>> files.
> 
> Can't we just make it mandatory in the bindings, as long as we treat it as
> optional in drivers ?
> 

How to progress on this patch? Do you have any suggestions?

Many thanks for your help,
Philippe :-)

>>>> Rob,
>>>> could you please confirm these power supply properties should be
>>>> "mandatory"? if yes, should we then modify other optional properties like
>>>> the reset-gpios too in the future?
>>>
>>> The GPIOs properties are different in my opinion, as there's no
>>> requirement to connect for instance the reset pin to a GPIO controllable
>>> by the SoC. The pin could be hardwired to VCC, or connected to a system
>>> reset that is automatically managed without SoC intervention. The power
>>> supplies, however, are mandatory, in the sense that the chip will not work
>>> if you leave the power supplies unconnected.
>>
>> DT only needs to describe what matters to s/w. If a regulator is
>> fixed and you don't need to know its voltage (or other read-only
>> parameters), then there's not much point in putting it in DT.
>>
>> I'd probably base this more at a platform level and you either use
>> regulator binding or you don't. It's perfectly valid that you want to do
>> things like regulator setup, pin ctrl and muxing setup, etc. all in
>> firmware and the OS doesn't touch any of that.
>>
>> That's all a big can of worms which we shouldn't solve on this 2 line
>> change. I think this change is fine as-is, so:
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
> 

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Rob & Laurent :)

On 04/26/2018 12:05 AM, Laurent Pinchart wrote:
> Hi Rob,
> 
> On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
>> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
>>> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
>>>> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
>>>>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
>>>>>> Add optional power supplies using the description found in
>>>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>>>>
>>>>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
>>>>>> (digital core) and avcc12 (TMDS analog) are derived because according
>>>>>> to this data sheet:
>>>>>> "cvcc12 and avcc12 can be derived from the same power source"
>>>>>
>>>>> Shouldn't the power supplies be mandatory, as explained by Mark in
>>>>> https://lists.freedesktop.org/archives/dri-devel/2018-April/172400.html
>>>>> ?
>>>>
>>>> Laurent,
>>>> Many thanks Laurent for your comment, I understood the merge of the two
>>>> 1v2 power supplies but missed the "mandatory" part... maybe because this
>>>> patch (with optional power supplies) already got the reviewed-by from
>>>> Rob, I thought the discussion thread you pointed out was applicable
>>>> "only" to totally new driver documentation.
>>>>
>>>> So, on my side, as a "new user" of sii902x IC, no problem to put these
>>>> power supplies as mandatory instead of optional properties but I would
>>>> like to be sure this is applicable to both old and new bindings doc : )
>>>
>>> We obviously need to retain backward compatibility, so on the driver side
>>> you need to treat those power supplies as optional. From a DT bindings
>>> point of view, however, I think they should be mandatory for new DT.
>>
>> We don't really have a way to describe these 3 conditions (required for
>> all, optional for all, and required for new). So generally we make
>> additions optional. The exception sometimes is if we update all the dts
>> files.
> 
> Can't we just make it mandatory in the bindings, as long as we treat it as
> optional in drivers ?
> 

How to progress on this patch? Do you have any suggestions?

Many thanks for your help,
Philippe :-)

>>>> Rob,
>>>> could you please confirm these power supply properties should be
>>>> "mandatory"? if yes, should we then modify other optional properties like
>>>> the reset-gpios too in the future?
>>>
>>> The GPIOs properties are different in my opinion, as there's no
>>> requirement to connect for instance the reset pin to a GPIO controllable
>>> by the SoC. The pin could be hardwired to VCC, or connected to a system
>>> reset that is automatically managed without SoC intervention. The power
>>> supplies, however, are mandatory, in the sense that the chip will not work
>>> if you leave the power supplies unconnected.
>>
>> DT only needs to describe what matters to s/w. If a regulator is
>> fixed and you don't need to know its voltage (or other read-only
>> parameters), then there's not much point in putting it in DT.
>>
>> I'd probably base this more at a platform level and you either use
>> regulator binding or you don't. It's perfectly valid that you want to do
>> things like regulator setup, pin ctrl and muxing setup, etc. all in
>> firmware and the OS doesn't touch any of that.
>>
>> That's all a big can of worms which we shouldn't solve on this 2 line
>> change. I think this change is fine as-is, so:
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent, Archit, Andrzej & Yannick,

Do you have any comments on this v2 driver part?
(more details regarding v1/v2 differences in the cover letter 
https://www.spinics.net/lists/dri-devel/msg174137.html)

Thank you,
Philippe :-)


On 04/25/2018 09:53 AM, Philippe Cornu wrote:
> Add the optional power supplies using the description found in
> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> 
> The sii902x input IOs are not "io safe" so it is important to
> enable/disable voltage regulators during probe/remove phases to
> avoid damages.
> 
> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>   drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
>   1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 60373d7eb220..c367d7b91ade 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -24,6 +24,7 @@
>   #include <linux/i2c.h>
>   #include <linux/module.h>
>   #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>   
>   #include <drm/drmP.h>
>   #include <drm/drm_atomic_helper.h>
> @@ -86,6 +87,7 @@ struct sii902x {
>   	struct drm_bridge bridge;
>   	struct drm_connector connector;
>   	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[2];
>   };
>   
>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>   		return PTR_ERR(sii902x->reset_gpio);
>   	}
>   
> +	sii902x->supplies[0].supply = "iovcc";
> +	sii902x->supplies[1].supply = "vcc12";
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
> +				      sii902x->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to get power supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
> +				    sii902x->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(10000, 20000);
> +
>   	sii902x_reset(sii902x);
>   
>   	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>   	if (ret)
> -		return ret;
> +		goto err_disable_regulator;
>   
>   	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>   			       &chipid, 4);
>   	if (ret) {
>   		dev_err(dev, "regmap_read failed %d\n", ret);
> -		return ret;
> +		goto err_disable_regulator;
>   	}
>   
>   	if (chipid[0] != 0xb0) {
>   		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>   			chipid[0]);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_disable_regulator;
>   	}
>   
>   	/* Clear all pending interrupts */
> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>   						IRQF_ONESHOT, dev_name(dev),
>   						sii902x);
>   		if (ret)
> -			return ret;
> +			goto err_disable_regulator;
>   	}
>   
>   	sii902x->bridge.funcs = &sii902x_bridge_funcs;
> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>   	i2c_set_clientdata(client, sii902x);
>   
>   	return 0;
> +
> +err_disable_regulator:
> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +			       sii902x->supplies);
> +
> +	return ret;
>   }
>   
>   static int sii902x_remove(struct i2c_client *client)
> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>   
>   	drm_bridge_remove(&sii902x->bridge);
>   
> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +			       sii902x->supplies);
> +
>   	return 0;
>   }
>   
> 

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Laurent, Archit, Andrzej & Yannick,

Do you have any comments on this v2 driver part?
(more details regarding v1/v2 differences in the cover letter 
https://www.spinics.net/lists/dri-devel/msg174137.html)

Thank you,
Philippe :-)


On 04/25/2018 09:53 AM, Philippe Cornu wrote:
> Add the optional power supplies using the description found in
> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> 
> The sii902x input IOs are not "io safe" so it is important to
> enable/disable voltage regulators during probe/remove phases to
> avoid damages.
> 
> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> ---
>   drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
>   1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 60373d7eb220..c367d7b91ade 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -24,6 +24,7 @@
>   #include <linux/i2c.h>
>   #include <linux/module.h>
>   #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>   
>   #include <drm/drmP.h>
>   #include <drm/drm_atomic_helper.h>
> @@ -86,6 +87,7 @@ struct sii902x {
>   	struct drm_bridge bridge;
>   	struct drm_connector connector;
>   	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[2];
>   };
>   
>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>   		return PTR_ERR(sii902x->reset_gpio);
>   	}
>   
> +	sii902x->supplies[0].supply = "iovcc";
> +	sii902x->supplies[1].supply = "vcc12";
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
> +				      sii902x->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to get power supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
> +				    sii902x->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(10000, 20000);
> +
>   	sii902x_reset(sii902x);
>   
>   	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>   	if (ret)
> -		return ret;
> +		goto err_disable_regulator;
>   
>   	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>   			       &chipid, 4);
>   	if (ret) {
>   		dev_err(dev, "regmap_read failed %d\n", ret);
> -		return ret;
> +		goto err_disable_regulator;
>   	}
>   
>   	if (chipid[0] != 0xb0) {
>   		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>   			chipid[0]);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_disable_regulator;
>   	}
>   
>   	/* Clear all pending interrupts */
> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>   						IRQF_ONESHOT, dev_name(dev),
>   						sii902x);
>   		if (ret)
> -			return ret;
> +			goto err_disable_regulator;
>   	}
>   
>   	sii902x->bridge.funcs = &sii902x_bridge_funcs;
> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>   	i2c_set_clientdata(client, sii902x);
>   
>   	return 0;
> +
> +err_disable_regulator:
> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +			       sii902x->supplies);
> +
> +	return ret;
>   }
>   
>   static int sii902x_remove(struct i2c_client *client)
> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>   
>   	drm_bridge_remove(&sii902x->bridge);
>   
> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
> +			       sii902x->supplies);
> +
>   	return 0;
>   }
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Andrzej Hajda]

On 14.05.2018 11:38, Philippe CORNU wrote:
> Hi Laurent, Archit, Andrzej & Yannick,
>
> Do you have any comments on this v2 driver part?
> (more details regarding v1/v2 differences in the cover letter 
> https://www.spinics.net/lists/dri-devel/msg174137.html)
>
> Thank you,
> Philippe :-)
>
>
> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
>> Add the optional power supplies using the description found in
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> The sii902x input IOs are not "io safe" so it is important to
>> enable/disable voltage regulators during probe/remove phases to
>> avoid damages.

What exactly does it mean? Ie I understand that the chip has some
limitations, but why enabling/disabling regulators in probe/remove
should solve it?

Regards
Andrzej

>>
>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
>>   1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>> index 60373d7eb220..c367d7b91ade 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/i2c.h>
>>   #include <linux/module.h>
>>   #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>>   
>>   #include <drm/drmP.h>
>>   #include <drm/drm_atomic_helper.h>
>> @@ -86,6 +87,7 @@ struct sii902x {
>>   	struct drm_bridge bridge;
>>   	struct drm_connector connector;
>>   	struct gpio_desc *reset_gpio;
>> +	struct regulator_bulk_data supplies[2];
>>   };
>>   
>>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>>   		return PTR_ERR(sii902x->reset_gpio);
>>   	}
>>   
>> +	sii902x->supplies[0].supply = "iovcc";
>> +	sii902x->supplies[1].supply = "vcc12";
>> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>> +				      sii902x->supplies);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to get power supplies: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>> +				    sii902x->supplies);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usleep_range(10000, 20000);
>> +
>>   	sii902x_reset(sii902x);
>>   
>>   	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>   	if (ret)
>> -		return ret;
>> +		goto err_disable_regulator;
>>   
>>   	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>>   			       &chipid, 4);
>>   	if (ret) {
>>   		dev_err(dev, "regmap_read failed %d\n", ret);
>> -		return ret;
>> +		goto err_disable_regulator;
>>   	}
>>   
>>   	if (chipid[0] != 0xb0) {
>>   		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>   			chipid[0]);
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +		goto err_disable_regulator;
>>   	}
>>   
>>   	/* Clear all pending interrupts */
>> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>>   						IRQF_ONESHOT, dev_name(dev),
>>   						sii902x);
>>   		if (ret)
>> -			return ret;
>> +			goto err_disable_regulator;
>>   	}
>>   
>>   	sii902x->bridge.funcs = &sii902x_bridge_funcs;
>> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>>   	i2c_set_clientdata(client, sii902x);
>>   
>>   	return 0;
>> +
>> +err_disable_regulator:
>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +			       sii902x->supplies);
>> +
>> +	return ret;
>>   }
>>   
>>   static int sii902x_remove(struct i2c_client *client)
>> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>>   
>>   	drm_bridge_remove(&sii902x->bridge);
>>   
>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +			       sii902x->supplies);
>> +
>>   	return 0;
>>   }
>>   

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Andrzej Hajda]

On 14.05.2018 11:38, Philippe CORNU wrote:
> Hi Laurent, Archit, Andrzej & Yannick,
>
> Do you have any comments on this v2 driver part?
> (more details regarding v1/v2 differences in the cover letter 
> https://www.spinics.net/lists/dri-devel/msg174137.html)
>
> Thank you,
> Philippe :-)
>
>
> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
>> Add the optional power supplies using the description found in
>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>
>> The sii902x input IOs are not "io safe" so it is important to
>> enable/disable voltage regulators during probe/remove phases to
>> avoid damages.

What exactly does it mean? Ie I understand that the chip has some
limitations, but why enabling/disabling regulators in probe/remove
should solve it?

Regards
Andrzej

>>
>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
>>   1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>> index 60373d7eb220..c367d7b91ade 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/i2c.h>
>>   #include <linux/module.h>
>>   #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>>   
>>   #include <drm/drmP.h>
>>   #include <drm/drm_atomic_helper.h>
>> @@ -86,6 +87,7 @@ struct sii902x {
>>   	struct drm_bridge bridge;
>>   	struct drm_connector connector;
>>   	struct gpio_desc *reset_gpio;
>> +	struct regulator_bulk_data supplies[2];
>>   };
>>   
>>   static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>>   		return PTR_ERR(sii902x->reset_gpio);
>>   	}
>>   
>> +	sii902x->supplies[0].supply = "iovcc";
>> +	sii902x->supplies[1].supply = "vcc12";
>> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>> +				      sii902x->supplies);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to get power supplies: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>> +				    sii902x->supplies);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usleep_range(10000, 20000);
>> +
>>   	sii902x_reset(sii902x);
>>   
>>   	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>   	if (ret)
>> -		return ret;
>> +		goto err_disable_regulator;
>>   
>>   	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>>   			       &chipid, 4);
>>   	if (ret) {
>>   		dev_err(dev, "regmap_read failed %d\n", ret);
>> -		return ret;
>> +		goto err_disable_regulator;
>>   	}
>>   
>>   	if (chipid[0] != 0xb0) {
>>   		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>   			chipid[0]);
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +		goto err_disable_regulator;
>>   	}
>>   
>>   	/* Clear all pending interrupts */
>> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>>   						IRQF_ONESHOT, dev_name(dev),
>>   						sii902x);
>>   		if (ret)
>> -			return ret;
>> +			goto err_disable_regulator;
>>   	}
>>   
>>   	sii902x->bridge.funcs = &sii902x_bridge_funcs;
>> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>>   	i2c_set_clientdata(client, sii902x);
>>   
>>   	return 0;
>> +
>> +err_disable_regulator:
>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +			       sii902x->supplies);
>> +
>> +	return ret;
>>   }
>>   
>>   static int sii902x_remove(struct i2c_client *client)
>> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>>   
>>   	drm_bridge_remove(&sii902x->bridge);
>>   
>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>> +			       sii902x->supplies);
>> +
>>   	return 0;
>>   }
>>   


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

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

On Monday, 14 May 2018 12:22:16 EEST Philippe CORNU wrote:
> On 04/26/2018 12:05 AM, Laurent Pinchart wrote:
> > On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
> >> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
> >>> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> >>>> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> >>>>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> >>>>>> Add optional power supplies using the description found in
> >>>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>>>>>
> >>>>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> >>>>>> (digital core) and avcc12 (TMDS analog) are derived because
> >>>>>> according to this data sheet:
> >>>>>> "cvcc12 and avcc12 can be derived from the same power source"
> >>>>>
> >>>>> Shouldn't the power supplies be mandatory, as explained by Mark in
> >>>>> https://lists.freedesktop.org/archives/dri-devel/2018-April/
> >>>>> 172400.html ?
> >>>>
> >>>> Laurent,
> >>>> Many thanks Laurent for your comment, I understood the merge of the
> >>>> two 1v2 power supplies but missed the "mandatory" part... maybe because
> >>>> this patch (with optional power supplies) already got the reviewed-by
> >>>> from Rob, I thought the discussion thread you pointed out was
> >>>> applicable "only" to totally new driver documentation.
> >>>>
> >>>> So, on my side, as a "new user" of sii902x IC, no problem to put these
> >>>> power supplies as mandatory instead of optional properties but I would
> >>>> like to be sure this is applicable to both old and new bindings doc :
> >>>> )
> >>>
> >>> We obviously need to retain backward compatibility, so on the driver
> >>> side you need to treat those power supplies as optional. From a DT
> >>> bindings point of view, however, I think they should be mandatory for
> >>> new DT.
> >>
> >> We don't really have a way to describe these 3 conditions (required for
> >> all, optional for all, and required for new). So generally we make
> >> additions optional. The exception sometimes is if we update all the dts
> >> files.
> > 
> > Can't we just make it mandatory in the bindings, as long as we treat it
> > as optional in drivers ?
> 
> How to progress on this patch? Do you have any suggestions?

By seeing what Rob thinks about my proposal above ? :-)

> >>>> Rob,
> >>>> could you please confirm these power supply properties should be
> >>>> "mandatory"? if yes, should we then modify other optional properties
> >>>> like the reset-gpios too in the future?
> >>>
> >>> The GPIOs properties are different in my opinion, as there's no
> >>> requirement to connect for instance the reset pin to a GPIO controllable
> >>> by the SoC. The pin could be hardwired to VCC, or connected to a system
> >>> reset that is automatically managed without SoC intervention. The power
> >>> supplies, however, are mandatory, in the sense that the chip will not
> >>> work if you leave the power supplies unconnected.
> >>
> >> DT only needs to describe what matters to s/w. If a regulator is
> >> fixed and you don't need to know its voltage (or other read-only
> >> parameters), then there's not much point in putting it in DT.
> >>
> >> I'd probably base this more at a platform level and you either use
> >> regulator binding or you don't. It's perfectly valid that you want to do
> >> things like regulator setup, pin ctrl and muxing setup, etc. all in
> >> firmware and the OS doesn't touch any of that.
> >>
> >> That's all a big can of worms which we shouldn't solve on this 2 line
> >> change. I think this change is fine as-is, so:
> >>
> >> Reviewed-by: Rob Herring <robh@kernel.org>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] dt-bindings/display/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

On Monday, 14 May 2018 12:22:16 EEST Philippe CORNU wrote:
> On 04/26/2018 12:05 AM, Laurent Pinchart wrote:
> > On Wednesday, 25 April 2018 20:11:23 EEST Rob Herring wrote:
> >> On Wed, Apr 25, 2018 at 04:17:25PM +0300, Laurent Pinchart wrote:
> >>> On Wednesday, 25 April 2018 15:20:04 EEST Philippe CORNU wrote:
> >>>> On 04/25/2018 11:01 AM, Laurent Pinchart wrote:
> >>>>> On Wednesday, 25 April 2018 10:53:13 EEST Philippe Cornu wrote:
> >>>>>> Add optional power supplies using the description found in
> >>>>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>>>>>
> >>>>>> There is a single 1v2 supply voltage named vcc12 from which cvcc12
> >>>>>> (digital core) and avcc12 (TMDS analog) are derived because
> >>>>>> according to this data sheet:
> >>>>>> "cvcc12 and avcc12 can be derived from the same power source"
> >>>>>
> >>>>> Shouldn't the power supplies be mandatory, as explained by Mark in
> >>>>> https://lists.freedesktop.org/archives/dri-devel/2018-April/
> >>>>> 172400.html ?
> >>>>
> >>>> Laurent,
> >>>> Many thanks Laurent for your comment, I understood the merge of the
> >>>> two 1v2 power supplies but missed the "mandatory" part... maybe because
> >>>> this patch (with optional power supplies) already got the reviewed-by
> >>>> from Rob, I thought the discussion thread you pointed out was
> >>>> applicable "only" to totally new driver documentation.
> >>>>
> >>>> So, on my side, as a "new user" of sii902x IC, no problem to put these
> >>>> power supplies as mandatory instead of optional properties but I would
> >>>> like to be sure this is applicable to both old and new bindings doc :
> >>>> )
> >>>
> >>> We obviously need to retain backward compatibility, so on the driver
> >>> side you need to treat those power supplies as optional. From a DT
> >>> bindings point of view, however, I think they should be mandatory for
> >>> new DT.
> >>
> >> We don't really have a way to describe these 3 conditions (required for
> >> all, optional for all, and required for new). So generally we make
> >> additions optional. The exception sometimes is if we update all the dts
> >> files.
> > 
> > Can't we just make it mandatory in the bindings, as long as we treat it
> > as optional in drivers ?
> 
> How to progress on this patch? Do you have any suggestions?

By seeing what Rob thinks about my proposal above ? :-)

> >>>> Rob,
> >>>> could you please confirm these power supply properties should be
> >>>> "mandatory"? if yes, should we then modify other optional properties
> >>>> like the reset-gpios too in the future?
> >>>
> >>> The GPIOs properties are different in my opinion, as there's no
> >>> requirement to connect for instance the reset pin to a GPIO controllable
> >>> by the SoC. The pin could be hardwired to VCC, or connected to a system
> >>> reset that is automatically managed without SoC intervention. The power
> >>> supplies, however, are mandatory, in the sense that the chip will not
> >>> work if you leave the power supplies unconnected.
> >>
> >> DT only needs to describe what matters to s/w. If a regulator is
> >> fixed and you don't need to know its voltage (or other read-only
> >> parameters), then there's not much point in putting it in DT.
> >>
> >> I'd probably base this more at a platform level and you either use
> >> regulator binding or you don't. It's perfectly valid that you want to do
> >> things like regulator setup, pin ctrl and muxing setup, etc. all in
> >> firmware and the OS doesn't touch any of that.
> >>
> >> That's all a big can of worms which we shouldn't solve on this 2 line
> >> change. I think this change is fine as-is, so:
> >>
> >> Reviewed-by: Rob Herring <robh@kernel.org>

-- 
Regards,

Laurent Pinchart



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

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Andrzej,

On 05/14/2018 12:33 PM, Andrzej Hajda wrote:
> On 14.05.2018 11:38, Philippe CORNU wrote:
>> Hi Laurent, Archit, Andrzej & Yannick,
>>
>> Do you have any comments on this v2 driver part?
>> (more details regarding v1/v2 differences in the cover letter
>> https://www.spinics.net/lists/dri-devel/msg174137.html)
>>
>> Thank you,
>> Philippe :-)
>>
>>
>> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
>>> Add the optional power supplies using the description found in
>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>
>>> The sii902x input IOs are not "io safe" so it is important to
>>> enable/disable voltage regulators during probe/remove phases to
>>> avoid damages.
> 
> What exactly does it mean? Ie I understand that the chip has some
> limitations, but why enabling/disabling regulators in probe/remove
> should solve it?

thank you for your comment.

And sorry for the "bad" explanation in the 2nd paragraph about the fact 
that inputs are not "io safe". I added this 2nd paragraph in v2 
following a good comment from Laurent on adding the management of the 
regulators outside the probe/remove for a better power consumption 
management (enable/disable regulators only when the ic is used for 
displaying something for instance...). But after a deeper analysis, I 
realized that the only way to improve the power consumption is to 
implement & test the sii902x various sleep modes, that is out-of-scope 
of this small patch and also out-of-scope of my test board I use on 
which the sii902x bridge ic power consumption is very low compare to the 
rest of the system...

I will remove this "explanation" in v3 as it creates confusion.

Many thanks,
Philippe :-)

> 
> Regards
> Andrzej
> 
>>>
>>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>>> ---
>>>    drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
>>>    1 file changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>>> index 60373d7eb220..c367d7b91ade 100644
>>> --- a/drivers/gpu/drm/bridge/sii902x.c
>>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>>> @@ -24,6 +24,7 @@
>>>    #include <linux/i2c.h>
>>>    #include <linux/module.h>
>>>    #include <linux/regmap.h>
>>> +#include <linux/regulator/consumer.h>
>>>    
>>>    #include <drm/drmP.h>
>>>    #include <drm/drm_atomic_helper.h>
>>> @@ -86,6 +87,7 @@ struct sii902x {
>>>    	struct drm_bridge bridge;
>>>    	struct drm_connector connector;
>>>    	struct gpio_desc *reset_gpio;
>>> +	struct regulator_bulk_data supplies[2];
>>>    };
>>>    
>>>    static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>>> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>>>    		return PTR_ERR(sii902x->reset_gpio);
>>>    	}
>>>    
>>> +	sii902x->supplies[0].supply = "iovcc";
>>> +	sii902x->supplies[1].supply = "vcc12";
>>> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>>> +				      sii902x->supplies);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to get power supplies: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>>> +				    sii902x->supplies);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	usleep_range(10000, 20000);
>>> +
>>>    	sii902x_reset(sii902x);
>>>    
>>>    	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>>    	if (ret)
>>> -		return ret;
>>> +		goto err_disable_regulator;
>>>    
>>>    	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>>>    			       &chipid, 4);
>>>    	if (ret) {
>>>    		dev_err(dev, "regmap_read failed %d\n", ret);
>>> -		return ret;
>>> +		goto err_disable_regulator;
>>>    	}
>>>    
>>>    	if (chipid[0] != 0xb0) {
>>>    		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>>    			chipid[0]);
>>> -		return -EINVAL;
>>> +		ret = -EINVAL;
>>> +		goto err_disable_regulator;
>>>    	}
>>>    
>>>    	/* Clear all pending interrupts */
>>> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>>>    						IRQF_ONESHOT, dev_name(dev),
>>>    						sii902x);
>>>    		if (ret)
>>> -			return ret;
>>> +			goto err_disable_regulator;
>>>    	}
>>>    
>>>    	sii902x->bridge.funcs = &sii902x_bridge_funcs;
>>> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>>>    	i2c_set_clientdata(client, sii902x);
>>>    
>>>    	return 0;
>>> +
>>> +err_disable_regulator:
>>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>>> +			       sii902x->supplies);
>>> +
>>> +	return ret;
>>>    }
>>>    
>>>    static int sii902x_remove(struct i2c_client *client)
>>> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>>>    
>>>    	drm_bridge_remove(&sii902x->bridge);
>>>    
>>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>>> +			       sii902x->supplies);
>>> +
>>>    	return 0;
>>>    }
>>>    
> 
> 

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Philippe CORNU]

Hi Andrzej,

On 05/14/2018 12:33 PM, Andrzej Hajda wrote:
> On 14.05.2018 11:38, Philippe CORNU wrote:
>> Hi Laurent, Archit, Andrzej & Yannick,
>>
>> Do you have any comments on this v2 driver part?
>> (more details regarding v1/v2 differences in the cover letter
>> https://www.spinics.net/lists/dri-devel/msg174137.html)
>>
>> Thank you,
>> Philippe :-)
>>
>>
>> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
>>> Add the optional power supplies using the description found in
>>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
>>>
>>> The sii902x input IOs are not "io safe" so it is important to
>>> enable/disable voltage regulators during probe/remove phases to
>>> avoid damages.
> 
> What exactly does it mean? Ie I understand that the chip has some
> limitations, but why enabling/disabling regulators in probe/remove
> should solve it?

thank you for your comment.

And sorry for the "bad" explanation in the 2nd paragraph about the fact 
that inputs are not "io safe". I added this 2nd paragraph in v2 
following a good comment from Laurent on adding the management of the 
regulators outside the probe/remove for a better power consumption 
management (enable/disable regulators only when the ic is used for 
displaying something for instance...). But after a deeper analysis, I 
realized that the only way to improve the power consumption is to 
implement & test the sii902x various sleep modes, that is out-of-scope 
of this small patch and also out-of-scope of my test board I use on 
which the sii902x bridge ic power consumption is very low compare to the 
rest of the system...

I will remove this "explanation" in v3 as it creates confusion.

Many thanks,
Philippe :-)

> 
> Regards
> Andrzej
> 
>>>
>>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>>> ---
>>>    drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++++++----
>>>    1 file changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>>> index 60373d7eb220..c367d7b91ade 100644
>>> --- a/drivers/gpu/drm/bridge/sii902x.c
>>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>>> @@ -24,6 +24,7 @@
>>>    #include <linux/i2c.h>
>>>    #include <linux/module.h>
>>>    #include <linux/regmap.h>
>>> +#include <linux/regulator/consumer.h>
>>>    
>>>    #include <drm/drmP.h>
>>>    #include <drm/drm_atomic_helper.h>
>>> @@ -86,6 +87,7 @@ struct sii902x {
>>>    	struct drm_bridge bridge;
>>>    	struct drm_connector connector;
>>>    	struct gpio_desc *reset_gpio;
>>> +	struct regulator_bulk_data supplies[2];
>>>    };
>>>    
>>>    static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>>> @@ -392,23 +394,42 @@ static int sii902x_probe(struct i2c_client *client,
>>>    		return PTR_ERR(sii902x->reset_gpio);
>>>    	}
>>>    
>>> +	sii902x->supplies[0].supply = "iovcc";
>>> +	sii902x->supplies[1].supply = "vcc12";
>>> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(sii902x->supplies),
>>> +				      sii902x->supplies);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to get power supplies: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = regulator_bulk_enable(ARRAY_SIZE(sii902x->supplies),
>>> +				    sii902x->supplies);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to enable power supplies: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	usleep_range(10000, 20000);
>>> +
>>>    	sii902x_reset(sii902x);
>>>    
>>>    	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>>    	if (ret)
>>> -		return ret;
>>> +		goto err_disable_regulator;
>>>    
>>>    	ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
>>>    			       &chipid, 4);
>>>    	if (ret) {
>>>    		dev_err(dev, "regmap_read failed %d\n", ret);
>>> -		return ret;
>>> +		goto err_disable_regulator;
>>>    	}
>>>    
>>>    	if (chipid[0] != 0xb0) {
>>>    		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
>>>    			chipid[0]);
>>> -		return -EINVAL;
>>> +		ret = -EINVAL;
>>> +		goto err_disable_regulator;
>>>    	}
>>>    
>>>    	/* Clear all pending interrupts */
>>> @@ -424,7 +445,7 @@ static int sii902x_probe(struct i2c_client *client,
>>>    						IRQF_ONESHOT, dev_name(dev),
>>>    						sii902x);
>>>    		if (ret)
>>> -			return ret;
>>> +			goto err_disable_regulator;
>>>    	}
>>>    
>>>    	sii902x->bridge.funcs = &sii902x_bridge_funcs;
>>> @@ -434,6 +455,12 @@ static int sii902x_probe(struct i2c_client *client,
>>>    	i2c_set_clientdata(client, sii902x);
>>>    
>>>    	return 0;
>>> +
>>> +err_disable_regulator:
>>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>>> +			       sii902x->supplies);
>>> +
>>> +	return ret;
>>>    }
>>>    
>>>    static int sii902x_remove(struct i2c_client *client)
>>> @@ -443,6 +470,9 @@ static int sii902x_remove(struct i2c_client *client)
>>>    
>>>    	drm_bridge_remove(&sii902x->bridge);
>>>    
>>> +	regulator_bulk_disable(ARRAY_SIZE(sii902x->supplies),
>>> +			       sii902x->supplies);
>>> +
>>>    	return 0;
>>>    }
>>>    
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

On Monday, 14 May 2018 21:58:48 EEST Philippe CORNU wrote:
> On 05/14/2018 12:33 PM, Andrzej Hajda wrote:
> > On 14.05.2018 11:38, Philippe CORNU wrote:
> >> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
> >>> Add the optional power supplies using the description found in
> >>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>>
> >>> The sii902x input IOs are not "io safe" so it is important to
> >>> enable/disable voltage regulators during probe/remove phases to
> >>> avoid damages.
> > 
> > What exactly does it mean? Ie I understand that the chip has some
> > limitations, but why enabling/disabling regulators in probe/remove
> > should solve it?
> 
> thank you for your comment.
> 
> And sorry for the "bad" explanation in the 2nd paragraph about the fact 
> that inputs are not "io safe". I added this 2nd paragraph in v2 
> following a good comment from Laurent on adding the management of the 
> regulators outside the probe/remove for a better power consumption 
> management (enable/disable regulators only when the ic is used for 
> displaying something for instance...). But after a deeper analysis, I 
> realized that the only way to improve the power consumption is to 
> implement & test the sii902x various sleep modes, that is out-of-scope 
> of this small patch and also out-of-scope of my test board I use on 
> which the sii902x bridge ic power consumption is very low compare to the 
> rest of the system...
> 
> I will remove this "explanation" in v3 as it creates confusion.

I'd rather keep it and expand it explain why enabling/disabling regulators at 
probe/remove solves the problem. Your patch otherwise looks OK (although if 
you submit a v3 anyway you could also rename err_disable_regulator to 
err_disable_regulators).

> >>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> >>> ---
> >>> 
> >>> drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++----
> >>> 1 file changed, 34 insertions(+), 4 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

[Laurent Pinchart]

Hi Philippe,

On Monday, 14 May 2018 21:58:48 EEST Philippe CORNU wrote:
> On 05/14/2018 12:33 PM, Andrzej Hajda wrote:
> > On 14.05.2018 11:38, Philippe CORNU wrote:
> >> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
> >>> Add the optional power supplies using the description found in
> >>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>>
> >>> The sii902x input IOs are not "io safe" so it is important to
> >>> enable/disable voltage regulators during probe/remove phases to
> >>> avoid damages.
> > 
> > What exactly does it mean? Ie I understand that the chip has some
> > limitations, but why enabling/disabling regulators in probe/remove
> > should solve it?
> 
> thank you for your comment.
> 
> And sorry for the "bad" explanation in the 2nd paragraph about the fact 
> that inputs are not "io safe". I added this 2nd paragraph in v2 
> following a good comment from Laurent on adding the management of the 
> regulators outside the probe/remove for a better power consumption 
> management (enable/disable regulators only when the ic is used for 
> displaying something for instance...). But after a deeper analysis, I 
> realized that the only way to improve the power consumption is to 
> implement & test the sii902x various sleep modes, that is out-of-scope 
> of this small patch and also out-of-scope of my test board I use on 
> which the sii902x bridge ic power consumption is very low compare to the 
> rest of the system...
> 
> I will remove this "explanation" in v3 as it creates confusion.

I'd rather keep it and expand it explain why enabling/disabling regulators at 
probe/remove solves the problem. Your patch otherwise looks OK (although if 
you submit a v3 anyway you could also rename err_disable_regulator to 
err_disable_regulators).

> >>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> >>> ---
> >>> 
> >>> drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++----
> >>> 1 file changed, 34 insertions(+), 4 deletions(-)

-- 
Regards,

Laurent Pinchart



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