| Message ID | 20241118-add_charger_state-v1-2-94997079f35a@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce new driver cros-ec-charge-state | expand |
On Mon, 18 Nov 2024 17:33:47 +0800, Sung-Chi, Li wrote: > Add new dt bindings for charge chip control. The charge chip control > dt configuration is used by the driver 'cros-ec-charge-state', which is > added in the commit "platform/chrome: cros_ec_charge_state: add new > driver to control charge". > > As these charge chip controls are connected under the ChromeOS Embedded > Controller (EC), also add the patternProperties to the > mfd/google,cros-ec bindings. > > Signed-off-by: Sung-Chi, Li <lschyi@chromium.org> > --- > .../bindings/chrome/google,cros-charge-state.yaml | 62 ++++++++++++++++++++++ > .../devicetree/bindings/mfd/google,cros-ec.yaml | 4 ++ > 2 files changed, 66 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/chrome/google,cros-charge-state.example.dtb: ec@0: charge-chip-battery:compatible:0: 'google,cros-ec-charge-state' was expected from schema $id: http://devicetree.org/schemas/mfd/google,cros-ec.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/chrome/google,cros-charge-state.example.dtb: ec@0: charge-chip-battery: 'man-milliamp' is a required property from schema $id: http://devicetree.org/schemas/mfd/google,cros-ec.yaml# Documentation/devicetree/bindings/chrome/google,cros-charge-state.example.dtb: /example-0/spi/ec@0/charge-chip-battery: failed to match any schema with compatible: ['google,cros-ec-charge'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241118-add_charger_state-v1-2-94997079f35a@chromium.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Mon, Nov 18, 2024 at 05:33:47PM +0800, Sung-Chi, Li wrote: > Add new dt bindings for charge chip control. The charge chip control > dt configuration is used by the driver 'cros-ec-charge-state', which is > added in the commit "platform/chrome: cros_ec_charge_state: add new > driver to control charge". > > As these charge chip controls are connected under the ChromeOS Embedded > Controller (EC), also add the patternProperties to the > mfd/google,cros-ec bindings. > > Signed-off-by: Sung-Chi, Li <lschyi@chromium.org> > --- > .../bindings/chrome/google,cros-charge-state.yaml | 62 ++++++++++++++++++++++ > .../devicetree/bindings/mfd/google,cros-ec.yaml | 4 ++ > 2 files changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml > new file mode 100644 > index 000000000000..40e8f6988769 > --- /dev/null > +++ b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml > @@ -0,0 +1,62 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/chrome/google,cros-charge-state.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Google Chrome OS EC(Embedded Controller) charge state driver. > + > +maintainers: > + - Sung-Chi, Li <lschyi@chromium.org> > + > +description: > + Chrome OS devices have an Embedded Controller(EC) which has access to > + battery charger IC. This node is intended to allow the host to read and > + control the charger current. The node for this device should be under a > + cros-ec node like google,cros-ec-spi. > + > +properties: > + compatible: > + const: google,cros-ec-charge-state > + > + min-milliamp: > + description: min current in milliamp. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + max-milliamp: > + description: max current in milliamp. > + $ref: /schemas/types.yaml#/definitions/uint32 Use standard units defined in property-units.yaml. No constraints? 4000000 amps is okay? > + > + type: Too generic. Property types are global. You need a vendor prefix for starters. > + description: current limit type. > + enum: > + - charge > + - input What if you need to describe both? > + > +required: > + - compatible > + - min-milliamp > + - man-milliamp > + - type > + > +additionalProperties: false > + > +examples: > + - |+ > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + cros_ec: ec@0 { > + compatible = "google,cros-ec-spi"; > + reg = <0>; > + interrupts = <35 0>; > + > + charge_chip_battery_current: charge-chip-battery { > + compatible = "google,cros-ec-charge"; > + type = "charge"; > + min-milliamp = <150>; > + max-milliamp = <5000>; > + }; > + }; > + }; > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > index aac8819bd00b..3db4a48d5176 100644 > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > @@ -166,6 +166,10 @@ patternProperties: > type: object > $ref: /schemas/extcon/extcon-usbc-cros-ec.yaml# > > + "^charge-chip-*": > + type: object > + $ref: /schemas/chrome/google,cros-charge-state.yaml# > + > required: > - compatible > > > -- > 2.47.0.338.g60cca15819-goog >
On Mon, Nov 18, 2024 at 02:25:20PM -0600, Rob Herring wrote: > > +properties: > > + compatible: > > + const: google,cros-ec-charge-state > > + > > + min-milliamp: > > + description: min current in milliamp. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + > > + max-milliamp: > > + description: max current in milliamp. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > Use standard units defined in property-units.yaml. > > No constraints? 4000000 amps is okay? > Hi, I cannot find a good value as the max value, the will depend on what charge chip is used on that device. This is like a upper bound set from the kernel side, so setting it to the max uint32 value is acceptable (from the driver side when loading this config), not the desired current value. It is equivalent to kernel side do not specify any constraint. Surely, if kernel set a value that is larger than the max value, the EC will reject that request, either. The real current is bounded by the EC and the charge chip, so it will not damage any hardware. If we can find a meaningful constraint value to document it here, that would be great. Would it be sufficient that I add the explanation to the description? > > + > > + type: > > Too generic. Property types are global. You need a vendor prefix for > starters. > Thank you, I will use a more specific name in the following patches. > > + description: current limit type. > > + enum: > > + - charge > > + - input > > What if you need to describe both? > We need to declare different DTS nods for each. This node is representing the constraint, not the charge chip itself. The voltage, min and max milliamp on each current type are different on a single charge chip. For example, I have a device that uses the charge chip rt9490, and it has the following set up: - Input current - min-milliamp: 100 - max-milliamp: 3300 - Charge current - min-milliamp: 150 - max-milliamp: 5000 I cannot find a clean way to merge different current type, max, and min milliamp just in a single DTS node. Also, we need to split different constraints into its own DTS node. It is because the a cooling device in the thermal framework need its own DTS node, so we can use it in the trip section.
On 19/11/2024 03:23, Sung-Chi, Li wrote: > >>> + >>> + type: >> >> Too generic. Property types are global. You need a vendor prefix for >> starters. >> > > Thank you, I will use a more specific name in the following patches. > >>> + description: current limit type. >>> + enum: >>> + - charge >>> + - input >> >> What if you need to describe both? >> > > We need to declare different DTS nods for each. This node is representing the > constraint, not the charge chip itself. Looks like you are re-implementing charger manager. Even title says: driver. Use standard psy properties including battery. All this is supposed to describe hardware, not your driver. > The voltage, min and max milliamp on each current type are different on a single > charge chip. For example, I have a device that uses the charge chip rt9490, and > it has the following set up: > > - Input current > - min-milliamp: 100 > - max-milliamp: 3300 > - Charge current > - min-milliamp: 150 > - max-milliamp: 5000 > > I cannot find a clean way to merge different current type, max, and min milliamp > just in a single DTS node. Well, all other bindings were able, so I really do not get why this one is so special. > Also, we need to split different constraints into its own DTS node. It is > because the a cooling device in the thermal framework need its own DTS node, so > we can use it in the trip section. So fix thermal framework. Best regards, Krzysztof
On 18/11/2024 10:33, Sung-Chi, Li wrote: > Add new dt bindings for charge chip control. The charge chip control > dt configuration is used by the driver 'cros-ec-charge-state', which is > added in the commit "platform/chrome: cros_ec_charge_state: add new > driver to control charge". > > As these charge chip controls are connected under the ChromeOS Embedded > Controller (EC), also add the patternProperties to the > mfd/google,cros-ec bindings. > > Signed-off-by: Sung-Chi, Li <lschyi@chromium.org> > --- > .../bindings/chrome/google,cros-charge-state.yaml | 62 ++++++++++++++++++++++ > .../devicetree/bindings/mfd/google,cros-ec.yaml | 4 ++ > 2 files changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml > new file mode 100644 > index 000000000000..40e8f6988769 > --- /dev/null > +++ b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml > @@ -0,0 +1,62 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/chrome/google,cros-charge-state.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Google Chrome OS EC(Embedded Controller) charge state driver. Capitalize, drop driver, drop full stop. ... > +examples: > + - |+ No need for + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + cros_ec: ec@0 { > + compatible = "google,cros-ec-spi"; > + reg = <0>; > + interrupts = <35 0>; > + > + charge_chip_battery_current: charge-chip-battery { 1. Drop unused label. 2. So this is a battery? Then just "battery"... or this is a charger? Please look how power supplies are done. This should not be different. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml new file mode 100644 index 000000000000..40e8f6988769 --- /dev/null +++ b/Documentation/devicetree/bindings/chrome/google,cros-charge-state.yaml @@ -0,0 +1,62 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/chrome/google,cros-charge-state.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Google Chrome OS EC(Embedded Controller) charge state driver. + +maintainers: + - Sung-Chi, Li <lschyi@chromium.org> + +description: + Chrome OS devices have an Embedded Controller(EC) which has access to + battery charger IC. This node is intended to allow the host to read and + control the charger current. The node for this device should be under a + cros-ec node like google,cros-ec-spi. + +properties: + compatible: + const: google,cros-ec-charge-state + + min-milliamp: + description: min current in milliamp. + $ref: /schemas/types.yaml#/definitions/uint32 + + max-milliamp: + description: max current in milliamp. + $ref: /schemas/types.yaml#/definitions/uint32 + + type: + description: current limit type. + enum: + - charge + - input + +required: + - compatible + - min-milliamp + - man-milliamp + - type + +additionalProperties: false + +examples: + - |+ + spi { + #address-cells = <1>; + #size-cells = <0>; + + cros_ec: ec@0 { + compatible = "google,cros-ec-spi"; + reg = <0>; + interrupts = <35 0>; + + charge_chip_battery_current: charge-chip-battery { + compatible = "google,cros-ec-charge"; + type = "charge"; + min-milliamp = <150>; + max-milliamp = <5000>; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml index aac8819bd00b..3db4a48d5176 100644 --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml @@ -166,6 +166,10 @@ patternProperties: type: object $ref: /schemas/extcon/extcon-usbc-cros-ec.yaml# + "^charge-chip-*": + type: object + $ref: /schemas/chrome/google,cros-charge-state.yaml# + required: - compatible
Add new dt bindings for charge chip control. The charge chip control dt configuration is used by the driver 'cros-ec-charge-state', which is added in the commit "platform/chrome: cros_ec_charge_state: add new driver to control charge". As these charge chip controls are connected under the ChromeOS Embedded Controller (EC), also add the patternProperties to the mfd/google,cros-ec bindings. Signed-off-by: Sung-Chi, Li <lschyi@chromium.org> --- .../bindings/chrome/google,cros-charge-state.yaml | 62 ++++++++++++++++++++++ .../devicetree/bindings/mfd/google,cros-ec.yaml | 4 ++ 2 files changed, 66 insertions(+)