[PATCH 0/3] power: supply: twl6030/32 charger

[PATCH 0/3] power: supply: twl6030/32 charger

[Andreas Kemnade]

Add basic support for the charger in the TWL6030/32. Supported is the USB
path. AC path is not handled yet, also there is no entry yet
in /sys/class/power_supply with type battery yet.

Without this series, devices will happily drain battery when running
on mainline.

Andreas Kemnade (3):
  dt-bindings: power: supply: Add TI TWL603X charger
  dt-bindings: mfd: twl: add charger node also for TWL603x
  power: supply: initial support for TWL6030/32

 .../devicetree/bindings/mfd/ti,twl.yaml       |  18 +
 .../power/supply/ti,twl6030-charger.yaml      |  62 ++
 drivers/power/supply/Kconfig                  |  10 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/twl6030_charger.c        | 566 ++++++++++++++++++
 5 files changed, 657 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
 create mode 100644 drivers/power/supply/twl6030_charger.c

-- 
2.39.2

[PATCH 1/3] dt-bindings: power: supply: Add TI TWL603X charger

[Andreas Kemnade]

Use a fallback compatible since for especially for generic
defensive setup of parameters, both 6030 and 6032 are the same and
U-Boot actually uses a generic 6030/32 function to enable the
charger.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 .../power/supply/ti,twl6030-charger.yaml      | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml b/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
new file mode 100644
index 0000000000000..fe0fe9a78761c
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/ti,twl6030-charger.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TWL6030/32 BCI (Battery Charger Interface)
+
+description: |
+  The battery charger needs to be configured to do any charging besides of
+  precharging. The GPADC in the PMIC has to be used to get the related
+  voltages.
+
+maintainers:
+  - Andreas Kemnade <andreas@kemnade.info>
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: "ti,twl6030-charger"
+      - items:
+          - const: "ti,twl6032-charger"
+          - const: "ti,twl6030-charger"
+
+  interrupts:
+    minItems: 2
+    maxItems: 2
+
+  io-channels:
+    items:
+      - description: VBUS Voltage Channel
+
+  io-channel-names:
+    items:
+      - const: vusb
+
+  monitored-battery:
+    description:
+      phandle of battery characteristics devicetree node
+
+required:
+  - compatible
+  - interrupts
+  - monitored-battery
+
+additionalProperties: false
+
+examples:
+  - |
+    pmic {
+      bci {
+        compatible = "ti,twl6032-charger", "ti,twl6030-charger";
+        interrupts = <2>, <5>;
+        io-channels = <&gpadc 10>;
+        io-channel-names = "vusb";
+        monitored-battery = <&bat>;
+      };
+    };
-- 
2.39.2

Re: [PATCH 1/3] dt-bindings: power: supply: Add TI TWL603X charger

[Krzysztof Kozlowski]

On 18/09/2024 10:41, Andreas Kemnade wrote:
> Use a fallback compatible since for especially for generic
> defensive setup of parameters, both 6030 and 6032 are the same and
> U-Boot actually uses a generic 6030/32 function to enable the
> charger.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  .../power/supply/ti,twl6030-charger.yaml      | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml b/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
> new file mode 100644
> index 0000000000000..fe0fe9a78761c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license. See checkpatch.

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/ti,twl6030-charger.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TWL6030/32 BCI (Battery Charger Interface)
> +
> +description: |

Do not need '|' unless you need to preserve formatting.


> +  The battery charger needs to be configured to do any charging besides of
> +  precharging. The GPADC in the PMIC has to be used to get the related
> +  voltages.
> +
> +maintainers:
> +  - Andreas Kemnade <andreas@kemnade.info>
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:

No need for items here. Just const directly.

> +          - const: "ti,twl6030-charger"

Not tested.

Drop ""

> +      - items:
> +          - const: "ti,twl6032-charger"
> +          - const: "ti,twl6030-charger"
> +
> +  interrupts:
> +    minItems: 2

Drop minItems

> +    maxItems: 2

... and actually drop both and instead list items with description
(items: -descriptio: ... - description: ....)

> +
> +  io-channels:
> +    items:
> +      - description: VBUS Voltage Channel
> +
> +  io-channel-names:
> +    items:
> +      - const: vusb
> +
> +  monitored-battery:

Just : true

> +    description:
> +      phandle of battery characteristics devicetree node

That's redundant, you do no say anything useful here.

> +
> +required:
> +  - compatible
> +  - interrupts
> +  - monitored-battery
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pmic {

Drop... or this is supposed to be part of parent schema?

> +      bci {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

e.g. charger

> +        compatible = "ti,twl6032-charger", "ti,twl6030-charger";
> +        interrupts = <2>, <5>;
> +        io-channels = <&gpadc 10>;
> +        io-channel-names = "vusb";
> +        monitored-battery = <&bat>;
> +      };
> +    };

Best regards,
Krzysztof

[PATCH 2/3] dt-bindings: mfd: twl: add charger node also for TWL603x

[Andreas Kemnade]

Also the TWL603X devices have a charger, so allow to specify it here.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 .../devicetree/bindings/mfd/ti,twl.yaml        | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
index e94b0fd7af0f8..4064a228cb0fc 100644
--- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
@@ -105,6 +105,11 @@ allOf:
             regulator-initial-mode: false
 
       properties:
+        bci:
+          type: object
+          properties:
+            compatible:
+              const: ti,twl6030-charger
         gpadc:
           type: object
           properties:
@@ -136,6 +141,13 @@ allOf:
             regulator-initial-mode: false
 
       properties:
+        bci:
+          type: object
+          properties:
+            compatible:
+              items:
+                - const: ti,twl6032-charger
+                - const: ti,twl6030-charger
         gpadc:
           type: object
           properties:
@@ -222,6 +234,12 @@ examples:
         interrupt-controller;
         #interrupt-cells = <1>;
 
