On Sat, Jul 15, 2023 at 1:07 AM Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> NAK - checkpatch.pl is a (strongish) guideline, but not a rule. In the cases
> corrected in the patch series here, we format the switch cases on single lines
> as it dramatically improves the readability of what is otherwise just a /long/
> list of slightly different static mappings. I don't believe we're the only
> part of the kernel to do this either.
>
I wished there was a place to document something like "patches whose
only reason is 'checkpatch.pl' said so" will be rejected.
I think following some checkpatch rules are sane, but then the
argument should be "makes code more clear" or "converts risky coding
practises to less risky ones". Or do we have such a place? Maybe we
should patch checkpatch.pl instead to throw a warning
> On Fri, 2023-07-14 at 14:58 +0800, huzhi001@xxxxxxxxxx wrote:
> > Signed-off-by: ZhiHu <huzhi001@xxxxxxxxxx>
> > ---
> > .../gpu/drm/nouveau/nvkm/engine/fifo/gk104.c | 40 ++++++++++++++-----
> > 1 file changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > index d8a4d773a58c..b99e0a7c96bb 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c
> > @@ -137,15 +137,29 @@ gk104_ectx_bind(struct nvkm_engn *engn, struct
> > nvkm_cctx *cctx, struct nvkm_chan
> > u64 addr = 0ULL;
> >
> > switch (engn->engine->subdev.type) {
> > - case NVKM_ENGINE_SW : return;
> > - case NVKM_ENGINE_GR : ptr0 = 0x0210; break;
> > - case NVKM_ENGINE_SEC : ptr0 = 0x0220; break;
> > - case NVKM_ENGINE_MSPDEC: ptr0 = 0x0250; break;
> > - case NVKM_ENGINE_MSPPP : ptr0 = 0x0260; break;
> > - case NVKM_ENGINE_MSVLD : ptr0 = 0x0270; break;
> > - case NVKM_ENGINE_VIC : ptr0 = 0x0280; break;
> > - case NVKM_ENGINE_MSENC : ptr0 = 0x0290; break;
> > - case NVKM_ENGINE_NVDEC :
> > + case NVKM_ENGINE_SW:
> > + return;
> > + case NVKM_ENGINE_GR:
> > + ptr0 = 0x0210;
> > + break;
> > + case NVKM_ENGINE_SEC:
> > + ptr0 = 0x0220;
> > + break;
> > + case NVKM_ENGINE_MSPDEC:
> > + ptr0 = 0x0250;
> > + break;
> > + case NVKM_ENGINE_MSPPP:
> > + ptr0 = 0x0260;
> > + break;
> > + case NVKM_ENGINE_MSVLD:
> > + ptr0 = 0x0270;
> > + break;
> > + case NVKM_ENGINE_VIC:
> > + ptr0 = 0x0280; break;
> > + case NVKM_ENGINE_MSENC:
> > + ptr0 = 0x0290;
> > + break;
> > + case NVKM_ENGINE_NVDEC:
> > ptr1 = 0x0270;
> > ptr0 = 0x0210;
> > break;
> > @@ -435,8 +449,12 @@ gk104_runl_commit(struct nvkm_runl *runl, struct
> > nvkm_memory *memory, u32 start,
> > int target;
> >
> > switch (nvkm_memory_target(memory)) {
> > - case NVKM_MEM_TARGET_VRAM: target = 0; break;
> > - case NVKM_MEM_TARGET_NCOH: target = 3; break;
> > + case NVKM_MEM_TARGET_VRAM:
> > + target = 0;
> > + break;
> > + case NVKM_MEM_TARGET_NCOH:
> > + target = 3;
> > + break;
>
> This one isn't very long, but I'd still say it's definitely a lot easier to
> read in the compact form. If anything, the only change I would make here is
> formatting the default: case to be on a single line as well
>
> > default:
> > WARN_ON(1);
> > return;
>
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
