| Message ID | 20240503-mbly-olb-v2-1-95ce5a1e18fe@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Mobileye EyeQ system controller support (clk, reset, pinctrl) | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 0 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 03/05/2024 16:20, Théo Lebrun wrote: > Switch from sub-nodes in system-controller for each functionality to a > single node representing the entire OLB instance. dt-bindings is > unnecessary and soc/mobileye/mobileye,eyeq5-olb.yaml will inherit all > properties. Why changing this? You just added these bindings not so long time ago... This is very confusing to push bindings and then immediately ask to remove them. Best regards, Krzysztof
On 03/05/2024 17:57, Krzysztof Kozlowski wrote: > On 03/05/2024 16:20, Théo Lebrun wrote: >> Switch from sub-nodes in system-controller for each functionality to a >> single node representing the entire OLB instance. dt-bindings is >> unnecessary and soc/mobileye/mobileye,eyeq5-olb.yaml will inherit all >> properties. > > Why changing this? You just added these bindings not so long time ago... > This is very confusing to push bindings and then immediately ask to > remove them. One more point - anyway this should be revert with clear explanation WHY you are reverting bindings. Same for second patch. Best regards, Krzysztof
Hello, On Fri May 3, 2024 at 6:05 PM CEST, Krzysztof Kozlowski wrote: > On 03/05/2024 17:57, Krzysztof Kozlowski wrote: > > On 03/05/2024 16:20, Théo Lebrun wrote: > >> Switch from sub-nodes in system-controller for each functionality to a > >> single node representing the entire OLB instance. dt-bindings is > >> unnecessary and soc/mobileye/mobileye,eyeq5-olb.yaml will inherit all > >> properties. > > > > Why changing this? You just added these bindings not so long time ago... > > This is very confusing to push bindings and then immediately ask to > > remove them. See this revision as a proposal of something that has been asked multiple times in previous reviews. See message from Stephen Boyd on last revision [0], or discussion with Rob Herring on much earlier revision [1]. Proposal from Stephen Boyd of using auxiliary devices makes sense, that could be the future direction of this series. It won't change the dt-bindings aspect of it, only the driver implementations. [0]: https://lore.kernel.org/lkml/daa732cb31d947c308513b535930c729.sboyd@kernel.org/ [1]: https://lore.kernel.org/lkml/20240124151405.GA930997-robh@kernel.org/ > One more point - anyway this should be revert with clear explanation WHY > you are reverting bindings. I'll make sure to use standard revert formatting and explain why it is being done. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 07/05/2024 17:07, Théo Lebrun wrote: > Hello, > > On Fri May 3, 2024 at 6:05 PM CEST, Krzysztof Kozlowski wrote: >> On 03/05/2024 17:57, Krzysztof Kozlowski wrote: >>> On 03/05/2024 16:20, Théo Lebrun wrote: >>>> Switch from sub-nodes in system-controller for each functionality to a >>>> single node representing the entire OLB instance. dt-bindings is >>>> unnecessary and soc/mobileye/mobileye,eyeq5-olb.yaml will inherit all >>>> properties. >>> >>> Why changing this? You just added these bindings not so long time ago... >>> This is very confusing to push bindings and then immediately ask to >>> remove them. > > See this revision as a proposal of something that has been asked > multiple times in previous reviews. See message from Stephen Boyd on That's driver, we talk about bindings. > last revision [0], or discussion with Rob Herring on much earlier > revision [1]. > > Proposal from Stephen Boyd of using auxiliary devices makes sense, that > could be the future direction of this series. It won't change the > dt-bindings aspect of it, only the driver implementations. > > [0]: https://lore.kernel.org/lkml/daa732cb31d947c308513b535930c729.sboyd@kernel.org/ > [1]: https://lore.kernel.org/lkml/20240124151405.GA930997-robh@kernel.org/ So after Robs comment above, you still pushed the wrong approach and now you revert it? Why v7 was sent ignoring Rob's comments: https://lore.kernel.org/all/20240221-mbly-clk-v7-3-31d4ce3630c3@bootlin.com/ Best regards, Krzysztof
Hello Krzysztof, On Tue May 7, 2024 at 5:34 PM CEST, Krzysztof Kozlowski wrote: > On 07/05/2024 17:07, Théo Lebrun wrote: > > Proposal from Stephen Boyd of using auxiliary devices makes sense, that > > could be the future direction of this series. It won't change the > > dt-bindings aspect of it, only the driver implementations. > > > > [0]: https://lore.kernel.org/lkml/daa732cb31d947c308513b535930c729.sboyd@kernel.org/ > > [1]: https://lore.kernel.org/lkml/20240124151405.GA930997-robh@kernel.org/ > > So after Robs comment above, you still pushed the wrong approach and now > you revert it? Yes. The gist of it is that I had misunderstood the messages. Mostly, I did not understand how to implement separate Linux driver with the desired devicetree structure (no subnode on the syscon for each feature). I was missing knowledge about Linux infrastructure allowing for that. MFD and auxdevs are two approaches, with auxdevs being preferred. The latest revision finally takes those comments into account. Thanks Krzysztof, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml b/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml deleted file mode 100644 index 2d4f2cde1e58..000000000000 --- a/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml +++ /dev/null @@ -1,51 +0,0 @@ -# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) -%YAML 1.2 ---- -$id: http://devicetree.org/schemas/clock/mobileye,eyeq5-clk.yaml# -$schema: http://devicetree.org/meta-schemas/core.yaml# - -title: Mobileye EyeQ5 clock controller - -description: - The EyeQ5 clock controller handles 10 read-only PLLs derived from the main - crystal clock. It also exposes one divider clock, a child of one of the PLLs. - Its registers live in a shared region called OLB. - -maintainers: - - Grégory Clement <gregory.clement@bootlin.com> - - Théo Lebrun <theo.lebrun@bootlin.com> - - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> - -properties: - compatible: - const: mobileye,eyeq5-clk - - reg: - maxItems: 2 - - reg-names: - items: - - const: plls - - const: ospi - - "#clock-cells": - const: 1 - - clocks: - maxItems: 1 - description: - Input parent clock to all PLLs. Expected to be the main crystal. - - clock-names: - items: - - const: ref - -required: - - compatible - - reg - - reg-names - - "#clock-cells" - - clocks - - clock-names - -additionalProperties: false
Switch from sub-nodes in system-controller for each functionality to a single node representing the entire OLB instance. dt-bindings is unnecessary and soc/mobileye/mobileye,eyeq5-olb.yaml will inherit all properties. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- .../bindings/clock/mobileye,eyeq5-clk.yaml | 51 ---------------------- 1 file changed, 51 deletions(-)