Re: [PATCH v15 0/4] Add driver support for ESWIN eic700 SoC clock controller

[Brian Masney <bmasney@redhat.com>]

Hi Xuyang (and Stephen),

On Mon, Mar 16, 2026 at 02:54:00PM +0800, Xuyang Dong wrote:
> > 
> > The link [1] provides the official documentation for the EIC7700. Section 3.2
> > covers the clock subsystem.
> > 
> > [1] https://www.sifive.com/document-file/eic7700x-datasheet
> > 
> > Updates:
> >   Change in v15:
> >   - Updated driver file
> >     - Add 'Reviewed-by: Brian Masney <bmasney@redhat.com>' for patch 2/4, 3/4 and 4/4.
> >     - Add space between the name and < in MODULE_AUTHOR.
> >     - Remove memset_p() and commit in eswin_clk_init().
> >       The current implementation ensures that parent clocks are registered
> >       before child clocks, thus preventing the creation of invalid clocks.
> > 
> 
> Hi Stephen,
> 
> I received your review comments on v9. After addressing them and submitting
> v10 through v14, I received additional comments from Brian, Bo, Marcel, 
> and the kernel test robot. 
> v15 now has a Reviewed-by tag from Brian and a Tested-by tag from Marcel.
> Do you have any further comments on v15? I would like to send a GIT PULL 
> request for 7.0-rc4 by the end of this week.
> Do you think this is okay?

It's been a month since Stephen posted on the list, so I'm not sure when
he'll be able to reply. I know he's been busy.
https://lore.kernel.org/linux-clk/?q=f%3Asboyd%40kernel.org

If you don't hear from him by rc7 (or even rc6), then I think that you
should send him a pull request. He'll either pull it, or tell you what
needs to change.

Once this driver is in tree, you'll still need to post patches to the
list, get them reviewed on list, and then send Stephen a pull for that
merge window. That's what the other SoC vendors do.

Stephen: I went through all of your v9 review comments for this driver,
and ensured that Xuyang addressed all of your concerns. The only thing
is that drivers/clk/eswin/clk-eic7700.c still includes linux/clk.h,
and you asked if it could be dropped. If we remove it, then it fails to
compile because of the clk notifier support:

drivers/clk/eswin/clk-eic7700.c: In function ‘eic7700_clk_pll_cpu_notifier_cb’:
drivers/clk/eswin/clk-eic7700.c:1270:23: error: ‘PRE_RATE_CHANGE’ undeclared (first use in this function)
 1270 |         if (action == PRE_RATE_CHANGE) {
      |                       ^~~~~~~~~~~~~~~
drivers/clk/eswin/clk-eic7700.c:1270:23: note: each undeclared identifier is reported only once for each function it appears in
drivers/clk/eswin/clk-eic7700.c:1273:30: error: ‘POST_RATE_CHANGE’ undeclared (first use in this function)
 1273 |         } else if (action == POST_RATE_CHANGE) {
      |                              ^~~~~~~~~~~~~~~~
drivers/clk/eswin/clk-eic7700.c: In function ‘eic7700_clk_probe’:
drivers/clk/eswin/clk-eic7700.c:1313:15: error: implicit declaration of function ‘devm_clk_notifier_register’; did you mean ‘mmu_notifier_register’? [-Wimplicit-function-declaration]
 1313 |         ret = devm_clk_notifier_register(dev, pll_clk, &clk_data->pll_nb);
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~
      |               mmu_notifier_register

These are currently defined in clk.h. This code was present in the v9.

Brian