Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc

[Tomi Valkeinen]

On 21/03/2024 17:33, Sean Anderson wrote:
> On 3/20/24 02:05, Randy Dunlap wrote:
> > On 3/19/24 22:42, Tomi Valkeinen wrote:
> > > On 20/03/2024 00:51, Sean Anderson wrote:
> > > > Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
> > > > members.
> > > >
> > > > Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - New
> > > >
> > > >    drivers/gpu/drm/xlnx/zynqmp_disp.c  | 6 +++---
> > > >    drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
> > > >    drivers/gpu/drm/xlnx/zynqmp_kms.h   | 4 ++--
> > > >    3 files changed, 6 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > > > index 407bc07cec69..f79bf3fb8110 100644
> > > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > > > @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
> > > >     * struct zynqmp_disp - Display controller
> > > >     * @dev: Device structure
> > > >     * @dpsub: Display subsystem
> > > > - * @blend.base: Register I/O base address for the blender
> > > > - * @avbuf.base: Register I/O base address for the audio/video buffer manager
> > > > - * @audio.base: Registers I/O base address for the audio mixer
> > > > + * @blend: Register I/O base address for the blender
> > > > + * @avbuf: Register I/O base address for the audio/video buffer manager
> > > > + * @audio: Registers I/O base address for the audio mixer
> > >
> > > Afaics, the kernel doc guide:
> > >
> > > https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions
> > >
> > > says that the current version is correct. Or is the issue that while, say, 'base' is documented, 'blend' was not?
> >
> > Hi,
> >
> > I would do it more like so:
> >
> > ---
> >   drivers/gpu/drm/xlnx/zynqmp_disp.c |    3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff -- a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -128,8 +128,11 @@ struct zynqmp_disp_layer {
> >    * struct zynqmp_disp - Display controller
> >    * @dev: Device structure
> >    * @dpsub: Display subsystem
> > + * @blend: blender iomem info
> >    * @blend.base: Register I/O base address for the blender
> > + * @avbuf: audio/video buffer iomem info
> >    * @avbuf.base: Register I/O base address for the audio/video buffer manager
> > + * @audio: audio mixer iomem info
> >    * @audio.base: Registers I/O base address for the audio mixer
> >    * @layers: Layers (planes)
> >    */
> >
> >
> > but in my testing, Sean's way or my way result in no warning/errors.
>
> The specific errors are:
>
> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'blend' not described in 'zynqmp_disp'
> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'avbuf' not described in 'zynqmp_disp'
> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'audio' not described in 'zynqmp_disp'
>
> I don't see the need to document a single-member struct twice. Actually,

But if only the struct is documented, then we're documenting the wrong 
thing. A tool showing to the user what blend.base is would miss that 
documentation.

> maybe it would be better to just lift the .base member to live in
> zynqmp_disp. But I think that would be better in another series.

Yes, there's not much point with the structs.

 Tomi