diff mbox series

[6/7] power: supply: bq25890: Add get_voltage support to Vbus regulator

Message ID 20221010210310.165461-6-marex@denx.de (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [1/7] power: supply: bq25890: Document POWER_SUPPLY_PROP_CURRENT_NOW | expand

Commit Message

Marek Vasut Oct. 10, 2022, 9:03 p.m. UTC 
The chip is capable of reporting Vbus voltage, add .get_voltage
implementation to Vbus regulator to report current Vbus voltage.
This requires for the Vbus regulator to be registered always
instead of the current state where the regulator is registered
only in case USB PHY is not found.

Do not provide Vbus regulator enable/disable ops in case USB PHY
is present, as they would race with USB PHY notifier which is also
used to toggle OTG boost mode.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
To: linux-pm@vger.kernel.org
---
 drivers/power/supply/bq25890_charger.c | 33 ++++++++++++++++++++------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Hans de Goede Oct. 13, 2022, 9:29 a.m. UTC | #1 
Hi,

On 10/10/22 23:03, Marek Vasut wrote:
> The chip is capable of reporting Vbus voltage, add .get_voltage
> implementation to Vbus regulator to report current Vbus voltage.
> This requires for the Vbus regulator to be registered always
> instead of the current state where the regulator is registered
> only in case USB PHY is not found.
> 
> Do not provide Vbus regulator enable/disable ops in case USB PHY
> is present, as they would race with USB PHY notifier which is also
> used to toggle OTG boost mode.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
> To: linux-pm@vger.kernel.org
> ---
>  drivers/power/supply/bq25890_charger.c | 33 ++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 7ab27a9dce14a..2be5861cfcb66 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1099,38 +1099,57 @@ static int bq25890_vbus_is_enabled(struct regulator_dev *rdev)
>  	return bq25890_field_read(bq, F_OTG_CFG);
>  }
>  
> +static int bq25890_vbus_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct bq25890_device *bq = rdev_get_drvdata(rdev);
> +
> +	return bq25890_get_vbus_voltage(bq);
> +}
> +
>  static const struct regulator_ops bq25890_vbus_ops = {
> +	.get_voltage = bq25890_vbus_get_voltage,
> +};
> +
> +static const struct regulator_desc bq25890_vbus_desc = {
> +	.name = "usb_otg_vbus",
> +	.of_match = "usb-otg-vbus",
> +	.type = REGULATOR_VOLTAGE,
> +	.owner = THIS_MODULE,
> +	.ops = &bq25890_vbus_ops,
> +};
> +
> +static const struct regulator_ops bq25890_vbus_boost_ops = {
>  	.enable = bq25890_vbus_enable,
>  	.disable = bq25890_vbus_disable,
>  	.is_enabled = bq25890_vbus_is_enabled,
> +	.get_voltage = bq25890_vbus_get_voltage,
>  };
>  
> -static const struct regulator_desc bq25890_vbus_desc = {
> +static const struct regulator_desc bq25890_vbus_boost_desc = {
>  	.name = "usb_otg_vbus",
>  	.of_match = "usb-otg-vbus",
>  	.type = REGULATOR_VOLTAGE,
>  	.owner = THIS_MODULE,
> -	.ops = &bq25890_vbus_ops,
> -	.fixed_uV = 5000000,
> -	.n_voltages = 1,
> +	.ops = &bq25890_vbus_boost_ops,
>  };
>  
>  static int bq25890_register_regulator(struct bq25890_device *bq)
>  {
>  	struct bq25890_platform_data *pdata = dev_get_platdata(bq->dev);
> +	const struct regulator_desc *desc;
>  	struct regulator_config cfg = {
>  		.dev = bq->dev,
>  		.driver_data = bq,
>  	};
>  	struct regulator_dev *reg;
>  
> -	if (!IS_ERR_OR_NULL(bq->usb_phy))
> -		return 0;
> +	desc = IS_ERR_OR_NULL(bq->usb_phy) ?
> +	       &bq25890_vbus_boost_desc : &bq25890_vbus_desc;

I'm not sure this trickery with 2 separate sets of ops +
2 descs pointing to the different ops is necessary.

The core will only allow calling the enable/disable callbacks
if the regulator constraints (from either platform_data or
from dt) contain something like this (platform_data example):

static const struct regulator_init_data bq2589x_vbus_init_data = {
        .constraints = {
                .valid_ops_mask = REGULATOR_CHANGE_STATUS,
        },
        .consumer_supplies = &bq2589x_vbus_consumer,
        .num_consumer_supplies = 1,
};

AFAIK if the Vboost regulator is not referenced in dt because
it is controller through the usb-phy framework then valid_ops_mask
will be empty, so the 2 sets of ops + 2 descs are not necessary
I believe.

So I believe this can be simplified to just adding
bq25890_vbus_get_voltage to the ops, dropping .fixed_uV and
.n_voltages from the desc, and just completely dropping
the IS_ERR_OR_NULL(bq->usb_phy) check.

Regards,

Hans



>  
>  	if (pdata)
>  		cfg.init_data = pdata->regulator_init_data;
>  
> -	reg = devm_regulator_register(bq->dev, &bq25890_vbus_desc, &cfg);
> +	reg = devm_regulator_register(bq->dev, desc, &cfg);
>  	if (IS_ERR(reg)) {
>  		return dev_err_probe(bq->dev, PTR_ERR(reg),
>  				     "registering vbus regulator");
diff mbox series

Patch

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 7ab27a9dce14a..2be5861cfcb66 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -1099,38 +1099,57 @@  static int bq25890_vbus_is_enabled(struct regulator_dev *rdev)
 	return bq25890_field_read(bq, F_OTG_CFG);
 }
 
+static int bq25890_vbus_get_voltage(struct regulator_dev *rdev)
+{
+	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+
+	return bq25890_get_vbus_voltage(bq);
+}
+
 static const struct regulator_ops bq25890_vbus_ops = {
+	.get_voltage = bq25890_vbus_get_voltage,
+};
+
+static const struct regulator_desc bq25890_vbus_desc = {
+	.name = "usb_otg_vbus",
+	.of_match = "usb-otg-vbus",
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+	.ops = &bq25890_vbus_ops,
+};
+
+static const struct regulator_ops bq25890_vbus_boost_ops = {
 	.enable = bq25890_vbus_enable,
 	.disable = bq25890_vbus_disable,
 	.is_enabled = bq25890_vbus_is_enabled,
+	.get_voltage = bq25890_vbus_get_voltage,
 };
 
-static const struct regulator_desc bq25890_vbus_desc = {
+static const struct regulator_desc bq25890_vbus_boost_desc = {
 	.name = "usb_otg_vbus",
 	.of_match = "usb-otg-vbus",
 	.type = REGULATOR_VOLTAGE,
 	.owner = THIS_MODULE,
-	.ops = &bq25890_vbus_ops,
-	.fixed_uV = 5000000,
-	.n_voltages = 1,
+	.ops = &bq25890_vbus_boost_ops,
 };
 
 static int bq25890_register_regulator(struct bq25890_device *bq)
 {
 	struct bq25890_platform_data *pdata = dev_get_platdata(bq->dev);
+	const struct regulator_desc *desc;
 	struct regulator_config cfg = {
 		.dev = bq->dev,
 		.driver_data = bq,
 	};
 	struct regulator_dev *reg;
 
-	if (!IS_ERR_OR_NULL(bq->usb_phy))
-		return 0;
+	desc = IS_ERR_OR_NULL(bq->usb_phy) ?
+	       &bq25890_vbus_boost_desc : &bq25890_vbus_desc;
 
 	if (pdata)
 		cfg.init_data = pdata->regulator_init_data;
 
-	reg = devm_regulator_register(bq->dev, &bq25890_vbus_desc, &cfg);
+	reg = devm_regulator_register(bq->dev, desc, &cfg);
 	if (IS_ERR(reg)) {
 		return dev_err_probe(bq->dev, PTR_ERR(reg),
 				     "registering vbus regulator");