+        bci {
+          compatible = "ti,twl6030-charger";
+          interrupts = <2>, <5>;
+          monitored-battery = <&bat>;
+        };
+
         gpadc {
           compatible = "ti,twl6030-gpadc";
           interrupts = <6>;
-- 
2.39.2

Re: [PATCH 2/3] dt-bindings: mfd: twl: add charger node also for TWL603x

[Krzysztof Kozlowski]

On 18/09/2024 10:41, Andreas Kemnade wrote:
> Also the TWL603X devices have a charger, so allow to specify it here.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  .../devicetree/bindings/mfd/ti,twl.yaml        | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> index e94b0fd7af0f8..4064a228cb0fc 100644
> --- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> +++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> @@ -105,6 +105,11 @@ allOf:
>              regulator-initial-mode: false
>  
>        properties:
> +        bci:

charger

> +          type: object

additionalProperties: true

Each node must end with additionalProperties or unevaluated. I think you
never tested it, because dtschema reports this.

> +          properties:
> +            compatible:
> +              const: ti,twl6030-charger
>          gpadc:
>            type: object
>            properties:
> @@ -136,6 +141,13 @@ allOf:
>              regulator-initial-mode: false
>  
>        properties:
> +        bci:
> +          type: object
> +          properties:
> +            compatible:
> +              items:
> +                - const: ti,twl6032-charger
> +                - const: ti,twl6030-charger
>          gpadc:
>            type: object
>            properties:
> @@ -222,6 +234,12 @@ examples:
>          interrupt-controller;
>          #interrupt-cells = <1>;
>  
> +        bci {
> +          compatible = "ti,twl6030-charger";
> +          interrupts = <2>, <5>;
> +          monitored-battery = <&bat>;

One complete example in parent node, so you can drop example from patch #1.

> +        };
> +
>          gpadc {
>            compatible = "ti,twl6030-gpadc";
>            interrupts = <6>;

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: mfd: twl: add charger node also for TWL603x

[Andreas Kemnade]

Am Wed, 18 Sep 2024 12:47:22 +0200
schrieb Krzysztof Kozlowski <krzk@kernel.org>:

> On 18/09/2024 10:41, Andreas Kemnade wrote:
> > Also the TWL603X devices have a charger, so allow to specify it
> > here.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  .../devicetree/bindings/mfd/ti,twl.yaml        | 18
> > ++++++++++++++++++ 1 file changed, 18 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > b/Documentation/devicetree/bindings/mfd/ti,twl.yaml index
> > e94b0fd7af0f8..4064a228cb0fc 100644 ---
> > a/Documentation/devicetree/bindings/mfd/ti,twl.yaml +++
> > b/Documentation/devicetree/bindings/mfd/ti,twl.yaml @@ -105,6
> > +105,11 @@ allOf: regulator-initial-mode: false
> >  
> >        properties:
> > +        bci:  
> 
> charger
> 
> > +          type: object  
> 
> additionalProperties: true
> 
> Each node must end with additionalProperties or unevaluated. I think
> you never tested it, because dtschema reports this.
> 
I did run it, no complaints:

andi@aktux:~/kernel$ touch Documentation/devicetree/bindings/mfd/ti,twl.yaml 
andi@aktux:~/kernel$ touch Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml 
andi@aktux:~/kernel$ make ARCH=arm dt_binding_check DT_SCHEMA_FILES=twl
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/home/andi/kernel/Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition
  CHKDT   Documentation/devicetree/bindings
  LINT    Documentation/devicetree/bindings
  DTC [C] Documentation/devicetree/bindings/power/supply/twl4030-charger.example.dtb
  DTEX    Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.example.dts
  DTC [C] Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.example.dtb
  DTC [C] Documentation/devicetree/bindings/iio/adc/ti,twl6030-gpadc.example.dtb
  DTC [C] Documentation/devicetree/bindings/iio/adc/ti,twl4030-madc.example.dtb
  DTEX    Documentation/devicetree/bindings/mfd/ti,twl.example.dts
  DTC [C] Documentation/devicetree/bindings/mfd/ti,twl.example.dtb
andi@aktux:~/kernel$

Regards,
Andreas

Re: [PATCH 2/3] dt-bindings: mfd: twl: add charger node also for TWL603x

[Rob Herring]

On Wed, Sep 18, 2024 at 12:47:22PM +0200, Krzysztof Kozlowski wrote:
> On 18/09/2024 10:41, Andreas Kemnade wrote:
> > Also the TWL603X devices have a charger, so allow to specify it here.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  .../devicetree/bindings/mfd/ti,twl.yaml        | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > index e94b0fd7af0f8..4064a228cb0fc 100644
> > --- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > @@ -105,6 +105,11 @@ allOf:
> >              regulator-initial-mode: false
> >  
> >        properties:
> > +        bci:
> 
> charger
> 
> > +          type: object
> 
> additionalProperties: true
> 
> Each node must end with additionalProperties or unevaluated. I think you
> never tested it, because dtschema reports this.

This is under an if/then schema is why there's no errors.

This schema probably should have been 3 with a ti,twl-common.yaml schema 
for the common properties, but I'm not sure it is worth changing now.

Rob

Re: [PATCH 2/3] dt-bindings: mfd: twl: add charger node also for TWL603x

[Andreas Kemnade]

Am Fri, 20 Sep 2024 19:51:25 -0500
schrieb Rob Herring <robh@kernel.org>:

> On Wed, Sep 18, 2024 at 12:47:22PM +0200, Krzysztof Kozlowski wrote:
> > On 18/09/2024 10:41, Andreas Kemnade wrote:  
> > > Also the TWL603X devices have a charger, so allow to specify it
> > > here.
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > ---
> > >  .../devicetree/bindings/mfd/ti,twl.yaml        | 18
> > > ++++++++++++++++++ 1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > > b/Documentation/devicetree/bindings/mfd/ti,twl.yaml index
> > > e94b0fd7af0f8..4064a228cb0fc 100644 ---
> > > a/Documentation/devicetree/bindings/mfd/ti,twl.yaml +++
> > > b/Documentation/devicetree/bindings/mfd/ti,twl.yaml @@ -105,6
> > > +105,11 @@ allOf: regulator-initial-mode: false
> > >  
> > >        properties:
> > > +        bci:  
> > 
> > charger
> >   
> > > +          type: object  
> > 
> > additionalProperties: true
> > 
Thinking again. Why additionalProperties? unevaluatedProperties
looks more reasonable for me. There are additional properties but
they should be evaluated by another schema.

> > Each node must end with additionalProperties or unevaluated. I
> > think you never tested it, because dtschema reports this.  
> 
> This is under an if/then schema is why there's no errors.
> 
and then it just accepts anything with compatible twl6032-charger e.g.
and does not care about anything in patch 2, because it has a different
compatible.

> This schema probably should have been 3 with a ti,twl-common.yaml
> schema for the common properties, but I'm not sure it is worth
> changing now.
> 
Or a ti,twl4030.yaml and a ti,twl603X.yaml. 6030 and 6032 have more in
common than the 4030.
I would propose that is something for the next more final cleaning
up/conversion round. First I would like to avoid having drained
batteries because of no charging, so allow for more automated testing
and bisecting. 
I think I will prepare a v2 series on monday.

Regards,
Andreas

[PATCH 3/3] power: supply: initial support for TWL6030/32

[Andreas Kemnade]

Add a driver for the charger in the TWL6030/32. For now it does not report
much in sysfs but parameters are set up for USB, charging is enabled with
the specified parameters. It stops charging when full and also restarts
charging.
This prevents ending up in a system setup where you run out of battery
although a charger is plugged in after precharge completed.

Battery voltage behavior was checked via the GPADC.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/power/supply/Kconfig           |  10 +
 drivers/power/supply/Makefile          |   1 +
 drivers/power/supply/twl6030_charger.c | 566 +++++++++++++++++++++++++
 3 files changed, 577 insertions(+)
 create mode 100644 drivers/power/supply/twl6030_charger.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index bcfa63fb9f1e2..9f2eef6787f7a 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -493,6 +493,16 @@ config CHARGER_TWL4030
 	help
 	  Say Y here to enable support for TWL4030 Battery Charge Interface.
 
+config CHARGER_TWL6030
+	tristate "OMAP TWL6030 BCI charger driver"
+	depends on IIO && TWL4030_CORE
+	help
+	  Say Y here to enable support for TWL6030/6032 Battery Charge
+	  Interface.
+
+	  This driver can be build as a module. If so, the module will be
+	  called twl6030_charger.
+
 config CHARGER_LP8727
 	tristate "TI/National Semiconductor LP8727 charger driver"
 	depends on I2C
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 8dcb415453171..59c4a9f40d28a 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_CHARGER_CPCAP)	+= cpcap-charger.o
 obj-$(CONFIG_CHARGER_ISP1704)	+= isp1704_charger.o
 obj-$(CONFIG_CHARGER_MAX8903)	+= max8903_charger.o
 obj-$(CONFIG_CHARGER_TWL4030)	+= twl4030_charger.o
+obj-$(CONFIG_CHARGER_TWL6030)	+= twl6030_charger.o
 obj-$(CONFIG_CHARGER_LP8727)	+= lp8727_charger.o
 obj-$(CONFIG_CHARGER_LP8788)	+= lp8788-charger.o
 obj-$(CONFIG_CHARGER_GPIO)	+= gpio-charger.o
diff --git a/drivers/power/supply/twl6030_charger.c b/drivers/power/supply/twl6030_charger.c
new file mode 100644
index 0000000000000..5704b8ca9d22b
--- /dev/null
+++ b/drivers/power/supply/twl6030_charger.c
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TWL6030 charger
+ *
+ * Copyright (C) 2024 Andreas Kemnade <andreas@kemnade.info>
+ *
+ * based on older 6030 driver found in a v3.0 vendor kernel
+ *
+ * based on twl4030_bci_battery.c by TI
+ * Copyright (C) 2008 Texas Instruments, Inc.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/bits.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/twl.h>
+#include <linux/power_supply.h>
+#include <linux/notifier.h>
+#include <linux/usb/otg.h>
+#include <linux/iio/consumer.h>
+#include <linux/devm-helpers.h>
+
+#define CONTROLLER_INT_MASK	0x00
+#define CONTROLLER_CTRL1	0x01
+#define CONTROLLER_WDG		0x02
+#define CONTROLLER_STAT1	0x03
+#define CHARGERUSB_INT_STATUS	0x04
+#define CHARGERUSB_INT_MASK	0x05
+#define CHARGERUSB_STATUS_INT1	0x06
+#define CHARGERUSB_STATUS_INT2	0x07
+#define CHARGERUSB_CTRL1	0x08
+#define CHARGERUSB_CTRL2	0x09
+#define CHARGERUSB_CTRL3	0x0A
+#define CHARGERUSB_STAT1	0x0B
+#define CHARGERUSB_VOREG	0x0C
+#define CHARGERUSB_VICHRG	0x0D
+#define CHARGERUSB_CINLIMIT	0x0E
+#define CHARGERUSB_CTRLLIMIT1	0x0F
+#define CHARGERUSB_CTRLLIMIT2	0x10
+#define ANTICOLLAPSE_CTRL1	0x11
+#define ANTICOLLAPSE_CTRL2	0x12
+
+/* TWL6032 registers 0xDA to 0xDE - TWL6032_MODULE_CHARGER */
+#define CONTROLLER_CTRL2	0x00
+#define CONTROLLER_VSEL_COMP	0x01
+#define CHARGERUSB_VSYSREG	0x02
+#define CHARGERUSB_VICHRG_PC	0x03
+#define LINEAR_CHRG_STS		0x04
+
+#define LINEAR_CHRG_STS_CRYSTL_OSC_OK	0x40
+#define LINEAR_CHRG_STS_END_OF_CHARGE	0x20
+#define LINEAR_CHRG_STS_VBATOV		0x10
+#define LINEAR_CHRG_STS_VSYSOV		0x08
+#define LINEAR_CHRG_STS_DPPM_STS	0x04
+#define LINEAR_CHRG_STS_CV_STS		0x02
+#define LINEAR_CHRG_STS_CC_STS		0x01
+
+#define FG_REG_00	0x00
+#define FG_REG_01	0x01
+#define FG_REG_02	0x02
+#define FG_REG_03	0x03
+#define FG_REG_04	0x04
+#define FG_REG_05	0x05
+#define FG_REG_06	0x06
+#define FG_REG_07	0x07
+#define FG_REG_08	0x08
+#define FG_REG_09	0x09
+#define FG_REG_10	0x0A
+#define FG_REG_11	0x0B
+
+/* CONTROLLER_INT_MASK */
+#define MVAC_FAULT		BIT(7)
+#define MAC_EOC			BIT(6)
+#define LINCH_GATED		BIT(5)
+#define MBAT_REMOVED		BIT(4)
+#define MFAULT_WDG		BIT(3)
+#define MBAT_TEMP		BIT(2)
+#define MVBUS_DET		BIT(1)
+#define MVAC_DET		BIT(0)
+
+/* CONTROLLER_CTRL1 */
+#define CONTROLLER_CTRL1_EN_LINCH	BIT(5)
+#define CONTROLLER_CTRL1_EN_CHARGER	BIT(4)
+#define CONTROLLER_CTRL1_SEL_CHARGER	BIT(3)
+
+/* CONTROLLER_STAT1 */
+#define CONTROLLER_STAT1_EXTCHRG_STATZ	BIT(7)
+#define CONTROLLER_STAT1_LINCH_GATED	BIT(6)
+#define CONTROLLER_STAT1_CHRG_DET_N	BIT(5)
+#define CONTROLLER_STAT1_FAULT_WDG	BIT(4)
+#define CONTROLLER_STAT1_VAC_DET	BIT(3)
+#define VAC_DET	BIT(3)
+#define CONTROLLER_STAT1_VBUS_DET	BIT(2)
+#define VBUS_DET	BIT(2)
+#define CONTROLLER_STAT1_BAT_REMOVED	BIT(1)
+#define CONTROLLER_STAT1_BAT_TEMP_OVRANGE BIT(0)
+
+/* CHARGERUSB_INT_STATUS */
+#define EN_LINCH		BIT(4)
+#define CURRENT_TERM_INT	BIT(3)
+#define CHARGERUSB_STAT		BIT(2)
+#define CHARGERUSB_THMREG	BIT(1)
+#define CHARGERUSB_FAULT	BIT(0)
+
+/* CHARGERUSB_INT_MASK */
+#define MASK_MCURRENT_TERM		BIT(3)
+#define MASK_MCHARGERUSB_STAT		BIT(2)
+#define MASK_MCHARGERUSB_THMREG		BIT(1)
+#define MASK_MCHARGERUSB_FAULT		BIT(0)
+
+/* CHARGERUSB_STATUS_INT1 */
+#define CHARGERUSB_STATUS_INT1_TMREG	BIT(7)
+#define CHARGERUSB_STATUS_INT1_NO_BAT	BIT(6)
+#define CHARGERUSB_STATUS_INT1_BST_OCP	BIT(5)
+#define CHARGERUSB_STATUS_INT1_TH_SHUTD	BIT(4)
+#define CHARGERUSB_STATUS_INT1_BAT_OVP	BIT(3)
+#define CHARGERUSB_STATUS_INT1_POOR_SRC	BIT(2)
+#define CHARGERUSB_STATUS_INT1_SLP_MODE	BIT(1)
+#define CHARGERUSB_STATUS_INT1_VBUS_OVP	BIT(0)
+
+/* CHARGERUSB_STATUS_INT2 */
+#define ICCLOOP		BIT(3)
+#define CURRENT_TERM	BIT(2)
+#define CHARGE_DONE	BIT(1)
+#define ANTICOLLAPSE	BIT(0)
+
+/* CHARGERUSB_CTRL1 */
+#define SUSPEND_BOOT	BIT(7)
+#define OPA_MODE	BIT(6)
+#define HZ_MODE		BIT(5)
+#define TERM		BIT(4)
+
+/* CHARGERUSB_CTRL2 */
+#define UA_TO_VITERM(x) (((x) / 50000 - 1) << 5)
+
+/* CHARGERUSB_CTRL3 */
+#define VBUSCHRG_LDO_OVRD	BIT(7)
+#define CHARGE_ONCE		BIT(6)
+#define BST_HW_PR_DIS		BIT(5)
+#define AUTOSUPPLY		BIT(3)
+#define BUCK_HSILIM		BIT(0)
+
+/* CHARGERUSB_VOREG */
+#define UV_TO_VOREG(x) (((x) - 3500000) / 20000)
+#define VOREG_TO_UV(x) (((x) & 0x3F) * 20000 + 3500000)
+#define CHARGERUSB_VOREG_3P52		0x01
+#define CHARGERUSB_VOREG_4P0		0x19
+#define CHARGERUSB_VOREG_4P2		0x23
+#define CHARGERUSB_VOREG_4P76		0x3F
+
+/* CHARGERUSB_VICHRG */
+/*
+ * might be inaccurate for < 500 mA, diffent scale might apply,
+ * either starting from 100 mA or 300 mA
+ */
+#define UA_TO_VICHRG(x) (((x) / 100000) - 1)
+#define VICHRG_TO_UA(x) (((x) & 0xf) * 100000 + 100000)
+
+/* CHARGERUSB_CINLIMIT */
+#define CHARGERUSB_CIN_LIMIT_100	0x1
+#define CHARGERUSB_CIN_LIMIT_300	0x5
+#define CHARGERUSB_CIN_LIMIT_500	0x9
+#define CHARGERUSB_CIN_LIMIT_NONE	0xF
+
+/* CHARGERUSB_CTRLLIMIT2 */
+#define CHARGERUSB_CTRLLIMIT2_1500	0x0E
+#define		LOCK_LIMIT		BIT(4)
+
+/* ANTICOLLAPSE_CTRL2 */
+#define BUCK_VTH_SHIFT			5
+
+/* FG_REG_00 */
+#define CC_ACTIVE_MODE_SHIFT	6
+#define CC_AUTOCLEAR		BIT(2)
+#define CC_CAL_EN		BIT(1)
+#define CC_PAUSE		BIT(0)
+
+#define REG_TOGGLE1		0x90
+#define REG_PWDNSTATUS1		0x93
+#define FGDITHS			BIT(7)
+#define FGDITHR			BIT(6)
+#define FGS			BIT(5)
+#define FGR			BIT(4)
+#define BBSPOR_CFG		0xE6
+#define	BB_CHG_EN		BIT(3)
+
+struct twl6030_charger_info {
+	struct device		*dev;
+	struct power_supply	*usb;
+	struct power_supply_battery_info *binfo;
+	struct work_struct	work;
+	int			irq_chg;
+	int			input_current_limit;
+	struct iio_channel	*channel_vusb;
+	struct delayed_work	charger_monitor;
+};
+
+static int twl6030_charger_read(u8 reg, u8 *val)
+{
+	return twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE, val, reg);
+}
+
+static int twl6030_charger_write(u8 reg, u8 val)
+{
+	return twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE, val, reg);
+}
+
+static int twl6030_config_cinlimit_reg(struct twl6030_charger_info *charger,
+				       unsigned int ua)
+{
+	if (ua >= 50000 && ua <= 750000) {
+		ua = (ua - 50000) / 50000;
+	} else if ((ua > 750000) && (ua <= 1500000) &&
+		 (device_is_compatible(charger->dev, "ti,twl6032-charger"))) {
+		ua = ((ua % 100000) ? 0x30 : 0x20) + ((ua - 100000) / 100000);
+	} else {
+		if (ua < 50000) {
+			dev_err(charger->dev, "invalid input current limit\n");
+			return -EINVAL;
+		}
+		/* This is no current limit */
+		ua = 0x0F;
+	}
+
+	return twl6030_charger_write(CHARGERUSB_CINLIMIT, ua);
+}
+
+/*
+ * rewriting all stuff here, resets to extremely conservative defaults were
+ * seen under some circumstances, like charge voltage to 3.5V
+ */
+static int twl6030_enable_usb(struct twl6030_charger_info *charger)
+{
+	int ret;
+
+	ret = twl6030_charger_write(CHARGERUSB_VICHRG,
+				    UA_TO_VICHRG(charger->binfo->constant_charge_current_max_ua));
+	if (ret < 0)
+		return ret;
+
+	ret = twl6030_charger_write(CONTROLLER_WDG, 0xff);
+	if (ret < 0)
+		return ret;
+
+	charger->input_current_limit = 500000;
+	ret = twl6030_config_cinlimit_reg(charger, charger->input_current_limit);
+	if (ret < 0)
+		return ret;
+
+	ret = twl6030_charger_write(CHARGERUSB_CINLIMIT, CHARGERUSB_CIN_LIMIT_500);
+	if (ret < 0)
+		return ret;
+
+	ret = twl6030_charger_write(CHARGERUSB_VOREG,
+				    UV_TO_VOREG(charger->binfo->constant_charge_voltage_max_uv));
+	if (ret < 0)
+		return ret;
+
+	ret = twl6030_charger_write(CHARGERUSB_CTRL1, TERM);
+	if (ret < 0)
+		return ret;
+
+	if (charger->binfo->charge_term_current_ua != -EINVAL) {
+		ret = twl6030_charger_write(CHARGERUSB_CTRL2,
+					    UA_TO_VITERM(charger->binfo->charge_term_current_ua));
+		if (ret < 0)
+			return ret;
+	}
+
+	return twl6030_charger_write(CONTROLLER_CTRL1, CONTROLLER_CTRL1_EN_CHARGER);
+}
+
+static void twl6030_charger_wdg(struct work_struct *data)
+{
+	struct twl6030_charger_info *charger =
+		container_of(data, struct twl6030_charger_info,
+			     charger_monitor.work);
+
+	u8 val;
+	u8 int_stat;
+	u8 stat_int1;
+	u8 stat_int2;
+
+	twl6030_charger_read(CONTROLLER_STAT1, &val);
+	twl6030_charger_read(CHARGERUSB_INT_STATUS, &int_stat);
+	twl6030_charger_read(CHARGERUSB_STATUS_INT1, &stat_int1);
+	twl6030_charger_read(CHARGERUSB_STATUS_INT2, &stat_int2);
+	dev_dbg(charger->dev,
+		"wdg: stat1: %02x %s INT_STATUS %02x STATUS_INT1 %02x STATUS_INT2 %02x\n",
+		val, (val & VBUS_DET) ? "usb online" :  "usb offline",
+		int_stat, stat_int1, stat_int2);
+
+	twl6030_charger_write(CONTROLLER_WDG, 0xff);
+	schedule_delayed_work(&charger->charger_monitor,
+			      msecs_to_jiffies(10000));
+}
+
+static irqreturn_t twl6030_charger_interrupt(int irq, void *arg)
+{
+	struct twl6030_charger_info *charger = arg;
+	u8 val;
+	u8 int_stat;
+	u8 stat_int1;
+	u8 stat_int2;
+
+	if (twl6030_charger_read(CONTROLLER_STAT1, &val) < 0)
+		return IRQ_HANDLED;
+
+	if (twl6030_charger_read(CHARGERUSB_INT_STATUS, &int_stat) < 0)
+		return IRQ_HANDLED;
+
+	if (twl6030_charger_read(CHARGERUSB_STATUS_INT1, &stat_int1) < 0)
+		return IRQ_HANDLED;
+
+	if (twl6030_charger_read(CHARGERUSB_STATUS_INT2, &stat_int2) < 0)
+		return IRQ_HANDLED;
+
+	dev_dbg(charger->dev,
+		"charger irq: stat1: %02x %s INT_STATUS %02x STATUS_INT1 %02x STATUS_INT2 %02x\n",
+		val, (val & VBUS_DET) ? "usb online" :  "usb offline",
+		int_stat, stat_int1, stat_int2);
+	power_supply_changed(charger->usb);
+
+	if (val & VBUS_DET) {
+		if (twl6030_charger_read(CONTROLLER_CTRL1, &val) < 0)
+			return IRQ_HANDLED;
+
+		if (!(val & CONTROLLER_CTRL1_EN_CHARGER)) {
+			if (twl6030_enable_usb(charger) < 0)
+				return IRQ_HANDLED;
+
+			schedule_delayed_work(&charger->charger_monitor,
+					      msecs_to_jiffies(10000));
+		}
+	} else {
+		cancel_delayed_work(&charger->charger_monitor);
+	}
+	return IRQ_HANDLED;
+}
+
+static int twl6030_charger_usb_get_property(struct power_supply *psy,
+					    enum power_supply_property psp,
+					    union power_supply_propval *val)
+{
+	struct twl6030_charger_info *charger = power_supply_get_drvdata(psy);
+	int ret;
+	u8 stat1;
+
+	ret = twl6030_charger_read(CONTROLLER_STAT1, &stat1);
+	if (ret)
+		return ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		u8 intstat;
+
+		if (!(stat1 & VBUS_DET)) {
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+			break;
+		}
+		ret = twl6030_charger_read(CHARGERUSB_STATUS_INT2, &intstat);
+		if (ret)
+			return ret;
+
+		if (intstat & CHARGE_DONE)
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+		else if (intstat & CURRENT_TERM)
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		else
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		if (!charger->channel_vusb)
+			return -ENODATA;
+
+		ret = iio_read_channel_processed_scale(charger->channel_vusb, &val->intval, 1000);
+		if (ret < 0)
+			return ret;
+
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = !!(stat1 & VBUS_DET);
+		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		val->intval = charger->input_current_limit;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int twl6030_charger_usb_set_property(struct power_supply *psy,
+					    enum power_supply_property psp,
+					    const union power_supply_propval *val)
+{
+	struct twl6030_charger_info *charger = power_supply_get_drvdata(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		charger->input_current_limit = val->intval;
+		return twl6030_config_cinlimit_reg(charger, charger->input_current_limit);
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int twl6030_charger_usb_property_is_writeable(struct power_supply *psy,
+						     enum power_supply_property psp)
+{
+	dev_info(&psy->dev, "is %d writeable?\n", (int)psp);
+	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static enum power_supply_property twl6030_charger_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+};
+
+static const struct power_supply_desc twl6030_charger_usb_desc = {
+	.name		= "twl6030_usb",
+	.type		= POWER_SUPPLY_TYPE_USB,
+	.properties	= twl6030_charger_props,
+	.num_properties	= ARRAY_SIZE(twl6030_charger_props),
+	.get_property	= twl6030_charger_usb_get_property,
+	.set_property	= twl6030_charger_usb_set_property,
+	.property_is_writeable	= twl6030_charger_usb_property_is_writeable,
+};
+
+static int twl6030_charger_probe(struct platform_device *pdev)
+{
+	struct twl6030_charger_info *charger;
+	struct power_supply_config psy_cfg = {};
+	int ret;
+	u8 val;
+
+	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
+	if (!charger)
+		return -ENOMEM;
+
+	charger->dev = &pdev->dev;
+	charger->irq_chg = platform_get_irq(pdev, 0);
+
+	platform_set_drvdata(pdev, charger);
+	psy_cfg.drv_data = charger;
+
+	charger->channel_vusb = devm_iio_channel_get(&pdev->dev, "vusb");
+	if (IS_ERR(charger->channel_vusb)) {
+		ret = PTR_ERR(charger->channel_vusb);
+		if (ret == -EPROBE_DEFER)
+			return ret;	/* iio not ready */
+		dev_warn(&pdev->dev, "could not request vusb iio channel (%d)",
+			 ret);
+		charger->channel_vusb = NULL;
+	}
+
+	charger->usb = devm_power_supply_register(&pdev->dev,
+						  &twl6030_charger_usb_desc,
+						  &psy_cfg);
+	if (IS_ERR(charger->usb)) {
+		return dev_err_probe(&pdev->dev, PTR_ERR(charger->usb),
+				     "Failed to register usb\n");
+	}
+
+	ret = power_supply_get_battery_info(charger->usb, &charger->binfo);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to get battery info\n");
+
+	dev_info(&pdev->dev, "battery with vmax %d imax: %d\n",
+		 charger->binfo->constant_charge_voltage_max_uv,
+		 charger->binfo->constant_charge_current_max_ua);
+
+	if (charger->binfo->constant_charge_voltage_max_uv == -EINVAL) {
+		ret = twl6030_charger_read(CHARGERUSB_CTRLLIMIT1, &val);
+		if (ret < 0)
+			return ret;
+
+		charger->binfo->constant_charge_voltage_max_uv =
+			VOREG_TO_UV(val);
+	}
+
+	if (charger->binfo->constant_charge_voltage_max_uv > 4760000 ||
+	    charger->binfo->constant_charge_voltage_max_uv < 350000)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "Invalid charge voltage\n");
+
+	if (charger->binfo->constant_charge_current_max_ua == -EINVAL) {
+		ret = twl6030_charger_read(CHARGERUSB_CTRLLIMIT2, &val);
+		if (ret < 0)
+			return ret;
+
+		charger->binfo->constant_charge_current_max_ua = VICHRG_TO_UA(val);
+	}
+
+	if (charger->binfo->constant_charge_current_max_ua < 100000 ||
+	    charger->binfo->constant_charge_current_max_ua > 1500000) {
+		return dev_err_probe(&pdev->dev, -EINVAL,
+			 "Invalid charge current\n");
+	}
+
+	if ((charger->binfo->charge_term_current_ua != -EINVAL) &&
+	    (charger->binfo->charge_term_current_ua > 400000 ||
+	     charger->binfo->charge_term_current_ua < 50000)) {
+		return dev_err_probe(&pdev->dev, -EINVAL,
+			"Invalid charge termination current\n");
+	}
+
+	ret = devm_delayed_work_autocancel(&pdev->dev,
+					   &charger->charger_monitor,
+					   twl6030_charger_wdg);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to register delayed work\n");
+
+	ret = devm_request_threaded_irq(&pdev->dev, charger->irq_chg, NULL,
+					twl6030_charger_interrupt,
+					IRQF_ONESHOT, pdev->name,
+					charger);
+	if (ret < 0) {
+		return dev_err_probe(&pdev->dev, ret,
+				     "could not request irq %d\n",
+				     charger->irq_chg);
+	}
+
+	/* turing to charging to configure things */
+	twl6030_charger_write(CONTROLLER_CTRL1, 0);
+	twl6030_charger_interrupt(0, charger);
+
+	return 0;
+}
+
+static const struct of_device_id twl_charger_of_match[] __maybe_unused = {
+	{.compatible = "ti,twl6030-charger", },
+	{.compatible = "ti,twl6032-charger", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, twl_charger_of_match);
+
+static struct platform_driver twl6030_charger_driver = {
+	.probe = twl6030_charger_probe,
+	.driver	= {
+		.name	= "twl6030_charger",
+		.of_match_table = of_match_ptr(twl_charger_of_match),
+	},
+};
+module_platform_driver(twl6030_charger_driver);
+
+MODULE_DESCRIPTION("TWL6030 Battery Charger Interface driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:twl6030_charger");
-- 
2.39.2

Re: [PATCH 3/3] power: supply: initial support for TWL6030/32

[Krzysztof Kozlowski]

On 18/09/2024 10:41, Andreas Kemnade wrote:
> Add a driver for the charger in the TWL6030/32. For now it does not report
> much in sysfs but parameters are set up for USB, charging is enabled with
> the specified parameters. It stops charging when full and also restarts
> charging.
> This prevents ending up in a system setup where you run out of battery
> although a charger is plugged in after precharge completed.
> 
> Battery voltage behavior was checked via the GPADC.
> 

Few stylistic comments below.

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/power/supply/Kconfig           |  10 +
>  drivers/power/supply/Makefile          |   1 +
>  drivers/power/supply/twl6030_charger.c | 566 +++++++++++++++++++++++++
>  3 files changed, 577 insertions(+)
>  create mode 100644 drivers/power/supply/twl6030_charger.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index bcfa63fb9f1e2..9f2eef6787f7a 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -493,6 +493,16 @@ config CHARGER_TWL4030
>  	help
>  	  Say Y here to enable support for TWL4030 Battery Charge Interface.
>  
> +config CHARGER_TWL6030
> +	tristate "OMAP TWL6030 BCI charger driver"
> +	depends on IIO && TWL4030_CORE

|| COMPILE_TEST, at least for TWL part
(but please test first)

> +	help
> +	  Say Y here to enable support for TWL6030/6032 Battery Charge
> +	  Interface.
> +
> +	  This driver can be build as a module. If so, the module will be
> +	  called twl6030_charger.
> +



> +
> +static int twl6030_charger_probe(struct platform_device *pdev)
> +{
> +	struct twl6030_charger_info *charger;
> +	struct power_supply_config psy_cfg = {};
> +	int ret;
> +	u8 val;
> +
> +	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> +	if (!charger)
> +		return -ENOMEM;
> +
> +	charger->dev = &pdev->dev;
> +	charger->irq_chg = platform_get_irq(pdev, 0);
> +
> +	platform_set_drvdata(pdev, charger);
> +	psy_cfg.drv_data = charger;
> +
> +	charger->channel_vusb = devm_iio_channel_get(&pdev->dev, "vusb");
> +	if (IS_ERR(charger->channel_vusb)) {
> +		ret = PTR_ERR(charger->channel_vusb);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;	/* iio not ready */
> +		dev_warn(&pdev->dev, "could not request vusb iio channel (%d)",
> +			 ret);
> +		charger->channel_vusb = NULL;
> +	}
> +
> +	charger->usb = devm_power_supply_register(&pdev->dev,
> +						  &twl6030_charger_usb_desc,
> +						  &psy_cfg);
> +	if (IS_ERR(charger->usb)) {

Checkpatch...

> +		return dev_err_probe(&pdev->dev, PTR_ERR(charger->usb),
> +				     "Failed to register usb\n");
> +	}
> +
> +	ret = power_supply_get_battery_info(charger->usb, &charger->binfo);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to get battery info\n");
> +
> +	dev_info(&pdev->dev, "battery with vmax %d imax: %d\n",
> +		 charger->binfo->constant_charge_voltage_max_uv,
> +		 charger->binfo->constant_charge_current_max_ua);
> +
> +	if (charger->binfo->constant_charge_voltage_max_uv == -EINVAL) {
> +		ret = twl6030_charger_read(CHARGERUSB_CTRLLIMIT1, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		charger->binfo->constant_charge_voltage_max_uv =
> +			VOREG_TO_UV(val);
> +	}
> +
> +	if (charger->binfo->constant_charge_voltage_max_uv > 4760000 ||
> +	    charger->binfo->constant_charge_voltage_max_uv < 350000)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "Invalid charge voltage\n");
> +
> +	if (charger->binfo->constant_charge_current_max_ua == -EINVAL) {
> +		ret = twl6030_charger_read(CHARGERUSB_CTRLLIMIT2, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		charger->binfo->constant_charge_current_max_ua = VICHRG_TO_UA(val);
> +	}
> +
> +	if (charger->binfo->constant_charge_current_max_ua < 100000 ||
> +	    charger->binfo->constant_charge_current_max_ua > 1500000) {
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +			 "Invalid charge current\n");
> +	}
> +
> +	if ((charger->binfo->charge_term_current_ua != -EINVAL) &&
> +	    (charger->binfo->charge_term_current_ua > 400000 ||
> +	     charger->binfo->charge_term_current_ua < 50000)) {
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +			"Invalid charge termination current\n");
> +	}
> +
> +	ret = devm_delayed_work_autocancel(&pdev->dev,
> +					   &charger->charger_monitor,
> +					   twl6030_charger_wdg);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to register delayed work\n");
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, charger->irq_chg, NULL,
> +					twl6030_charger_interrupt,
> +					IRQF_ONESHOT, pdev->name,
> +					charger);
> +	if (ret < 0) {

Drop {}, see checkpatch.

> +		return dev_err_probe(&pdev->dev, ret,
> +				     "could not request irq %d\n",
> +				     charger->irq_chg);
> +	}
> +
> +	/* turing to charging to configure things */
> +	twl6030_charger_write(CONTROLLER_CTRL1, 0);
> +	twl6030_charger_interrupt(0, charger);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id twl_charger_of_match[] __maybe_unused = {
> +	{.compatible = "ti,twl6030-charger", },
> +	{.compatible = "ti,twl6032-charger", },

So they are compatible? Why two entries in such case?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, twl_charger_of_match);
> +
> +static struct platform_driver twl6030_charger_driver = {
> +	.probe = twl6030_charger_probe,
> +	.driver	= {
> +		.name	= "twl6030_charger",
> +		.of_match_table = of_match_ptr(twl_charger_of_match),

I propose to drop of_match_ptr and maybe_unused, so this won't be
restricted only to OF

> +	},
> +};
> +module_platform_driver(twl6030_charger_driver);
> +
> +MODULE_DESCRIPTION("TWL6030 Battery Charger Interface driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:twl6030_charger");


You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


Best regards,
Krzysztof

Re: [PATCH 3/3] power: supply: initial support for TWL6030/32

[Andreas Kemnade]

Am Wed, 18 Sep 2024 12:43:01 +0200
schrieb Krzysztof Kozlowski <krzk@kernel.org>:

[...]
> Drop {}, see checkpatch.
> 
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "could not request irq %d\n",
> > +				     charger->irq_chg);
> > +	}
> > +

