[PATCH 0/2] Add charger support for the Nintendo Switch

[PATCH 0/2] Add charger support for the Nintendo Switch

[Emmanuel Gil Peyrot]

These aren’t my patches, but are required in order to charge this
console.  I have tested them on a downstream 6.0 kernel[0] and they seem
to work properly.

This is the first part of a bunch of upstreaming I’m going to do for the
various drivers required to support this console in mainline.

[0] https://gitlab.azka.li/l4t-community/kernel/mainline/linux/-/commits/icosa-v6.0

Alexandre Courbot (2):
  power: supply: bq24190: Support bq24193
  power: supply: bq24190_charger: Export current regulator

 .../bindings/power/supply/bq24190.yaml        |  1 +
 drivers/power/supply/bq24190_charger.c        | 86 +++++++++++++++++++
 2 files changed, 87 insertions(+)

-- 
2.42.0

[PATCH 1/2] power: supply: bq24190: Support bq24193

[Emmanuel Gil Peyrot]

From: Alexandre Courbot <acourbot@nvidia.com>

This charger is working with the driver as-is, so enable it to be probed.

It is used in the Nintendo Switch for instance.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/devicetree/bindings/power/supply/bq24190.yaml | 1 +
 drivers/power/supply/bq24190_charger.c                      | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.yaml b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
index d3ebc9de8c0b..92a28d3c3070 100644
--- a/Documentation/devicetree/bindings/power/supply/bq24190.yaml
+++ b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - ti,bq24190
       - ti,bq24192
+      - ti,bq24193
       - ti,bq24192i
       - ti,bq24196
 
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index ef8235848f56..a56122b39687 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -2018,6 +2018,7 @@ static const struct dev_pm_ops bq24190_pm_ops = {
 static const struct i2c_device_id bq24190_i2c_ids[] = {
 	{ "bq24190" },
 	{ "bq24192" },
+	{ "bq24193" },
 	{ "bq24192i" },
 	{ "bq24196" },
 	{ },
@@ -2027,6 +2028,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
 static const struct of_device_id bq24190_of_match[] = {
 	{ .compatible = "ti,bq24190", },
 	{ .compatible = "ti,bq24192", },
+	{ .compatible = "ti,bq24193", },
 	{ .compatible = "ti,bq24192i", },
 	{ .compatible = "ti,bq24196", },
 	{ },
-- 
2.42.0

[PATCH 2/2] power: supply: bq24190_charger: Export current regulator

[Emmanuel Gil Peyrot]

From: Alexandre Courbot <acourbot@nvidia.com>

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/power/supply/bq24190_charger.c | 84 ++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a56122b39687..8c026e86aeb3 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -530,6 +530,81 @@ static int bq24190_set_otg_vbus(struct bq24190_dev_info *bdi, bool enable)
 }
 
 #ifdef CONFIG_REGULATOR
+static int bq24190_set_charging_current(struct regulator_dev *dev,
+			int min_uA, int max_uA)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	u8 ss_reg;
+	int in_current_limit;
+	int ret = 0;
+
+	dev_info(bdi->dev, "Setting charging current %d mA\n", max_uA/1000);
+
+	ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
+	if (ret < 0)
+		goto error;
+
+	if (max_uA == 0 && ss_reg != 0)
+		return ret;
+
+	if (!(ss_reg & BQ24190_REG_SS_VBUS_STAT_MASK))
+		in_current_limit = 500;
+	else
+		in_current_limit = max_uA / 1000;
+
+	return bq24190_set_field_val(bdi, BQ24190_REG_ISC,
+			BQ24190_REG_ISC_IINLIM_MASK,
+			BQ24190_REG_ISC_IINLIM_SHIFT,
+			bq24190_isc_iinlim_values,
+			ARRAY_SIZE(bq24190_isc_iinlim_values),
+			in_current_limit);
+error:
+	dev_err(bdi->dev, "Charger enable failed, err = %d\n", ret);
+	return ret;
+}
+
+static const struct regulator_ops bq24190_chrg_ops = {
+	.set_current_limit = bq24190_set_charging_current,
+};
+
+static const struct regulator_desc bq24190_chrg_desc = {
+	.name = "charger",
+	.of_match = "charger",
+	.type = REGULATOR_CURRENT,
+	.owner = THIS_MODULE,
+	.ops = &bq24190_chrg_ops,
+};
+
+static const struct regulator_init_data bq24190_chrg_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS | REGULATOR_CHANGE_CURRENT,
+		.min_uA = 0,
+		.max_uA = 3000000,
+	},
+};
+
+static int bq24190_register_chrg_regulator(struct bq24190_dev_info *bdi)
+{
+	struct bq24190_platform_data *pdata = bdi->dev->platform_data;
+	struct regulator_config cfg = { };
+	struct regulator_dev *reg;
+	int ret = 0;
+
+	cfg.dev = bdi->dev;
+	if (pdata && pdata->regulator_init_data)
+		cfg.init_data = pdata->regulator_init_data;
+	else
+		cfg.init_data = &bq24190_chrg_init_data;
+	cfg.driver_data = bdi;
+	reg = devm_regulator_register(bdi->dev, &bq24190_chrg_desc, &cfg);
+	if (IS_ERR(reg)) {
+		ret = PTR_ERR(reg);
+		dev_err(bdi->dev, "Can't register regulator: %d\n", ret);
+	}
+
+	return ret;
+}
+
 static int bq24190_vbus_enable(struct regulator_dev *dev)
 {
 	return bq24190_set_otg_vbus(rdev_get_drvdata(dev), true);
@@ -611,6 +686,11 @@ static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
 	return ret;
 }
 #else
