[PATCH] backlight: ili922x: fix W=1 kernel-doc warnings

[PATCH] backlight: ili922x: fix W=1 kernel-doc warnings

[Randy Dunlap]

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 |
  *   ----------------------------------
+ *
  * @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 {			\

Re: [PATCH] backlight: ili922x: fix W=1 kernel-doc warnings

[Daniel Thompson]

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 {			\

Re: [PATCH] backlight: ili922x: fix W=1 kernel-doc warnings

[Lee Jones]

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.

-- 
Lee Jones [李琼斯]

Re: [PATCH] backlight: ili922x: fix W=1 kernel-doc warnings

[Randy Dunlap]

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.
-- 
~Randy