[PATCH v1 2/3] usb: tcpm: fusb302: add support for set_vbus() callback.

[Jonas Karlman]

Hi Maksim,

On 2024-11-01 19:17, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Nov 01, 2024 at 01:34:51PM +0300, bigunclemax at gmail.com wrote:
>> From: Maksim Kiselev <bigunclemax at gmail.com>
>>
>> Add support for VBUS supply regulator.
>>
>> When our type-c port acts as a host(SRC), this regulator
>> used for control VBUS supply.
>>
>> Signed-off-by: Maksim Kiselev <bigunclemax at gmail.com>
>> ---
>>  drivers/usb/tcpm/fusb302.c | 44 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/tcpm/fusb302.c b/drivers/usb/tcpm/fusb302.c
>> index ee283782792..2a258d6429b 100644
>> --- a/drivers/usb/tcpm/fusb302.c
>> +++ b/drivers/usb/tcpm/fusb302.c
>> @@ -10,6 +10,7 @@
>>  #include <asm/gpio.h>
>>  #include <linux/delay.h>
>>  #include <linux/err.h>
>> +#include <power/regulator.h>
>>  #include <dm/device_compat.h>
>>  #include <usb/tcpm.h>
>>  #include "fusb302_reg.h"
>> @@ -50,10 +51,13 @@ struct fusb302_chip {
>>  
>>  	/* port status */
>>  	bool vconn_on;
>> +	bool vbus_on;
>>  	bool vbus_present;
>>  	enum typec_cc_polarity cc_polarity;
>>  	enum typec_cc_status cc1;
>>  	enum typec_cc_status cc2;
>> +
>> +	struct udevice *vbus;
>>  };
>>  
>>  static int fusb302_i2c_write(struct udevice *dev, u8 address, u8 data)
>> @@ -506,7 +510,28 @@ done:
>>  
>>  static int fusb302_set_vbus(struct udevice *dev, bool on, bool charge)
>>  {
>> -	return 0;
>> +	struct fusb302_chip *chip = dev_get_priv(dev);
>> +	int ret = 0;
>> +
>> +	if (chip->vbus_on == on) {
>> +		dev_dbg(dev, "vbus is already %s\n", on ? "On" : "Off");
>> +	} else {
>> +		if (CONFIG_IS_ENABLED(DM_REGULATOR) && chip->vbus) {
> 
> should be enough to check for chip->vbus, since the regulator
> device should be NULL when DM_REGULATOR is disabled. If due to
> some bug its not NULL regulator_set_enable would fail gracefully
> with -ENOSYS, so that somebody can debug the problem.
> 
>> +			if (on)
>> +				ret = regulator_set_enable(chip->vbus, true);
>> +			else
>> +				ret = regulator_set_enable(chip->vbus, false);
> 
> regulator_set_enable(chip->vbus, on);

Please use something like following:

  ret = regulator_set_enable_if_allowed(chip->vbus, on);
  if (ret && ret != -ENOSYS)

Should be safe when DM_REGULATOR disabled or when chip->vbus is NULL,
also work when regulator is always-on or already enabled.

> 
>> +			if (ret < 0) {
>> +				dev_dbg(dev, "cannot %s vbus regulator, ret=%d\n",
>> +					on ? "enable" : "disable", ret);
> 
> dev_err()?
> 
>> +				return ret;
>> +			}
>> +		}
>> +		chip->vbus_on = on;

vbus_on could probably be dropped, fixed and gpio regulators is
currently already reference counted. Or is the set_vbus ops not called
evenly and current state needs to be known?

>> +		dev_dbg(dev, "vbus := %s\n", on ? "On" : "Off");
>> +	}
>> +
>> +	return ret;
> 
> I suggest converting this to
> 
> int ret;
> 
> if (chip->vbus_on == on) {
>     dev_dbg(...);
>     return 0;
> }
> 
> ... remaining code without extra indent ...
> 
> Otherwise LGTM.
> 
> -- Sebastian
> 
> 
>>  }
>>  
>>  static int fusb302_pd_tx_flush(struct udevice *dev)
>> @@ -1293,6 +1318,22 @@ static int fusb302_get_connector_node(struct udevice *dev, ofnode *connector_nod
>>  	return 0;
>>  }
>>  
>> +static int fusb302_probe(struct udevice *dev)
>> +{
>> +	struct fusb302_chip *chip = dev_get_priv(dev);
>> +	int ret;
>> +
>> +	if (CONFIG_IS_ENABLED(DM_REGULATOR)) {
>> +		ret = device_get_supply_regulator(dev, "vbus-supply", &chip->vbus);
>> +		if (ret && ret != -ENOENT) {

You could probably just drop the DM_REGULATOR wrap and use:

  if (ret && ret != -ENOSYS && ret != -ENOENT) {

Regards,
Jonas

>> +			dev_err(dev, "Failed to get vbus-supply regulator\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static struct dm_tcpm_ops fusb302_ops = {
>>  	.get_connector_node = fusb302_get_connector_node,
>>  	.init = fusb302_init,
>> @@ -1320,4 +1361,5 @@ U_BOOT_DRIVER(fusb302) = {
>>  	.of_match = fusb302_ids,
>>  	.ops = &fusb302_ops,
>>  	.priv_auto = sizeof(struct fusb302_chip),
>> +	.probe = fusb302_probe,
>>  };
>> -- 
>> 2.45.2
>>