Hi Ralph,
ralph.siemsen@xxxxxxxxxx wrote on Mon, 27 Feb 2023 13:39:35 -0500:
> Add some kerneldoc comments for the structures.
>
> Signed-off-by: Ralph Siemsen <ralph.siemsen@xxxxxxxxxx>
> ---
>
> 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
