| Message ID | 20231205225638.32563-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | backlight: ili922x: fix W=1 kernel-doc warnings | expand |
On Tue, Dec 05, 2023 at 02:56:38PM -0800, Randy Dunlap wrote: > Fix kernel-doc warnings found when using "W=1". > > ili922x.c:85: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst > ili922x.c:85: warning: missing initial short description on line: > * START_BYTE(id, rs, rw) > ili922x.c:91: warning: contents before sections > ili922x.c:118: warning: expecting prototype for CHECK_FREQ_REG(spi_device s, spi_transfer x)(). Prototype was for CHECK_FREQ_REG() instead > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Cc: Lee Jones <lee@kernel.org> > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Jingoo Han <jingoohan1@gmail.com> > Cc: Helge Deller <deller@gmx.de> > Cc: linux-fbdev@vger.kernel.org > --- > drivers/video/backlight/ili922x.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff -- a/drivers/video/backlight/ili922x.c b/drivers/video/backlight/ili922x.c > --- a/drivers/video/backlight/ili922x.c > +++ b/drivers/video/backlight/ili922x.c > @@ -82,13 +82,12 @@ > #define START_RW_READ 1 > > /** > - * START_BYTE(id, rs, rw) > - * > - * Set the start byte according to the required operation. > + * START_BYTE() - Set the start byte according to the required operation. > * The start byte is defined as: > * ---------------------------------- > * | 0 | 1 | 1 | 1 | 0 | ID | RS | RW | > * ---------------------------------- I'm not sure we want "The start byte is defined as" in the brief description. Needs a blank line between the brief and full description (or hoist the argument descriptions up to match the idiomatic form for a kernel-doc comment in the docs if you prefer). > + * > * @id: display's id as set by the manufacturer > * @rs: operation type bit, one of: > * - START_RS_INDEX set the index register > @@ -101,14 +100,14 @@ > (0x70 | (((id) & 0x01) << 2) | (((rs) & 0x01) << 1) | ((rw) & 0x01)) > > /** > - * CHECK_FREQ_REG(spi_device s, spi_transfer x) - Check the frequency > + * CHECK_FREQ_REG() - Check the frequency > * for the SPI transfer. Likewise I think there is no need for "According to the datasheet..." to be included in the brief description. Daniel. > According to the datasheet, the controller > * accept higher frequency for the GRAM transfer, but it requires > * lower frequency when the registers are read/written. > * The macro sets the frequency in the spi_transfer structure if > * the frequency exceeds the maximum value. > * @s: pointer to an SPI device > - * @x: pointer to the read/write buffer pair > + * @x: pointer to the &spi_transfer read/write buffer pair > */ > #define CHECK_FREQ_REG(s, x) \ > do { \
On Wed, 06 Dec 2023, Daniel Thompson wrote: > On Tue, Dec 05, 2023 at 02:56:38PM -0800, Randy Dunlap wrote: > > Fix kernel-doc warnings found when using "W=1". > > > > ili922x.c:85: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst > > ili922x.c:85: warning: missing initial short description on line: > > * START_BYTE(id, rs, rw) > > ili922x.c:91: warning: contents before sections > > ili922x.c:118: warning: expecting prototype for CHECK_FREQ_REG(spi_device s, spi_transfer x)(). Prototype was for CHECK_FREQ_REG() instead > > > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > > Cc: Lee Jones <lee@kernel.org> > > Cc: Daniel Thompson <daniel.thompson@linaro.org> > > Cc: Jingoo Han <jingoohan1@gmail.com> > > Cc: Helge Deller <deller@gmx.de> > > Cc: linux-fbdev@vger.kernel.org > > --- > > drivers/video/backlight/ili922x.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff -- a/drivers/video/backlight/ili922x.c b/drivers/video/backlight/ili922x.c > > --- a/drivers/video/backlight/ili922x.c > > +++ b/drivers/video/backlight/ili922x.c > > @@ -82,13 +82,12 @@ > > #define START_RW_READ 1 > > > > /** > > - * START_BYTE(id, rs, rw) > > - * > > - * Set the start byte according to the required operation. > > + * START_BYTE() - Set the start byte according to the required operation. > > * The start byte is defined as: > > * ---------------------------------- > > * | 0 | 1 | 1 | 1 | 0 | ID | RS | RW | > > * ---------------------------------- > > I'm not sure we want "The start byte is defined as" in the brief > description. Needs a blank line between the brief and full description > (or hoist the argument descriptions up to match the idiomatic > form for a kernel-doc comment in the docs if you prefer). I'd consider dropping the kernel-docness of this header entirely. Kerneldoc is designed for documenting exported (or at least externally available) functions and data structures, with allowances for static functions in the name of consistency or in cases of excessive complication. I've fixed A LOT of kernel-doc headers in my time and I can't say I remember coming across MACROs being documented this way before now.
On 12/6/23 05:25, Lee Jones wrote: > On Wed, 06 Dec 2023, Daniel Thompson wrote: > >> On Tue, Dec 05, 2023 at 02:56:38PM -0800, Randy Dunlap wrote: >>> Fix kernel-doc warnings found when using "W=1". >>> >>> ili922x.c:85: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst >>> ili922x.c:85: warning: missing initial short description on line: >>> * START_BYTE(id, rs, rw) >>> ili922x.c:91: warning: contents before sections >>> ili922x.c:118: warning: expecting prototype for CHECK_FREQ_REG(spi_device s, spi_transfer x)(). Prototype was for CHECK_FREQ_REG() instead >>> >>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >>> Cc: Lee Jones <lee@kernel.org> >>> Cc: Daniel Thompson <daniel.thompson@linaro.org> >>> Cc: Jingoo Han <jingoohan1@gmail.com> >>> Cc: Helge Deller <deller@gmx.de> >>> Cc: linux-fbdev@vger.kernel.org >>> --- >>> drivers/video/backlight/ili922x.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff -- a/drivers/video/backlight/ili922x.c b/drivers/video/backlight/ili922x.c >>> --- a/drivers/video/backlight/ili922x.c >>> +++ b/drivers/video/backlight/ili922x.c >>> @@ -82,13 +82,12 @@ >>> #define START_RW_READ 1 >>> >>> /** >>> - * START_BYTE(id, rs, rw) >>> - * >>> - * Set the start byte according to the required operation. >>> + * START_BYTE() - Set the start byte according to the required operation. >>> * The start byte is defined as: >>> * ---------------------------------- >>> * | 0 | 1 | 1 | 1 | 0 | ID | RS | RW | >>> * ---------------------------------- >> >> I'm not sure we want "The start byte is defined as" in the brief >> description. Needs a blank line between the brief and full description >> (or hoist the argument descriptions up to match the idiomatic >> form for a kernel-doc comment in the docs if you prefer). > > I'd consider dropping the kernel-docness of this header entirely. > Kerneldoc is designed for documenting exported (or at least externally > available) functions and data structures, with allowances for static > functions in the name of consistency or in cases of excessive > complication. I've fixed A LOT of kernel-doc headers in my time and I > can't say I remember coming across MACROs being documented this way > before now. > I've seen several macros that are documented, but I am happy to just drop the kernel-doc for these local macros. I'll send a patch for that. Thanks.
diff -- a/drivers/video/backlight/ili922x.c b/drivers/video/backlight/ili922x.c --- a/drivers/video/backlight/ili922x.c +++ b/drivers/video/backlight/ili922x.c @@ -82,13 +82,12 @@ #define START_RW_READ 1 /** - * START_BYTE(id, rs, rw) - * - * Set the start byte according to the required operation. + * START_BYTE() - Set the start byte according to the required operation. * The start byte is defined as: * ---------------------------------- * | 0 | 1 | 1 | 1 | 0 | ID | RS | RW | * ---------------------------------- + * * @id: display's id as set by the manufacturer * @rs: operation type bit, one of: * - START_RS_INDEX set the index register @@ -101,14 +100,14 @@ (0x70 | (((id) & 0x01) << 2) | (((rs) & 0x01) << 1) | ((rw) & 0x01)) /** - * CHECK_FREQ_REG(spi_device s, spi_transfer x) - Check the frequency + * CHECK_FREQ_REG() - Check the frequency * for the SPI transfer. According to the datasheet, the controller * accept higher frequency for the GRAM transfer, but it requires * lower frequency when the registers are read/written. * The macro sets the frequency in the spi_transfer structure if * the frequency exceeds the maximum value. * @s: pointer to an SPI device - * @x: pointer to the read/write buffer pair + * @x: pointer to the &spi_transfer read/write buffer pair */ #define CHECK_FREQ_REG(s, x) \ do { \
Fix kernel-doc warnings found when using "W=1". ili922x.c:85: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst ili922x.c:85: warning: missing initial short description on line: * START_BYTE(id, rs, rw) ili922x.c:91: warning: contents before sections ili922x.c:118: warning: expecting prototype for CHECK_FREQ_REG(spi_device s, spi_transfer x)(). Prototype was for CHECK_FREQ_REG() instead Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Cc: Lee Jones <lee@kernel.org> Cc: Daniel Thompson <daniel.thompson@linaro.org> Cc: Jingoo Han <jingoohan1@gmail.com> Cc: Helge Deller <deller@gmx.de> Cc: linux-fbdev@vger.kernel.org --- drivers/video/backlight/ili922x.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)