Hi AngeloGioacchino,
On Fri, May 20, 2022 at 02:51:28PM +0200, AngeloGioacchino Del Regno wrote:
> To enhance human readability, add kerneldoc to all driver structs.
I am doubtful that this is useful. The reason is that I believe
kerneldoc format is only useful for documenting cross-subsystem APIs.
Kerneldoc for driver-private data and functions simply pollutes API
docs.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> ---
> drivers/input/keyboard/mtk-pmic-keys.c | 30 +++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c31ab4368388..8e4fa7cd16e6 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -34,6 +34,13 @@
> #define MTK_PMIC_HOMEKEY_INDEX 1
> #define MTK_PMIC_MAX_KEY_COUNT 2
>
> +/**
> + * struct mtk_pmic_keys_regs - PMIC keys per-key registers
> + * @deb_reg: Debounced key status register
> + * @deb_mask: Bitmask of this key in status register
> + * @intsel_reg: Interrupt selector register
> + * @intsel_mask: Bitmask of this key in interrupt selector
> + */
> struct mtk_pmic_keys_regs {
> u32 deb_reg;
> u32 deb_mask;
> @@ -50,6 +57,11 @@ struct mtk_pmic_keys_regs {
> .intsel_mask = _intsel_mask, \
> }
>
> +/**
> + * struct mtk_pmic_regs - PMIC Keys registers
> + * @keys_regs: Specific key registers
This new description of the structure and of the keys_regs does not add
any information for me.
> + * @pmic_rst_reg: PMIC Keys reset register
> + */
> struct mtk_pmic_regs {
> const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
> u32 pmic_rst_reg;
> @@ -85,15 +97,31 @@ static const struct mtk_pmic_regs mt6358_regs = {
> .pmic_rst_reg = MT6358_TOP_RST_MISC,
> };
>
> +/**
> + * struct mtk_pmic_keys_info - PMIC Keys per-key params
> + * @keys: Pointer to main driver structure
That is obvious from the field definition.
> + * @regs: Register offsets/masks for this key
Ditto.
> + * @keycode: Key code for this key
Yep.
> + * @irq: Keypress or press/release interrupt
> + * @irq_r: Key release interrupt (optional)
> + * @wakeup: Indicates whether to use this key as a wakeup source
> + */
> struct mtk_pmic_keys_info {
> struct mtk_pmic_keys *keys;
> const struct mtk_pmic_keys_regs *regs;
> unsigned int keycode;
> int irq;
> - int irq_r; /* optional: release irq if different */
> + int irq_r;
> bool wakeup:1;
> };
>
> +/**
> + * struct mtk_pmic_keys - Main driver structure
> + * @input_dev: Input device pointer
I do not find this helpful.
> + * @dev: Device pointer
And neither this.
> + * @regmap: Regmap handle
Nor this.
> + * @keys: Per-key parameters
> + */
> struct mtk_pmic_keys {
> struct input_dev *input_dev;
> struct device *dev;
> --
> 2.35.1
>
In the end we ended up with something that now has a chance of
introducing warning when someone changes code, for very little benefit,
if any at all.
For driver-private data and functions we should rely on expressive
variable and function names and only use comments for something that
might be unclear or requires additional qualification.
Thanks.
--
Dmitry
