linux-kernel - Re: [PATCH 2/3] dt-bindings: chrome: add new binding google,cros-ec-chrage-state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zzv2ltfXqaW-2ALm@google.com>
Date: Tue, 19 Nov 2024 10:23:18 +0800
From: "Sung-Chi, Li" <lschyi@...omium.org>
To: Rob Herring <robh@...nel.org>
Cc: Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>,
	Guenter Roeck <groeck@...omium.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Lee Jones <lee@...nel.org>,
	linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev,
	devicetree@...r.kernel.org
Subject: Re: [PATCH 2/3] dt-bindings: chrome: add new binding
 google,cros-ec-chrage-state

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.