+static int bq24190_register_chrg_regulator(struct bq24190_dev_info *bdi)
+{
+	return 0;
+}
+
 static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
 {
 	return 0;
@@ -1879,6 +1959,10 @@ static int bq24190_probe(struct i2c_client *client)
 		goto out_charger;
 	}
 
+	ret = bq24190_register_chrg_regulator(bdi);
+	if (ret < 0)
+		goto out_charger;
+
 	ret = bq24190_register_vbus_regulator(bdi);
 	if (ret < 0)
 		goto out_charger;
-- 
2.42.0

Re: [PATCH 1/2] power: supply: bq24190: Support bq24193

[Krzysztof Kozlowski]

On 24/08/2023 13:27, Emmanuel Gil Peyrot wrote:
> From: Alexandre Courbot <acourbot@nvidia.com>
> 
> This charger is working with the driver as-is, so enable it to be probed.
> 
> It is used in the Nintendo Switch for instance.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  Documentation/devicetree/bindings/power/supply/bq24190.yaml | 1 +
>  drivers/power/supply/bq24190_charger.c                      | 2 ++

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Bindings are always separate.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

>  2 files changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.yaml b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> index d3ebc9de8c0b..92a28d3c3070 100644
> --- a/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> @@ -18,6 +18,7 @@ properties:
>      enum:
>        - ti,bq24190
>        - ti,bq24192
> +      - ti,bq24193
>        - ti,bq24192i
>        - ti,bq24196
>  
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index ef8235848f56..a56122b39687 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -2018,6 +2018,7 @@ static const struct dev_pm_ops bq24190_pm_ops = {
>  static const struct i2c_device_id bq24190_i2c_ids[] = {
>  	{ "bq24190" },
>  	{ "bq24192" },
> +	{ "bq24193" },
>  	{ "bq24192i" },
>  	{ "bq24196" },
>  	{ },
> @@ -2027,6 +2028,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
>  static const struct of_device_id bq24190_of_match[] = {
>  	{ .compatible = "ti,bq24190", },
>  	{ .compatible = "ti,bq24192", },
> +	{ .compatible = "ti,bq24193", },
>  	{ .compatible = "ti,bq24192i", },
>  	{ .compatible = "ti,bq24196", },

We should really stop doing this. All of them are compatible, aren't they?

Best regards,
Krzysztof

Re: [PATCH 2/2] power: supply: bq24190_charger: Export current regulator

[Krzysztof Kozlowski]

On 24/08/2023 13:27, Emmanuel Gil Peyrot wrote:
> From: Alexandre Courbot <acourbot@nvidia.com>

Empty commit msg. Sorry, we cannot take such commits.

> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Missing SoB.


> ---
>  drivers/power/supply/bq24190_charger.c | 84 ++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index a56122b39687..8c026e86aeb3 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -530,6 +530,81 @@ static int bq24190_set_otg_vbus(struct bq24190_dev_info *bdi, bool enable)
>  }
>  
>  #ifdef CONFIG_REGULATOR
> +static int bq24190_set_charging_current(struct regulator_dev *dev,
> +			int min_uA, int max_uA)
> +{
> +	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
> +	u8 ss_reg;
> +	int in_current_limit;
> +	int ret = 0;
> +
> +	dev_info(bdi->dev, "Setting charging current %d mA\n", max_uA/1000);

This is not info. It would pollute log and it is not really necessary,
because regulator framework gives such information. You can make it
debug if you need for debug purposes.



Best regards,
Krzysztof

Re: [PATCH 1/2] power: supply: bq24190: Support bq24193

[Emmanuel Gil Peyrot]

On Thu, Aug 24, 2023 at 02:40:10PM +0200, Krzysztof Kozlowski wrote:
[…]
> > diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.yaml b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> > index d3ebc9de8c0b..92a28d3c3070 100644
> > --- a/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> > +++ b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> > @@ -18,6 +18,7 @@ properties:
> >      enum:
> >        - ti,bq24190
> >        - ti,bq24192
> > +      - ti,bq24193
> >        - ti,bq24192i
> >        - ti,bq24196
> >  
> > diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> > index ef8235848f56..a56122b39687 100644
> > --- a/drivers/power/supply/bq24190_charger.c
> > +++ b/drivers/power/supply/bq24190_charger.c
> > @@ -2018,6 +2018,7 @@ static const struct dev_pm_ops bq24190_pm_ops = {
> >  static const struct i2c_device_id bq24190_i2c_ids[] = {
> >  	{ "bq24190" },
> >  	{ "bq24192" },
> > +	{ "bq24193" },
> >  	{ "bq24192i" },
> >  	{ "bq24196" },
> >  	{ },
> > @@ -2027,6 +2028,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
> >  static const struct of_device_id bq24190_of_match[] = {
> >  	{ .compatible = "ti,bq24190", },
> >  	{ .compatible = "ti,bq24192", },
> > +	{ .compatible = "ti,bq24193", },
> >  	{ .compatible = "ti,bq24192i", },
> >  	{ .compatible = "ti,bq24196", },
> 
> We should really stop doing this. All of them are compatible, aren't they?

From what I gather from the different datasheets, the main difference
between them is the maximum current they are able to provide, 1.5 A for
the bq24190 and bq24192i, 3 A for bq24192 and 4.5 A for bq24193. The
default current limit is also detected differently it seems.  But yeah,
those are otherwise similar enough to not require anything different in
the driver.

What would be a good way forward for that?  Adding a new ti,bq2419x
compatible and switching all devices to this one, as long as they don’t
require anything specific?

> 
> Best regards,
> Krzysztof
> 

Thanks,

-- 
Link Mauve

Re: [PATCH 2/2] power: supply: bq24190_charger: Export current regulator

[Emmanuel Gil Peyrot]

On Thu, Aug 24, 2023 at 02:41:41PM +0200, Krzysztof Kozlowski wrote:
[…]
> > 
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> Missing SoB.

Do I need to add myself even if I didn’t change anything over what the
original author wrote?

[…]
> 
> Best regards,
> Krzysztof
> 

Thanks,

-- 
Link Mauve

[PATCH v2 0/3] Add charger support for the Nintendo Switch

[Emmanuel Gil Peyrot]

These aren’t my patches, but are required in order to charge this
console.  I have tested them on a downstream 6.0 kernel[0] and they seem
to work properly.

This is the first part of a bunch of upstreaming I’m going to do for the
various drivers required to support this console in mainline.

[0] https://gitlab.azka.li/l4t-community/kernel/mainline/linux/-/commits/icosa-v6.0

Changes since v1:
- Split the dt-bindings change into its own patch.
- Add a message to the second patch.
- Add my own SoB.
- Remove a left-over dev_info().

Alexandre Courbot (2):
  power: supply: bq24190_charger: Support bq24193
  power: supply: bq24190_charger: Export current regulator

Emmanuel Gil Peyrot (1):
  dt-bindings: power: supply: bq24190: Add ti,bq24193

 .../bindings/power/supply/bq24190.yaml        |  1 +
 drivers/power/supply/bq24190_charger.c        | 84 +++++++++++++++++++
 2 files changed, 85 insertions(+)

-- 
2.42.0

[PATCH v2 1/3] dt-bindings: power: supply: bq24190: Add ti,bq24193

[Emmanuel Gil Peyrot]

This power supply device is used in the Nintendo Switch.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 Documentation/devicetree/bindings/power/supply/bq24190.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.yaml b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
index d3ebc9de8c0b..2ad107589a16 100644
--- a/Documentation/devicetree/bindings/power/supply/bq24190.yaml
+++ b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
@@ -19,6 +19,7 @@ properties:
       - ti,bq24190
       - ti,bq24192
       - ti,bq24192i
+      - ti,bq24193
       - ti,bq24196
 
   reg:
-- 
2.42.0

[PATCH v2 2/3] power: supply: bq24190_charger: Support bq24193

[Emmanuel Gil Peyrot]

From: Alexandre Courbot <acourbot@nvidia.com>

This charger is working with the driver as-is, so enable it to be probed.

It is used in the Nintendo Switch for instance.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 drivers/power/supply/bq24190_charger.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index ef8235848f56..a56122b39687 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -2018,6 +2018,7 @@ static const struct dev_pm_ops bq24190_pm_ops = {
 static const struct i2c_device_id bq24190_i2c_ids[] = {
 	{ "bq24190" },
 	{ "bq24192" },
+	{ "bq24193" },
 	{ "bq24192i" },
 	{ "bq24196" },
 	{ },
@@ -2027,6 +2028,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
 static const struct of_device_id bq24190_of_match[] = {
 	{ .compatible = "ti,bq24190", },
 	{ .compatible = "ti,bq24192", },
+	{ .compatible = "ti,bq24193", },
 	{ .compatible = "ti,bq24192i", },
 	{ .compatible = "ti,bq24196", },
 	{ },
-- 
2.42.0

[PATCH v2 3/3] power: supply: bq24190_charger: Export current regulator

[Emmanuel Gil Peyrot]

From: Alexandre Courbot <acourbot@nvidia.com>

This prevents the charger from ever going over the current limit.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 drivers/power/supply/bq24190_charger.c | 82 ++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a56122b39687..cc1bd87f4982 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -530,6 +530,79 @@ static int bq24190_set_otg_vbus(struct bq24190_dev_info *bdi, bool enable)
 }
 
 #ifdef CONFIG_REGULATOR
+static int bq24190_set_charging_current(struct regulator_dev *dev,
+			int min_uA, int max_uA)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	u8 ss_reg;
+	int in_current_limit;
+	int ret = 0;
+
+	ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
+	if (ret < 0)
+		goto error;
+
+	if (max_uA == 0 && ss_reg != 0)
+		return ret;
+
+	if (!(ss_reg & BQ24190_REG_SS_VBUS_STAT_MASK))
+		in_current_limit = 500;
+	else
+		in_current_limit = max_uA / 1000;
+
+	return bq24190_set_field_val(bdi, BQ24190_REG_ISC,
+			BQ24190_REG_ISC_IINLIM_MASK,
+			BQ24190_REG_ISC_IINLIM_SHIFT,
+			bq24190_isc_iinlim_values,
+			ARRAY_SIZE(bq24190_isc_iinlim_values),
+			in_current_limit);
+error:
+	dev_err(bdi->dev, "Charger enable failed, err = %d\n", ret);
+	return ret;
+}
+
+static const struct regulator_ops bq24190_chrg_ops = {
+	.set_current_limit = bq24190_set_charging_current,
+};
+
+static const struct regulator_desc bq24190_chrg_desc = {
+	.name = "charger",
+	.of_match = "charger",
+	.type = REGULATOR_CURRENT,
+	.owner = THIS_MODULE,
+	.ops = &bq24190_chrg_ops,
+};
+
+static const struct regulator_init_data bq24190_chrg_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS | REGULATOR_CHANGE_CURRENT,
+		.min_uA = 0,
+		.max_uA = 3000000,
+	},
+};
+
+static int bq24190_register_chrg_regulator(struct bq24190_dev_info *bdi)
+{
+	struct bq24190_platform_data *pdata = bdi->dev->platform_data;
+	struct regulator_config cfg = { };
+	struct regulator_dev *reg;
+	int ret = 0;
+
+	cfg.dev = bdi->dev;
+	if (pdata && pdata->regulator_init_data)
+		cfg.init_data = pdata->regulator_init_data;
+	else
+		cfg.init_data = &bq24190_chrg_init_data;
+	cfg.driver_data = bdi;
+	reg = devm_regulator_register(bdi->dev, &bq24190_chrg_desc, &cfg);
+	if (IS_ERR(reg)) {
+		ret = PTR_ERR(reg);
+		dev_err(bdi->dev, "Can't register regulator: %d\n", ret);
+	}
+
+	return ret;
+}
+
 static int bq24190_vbus_enable(struct regulator_dev *dev)
 {
 	return bq24190_set_otg_vbus(rdev_get_drvdata(dev), true);
@@ -611,6 +684,11 @@ static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
 	return ret;
 }
 #else
+static int bq24190_register_chrg_regulator(struct bq24190_dev_info *bdi)
+{
+	return 0;
+}
+
 static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
 {
 	return 0;
@@ -1879,6 +1957,10 @@ static int bq24190_probe(struct i2c_client *client)
 		goto out_charger;
 	}
 
+	ret = bq24190_register_chrg_regulator(bdi);
+	if (ret < 0)
+		goto out_charger;
+
 	ret = bq24190_register_vbus_regulator(bdi);
 	if (ret < 0)
 		goto out_charger;
-- 
2.42.0

Re: [PATCH 1/2] power: supply: bq24190: Support bq24193

[Krzysztof Kozlowski]

On 24/08/2023 15:00, Emmanuel Gil Peyrot wrote:

>>> @@ -2027,6 +2028,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
>>>  static const struct of_device_id bq24190_of_match[] = {
>>>  	{ .compatible = "ti,bq24190", },
>>>  	{ .compatible = "ti,bq24192", },
>>> +	{ .compatible = "ti,bq24193", },
>>>  	{ .compatible = "ti,bq24192i", },
>>>  	{ .compatible = "ti,bq24196", },
>>
>> We should really stop doing this. All of them are compatible, aren't they?
> 
> From what I gather from the different datasheets, the main difference
> between them is the maximum current they are able to provide, 1.5 A for
> the bq24190 and bq24192i, 3 A for bq24192 and 4.5 A for bq24193. The
> default current limit is also detected differently it seems.  But yeah,
> those are otherwise similar enough to not require anything different in
> the driver.
> 
> What would be a good way forward for that?  Adding a new ti,bq2419x
> compatible and switching all devices to this one, as long as they don’t
> require anything specific?

Not a wildcard but any of existing ones, e.g. "ti,bq24196", "ti,bq24190".

Best regards,
Krzysztof

Re: [PATCH 2/2] power: supply: bq24190_charger: Export current regulator

[Krzysztof Kozlowski]

On 24/08/2023 15:02, Emmanuel Gil Peyrot wrote:
> On Thu, Aug 24, 2023 at 02:41:41PM +0200, Krzysztof Kozlowski wrote:
> […]
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>> Missing SoB.
> 
> Do I need to add myself even if I didn’t change anything over what the
> original author wrote?

Yes.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: power: supply: bq24190: Add ti,bq24193

[Krzysztof Kozlowski]

On 24/08/2023 15:13, Emmanuel Gil Peyrot wrote:
> This power supply device is used in the Nintendo Switch.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  Documentation/devicetree/bindings/power/supply/bq24190.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.yaml b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> index d3ebc9de8c0b..2ad107589a16 100644
> --- a/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/bq24190.yaml
> @@ -19,6 +19,7 @@ properties:
>        - ti,bq24190
>        - ti,bq24192
>        - ti,bq24192i
> +      - ti,bq24193

Why are you sending new version so fast after previous one? Before we
finished discussion?  One version per day.

You did not include here my comments.

Best regards,
Krzysztof

Re: [PATCH 1/2] power: supply: bq24190: Support bq24193

[Linus Walleij]

On Thu, Aug 24, 2023 at 3:28 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 24/08/2023 15:00, Emmanuel Gil Peyrot wrote:
>
> >>> @@ -2027,6 +2028,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
> >>>  static const struct of_device_id bq24190_of_match[] = {
> >>>     { .compatible = "ti,bq24190", },
> >>>     { .compatible = "ti,bq24192", },
> >>> +   { .compatible = "ti,bq24193", },
> >>>     { .compatible = "ti,bq24192i", },
> >>>     { .compatible = "ti,bq24196", },
> >>
> >> We should really stop doing this. All of them are compatible, aren't they?
> >
> > From what I gather from the different datasheets, the main difference
> > between them is the maximum current they are able to provide, 1.5 A for
> > the bq24190 and bq24192i, 3 A for bq24192 and 4.5 A for bq24193. The
> > default current limit is also detected differently it seems.  But yeah,
> > those are otherwise similar enough to not require anything different in
> > the driver.
> >
> > What would be a good way forward for that?  Adding a new ti,bq2419x
> > compatible and switching all devices to this one, as long as they don’t
> > require anything specific?
>
> Not a wildcard but any of existing ones, e.g. "ti,bq24196", "ti,bq24190".

We usually encourage people to over-specify the hardware number,
because you never know when you need a quirk and then if you can't
tell them apart you are in a bad place. (But there are exceptions, such
as jedec-nor...)

The differences pointed out (charge current limit etc) can very well
result in different code paths at some point, especially if the charger
interacts with some other component.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] power: supply: bq24190_charger: Export current regulator

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4034 bytes --]

Hi,

On Thu, Aug 24, 2023 at 03:13:31PM +0200, Emmanuel Gil Peyrot wrote:
> From: Alexandre Courbot <acourbot@nvidia.com>
> 
> This prevents the charger from ever going over the current limit.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---

This would need to be documented in the DT binding, but it
duplicates control for POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT
anyways. Please introduce DT property "input-current-limit-microamp"
for this driver, just like e.g. bq2515x_charger.c (and update the DT
binding accordingly).

-- Sebastian

>  drivers/power/supply/bq24190_charger.c | 82 ++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index a56122b39687..cc1bd87f4982 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -530,6 +530,79 @@ static int bq24190_set_otg_vbus(struct bq24190_dev_info *bdi, bool enable)
>  }
>  
>  #ifdef CONFIG_REGULATOR
> +static int bq24190_set_charging_current(struct regulator_dev *dev,
> +			int min_uA, int max_uA)
> +{
> +	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
> +	u8 ss_reg;
> +	int in_current_limit;
> +	int ret = 0;
> +
> +	ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
> +	if (ret < 0)
> +		goto error;
> +
> +	if (max_uA == 0 && ss_reg != 0)
> +		return ret;
> +
> +	if (!(ss_reg & BQ24190_REG_SS_VBUS_STAT_MASK))
> +		in_current_limit = 500;
> +	else
> +		in_current_limit = max_uA / 1000;
> +
> +	return bq24190_set_field_val(bdi, BQ24190_REG_ISC,
> +			BQ24190_REG_ISC_IINLIM_MASK,
> +			BQ24190_REG_ISC_IINLIM_SHIFT,
> +			bq24190_isc_iinlim_values,
> +			ARRAY_SIZE(bq24190_isc_iinlim_values),
> +			in_current_limit);
> +error:
> +	dev_err(bdi->dev, "Charger enable failed, err = %d\n", ret);
> +	return ret;
> +}
> +
> +static const struct regulator_ops bq24190_chrg_ops = {
> +	.set_current_limit = bq24190_set_charging_current,
> +};
> +
> +static const struct regulator_desc bq24190_chrg_desc = {
> +	.name = "charger",
> +	.of_match = "charger",
> +	.type = REGULATOR_CURRENT,
> +	.owner = THIS_MODULE,
> +	.ops = &bq24190_chrg_ops,
> +};
> +
> +static const struct regulator_init_data bq24190_chrg_init_data = {
> +	.constraints = {
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS | REGULATOR_CHANGE_CURRENT,
> +		.min_uA = 0,
> +		.max_uA = 3000000,
> +	},
> +};
> +
> +static int bq24190_register_chrg_regulator(struct bq24190_dev_info *bdi)
> +{
> +	struct bq24190_platform_data *pdata = bdi->dev->platform_data;
> +	struct regulator_config cfg = { };
> +	struct regulator_dev *reg;
> +	int ret = 0;
> +
> +	cfg.dev = bdi->dev;
> +	if (pdata && pdata->regulator_init_data)
> +		cfg.init_data = pdata->regulator_init_data;
> +	else
> +		cfg.init_data = &bq24190_chrg_init_data;
> +	cfg.driver_data = bdi;
> +	reg = devm_regulator_register(bdi->dev, &bq24190_chrg_desc, &cfg);
> +	if (IS_ERR(reg)) {
> +		ret = PTR_ERR(reg);
> +		dev_err(bdi->dev, "Can't register regulator: %d\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
>  static int bq24190_vbus_enable(struct regulator_dev *dev)
>  {
>  	return bq24190_set_otg_vbus(rdev_get_drvdata(dev), true);
> @@ -611,6 +684,11 @@ static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
>  	return ret;
>  }
>  #else
> +static int bq24190_register_chrg_regulator(struct bq24190_dev_info *bdi)
> +{
> +	return 0;
> +}
> +
>  static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
>  {
>  	return 0;
> @@ -1879,6 +1957,10 @@ static int bq24190_probe(struct i2c_client *client)
>  		goto out_charger;
>  	}
>  
> +	ret = bq24190_register_chrg_regulator(bdi);
> +	if (ret < 0)
> +		goto out_charger;
> +
>  	ret = bq24190_register_vbus_regulator(bdi);
>  	if (ret < 0)
>  		goto out_charger;
> -- 
> 2.42.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] power: supply: bq24190_charger: Export current regulator

[Christophe JAILLET]

Le 24/08/2023 à 15:13, Emmanuel Gil Peyrot a écrit :
> From: Alexandre Courbot <acourbot@nvidia.com>
> 
> This prevents the charger from ever going over the current limit.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>   drivers/power/supply/bq24190_charger.c | 82 ++++++++++++++++++++++++++
>   1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index a56122b39687..cc1bd87f4982 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -530,6 +530,79 @@ static int bq24190_set_otg_vbus(struct bq24190_dev_info *bdi, bool enable)
>   }
>   
>   #ifdef CONFIG_REGULATOR
> +static int bq24190_set_charging_current(struct regulator_dev *dev,
> +			int min_uA, int max_uA)
> +{
> +	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
> +	u8 ss_reg;
> +	int in_current_limit;
> +	int ret = 0;

Nit: Un-needed init.

> +
> +	ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
> +	if (ret < 0)
> +		goto error;
> +
> +	if (max_uA == 0 && ss_reg != 0)
> +		return ret;

ret is known to be 0 here. If it is the intension, return 0 would be 
more explicit. Otherwise a ret = -<error_code> is missing.

Just my 2c,

CJ

> +
> +	if (!(ss_reg & BQ24190_REG_SS_VBUS_STAT_MASK))
> +		in_current_limit = 500;
> +	else
> +		in_current_limit = max_uA / 1000;
> +
> +	return bq24190_set_field_val(bdi, BQ24190_REG_ISC,
> +			BQ24190_REG_ISC_IINLIM_MASK,
> +			BQ24190_REG_ISC_IINLIM_SHIFT,
> +			bq24190_isc_iinlim_values,
> +			ARRAY_SIZE(bq24190_isc_iinlim_values),
> +			in_current_limit);
> +error:
> +	dev_err(bdi->dev, "Charger enable failed, err = %d\n", ret);
> +	return ret;
> +}

...