Apparently checkpatch only moans about {} around single *lines*
not single *statements*, even with --strict.

Coding-style says single statements,  so maybe checkpatch should be
fixed?

Same for other appearance of this pattern.

> > +	/* turing to charging to configure things */
> > +	twl6030_charger_write(CONTROLLER_CTRL1, 0);
> > +	twl6030_charger_interrupt(0, charger);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id twl_charger_of_match[]
> > __maybe_unused = {
> > +	{.compatible = "ti,twl6030-charger", },
> > +	{.compatible = "ti,twl6032-charger", },  
> 
> So they are compatible? Why two entries in such case?
> 
There is one device_is_compatible() in the file.

Regrads,
Andreas

Re: [PATCH 3/3] power: supply: initial support for TWL6030/32

[Krzysztof Kozlowski]

On 18/09/2024 14:43, Andreas Kemnade wrote:
> Am Wed, 18 Sep 2024 12:43:01 +0200
> schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> 
> [...]
>> Drop {}, see checkpatch.
>>
>>> +		return dev_err_probe(&pdev->dev, ret,
>>> +				     "could not request irq %d\n",
>>> +				     charger->irq_chg);
>>> +	}
>>> +
> 
> Apparently checkpatch only moans about {} around single *lines*
> not single *statements*, even with --strict.
> 
> Coding-style says single statements,  so maybe checkpatch should be
> fixed?
> 
> Same for other appearance of this pattern.

