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

[Philippe CORNU]

Hi Andrzej,

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

thank you for your comment.

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

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

Many thanks,
Philippe :-)

> 
> Regards
> Andrzej
> 
>>>
>>> Signed-off-by: Philippe Cornu <philippe.cornu@xxxxxx>
>>> ---
>>>    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;
>>>    }
>>>    
> 
>