Hi Maksim,
On 2024-11-01 19:17, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Nov 01, 2024 at 01:34:51PM +0300, [email protected] wrote:
>> From: Maksim Kiselev <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>>
