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(-)
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
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
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
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
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
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
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
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
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; }
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; }
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; }
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; }
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(-)
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Laurent Pinchart -
Philippe Cornu -
Philippe CORNU
-
Rob Herring