diff mbox series

[1/2] drm/bridge: sii902x: Enable I/O and core VCC supplies if present

Message ID 20200924200507.1175888-1-mr.nuke.me@gmail.com
State New, archived
Headers show
Series [1/2] drm/bridge: sii902x: Enable I/O and core VCC supplies if present | expand

Commit Message

Alexandru Gagniuc Sept. 24, 2020, 8:05 p.m. UTC
On the SII9022, the IOVCC and CVCC12 supplies must reach the correct
voltage before the reset sequence is initiated. On most boards, this
assumption is true at boot-up, so initialization succeeds.

However, when we try to initialize the chip with incorrect supply
voltages, it will not respond to I2C requests. sii902x_probe() fails
with -ENXIO.

To resolve this, look for the "iovcc" and "cvcc12" regulators, and
make sure they are enabled before starting the reset sequence. If
these supplies are not available in devicetree, then they will default
to dummy-regulator. In that case everything will work like before.

This was observed on a STM32MP157C-DK2 booting in u-boot falcon mode.
On this board, the supplies would be set by the second stage
bootloader, which does not run in falcon mode.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 54 ++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 6 deletions(-)

Comments

Alexandru Gagniuc Sept. 24, 2020, 8:34 p.m. UTC | #1
On 9/24/20 3:22 PM, Fabio Estevam wrote:
Hi Fabio,

