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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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 <[email protected]>
> >> ---
> >>
> >> 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
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 <[email protected]>
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 <[email protected]>
--
Regards,
Laurent Pinchart
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 <[email protected]>
>
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 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 <[email protected]>
--
Regards,
Laurent Pinchart
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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
> >>> ---
> >>>
> >>> drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++----
> >>> 1 file changed, 34 insertions(+), 4 deletions(-)
--
Regards,
Laurent Pinchart