| Message ID | 20241122-extend_power_limit-v1-2-a3ecd87afa76@chromium.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Extend the cros_usbpd-charger to make it a passive thermal cooling device | expand |
On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: > The cros_ec supports limiting the input current to act as a passive > thermal cooling device. Add the property '#cooling-cells' bindings, such > that thermal framework can recognize cros_ec as a valid thermal cooling > device. > > Signed-off-by: Sung-Chi Li <lschyi@chromium.org> > --- > Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > index aac8819bd00b..2b6f098057af 100644 > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > @@ -96,6 +96,9 @@ properties: > '#gpio-cells': > const: 2 > > + '#cooling-cells': > + const: 2 This is not a cooling device. BTW, your commit msg is somehow circular. "Add cooling to make it a cooling device because it will be then cooling device." Power supply already provides necessary framework for managing charging current and temperatures. If this is to stay, you need to explain why this is suitable to be considered a thermal zone or system cooling device (not power supply or input power cooling). Best regards, Krzysztof
On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote: > On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: > > The cros_ec supports limiting the input current to act as a passive > > thermal cooling device. Add the property '#cooling-cells' bindings, such > > that thermal framework can recognize cros_ec as a valid thermal cooling > > device. > > > > Signed-off-by: Sung-Chi Li <lschyi@chromium.org> > > --- > > Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > index aac8819bd00b..2b6f098057af 100644 > > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > @@ -96,6 +96,9 @@ properties: > > '#gpio-cells': > > const: 2 > > > > + '#cooling-cells': > > + const: 2 > > This is not a cooling device. BTW, your commit msg is somehow circular. > "Add cooling to make it a cooling device because it will be then cooling > device." > > Power supply already provides necessary framework for managing charging > current and temperatures. If this is to stay, you need to explain why > this is suitable to be considered a thermal zone or system cooling > device (not power supply or input power cooling). > > Best regards, > Krzysztof > Thank you, I will rephrase the commit message. The reason to not to use the managing charging current and temperatures in the power supply framework is that: - The EC may not have the thermal sensor value for the charger, and there is no protocol for getting the thermal sensor value for the charger (there is command for reading thermal sensor values, but there is no specification for what sensor index is for the charger, if the charger provides thermal value). - The managing mechanism only take the charger thermal value into account, and I would like to control the current based on the thermal condition of the whole device. I will include these explanation in the following changes.
On 25/11/2024 03:50, Sung-Chi, Li wrote: > On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote: >> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: >>> The cros_ec supports limiting the input current to act as a passive >>> thermal cooling device. Add the property '#cooling-cells' bindings, such >>> that thermal framework can recognize cros_ec as a valid thermal cooling >>> device. >>> >>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org> >>> --- >>> Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>> index aac8819bd00b..2b6f098057af 100644 >>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>> @@ -96,6 +96,9 @@ properties: >>> '#gpio-cells': >>> const: 2 >>> >>> + '#cooling-cells': >>> + const: 2 >> >> This is not a cooling device. BTW, your commit msg is somehow circular. >> "Add cooling to make it a cooling device because it will be then cooling >> device." >> >> Power supply already provides necessary framework for managing charging >> current and temperatures. If this is to stay, you need to explain why >> this is suitable to be considered a thermal zone or system cooling >> device (not power supply or input power cooling). >> >> Best regards, >> Krzysztof >> > > Thank you, I will rephrase the commit message. The reason to not to use the > managing charging current and temperatures in the power supply framework is > that: > > - The EC may not have the thermal sensor value for the charger, and there is no > protocol for getting the thermal sensor value for the charger (there is > command for reading thermal sensor values, but there is no specification for > what sensor index is for the charger, if the charger provides thermal value). > - The managing mechanism only take the charger thermal value into account, and > I would like to control the current based on the thermal condition of the > whole device. > > I will include these explanation in the following changes. This does not explain me why this is supposed to be thermal zone. I already said it, but let's repeat: This is not a thermal zone. This isn't thermal zone sensor, either. Best regards, Krzysztof
On 25/11/2024 08:32, Krzysztof Kozlowski wrote: > On 25/11/2024 03:50, Sung-Chi, Li wrote: >> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote: >>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: >>>> The cros_ec supports limiting the input current to act as a passive >>>> thermal cooling device. Add the property '#cooling-cells' bindings, such >>>> that thermal framework can recognize cros_ec as a valid thermal cooling >>>> device. >>>> >>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org> >>>> --- >>>> Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>> index aac8819bd00b..2b6f098057af 100644 >>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>> @@ -96,6 +96,9 @@ properties: >>>> '#gpio-cells': >>>> const: 2 >>>> >>>> + '#cooling-cells': >>>> + const: 2 >>> >>> This is not a cooling device. BTW, your commit msg is somehow circular. ^^^ And here which you ignored: this is not a cooling device. >>> "Add cooling to make it a cooling device because it will be then cooling >>> device." >>> >>> Power supply already provides necessary framework for managing charging >>> current and temperatures. If this is to stay, you need to explain why >>> this is suitable to be considered a thermal zone or system cooling >>> device (not power supply or input power cooling). >>> >>> Best regards, >>> Krzysztof >>> >> >> Thank you, I will rephrase the commit message. The reason to not to use the >> managing charging current and temperatures in the power supply framework is >> that: >> >> - The EC may not have the thermal sensor value for the charger, and there is no >> protocol for getting the thermal sensor value for the charger (there is >> command for reading thermal sensor values, but there is no specification for >> what sensor index is for the charger, if the charger provides thermal value). >> - The managing mechanism only take the charger thermal value into account, and >> I would like to control the current based on the thermal condition of the >> whole device. >> >> I will include these explanation in the following changes. > > > This does not explain me why this is supposed to be thermal zone. I > already said it, but let's repeat: This is not a thermal zone. This > isn't thermal zone sensor, either. And nothing from your "revised" commit msg explains why something which is not a cooling device is supposed to be a cooling device. Best regards, Krzysztof
On Mon, Nov 25, 2024 at 08:35:01AM +0100, Krzysztof Kozlowski wrote: > On 25/11/2024 08:32, Krzysztof Kozlowski wrote: > > On 25/11/2024 03:50, Sung-Chi, Li wrote: > >> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote: > >>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: > >>>> The cros_ec supports limiting the input current to act as a passive > >>>> thermal cooling device. Add the property '#cooling-cells' bindings, such > >>>> that thermal framework can recognize cros_ec as a valid thermal cooling > >>>> device. > >>>> > >>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org> > >>>> --- > >>>> Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > >>>> index aac8819bd00b..2b6f098057af 100644 > >>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > >>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > >>>> @@ -96,6 +96,9 @@ properties: > >>>> '#gpio-cells': > >>>> const: 2 > >>>> > >>>> + '#cooling-cells': > >>>> + const: 2 > >>> > >>> This is not a cooling device. BTW, your commit msg is somehow circular. > > > ^^^ And here which you ignored: this is not a cooling device. > Hi, I added the explanation in the commit message in the v2 version. Please have a look, it should explains why it is not a cooling device. > >>> "Add cooling to make it a cooling device because it will be then cooling > >>> device." > >>> > >>> Power supply already provides necessary framework for managing charging > >>> current and temperatures. If this is to stay, you need to explain why > >>> this is suitable to be considered a thermal zone or system cooling > >>> device (not power supply or input power cooling). > >>> > >>> Best regards, > >>> Krzysztof > >>> > >> > >> Thank you, I will rephrase the commit message. The reason to not to use the > >> managing charging current and temperatures in the power supply framework is > >> that: > >> > >> - The EC may not have the thermal sensor value for the charger, and there is no > >> protocol for getting the thermal sensor value for the charger (there is > >> command for reading thermal sensor values, but there is no specification for > >> what sensor index is for the charger, if the charger provides thermal value). > >> - The managing mechanism only take the charger thermal value into account, and > >> I would like to control the current based on the thermal condition of the > >> whole device. > >> > >> I will include these explanation in the following changes. > > > > > > This does not explain me why this is supposed to be thermal zone. I > > already said it, but let's repeat: This is not a thermal zone. This > > isn't thermal zone sensor, either. > > > And nothing from your "revised" commit msg explains why something which > is not a cooling device is supposed to be a cooling device. The revised commit message is sent, please have a look. > > > Best regards, > Krzysztof
On Mon, Nov 25, 2024 at 08:32:19AM +0100, Krzysztof Kozlowski wrote: > On 25/11/2024 03:50, Sung-Chi, Li wrote: > > On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote: > >> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: > >>> The cros_ec supports limiting the input current to act as a passive > >>> thermal cooling device. Add the property '#cooling-cells' bindings, such > >>> that thermal framework can recognize cros_ec as a valid thermal cooling > >>> device. > >>> > >>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org> > >>> --- > >>> Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > >>> index aac8819bd00b..2b6f098057af 100644 > >>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > >>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > >>> @@ -96,6 +96,9 @@ properties: > >>> '#gpio-cells': > >>> const: 2 > >>> > >>> + '#cooling-cells': > >>> + const: 2 > >> > >> This is not a cooling device. BTW, your commit msg is somehow circular. > >> "Add cooling to make it a cooling device because it will be then cooling > >> device." > >> > >> Power supply already provides necessary framework for managing charging > >> current and temperatures. If this is to stay, you need to explain why > >> this is suitable to be considered a thermal zone or system cooling > >> device (not power supply or input power cooling). > >> > >> Best regards, > >> Krzysztof > >> > > > > Thank you, I will rephrase the commit message. The reason to not to use the > > managing charging current and temperatures in the power supply framework is > > that: > > > > - The EC may not have the thermal sensor value for the charger, and there is no > > protocol for getting the thermal sensor value for the charger (there is > > command for reading thermal sensor values, but there is no specification for > > what sensor index is for the charger, if the charger provides thermal value). > > - The managing mechanism only take the charger thermal value into account, and > > I would like to control the current based on the thermal condition of the > > whole device. > > > > I will include these explanation in the following changes. > > > This does not explain me why this is supposed to be thermal zone. I > already said it, but let's repeat: This is not a thermal zone. This > isn't thermal zone sensor, either. Hi, I added the explanation in the commit message in v2, in short, I need to use different thermal sensors, and need finer thermal controls, so I have to use thermal zone. This is included in the v2 commit message. > > > Best regards, > Krzysztof
On 25/11/2024 09:43, Sung-Chi, Li wrote: > On Mon, Nov 25, 2024 at 08:32:19AM +0100, Krzysztof Kozlowski wrote: >> On 25/11/2024 03:50, Sung-Chi, Li wrote: >>> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote: >>>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: >>>>> The cros_ec supports limiting the input current to act as a passive >>>>> thermal cooling device. Add the property '#cooling-cells' bindings, such >>>>> that thermal framework can recognize cros_ec as a valid thermal cooling >>>>> device. >>>>> >>>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org> >>>>> --- >>>>> Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>>> index aac8819bd00b..2b6f098057af 100644 >>>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>>> @@ -96,6 +96,9 @@ properties: >>>>> '#gpio-cells': >>>>> const: 2 >>>>> >>>>> + '#cooling-cells': >>>>> + const: 2 >>>> >>>> This is not a cooling device. BTW, your commit msg is somehow circular. >>>> "Add cooling to make it a cooling device because it will be then cooling >>>> device." >>>> >>>> Power supply already provides necessary framework for managing charging >>>> current and temperatures. If this is to stay, you need to explain why >>>> this is suitable to be considered a thermal zone or system cooling >>>> device (not power supply or input power cooling). >>>> >>>> Best regards, >>>> Krzysztof >>>> >>> >>> Thank you, I will rephrase the commit message. The reason to not to use the >>> managing charging current and temperatures in the power supply framework is >>> that: >>> >>> - The EC may not have the thermal sensor value for the charger, and there is no >>> protocol for getting the thermal sensor value for the charger (there is >>> command for reading thermal sensor values, but there is no specification for >>> what sensor index is for the charger, if the charger provides thermal value). >>> - The managing mechanism only take the charger thermal value into account, and >>> I would like to control the current based on the thermal condition of the >>> whole device. >>> >>> I will include these explanation in the following changes. >> >> >> This does not explain me why this is supposed to be thermal zone. I >> already said it, but let's repeat: This is not a thermal zone. This >> isn't thermal zone sensor, either. > > Hi, I added the explanation in the commit message in v2, in short, I need to use > different thermal sensors, and need finer thermal controls, so I have to use > thermal zone. This is included in the v2 commit message. You resolved nothing there. I don't care that "you need to use thermal sensors". That's not a valid reason. If next time you say "I need to make it a current regulator", shall we accept incorrect description? No. I repeat multiple times: this is not a SoC cooling device, this is not a thermal zone and not a thermal sensor. This is a power supply or charger or battery. Eventually it might be hardware monitoring sensor. Use appropriate properties for this category of device. Best regards, Krzysztof
On 25/11/2024 09:48, Krzysztof Kozlowski wrote: > On 25/11/2024 09:43, Sung-Chi, Li wrote: >> On Mon, Nov 25, 2024 at 08:32:19AM +0100, Krzysztof Kozlowski wrote: >>> On 25/11/2024 03:50, Sung-Chi, Li wrote: >>>> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote: >>>>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote: >>>>>> The cros_ec supports limiting the input current to act as a passive >>>>>> thermal cooling device. Add the property '#cooling-cells' bindings, such >>>>>> that thermal framework can recognize cros_ec as a valid thermal cooling >>>>>> device. >>>>>> >>>>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org> >>>>>> --- >>>>>> Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>>>> index aac8819bd00b..2b6f098057af 100644 >>>>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml >>>>>> @@ -96,6 +96,9 @@ properties: >>>>>> '#gpio-cells': >>>>>> const: 2 >>>>>> >>>>>> + '#cooling-cells': >>>>>> + const: 2 >>>>> >>>>> This is not a cooling device. BTW, your commit msg is somehow circular. >>>>> "Add cooling to make it a cooling device because it will be then cooling >>>>> device." >>>>> >>>>> Power supply already provides necessary framework for managing charging >>>>> current and temperatures. If this is to stay, you need to explain why >>>>> this is suitable to be considered a thermal zone or system cooling >>>>> device (not power supply or input power cooling). >>>>> >>>>> Best regards, >>>>> Krzysztof >>>>> >>>> >>>> Thank you, I will rephrase the commit message. The reason to not to use the >>>> managing charging current and temperatures in the power supply framework is >>>> that: >>>> >>>> - The EC may not have the thermal sensor value for the charger, and there is no >>>> protocol for getting the thermal sensor value for the charger (there is >>>> command for reading thermal sensor values, but there is no specification for >>>> what sensor index is for the charger, if the charger provides thermal value). >>>> - The managing mechanism only take the charger thermal value into account, and >>>> I would like to control the current based on the thermal condition of the >>>> whole device. >>>> >>>> I will include these explanation in the following changes. >>> >>> >>> This does not explain me why this is supposed to be thermal zone. I >>> already said it, but let's repeat: This is not a thermal zone. This >>> isn't thermal zone sensor, either. >> >> Hi, I added the explanation in the commit message in v2, in short, I need to use >> different thermal sensors, and need finer thermal controls, so I have to use >> thermal zone. This is included in the v2 commit message. > You resolved nothing there. I don't care that "you need to use thermal > sensors". That's not a valid reason. If next time you say "I need to > make it a current regulator", shall we accept incorrect description? No. > > I repeat multiple times: this is not a SoC cooling device, this is not a > thermal zone and not a thermal sensor. > > This is a power supply or charger or battery. Eventually it might be > hardware monitoring sensor. Use appropriate properties for this category > of device. One more comment, inspired by re-thinking this why watching grey heron nearby: you sent first binding patch for thermal-sensor-cells, then later for cooling. Sorry, that's wrong process: you are supposed to send complete bindings for your hardware. Sending it piece by piece is a proof you do it for the driver, so again completely wrong intentions/rationale of making DT changes. It also hides from us complete picture of the hardware and makes decision difficult. Another reason for not accepting this and your previous bindings contribution. To recap: send *complete* bindings for the hardware. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml index aac8819bd00b..2b6f098057af 100644 --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml @@ -96,6 +96,9 @@ properties: '#gpio-cells': const: 2 + '#cooling-cells': + const: 2 + gpio-controller: true typec:
The cros_ec supports limiting the input current to act as a passive thermal cooling device. Add the property '#cooling-cells' bindings, such that thermal framework can recognize cros_ec as a valid thermal cooling device. Signed-off-by: Sung-Chi Li <lschyi@chromium.org> --- Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++ 1 file changed, 3 insertions(+)