Messages in this thread | | | Date | Mon, 28 Nov 2022 21:30:58 +0800 | Subject | Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings | From | Yu Tu <> |
| |
Hi Jerome ,
On 2022/11/28 20:33, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Mon 28 Nov 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote: > >> Hi Jerome, >> Thank you for your reply. >> >> On 2022/11/25 17:23, Jerome Brunet wrote: >>> [ EXTERNAL EMAIL ] >>> On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 23/11/2022 14:23, Neil Armstrong wrote: >>>>> Hi, >>>>> >>>>> On 23/11/2022 12:16, Yu Tu wrote: >>>>>> Hi Krzysztof, >>>>>> Thank you for your reply. >>>>>> >>>>>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>>>>> [ EXTERNAL EMAIL ] >>>>>>> >>>>>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>>>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>>>>>> >>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>>>>> --- >>>>>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>>>>> >>>>>>> This is v5 and still bindings are here? Bindings are always separate >>>>>>> patches. Use subject prefixes matching the subsystem (git log --oneline >>>>>>> -- ...). >>>>>>> >>>>>>> And this was split, wasn't it? What happened here?!? >>>>>> >>>>>> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >>>>>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >>>>> >>>>> Jerome was asking you to send 2 patchsets, one with : >>>>> - bindings in separate patches >>>>> - drivers in separate patches >>>>> and a second with DT changes. >>> Indeed, this is what was asked. It is aligned with Krzysztof's request. >> >> According to your discussion, I still should send patches in the previous >> way in series. But I'm going to change it like you suggested. >> I don't know, am I getting it right? > > 3 people tried to explain this already and we all told you the same thing. > > * 1 patchset per maintainer: clk and dt > * bindings must be dedicated patches - never mixed with driver code. > > I strongly suggest that you take some time to (re)read: > * https://docs.kernel.org/process/submitting-patches.html > * https://docs.kernel.org/devicetree/bindings/submitting-patches.html > > If still unclear, please take some time to look at the kernel mailing > list archive and see how others have done the same things. > > Thx.
I'll change it as you suggest.But I still don't understand what you suggested in V3.
I remember discussing it with you at V3. https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/
">>>> Also it would be nice to split this in two series. >>>> Bindings and drivers in one, arm64 dt in the other. These changes goes >>>> in through different trees. >>> At present, Bindings, DTS and drivers are three series. Do you mean to put >>> Bindings and drivers together? If so, checkpatch.pl will report a warning. >> Yes because patches are not in yet so there is a good reason to ignore >> the warning. Warning will never show up on the actual tree if the >> patches are correctly ordered. > > I think Binding, DTS and drivers use three series and you said two series > is not a big problem. Three series are recommended for checkpatch.pl, I > think it should be easy for that to separate and merge。
No - There is only 2 series. 1 for the bindings and clock drivers and one for the DT once things are in"
> >> >>> >>>>> >>>>> Then when the bindings + clocks patches are merged, a pull request of the bindings >>>>> can be done to me so I can merge it with DT. >>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> MAINTAINERS | 1 + >>>>>>>> drivers/clk/meson/Kconfig | 13 + >>>>>>>> drivers/clk/meson/Makefile | 1 + >>>>>>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>>>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>>>>> 7 files changed, 1059 insertions(+) >>>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>>>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>> new file mode 100644 >>>>>>>> index 000000000000..fd517e8ef14f >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>> @@ -0,0 +1,51 @@ >>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>>>> +%YAML 1.2 >>>>>>>> +--- >>>>>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>> + >>>>>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>>>>> + >>>>>>>> +maintainers: >>>>>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>>>>> + >>>>>>> One blank line. >>>>>> >>>>>> I will delete this, on next version patch. >>>>>> >>>>>>> >>>>>>>> + >>>>>>>> +properties: >>>>>>>> + compatible: >>>>>>>> + const: amlogic,s4-pll-clkc >>>>>>>> + >>>>>>>> + reg: >>>>>>>> + maxItems: 1 >>>>>>>> + >>>>>>>> + clocks: >>>>>>>> + maxItems: 1 >>>>>>>> + >>>>>>>> + clock-names: >>>>>>>> + items: >>>>>>>> + - const: xtal >>>>>>>> + >>>>>>>> + "#clock-cells": >>>>>>>> + const: 1 >>>>>>>> + >>>>>>>> +required: >>>>>>>> + - compatible >>>>>>>> + - reg >>>>>>>> + - clocks >>>>>>>> + - clock-names >>>>>>>> + - "#clock-cells" >>>>>>>> + >>>>>>>> +additionalProperties: false >>>>>>>> + >>>>>>>> +examples: >>>>>>>> + - | >>>>>>>> + clkc_pll: clock-controller@fe008000 { >>>>>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>>>>> + reg = <0xfe008000 0x1e8>; >>>>>>>> + clocks = <&xtal>; >>>>>>>> + clock-names = "xtal"; >>>>>>>> + #clock-cells = <1>; >>>>>>>> + }; >>>>>>> >>>>>>> >>>>>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>> new file mode 100644 >>>>>>>> index 000000000000..345f87023886 >>>>>>>> --- /dev/null >>>>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>> >>>>>>> This belongs to bindings patch, not driver. >>>>>>> >>>>>>>> @@ -0,0 +1,30 @@ >>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>>>>> +/* >>>>>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>> + >>>>>>>> +/* >>>>>>>> + * CLKID index values >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#define CLKID_FIXED_PLL 1 >>>>>>>> +#define CLKID_FCLK_DIV2 3 >>>>>>> >>>>>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>>>>> >>>>>>> NAK. >>>>>> >>>>>> I remember Jerome discussing this with you.You can look at this submission history. >>>>>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >>>>> >>>>> Historically we did that by only exposing part of the numbers, controlling which >>>>> clocks were part of the bindings. >>>>> >>>>> But it seems this doesn't make sens anymore, maybe it would be time to put all the >>>>> clock ids in the bindings for this new SoC and break with the previous strategy. >>> Krzysztof and I agreed there is nothing wrong with the current >>> approach, I believe. >>> It does not prevent someone from using an un-exposed clock, sure, or >>> exposing it in the future if necessary. >>> However, I think it clearly shows that an un-exposed element is not >>> expected to be used by an external consumers. It should be enough to >>> trigger a discussion if this expectation is wrong. >>> >>>> >>>> So the outcome of the previous discussion was somewhere later in that >>>> thread: >>>> >>>>> It is just a choice to not expose some IDs. >>>>> It is not tied to the implementation at all. >>>>> I think we actually follow the rules and the idea behind it. >>>> >>>> >>>> Best regards, >>>> Krzysztof >>> . > > .
| |