Re: [PATCH RESEND 3/9] clk: stm32: add STM32H7RS RCC driver

Hi Brian,

Thanks for the review.  All six comments are addressed in v2; the changes
to this patch are here:

  https://lore.kernel.org/r/MN0PR19MB6091BCCEA08218EB99DE07B8ACDF2@MN0PR19MB6091.namprd19.prod.outlook.com

> > +#include <linux/slab.h>
>
> Is slab.h needed here?

No.  The driver only uses devm_kzalloc() and struct_size(), so I dropped
the <linux/slab.h> include in v2.

> > +	u32 divr1, frac = 0, m, n, out_div, source;
>
> Reverse Christmas tree order.

Reordered the locals in stm32h7rs_pll_rate() longest-line-first in v2.

> > +	numerator = (u64)parent_rate * (n * 8192ULL + frac);
> > +	return div64_u64(numerator, (u64)m * 8192ULL * out_div);
>
> Newline before the return

Added the blank line before the return in v2.

> > +	hw = clk_hw_register_gate(NULL, name, parent_name, CLK_IGNORE_UNUSED,
>
> Use parent_data instead of parent_name. You can use
> clk_hw_register_gate_parent_data() here.

Done.  In v2 the gates are registered with a struct clk_parent_data that
points at the parent clk_hw, via devm_clk_hw_register_gate_parent_data().

> > +	base = of_iomap(np, 0);
>
> devm_of_iomap

Switched to devm_of_iomap() in v2, along with devm_clk_get() and the
devm clock registration/provider helpers, which let me drop the manual
failure-unwinding path.

> > +CLK_OF_DECLARE_DRIVER(stm32h7rs_rcc, "st,stm32h7rs-rcc", stm32h7rs_rcc_init);
>
> Can this be registered as a platform device instead of using
> CLK_OF_DECLARE_DRIVER?

Yes.  v2 registers it as a builtin_platform_driver() with a probe.

One consequence worth flagging: TIM5 is initialized through
TIMER_OF_DECLARE() before the platform bus is available, so it can no
longer obtain its clock from the RCC platform driver.  In v2 TIM5 is
clocked from a fixed-clock in DT at the rate the first-stage firmware
programs; UART4, the GPIO banks and SDMMC1 continue to use the RCC
provider.

Since the RCC platform driver owns the RCC node, the generic reset-simple
driver cannot bind to the same node.  The RCC probe therefore registers
the reset controller directly using reset_simple_ops (the driver now
selects RESET_SIMPLE), and the RCC node uses a single "st,stm32h7rs-rcc"
compatible.

Thanks,
Liu Changjie