Re: [PATCH 3/4] clk: renesas: r9a06g032: document structs

[Miquel Raynal <miquel.raynal@bootlin.com>]

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