| Message ID | 20230227183937.377612-4-ralph.siemsen@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Renesas r9a06g032 clock table improvements | expand |
Hi Ralph, ralph.siemsen@linaro.org wrote on Mon, 27 Feb 2023 13:39:35 -0500: > Add some kerneldoc comments for the structures. > > Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org> > --- > > drivers/clk/renesas/r9a06g032-clocks.c | 36 +++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c > index 8a1ab9da19ae..1b7801f14c8c 100644 > --- a/drivers/clk/renesas/r9a06g032-clocks.c > +++ b/drivers/clk/renesas/r9a06g032-clocks.c > @@ -27,6 +27,26 @@ > > #define R9A06G032_SYSCTRL_DMAMUX 0xA0 > Thanks for the change, I think it's always interesting to document a bit more the code and strucs, I would like to offer a few proposals, feel free to ignore if you disagree. > +/** > + * struct r9a06g032_gate - clock gate control bits > + * @gate: bit which enables/disables the clock Is this really a bit? I see below you explain what each field means in terms of offset/bitmask, so maybe we could be vague here, something like: "configuration to enable/disable the clock" > + * @reset: bit which controls module reset (active low) "clock module reset" ? > + * @ready: bit which indicates device is ready for access "the clock is stacle and the device fed should be ready for access" (not sure about this one) > + * @midle: bit which requests to idle the NoC interconnect > + * > + * Each of these fields describes a single bit in a register, > + * which controls some aspect of clock gating. The @gate field > + * is mandatory, this one enables/disables the clock. The > + * other fields are optional, with zero indicating "not used". > + * > + * In most cases there is a @reset bit which needs to be > + * de-asserted to bring the module out of reset. > + * > + * Modules may also need to signal when the are @ready to > + * handle requests (read/writes) from the NoC interconnect. > + * > + * Similarly, the @midle bit is used to idle the master. > + */ > struct r9a06g032_gate { > u16 gate, reset, ready, midle; > /* Unused fields omitted to save space */ > @@ -41,7 +61,21 @@ enum gate_type { > K_DUALGATE /* special for UARTs */ > }; > > -/* This is used to describe a clock for instantiation */ > +/** > + * struct r9a06g032_clkdesc - describe a single clock > + * @name: string describing this clock > + * @managed: boolan indicating if this clock should be typo: boolean > + * start/stop as part of power management started/stopped? > + * @type: see enum gate_type > + * @index: the ID of this clock element > + * @source: the ID+1 of the parent clock element. > + * Root clock uses ID of ~0 (PARENT_ID); > + * @gate: describes the bits which control clock gate I would just refer to the structure above (like @type). No description of the following fields? :-) :-) It's okay, but while you're at it... > + * > + * Describes a single element in the clock tree hierarchy. > + * As there are quite a large number of clock elements, this > + * structure is packed tightly to conserve space. > + */ > struct r9a06g032_clkdesc { > const char *name; > uint32_t managed:1; Thanks, Miquèl
Hi Miquèl, Thanks for reviewing. A few comments below. On Wed, Mar 1, 2023 at 6:24 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > +/** > > + * struct r9a06g032_gate - clock gate control bits > > + * @gate: bit which enables/disables the clock > > Is this really a bit? I see below you explain what each field means > in terms of offset/bitmask, so maybe we could be vague here, > something like: > > "configuration to enable/disable the clock" I'll drop the "bit" phrasing from all of them, to avoid confusion. > > + * @reset: bit which controls module reset (active low) > > "clock module reset" ? Sounds good! > > + * @ready: bit which indicates device is ready for access > > "the clock is stacle and the device fed should be ready for access" > (not sure about this one) This one controls whether read/write requests are forwarded to the device. I believe this is to prevent the AXI bus from hanging the module clock is not enabled. I tried to explain that here: > > + * Modules may also need to signal when the are @ready to > > + * handle requests (read/writes) from the NoC interconnect. I'll see if I can come up with a better description for both @ready and @midle. > > + * @type: see enum gate_type > > + * @index: the ID of this clock element > > + * @source: the ID+1 of the parent clock element. > > + * Root clock uses ID of ~0 (PARENT_ID); > > + * @gate: describes the bits which control clock gate > > I would just refer to the structure above (like @type). > > No description of the following fields? :-) :-) It's okay, but while > you're at it... Ack, I will add the others too. Ralph
diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c index 8a1ab9da19ae..1b7801f14c8c 100644 --- a/drivers/clk/renesas/r9a06g032-clocks.c +++ b/drivers/clk/renesas/r9a06g032-clocks.c @@ -27,6 +27,26 @@ #define R9A06G032_SYSCTRL_DMAMUX 0xA0 +/** + * struct r9a06g032_gate - clock gate control bits + * @gate: bit which enables/disables the clock + * @reset: bit which controls module reset (active low) + * @ready: bit which indicates device is ready for access + * @midle: bit which requests to idle the NoC interconnect + * + * Each of these fields describes a single bit in a register, + * which controls some aspect of clock gating. The @gate field + * is mandatory, this one enables/disables the clock. The + * other fields are optional, with zero indicating "not used". + * + * In most cases there is a @reset bit which needs to be + * de-asserted to bring the module out of reset. + * + * Modules may also need to signal when the are @ready to + * handle requests (read/writes) from the NoC interconnect. + * + * Similarly, the @midle bit is used to idle the master. + */ struct r9a06g032_gate { u16 gate, reset, ready, midle; /* Unused fields omitted to save space */ @@ -41,7 +61,21 @@ enum gate_type { K_DUALGATE /* special for UARTs */ }; -/* This is used to describe a clock for instantiation */ +/** + * struct r9a06g032_clkdesc - describe a single clock + * @name: string describing this clock + * @managed: boolan indicating if this clock should be + * start/stop as part of power management + * @type: see enum gate_type + * @index: the ID of this clock element + * @source: the ID+1 of the parent clock element. + * Root clock uses ID of ~0 (PARENT_ID); + * @gate: describes the bits which control clock gate + * + * Describes a single element in the clock tree hierarchy. + * As there are quite a large number of clock elements, this + * structure is packed tightly to conserve space. + */ struct r9a06g032_clkdesc { const char *name; uint32_t managed:1;
Add some kerneldoc comments for the structures. Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org> --- drivers/clk/renesas/r9a06g032-clocks.c | 36 +++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)