| Message ID | 20240920-mpll_spinlock-v1-0-5249a9a7e2b7@amlogic.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk: meson: Delete a meaningless spinlock from the MPLL | expand |
On Fri 20 Sep 2024 at 16:16, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: > The existing locking mechanism of CCF can effectively avoid concurrent > register access. struct meson_clk_mpll_data has no meaning in defining > a spinlock repeatedly. > > In addition, the register corresponding to MPLL does not share the same > register with other module drivers, so there is no concurrent access to > the register with other modules drivers. > > Every driver file with mpll defines a spinlock with the same name (even > if defined as "static"), giving the illusion of repeated definitions? > > Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> I'm ok with the patch in general but I have problem with the wording. The lock is not meaningless. It has a meaning but it does not serve a purpose, at least not anymore. You could write that it is useless, or superfluous if you want to, but not meaningless. Also, please squash the changes. 1 patch for this is fine. > --- > Chuan Liu (6): > clk: meson: mpll: Delete a meaningless spinlock from the MPLL > clk: meson: axg: Delete the spinlock from the MPLL > clk: meson: meson8b: Delete the spinlock from the MPLL > clk: meson: gxbb: Delete the spinlock from the MPLL > clk: meson: g12a: Delete the spinlock from the MPLL > clk: meson: s4: Delete the spinlock from the MPLL > > drivers/clk/meson/axg.c | 6 ------ > drivers/clk/meson/clk-mpll.c | 11 ----------- > drivers/clk/meson/clk-mpll.h | 1 - > drivers/clk/meson/g12a.c | 6 ------ > drivers/clk/meson/gxbb.c | 6 ------ > drivers/clk/meson/meson8b.c | 3 --- > drivers/clk/meson/s4-pll.c | 6 ------ > 7 files changed, 39 deletions(-) > --- > base-commit: 0ef513560b53d499c824b77220c537eafe1df90d > change-id: 20240918-mpll_spinlock-4b9b55c44fd5 > > Best regards,
hi Jerome: Thanks for your advice, I will modify the commit message and squash the patch before sending it. On 2024/9/24 16:35, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On Fri 20 Sep 2024 at 16:16, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote: > >> The existing locking mechanism of CCF can effectively avoid concurrent >> register access. struct meson_clk_mpll_data has no meaning in defining >> a spinlock repeatedly. >> >> In addition, the register corresponding to MPLL does not share the same >> register with other module drivers, so there is no concurrent access to >> the register with other modules drivers. >> >> Every driver file with mpll defines a spinlock with the same name (even >> if defined as "static"), giving the illusion of repeated definitions? >> >> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> > I'm ok with the patch in general but I have problem with the wording. > The lock is not meaningless. It has a meaning but it does not serve a > purpose, at least not anymore. You could write that it is useless, or > superfluous if you want to, but not meaningless. > > Also, please squash the changes. 1 patch for this is fine. > >> --- >> Chuan Liu (6): >> clk: meson: mpll: Delete a meaningless spinlock from the MPLL >> clk: meson: axg: Delete the spinlock from the MPLL >> clk: meson: meson8b: Delete the spinlock from the MPLL >> clk: meson: gxbb: Delete the spinlock from the MPLL >> clk: meson: g12a: Delete the spinlock from the MPLL >> clk: meson: s4: Delete the spinlock from the MPLL >> >> drivers/clk/meson/axg.c | 6 ------ >> drivers/clk/meson/clk-mpll.c | 11 ----------- >> drivers/clk/meson/clk-mpll.h | 1 - >> drivers/clk/meson/g12a.c | 6 ------ >> drivers/clk/meson/gxbb.c | 6 ------ >> drivers/clk/meson/meson8b.c | 3 --- >> drivers/clk/meson/s4-pll.c | 6 ------ >> 7 files changed, 39 deletions(-) >> --- >> base-commit: 0ef513560b53d499c824b77220c537eafe1df90d >> change-id: 20240918-mpll_spinlock-4b9b55c44fd5 >> >> Best regards, > -- > Jerome
The existing locking mechanism of CCF can effectively avoid concurrent register access. struct meson_clk_mpll_data has no meaning in defining a spinlock repeatedly. In addition, the register corresponding to MPLL does not share the same register with other module drivers, so there is no concurrent access to the register with other modules drivers. Every driver file with mpll defines a spinlock with the same name (even if defined as "static"), giving the illusion of repeated definitions? Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> --- Chuan Liu (6): clk: meson: mpll: Delete a meaningless spinlock from the MPLL clk: meson: axg: Delete the spinlock from the MPLL clk: meson: meson8b: Delete the spinlock from the MPLL clk: meson: gxbb: Delete the spinlock from the MPLL clk: meson: g12a: Delete the spinlock from the MPLL clk: meson: s4: Delete the spinlock from the MPLL drivers/clk/meson/axg.c | 6 ------ drivers/clk/meson/clk-mpll.c | 11 ----------- drivers/clk/meson/clk-mpll.h | 1 - drivers/clk/meson/g12a.c | 6 ------ drivers/clk/meson/gxbb.c | 6 ------ drivers/clk/meson/meson8b.c | 3 --- drivers/clk/meson/s4-pll.c | 6 ------ 7 files changed, 39 deletions(-) --- base-commit: 0ef513560b53d499c824b77220c537eafe1df90d change-id: 20240918-mpll_spinlock-4b9b55c44fd5 Best regards,