| Message ID | 20240319225122.3048400-2-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: zynqmp_dp: Misc. patches and debugfs support | expand |
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@linux.dev> > --- > > 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? Tomi
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@linux.dev> >> --- >> >> 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.
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@linux.dev> >>> --- >>> >>> 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, 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. --Sean
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@linux.dev> >>>> --- >>>> >>>> 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
On 3/22/24 01:50, Tomi Valkeinen wrote: > 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@linux.dev> >>>>> --- >>>>> >>>>> 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. Are there any such tools? kerneldoc e.g. just prints the definition and then a list of members with documentation. So from the user's perspective the only thing which changes is the name. --Sean >> 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 >
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 * @layers: Layers (planes) */ struct zynqmp_disp { diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h index 09ea01878f2a..b18554467e9c 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h @@ -53,6 +53,7 @@ enum zynqmp_dpsub_format { * @drm: The DRM/KMS device data * @bridge: The DP encoder bridge * @disp: The display controller + * @layers: Video and graphics layers * @dp: The DisplayPort controller * @dma_align: DMA alignment constraint (must be a power of 2) */ diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.h b/drivers/gpu/drm/xlnx/zynqmp_kms.h index 01be96b00e3f..cb13c6b8008e 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_kms.h +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.h @@ -22,9 +22,9 @@ struct zynqmp_dpsub; /** - * struct zynqmp_dpsub - ZynqMP DisplayPort Subsystem DRM/KMS data + * struct zynqmp_dpsub_drm - ZynqMP DisplayPort Subsystem DRM/KMS data * @dpsub: Backpointer to the DisplayPort subsystem - * @drm: The DRM/KMS device + * @dev: The DRM/KMS device * @planes: The DRM planes * @crtc: The DRM CRTC * @encoder: The dummy DRM encoder
Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed members. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- 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(-)