On 14/03/2023 12:48, Dmitry Rokosov wrote: > On Tue, Mar 14, 2023 at 12:28:40PM +0100, Krzysztof Kozlowski wrote: >> On 13/03/2023 21:12, Dmitry Rokosov wrote: > > [...] > >>> +#define CLKID_SPIFC 84 >>> +#define CLKID_USB_BUS 85 >>> +#define CLKID_SD_EMMC 86 >>> +#define CLKID_PSRAM 87 >>> +#define CLKID_DMC 88 >> >> And what is here? Between 88 and 121? >> > > Explained below. > >>> +#define CLKID_GEN_SEL 121 >>> + >>> +#endif /* __A1_CLKC_H */ >>> diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h >>> new file mode 100644 >>> index 000000000000..8e97d3fb9d30 >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h >>> @@ -0,0 +1,20 @@ >>> +/* SPDX-License-Identifier: GPL-2.0+ */ >> >> I found in changelog: >> "fix license issue, it's GPL-2.0+ only in the current version" >> and I do not understand. >> >> The license is wrong, so what did you fix? >> > > Sorry don't get you. Why is it wrong? Run checkpatch - it will tell you why wrong. The license is not correct. This is part of binding and should be the same as binding. > I've changed all new source files to GPL-2.0+ except yaml, because yaml > dt bindings schemas require the following license: > > # SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > I've pointed it in the changelog. The only thing I found was: "fix license issue, it's GPL-2.0+ only in the current version" so what exactly you pointed out in changelog? What was to fix? What was fixed? Correct license into incorrect? But why? > >>> +/* >>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved. >>> + * Author: Jian Hu <jian.hu@xxxxxxxxxxx> >>> + * >>> + * Copyright (c) 2023, SberDevices. All Rights Reserved. >>> + * Author: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> >>> + */ >>> + >>> +#ifndef __A1_PLL_CLKC_H >>> +#define __A1_PLL_CLKC_H >>> + >>> +#define CLKID_FIXED_PLL 1 >>> +#define CLKID_FCLK_DIV2 6 >>> +#define CLKID_FCLK_DIV3 7 >>> +#define CLKID_FCLK_DIV5 8 >>> +#define CLKID_FCLK_DIV7 9 >>> +#define CLKID_HIFI_PLL 10 >> >> >> Probably I asked about this... why indices are not continuous? You know >> that consumers are allowed to use number 2 and it will be your ABI, even >> though you did not write it in the binding? That's a tricky and >> confusing pattern for no real gains. > > Actually, indices are continuou but splitted into two parts: public and > private. The public part is located in the dt bindings and can be included > from device tree sources. The private part is in the drivers/clk/meson > folder, and only clk drivers can use it. > I know, there is some trick when the user just inserts a digit value and > doesn't use constants. This is not a trick. This is how DTS works. You have only indices/numbers. > But I'm starting from the assumption that such > dts changes will not be approved by maintainers. In other words, the user > *must* apply defined ABI constants from dt bindings; it's a strong > restriction. But it is not correct assumption. Defines are very important, but they are just helpers. Otherwise without defines you could not use any clock? We pretty often use IDs - for DTS to allow merging via different trees, for DT binding examples to not rely on headers. Your driver implements the ABI and the driver exposes for example clock ID=2, even if it is not in the header. These IDs are unfortunately undocumented ABI and you if you change them, users are allowed to complain. Solution: don't do this. Have all exposed clock IDs and clocks in sync (and continuous). Best regards, Krzysztof
