| Message ID | 20220520125132.229191-2-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MediaTek Helio X10 MT6795 - MT6331 PMIC Keys | expand |
On ven., mai 20, 2022 at 14:51, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > To enhance human readability, add kerneldoc to all driver structs. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > --- > 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 > + * @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 > + * @regs: Register offsets/masks for this key > + * @keycode: Key code for this key > + * @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 > + * @dev: Device pointer > + * @regmap: Regmap handle > + * @keys: Per-key parameters > + */ > struct mtk_pmic_keys { > struct input_dev *input_dev; > struct device *dev; > -- > 2.35.1
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@collabora.com> > --- > 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.
Il 23/05/22 06:33, Dmitry Torokhov ha scritto: > 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@collabora.com> >> --- >> 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. > Hello Dmitry, it's been very helpful for me to see kerneldoc documentation in the various drivers across the kernel - helped me understanding what was going on in an easier, more immediate way, especially when looking at drivers having some kind of "complicated" flow. About introducing warnings when someone changes code, I believe that this may also be helpful (for a developer) in some *corner* cases, but I agree that this is unnecessarily tedious in some others... in the end, it's all about personal opinions... Of course, some of the documentation being obvious is unavoidable when it comes to kerneldoc as you either document 'em all, or nothing. In any case, if you really dislike having this kind of documentation, I can drop these commits and eventually add in-line comments to some variables to make them perfectly understandable, or I can avoid documenting at all (even though I am strongly for documenting things clearly). Regards, Angelo > Thanks. >
On Mon, May 23, 2022 at 10:54:03AM +0200, AngeloGioacchino Del Regno wrote: > Il 23/05/22 06:33, Dmitry Torokhov ha scritto: > > 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@collabora.com> > > > --- > > > 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. > > > > Hello Dmitry, > > it's been very helpful for me to see kerneldoc documentation in the various > drivers across the kernel - helped me understanding what was going on in an > easier, more immediate way, especially when looking at drivers having some > kind of "complicated" flow. > About introducing warnings when someone changes code, I believe that this > may also be helpful (for a developer) in some *corner* cases, but I agree > that this is unnecessarily tedious in some others... in the end, it's all > about personal opinions... > > Of course, some of the documentation being obvious is unavoidable when it > comes to kerneldoc as you either document 'em all, or nothing. > > In any case, if you really dislike having this kind of documentation, I can > drop these commits and eventually add in-line comments to some variables to > make them perfectly understandable, or I can avoid documenting at all (even > though I am strongly for documenting things clearly). I might be mistaken, but I think what you appreciated is not presence of comments particularly in kerneldoc format (you did not generate htmldocs or something to study the drivers internal API disconnected from the code, did you?) but rather the fact that some drivers have been well-commented. To be clear, I am all for adding meaningful comments to the drivers structures and code, but kerneldoc is too rigid and adds too much noise. As I mentioned "this is a pointer to an input device" description might be OK when looking at HTML docs on the web or elsewhere, but really does not add anything when you see "struct input_dev *input_dev;" a few lines below. So my preference would be to add free-form comments to places where intent is not clear. For example the original comment to "irq_r" was pretty good IMO - it called out some irregularity and explained what it is. We could definitely add comment to "irq" as well to tell that we expect it to fire on both press and release. Thanks.
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 + * @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 + * @regs: Register offsets/masks for this key + * @keycode: Key code for this key + * @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 + * @dev: Device pointer + * @regmap: Regmap handle + * @keys: Per-key parameters + */ struct mtk_pmic_keys { struct input_dev *input_dev; struct device *dev;
To enhance human readability, add kerneldoc to all driver structs. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/input/keyboard/mtk-pmic-keys.c | 30 +++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)