Hm, could be. I think this still should be without {}, regardless of
checkpatch.

> 
>>> +	/* turing to charging to configure things */
>>> +	twl6030_charger_write(CONTROLLER_CTRL1, 0);
>>> +	twl6030_charger_interrupt(0, charger);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id twl_charger_of_match[]
>>> __maybe_unused = {
>>> +	{.compatible = "ti,twl6030-charger", },
>>> +	{.compatible = "ti,twl6032-charger", },  
>>
>> So they are compatible? Why two entries in such case?
>>
> There is one device_is_compatible() in the file.

Ah, you should rather use match data. Compatibles inside the code do not
scale.



> 
> Regrads,
> Andreas

Best regards,
Krzysztof

Re: [PATCH 3/3] power: supply: initial support for TWL6030/32

[Andreas Kemnade]

Am Wed, 18 Sep 2024 12:43:01 +0200
schrieb Krzysztof Kozlowski <krzk@kernel.org>:

> On 18/09/2024 10:41, Andreas Kemnade wrote:
> > Add a driver for the charger in the TWL6030/32. For now it does not
> > report much in sysfs but parameters are set up for USB, charging is
> > enabled with the specified parameters. It stops charging when full
> > and also restarts charging.
> > This prevents ending up in a system setup where you run out of
> > battery although a charger is plugged in after precharge completed.
> > 
> > Battery voltage behavior was checked via the GPADC.
> >   
> 
> Few stylistic comments below.
> 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  drivers/power/supply/Kconfig           |  10 +
> >  drivers/power/supply/Makefile          |   1 +
> >  drivers/power/supply/twl6030_charger.c | 566
> > +++++++++++++++++++++++++ 3 files changed, 577 insertions(+)
> >  create mode 100644 drivers/power/supply/twl6030_charger.c
> > 
> > diff --git a/drivers/power/supply/Kconfig
> > b/drivers/power/supply/Kconfig index bcfa63fb9f1e2..9f2eef6787f7a
> > 100644 --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -493,6 +493,16 @@ config CHARGER_TWL4030
> >  	help
> >  	  Say Y here to enable support for TWL4030 Battery Charge
> > Interface. 
> > +config CHARGER_TWL6030
> > +	tristate "OMAP TWL6030 BCI charger driver"
> > +	depends on IIO && TWL4030_CORE  
> 
> || COMPILE_TEST, at least for TWL part
> (but please test first)
> 
ERROR: modpost: "twl_i2c_write"
[drivers/power/supply/twl6030_charger.ko] undefined! ERROR: modpost:
"twl_i2c_read" [drivers/power/supply/twl6030_charger.ko] undefined!

>
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, twl_charger_of_match);
> > +
> > +static struct platform_driver twl6030_charger_driver = {
> > +	.probe = twl6030_charger_probe,
> > +	.driver	= {
> > +		.name	= "twl6030_charger",
> > +		.of_match_table =
> > of_match_ptr(twl_charger_of_match),  
> 
> I propose to drop of_match_ptr and maybe_unused, so this won't be
> restricted only to OF
>
so that more things get compile-tested without OF? But I see no reason
why .probe would be optimized away (and with it a lot more things) by
the compiler.

Regards,
Andreas

Re: [PATCH 3/3] power: supply: initial support for TWL6030/32

[Krzysztof Kozlowski]

On 23/09/2024 18:29, Andreas Kemnade wrote:
> Am Wed, 18 Sep 2024 12:43:01 +0200
> schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> 
>> On 18/09/2024 10:41, Andreas Kemnade wrote:
>>> Add a driver for the charger in the TWL6030/32. For now it does not
>>> report much in sysfs but parameters are set up for USB, charging is
>>> enabled with the specified parameters. It stops charging when full
>>> and also restarts charging.
>>> This prevents ending up in a system setup where you run out of
>>> battery although a charger is plugged in after precharge completed.
>>>
>>> Battery voltage behavior was checked via the GPADC.
>>>   
>>
>> Few stylistic comments below.
>>
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>> ---
>>>  drivers/power/supply/Kconfig           |  10 +
>>>  drivers/power/supply/Makefile          |   1 +
>>>  drivers/power/supply/twl6030_charger.c | 566
>>> +++++++++++++++++++++++++ 3 files changed, 577 insertions(+)
>>>  create mode 100644 drivers/power/supply/twl6030_charger.c
>>>
>>> diff --git a/drivers/power/supply/Kconfig
>>> b/drivers/power/supply/Kconfig index bcfa63fb9f1e2..9f2eef6787f7a
>>> 100644 --- a/drivers/power/supply/Kconfig
>>> +++ b/drivers/power/supply/Kconfig
>>> @@ -493,6 +493,16 @@ config CHARGER_TWL4030
>>>  	help
>>>  	  Say Y here to enable support for TWL4030 Battery Charge
>>> Interface. 
>>> +config CHARGER_TWL6030
>>> +	tristate "OMAP TWL6030 BCI charger driver"
>>> +	depends on IIO && TWL4030_CORE  
>>
>> || COMPILE_TEST, at least for TWL part
>> (but please test first)
>>
> ERROR: modpost: "twl_i2c_write"
> [drivers/power/supply/twl6030_charger.ko] undefined! ERROR: modpost:
> "twl_i2c_read" [drivers/power/supply/twl6030_charger.ko] undefined!

ok

> 
>>
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, twl_charger_of_match);
>>> +
>>> +static struct platform_driver twl6030_charger_driver = {
>>> +	.probe = twl6030_charger_probe,
>>> +	.driver	= {
>>> +		.name	= "twl6030_charger",
>>> +		.of_match_table =
>>> of_match_ptr(twl_charger_of_match),  
>>
>> I propose to drop of_match_ptr and maybe_unused, so this won't be
>> restricted only to OF
>>
> so that more things get compile-tested without OF? But I see no reason
> why .probe would be optimized away (and with it a lot more things) by
> the compiler.

No, it is not probe related by ACPI PRP0001

Best regards,
Krzysztof

Re: [PATCH 3/3] power: supply: initial support for TWL6030/32

[kernel test robot]

Hi Andreas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on sre-power-supply/for-next]
[also build test WARNING on lee-mfd/for-mfd-next lee-leds/for-leds-next linus/master v6.11 next-20240918]
[cannot apply to lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andreas-Kemnade/dt-bindings-power-supply-Add-TI-TWL603X-charger/20240918-164406
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20240918084132.928295-4-andreas%40kemnade.info
patch subject: [PATCH 3/3] power: supply: initial support for TWL6030/32
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20240919/202409190409.0mOjgpq4-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240919/202409190409.0mOjgpq4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409190409.0mOjgpq4-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/power/supply/twl6030_charger.c:20:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/power/supply/twl6030_charger.c:20:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/power/supply/twl6030_charger.c:20:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   In file included from drivers/power/supply/twl6030_charger.c:24:
   In file included from include/linux/usb/otg.h:13:
   In file included from include/linux/phy/phy.h:17:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/power/supply/twl6030_charger.c:360:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
     360 |                 u8 intstat;
         |                 ^
   8 warnings generated.


vim +360 drivers/power/supply/twl6030_charger.c

   345	
   346	static int twl6030_charger_usb_get_property(struct power_supply *psy,
   347						    enum power_supply_property psp,
   348						    union power_supply_propval *val)
   349	{
   350		struct twl6030_charger_info *charger = power_supply_get_drvdata(psy);
   351		int ret;
   352		u8 stat1;
   353	
   354		ret = twl6030_charger_read(CONTROLLER_STAT1, &stat1);
   355		if (ret)
   356			return ret;
   357	
   358		switch (psp) {
   359		case POWER_SUPPLY_PROP_STATUS:
 > 360			u8 intstat;
   361	
   362			if (!(stat1 & VBUS_DET)) {
   363				val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
   364				break;
   365			}
   366			ret = twl6030_charger_read(CHARGERUSB_STATUS_INT2, &intstat);
   367			if (ret)
   368				return ret;
   369	
   370			if (intstat & CHARGE_DONE)
   371				val->intval = POWER_SUPPLY_STATUS_FULL;
   372			else if (intstat & CURRENT_TERM)
   373				val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
   374			else
   375				val->intval = POWER_SUPPLY_STATUS_CHARGING;
   376			break;
   377		case POWER_SUPPLY_PROP_VOLTAGE_NOW:
   378			if (!charger->channel_vusb)
   379				return -ENODATA;
   380	
   381			ret = iio_read_channel_processed_scale(charger->channel_vusb, &val->intval, 1000);
   382			if (ret < 0)
   383				return ret;
   384	
   385			break;
   386		case POWER_SUPPLY_PROP_ONLINE:
   387			val->intval = !!(stat1 & VBUS_DET);
   388			break;
   389		case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
   390			val->intval = charger->input_current_limit;
   391			break;
   392		default:
   393			return -EINVAL;
   394		}
   395	
   396		return 0;
   397	}
   398	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki