| Message ID | 20240726110114.1509733-7-m.majewski2@samsung.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add initial Exynos850 support to the thermal driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 12 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Fri, Jul 26, 2024 at 6:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote: > > This is not true as of commit 5314b1543787 ("thermal/drivers/exynos: Use > set_trips ops"). > > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > v1 -> v2: remove an unnecessary sentence. > > .../devicetree/bindings/thermal/samsung,exynos-thermal.yaml | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml > index b8c0bb7f4263..b85b4c420cd3 100644 > --- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml > +++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml > @@ -40,11 +40,7 @@ properties: > interrupts: > description: | > The Exynos TMU supports generating interrupts when reaching given > - temperature thresholds. Number of supported thermal trip points depends > - on the SoC (only first trip points defined in DT will be configured):: > - - most of SoC: 4 > - - samsung,exynos5433-tmu: 8 > - - samsung,exynos7-tmu: 8 > + temperature thresholds. > maxItems: 1 > > reg: > -- > 2.45.1 >
On Fri, Jul 26, 2024 at 01:01:10PM +0200, Mateusz Majewski wrote: > This is not true as of commit 5314b1543787 ("thermal/drivers/exynos: Use > set_trips ops"). What is not true? How can the h/w change? I already asked that. Please make your commit message summarize prior discussions so that the patch stands on its own and you don't get the same response again. Assume the reviewers have 0 recollection of the prior versions because we don't. This is just one of 100s of patches a week... > > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- > v1 -> v2: remove an unnecessary sentence. > > .../devicetree/bindings/thermal/samsung,exynos-thermal.yaml | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml > index b8c0bb7f4263..b85b4c420cd3 100644 > --- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml > +++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml > @@ -40,11 +40,7 @@ properties: > interrupts: > description: | > The Exynos TMU supports generating interrupts when reaching given > - temperature thresholds. Number of supported thermal trip points depends > - on the SoC (only first trip points defined in DT will be configured):: > - - most of SoC: 4 > - - samsung,exynos5433-tmu: 8 > - - samsung,exynos7-tmu: 8 > + temperature thresholds. > maxItems: 1 > > reg: > -- > 2.45.1 >
On Tue, Jul 30, 2024 at 11:17 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Jul 26, 2024 at 01:01:10PM +0200, Mateusz Majewski wrote: > > This is not true as of commit 5314b1543787 ("thermal/drivers/exynos: Use > > set_trips ops"). > > What is not true? > > How can the h/w change? I already asked that. Please make your commit > message summarize prior discussions so that the patch stands on its own > and you don't get the same response again. Assume the reviewers have 0 > recollection of the prior versions because we don't. This is just one of > 100s of patches a week... > Hi Mateusz, Do I understand it correctly that the patch actually removes an outdated description of *driver* implementation, and not outdated hardware description? If so, then maybe it makes sense to rework the patch title and commit message in a way Rob suggests. I.e. rather than stating that the patch removes an outdated information, instead mention it removes *software* (driver) description which was incorrectly added earlier. Because bindings are only meant for hardware description and should be completely independent of driver's side of things. Also in that case it probably doesn't make much sense referencing that commit for using set_trips ops. Just my two cents. Thanks! [snip]
> Do I understand it correctly that the patch actually removes an > outdated description of *driver* implementation, and not outdated > hardware description? Correct. > If so, then maybe it makes sense to rework the > patch title and commit message in a way Rob suggests. I.e. rather than > stating that the patch removes an outdated information, instead > mention it removes *software* (driver) description which was > incorrectly added earlier. Because bindings are only meant for > hardware description and should be completely independent of driver's > side of things. Also in that case it probably doesn't make much sense > referencing that commit for using set_trips ops. Just my two cents. Makes sense, what do you think about this? dt-bindings: thermal: samsung,exynos: remove driver-specific information The number of supported trip points was only limited by the driver implementation at the time, which mapped each trip point defined in the devicetree source file to a hardware trip point. An implementation that does not have this limitation is possible; indeed, that is how the driver works currently. Therefore, this information should be removed from the bindings description, which are meant to be independent from the details of the driver implementation.
On Wed, Jul 31, 2024 at 4:14 PM Mateusz Majewski <m.majewski2@samsung.com> wrote: > > > Do I understand it correctly that the patch actually removes an > > outdated description of *driver* implementation, and not outdated > > hardware description? > > Correct. > > > If so, then maybe it makes sense to rework the > > patch title and commit message in a way Rob suggests. I.e. rather than > > stating that the patch removes an outdated information, instead > > mention it removes *software* (driver) description which was > > incorrectly added earlier. Because bindings are only meant for > > hardware description and should be completely independent of driver's > > side of things. Also in that case it probably doesn't make much sense > > referencing that commit for using set_trips ops. Just my two cents. > > Makes sense, what do you think about this? > > dt-bindings: thermal: samsung,exynos: remove driver-specific information > > The number of supported trip points was only limited by the driver > implementation at the time, which mapped each trip point defined in the > devicetree source file to a hardware trip point. An implementation that > does not have this limitation is possible; indeed, that is how the > driver works currently. Therefore, this information should be removed > from the bindings description, which are meant to be independent from > the details of the driver implementation. Looks good to me. But you already have my R-b tag :) More important if it's ok with Rob.
On Wed, Jul 31, 2024 at 11:14:42PM +0200, Mateusz Majewski wrote: > > Do I understand it correctly that the patch actually removes an > > outdated description of *driver* implementation, and not outdated > > hardware description? > > Correct. > > > If so, then maybe it makes sense to rework the > > patch title and commit message in a way Rob suggests. I.e. rather than > > stating that the patch removes an outdated information, instead > > mention it removes *software* (driver) description which was > > incorrectly added earlier. Because bindings are only meant for > > hardware description and should be completely independent of driver's > > side of things. Also in that case it probably doesn't make much sense > > referencing that commit for using set_trips ops. Just my two cents. > > Makes sense, what do you think about this? > > dt-bindings: thermal: samsung,exynos: remove driver-specific information > > The number of supported trip points was only limited by the driver > implementation at the time, which mapped each trip point defined in the > devicetree source file to a hardware trip point. An implementation that > does not have this limitation is possible; indeed, that is how the > driver works currently. Therefore, this information should be removed > from the bindings description, which are meant to be independent from > the details of the driver implementation. LGTM
diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml index b8c0bb7f4263..b85b4c420cd3 100644 --- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml +++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml @@ -40,11 +40,7 @@ properties: interrupts: description: | The Exynos TMU supports generating interrupts when reaching given - temperature thresholds. Number of supported thermal trip points depends - on the SoC (only first trip points defined in DT will be configured):: - - most of SoC: 4 - - samsung,exynos5433-tmu: 8 - - samsung,exynos7-tmu: 8 + temperature thresholds. maxItems: 1 reg:
This is not true as of commit 5314b1543787 ("thermal/drivers/exynos: Use set_trips ops"). Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> --- v1 -> v2: remove an unnecessary sentence. .../devicetree/bindings/thermal/samsung,exynos-thermal.yaml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)