> On Thu, Sep 24, 2020 at 5:16 PM Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> 
>> +       ret = regulator_enable(sii902x->cvcc12);
>> +       if (ret < 0) {
>> +               dev_err(dev, "Failed to enable cvcc12 supply: %d\n", ret);
>> +               regulator_disable(sii902x->iovcc);
>> +               return PTR_ERR(sii902x->cvcc12);
> 
> return ret;

Thank you for catching that. I will fix it in v2.

>>
>>          ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>> @@ -1012,11 +1052,11 @@ static int sii902x_probe(struct i2c_client *client,
>>          regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
>>          regmap_write(sii902x->regmap, SII902X_INT_STATUS, status);
>>
>> -       if (client->irq > 0) {
>> +       if (sii902x->i2c->irq > 0) {
> 
> Unrelated change.
[snip]
>   Unrelated change.
[snip]
> Unrelated change.
[snip]
> Unrelated change.

The i2c initialization is moved into a separate function. Hence 'client' 
is no longer available. Instead, it can be accessed via 'sii902x->i2c'.

Alex
Dan Carpenter Sept. 26, 2020, 12:50 p.m. UTC | #2
Hi Alexandru,

url:    https://github.com/0day-ci/linux/commits/Alexandru-Gagniuc/drm-bridge-sii902x-Enable-I-O-and-core-VCC-supplies-if-present/20200925-041504
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-m001-20200925 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/gpu/drm/bridge/sii902x.c:1013 sii902x_probe() warn: passing zero to 'PTR_ERR'

vim +/PTR_ERR +1013 drivers/gpu/drm/bridge/sii902x.c

6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24   999  	sii902x->cvcc12 = devm_regulator_get(dev, "cvcc12");
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1000  	if (IS_ERR(sii902x->cvcc12))
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1001  		return PTR_ERR(sii902x->cvcc12);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1002  
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1003  	ret = regulator_enable(sii902x->iovcc);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1004  	if (ret < 0) {
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1005  		dev_err(dev, "Failed to enable iovcc supply: %d\n", ret);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1006  		return ret;
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1007  	}
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1008  
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1009  	ret = regulator_enable(sii902x->cvcc12);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1010  	if (ret < 0) {
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1011  		dev_err(dev, "Failed to enable cvcc12 supply: %d\n", ret);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1012  		regulator_disable(sii902x->iovcc);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24 @1013  		return PTR_ERR(sii902x->cvcc12);

return ret;

6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1014  	}
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1015  
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1016  	ret = sii902x_init(sii902x);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1017  	if (ret < 0) {
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1018  		regulator_disable(sii902x->cvcc12);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1019  		regulator_disable(sii902x->iovcc);
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1020  	}
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1021  
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1022  	return ret;
6f6c7f7bbe3f177 Alexandru Gagniuc 2020-09-24  1023  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Sam Ravnborg Sept. 26, 2020, 6:49 p.m. UTC | #3
Hi Alexandru

On Thu, Sep 24, 2020 at 03:05:05PM -0500, Alexandru Gagniuc wrote:
> On the SII9022, the IOVCC and CVCC12 supplies must reach the correct
> voltage before the reset sequence is initiated. On most boards, this
> assumption is true at boot-up, so initialization succeeds.
> 
> However, when we try to initialize the chip with incorrect supply
> voltages, it will not respond to I2C requests. sii902x_probe() fails
> with -ENXIO.
> 
> To resolve this, look for the "iovcc" and "cvcc12" regulators, and
> make sure they are enabled before starting the reset sequence. If
> these supplies are not available in devicetree, then they will default
> to dummy-regulator. In that case everything will work like before.
> 
> This was observed on a STM32MP157C-DK2 booting in u-boot falcon mode.
> On this board, the supplies would be set by the second stage
> bootloader, which does not run in falcon mode.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

One nitpick here. The binding should be present in the tree before
you start using it. So this patch should be applied after the binding.

One detail below - I think others have already commented on the rest.

	Sam
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 54 ++++++++++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 33fd33f953ec..a5558d83e4c5 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -17,6 +17,7 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/clk.h>
>  
>  #include <drm/drm_atomic_helper.h>
> @@ -168,6 +169,8 @@ struct sii902x {
>  	struct drm_connector connector;
>  	struct gpio_desc *reset_gpio;
>  	struct i2c_mux_core *i2cmux;
> +	struct regulator *iovcc;
> +	struct regulator *cvcc12;
>  	/*
>  	 * Mutex protects audio and video functions from interfering
>  	 * each other, by keeping their i2c command sequences atomic.
> @@ -954,13 +957,13 @@ static const struct drm_bridge_timings default_sii902x_timings = {
>  		 | DRM_BUS_FLAG_DE_HIGH,
>  };
>  
> +static int sii902x_init(struct sii902x *sii902x);
> +
>  static int sii902x_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> -	unsigned int status = 0;
>  	struct sii902x *sii902x;
> -	u8 chipid[4];
>  	int ret;
>  
>  	ret = i2c_check_functionality(client->adapter,
> @@ -989,6 +992,43 @@ static int sii902x_probe(struct i2c_client *client,
>  
>  	mutex_init(&sii902x->mutex);
>  
> +	sii902x->iovcc = devm_regulator_get(dev, "iovcc");
> +	if (IS_ERR(sii902x->iovcc))
> +		return PTR_ERR(sii902x->iovcc);
Consider using dev_err_probe() here.

> +
> +	sii902x->cvcc12 = devm_regulator_get(dev, "cvcc12");
> +	if (IS_ERR(sii902x->cvcc12))
> +		return PTR_ERR(sii902x->cvcc12);
Consider using dev_err_probe() here.
> +
> +	ret = regulator_enable(sii902x->iovcc);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable iovcc supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(sii902x->cvcc12);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable cvcc12 supply: %d\n", ret);
> +		regulator_disable(sii902x->iovcc);
> +		return PTR_ERR(sii902x->cvcc12);
> +	}
> +
> +	ret = sii902x_init(sii902x);
> +	if (ret < 0) {
> +		regulator_disable(sii902x->cvcc12);
> +		regulator_disable(sii902x->iovcc);
> +	}
> +
> +	return ret;
> +}
> +
> +static int sii902x_init(struct sii902x *sii902x)
> +{
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned int status = 0;
> +	u8 chipid[4];
> +	int ret;
> +
>  	sii902x_reset(sii902x);
>  
>  	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
> @@ -1012,11 +1052,11 @@ static int sii902x_probe(struct i2c_client *client,
>  	regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
>  	regmap_write(sii902x->regmap, SII902X_INT_STATUS, status);
>  
> -	if (client->irq > 0) {
> +	if (sii902x->i2c->irq > 0) {
>  		regmap_write(sii902x->regmap, SII902X_INT_ENABLE,
>  			     SII902X_HOTPLUG_EVENT);
>  
> -		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +		ret = devm_request_threaded_irq(dev, sii902x->i2c->irq, NULL,
>  						sii902x_interrupt,
>  						IRQF_ONESHOT, dev_name(dev),
>  						sii902x);
> @@ -1031,9 +1071,9 @@ static int sii902x_probe(struct i2c_client *client,
>  
>  	sii902x_audio_codec_init(sii902x, dev);
>  
> -	i2c_set_clientdata(client, sii902x);
> +	i2c_set_clientdata(sii902x->i2c, sii902x);
>  
> -	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> +	sii902x->i2cmux = i2c_mux_alloc(sii902x->i2c->adapter, dev,
>  					1, 0, I2C_MUX_GATE,
>  					sii902x_i2c_bypass_select,
>  					sii902x_i2c_bypass_deselect);
> @@ -1051,6 +1091,8 @@ static int sii902x_remove(struct i2c_client *client)
>  
>  	i2c_mux_del_adapters(sii902x->i2cmux);
>  	drm_bridge_remove(&sii902x->bridge);
> +	regulator_disable(sii902x->cvcc12);
> +	regulator_disable(sii902x->iovcc);
>  
>  	return 0;
>  }
> -- 
> 2.25.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Alexandru Gagniuc Sept. 28, 2020, 5:35 p.m. UTC | #4
On 9/26/20 1:49 PM, Sam Ravnborg wrote:
> Hi Alexandru
> 
> On Thu, Sep 24, 2020 at 03:05:05PM -0500, Alexandru Gagniuc wrote:
>> On the SII9022, the IOVCC and CVCC12 supplies must reach the correct
>> voltage before the reset sequence is initiated. On most boards, this
>> assumption is true at boot-up, so initialization succeeds.
>>
>> However, when we try to initialize the chip with incorrect supply
>> voltages, it will not respond to I2C requests. sii902x_probe() fails
>> with -ENXIO.
>>
>> To resolve this, look for the "iovcc" and "cvcc12" regulators, and
>> make sure they are enabled before starting the reset sequence. If
>> these supplies are not available in devicetree, then they will default
>> to dummy-regulator. In that case everything will work like before.
>>
>> This was observed on a STM32MP157C-DK2 booting in u-boot falcon mode.
>> On this board, the supplies would be set by the second stage
>> bootloader, which does not run in falcon mode.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> 
> One nitpick here. The binding should be present in the tree before
> you start using it. So this patch should be applied after the binding.

It is :)
   * arch/arm/boot/dts/stm32mp15xx-dkx.dtsi

Alex

> One detail below - I think others have already commented on the rest.
[snip]
Sam Ravnborg Sept. 28, 2020, 7:03 p.m. UTC | #5
Hi Alex.

On Mon, Sep 28, 2020 at 12:35:01PM -0500, Alex G. wrote:
> On 9/26/20 1:49 PM, Sam Ravnborg wrote:
> > Hi Alexandru
> > 
> > On Thu, Sep 24, 2020 at 03:05:05PM -0500, Alexandru Gagniuc wrote:
> > > On the SII9022, the IOVCC and CVCC12 supplies must reach the correct
> > > voltage before the reset sequence is initiated. On most boards, this
> > > assumption is true at boot-up, so initialization succeeds.
> > > 
> > > However, when we try to initialize the chip with incorrect supply
> > > voltages, it will not respond to I2C requests. sii902x_probe() fails
> > > with -ENXIO.
> > > 
> > > To resolve this, look for the "iovcc" and "cvcc12" regulators, and
> > > make sure they are enabled before starting the reset sequence. If
> > > these supplies are not available in devicetree, then they will default
> > > to dummy-regulator. In that case everything will work like before.
> > > 
> > > This was observed on a STM32MP157C-DK2 booting in u-boot falcon mode.
> > > On this board, the supplies would be set by the second stage
> > > bootloader, which does not run in falcon mode.
> > > 
> > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > 
> > One nitpick here. The binding should be present in the tree before
> > you start using it. So this patch should be applied after the binding.
> 
> It is :)
>   * arch/arm/boot/dts/stm32mp15xx-dkx.dtsi

This is the device tree. So essentially there is part of the device
tree that is not yet documented - so in a perfect world all parts of the
device tree is documented in bindings
(Documentation/devicetree/bindings/* ) before the device tree is
committed.

	Sam
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 33fd33f953ec..a5558d83e4c5 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -17,6 +17,7 @@ 
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/clk.h>
 
 #include <drm/drm_atomic_helper.h>
@@ -168,6 +169,8 @@  struct sii902x {
 	struct drm_connector connector;
 	struct gpio_desc *reset_gpio;
 	struct i2c_mux_core *i2cmux;
+	struct regulator *iovcc;
+	struct regulator *cvcc12;
 	/*
 	 * Mutex protects audio and video functions from interfering
 	 * each other, by keeping their i2c command sequences atomic.
@@ -954,13 +957,13 @@  static const struct drm_bridge_timings default_sii902x_timings = {
 		 | DRM_BUS_FLAG_DE_HIGH,
 };
 
+static int sii902x_init(struct sii902x *sii902x);
+
 static int sii902x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
-	unsigned int status = 0;
 	struct sii902x *sii902x;
-	u8 chipid[4];
 	int ret;
 
 	ret = i2c_check_functionality(client->adapter,
@@ -989,6 +992,43 @@  static int sii902x_probe(struct i2c_client *client,
 
 	mutex_init(&sii902x->mutex);
 
+	sii902x->iovcc = devm_regulator_get(dev, "iovcc");
+	if (IS_ERR(sii902x->iovcc))
+		return PTR_ERR(sii902x->iovcc);
+
+	sii902x->cvcc12 = devm_regulator_get(dev, "cvcc12");
+	if (IS_ERR(sii902x->cvcc12))
+		return PTR_ERR(sii902x->cvcc12);
+
+	ret = regulator_enable(sii902x->iovcc);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable iovcc supply: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(sii902x->cvcc12);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable cvcc12 supply: %d\n", ret);
+		regulator_disable(sii902x->iovcc);
+		return PTR_ERR(sii902x->cvcc12);
+	}
+
+	ret = sii902x_init(sii902x);
+	if (ret < 0) {
+		regulator_disable(sii902x->cvcc12);
+		regulator_disable(sii902x->iovcc);
+	}
+
+	return ret;
+}
+
+static int sii902x_init(struct sii902x *sii902x)
+{
+	struct device *dev = &sii902x->i2c->dev;
+	unsigned int status = 0;
+	u8 chipid[4];
+	int ret;
+
 	sii902x_reset(sii902x);
 
 	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
@@ -1012,11 +1052,11 @@  static int sii902x_probe(struct i2c_client *client,
 	regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
 	regmap_write(sii902x->regmap, SII902X_INT_STATUS, status);
 
-	if (client->irq > 0) {
+	if (sii902x->i2c->irq > 0) {
 		regmap_write(sii902x->regmap, SII902X_INT_ENABLE,
 			     SII902X_HOTPLUG_EVENT);
 
-		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+		ret = devm_request_threaded_irq(dev, sii902x->i2c->irq, NULL,
 						sii902x_interrupt,
 						IRQF_ONESHOT, dev_name(dev),
 						sii902x);
@@ -1031,9 +1071,9 @@  static int sii902x_probe(struct i2c_client *client,
 
 	sii902x_audio_codec_init(sii902x, dev);
 
-	i2c_set_clientdata(client, sii902x);
+	i2c_set_clientdata(sii902x->i2c, sii902x);
 
-	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
+	sii902x->i2cmux = i2c_mux_alloc(sii902x->i2c->adapter, dev,
 					1, 0, I2C_MUX_GATE,
 					sii902x_i2c_bypass_select,
 					sii902x_i2c_bypass_deselect);
@@ -1051,6 +1091,8 @@  static int sii902x_remove(struct i2c_client *client)
 
 	i2c_mux_del_adapters(sii902x->i2cmux);
 	drm_bridge_remove(&sii902x->bridge);
+	regulator_disable(sii902x->cvcc12);
+	regulator_disable(sii902x->iovcc);
 
 	return 0;
 }