Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <5c894d0a8a178628ca200d054004be3c@208suo.com> <f25017a660f8a3a4e49258a1d96003dc@208suo.com>
 <2f4a7303e305d07fdd9c15a9dc95dc5f@208suo.com> <d0d767fb0dd3a9b7642559f379dbb0d6@208suo.com>
 <0ad4154429aaa41ca26da859c3f35784@208suo.com> <856d1130fad58062ea454da297fa64348382bda2.camel@redhat.com>
In-Reply-To: <856d1130fad58062ea454da297fa64348382bda2.camel@redhat.com>
From:   Karol Herbst <kherbst@redhat.com>
Date:   Sat, 15 Jul 2023 11:23:01 +0200
Message-ID: <CACO55tu08XTx=TZQK=RyCGRdFVDQN9m+X+y3F3V0H7nuwyycRw@mail.gmail.com>
Subject: Re: [PATCH] drm/nouveau/fifo:Fix Nineteen occurrences of the gk104.c
 error: ERROR: : trailing statements should be on next line
To:     Lyude Paul <lyude@redhat.com>
Cc:     huzhi001@208suo.com, bskeggs@redhat.com, airlied@gmail.com,
        daniel@ffwll.ch, dri-devel@lists.freedesktop.org,
        nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Jul 15, 2023 at 1:07=E2=80=AFAM Lyude Paul <lyude@redhat.com> wrote=
:
>
> NAK - checkpatch.pl is a (strongish) guideline, but not a rule. In the ca=
ses
> 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 onl=
y
> 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@208suo.com wrote:
> > Signed-off-by: ZhiHu <huzhi001@208suo.com>
> > ---
> >   .../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 =3D 0ULL;
> >
> >       switch (engn->engine->subdev.type) {
> > -    case NVKM_ENGINE_SW    : return;
> > -    case NVKM_ENGINE_GR    : ptr0 =3D 0x0210; break;
> > -    case NVKM_ENGINE_SEC   : ptr0 =3D 0x0220; break;
> > -    case NVKM_ENGINE_MSPDEC: ptr0 =3D 0x0250; break;
> > -    case NVKM_ENGINE_MSPPP : ptr0 =3D 0x0260; break;
> > -    case NVKM_ENGINE_MSVLD : ptr0 =3D 0x0270; break;
> > -    case NVKM_ENGINE_VIC   : ptr0 =3D 0x0280; break;
> > -    case NVKM_ENGINE_MSENC : ptr0 =3D 0x0290; break;
> > -    case NVKM_ENGINE_NVDEC :
> > +    case NVKM_ENGINE_SW:
> > +        return;
> > +    case NVKM_ENGINE_GR:
> > +        ptr0 =3D 0x0210;
> > +        break;
> > +    case NVKM_ENGINE_SEC:
> > +        ptr0 =3D 0x0220;
> > +        break;
> > +    case NVKM_ENGINE_MSPDEC:
> > +        ptr0 =3D 0x0250;
> > +        break;
> > +    case NVKM_ENGINE_MSPPP:
> > +        ptr0 =3D 0x0260;
> > +        break;
> > +    case NVKM_ENGINE_MSVLD:
> > +        ptr0 =3D 0x0270;
> > +        break;
> > +    case NVKM_ENGINE_VIC:
> > +        ptr0 =3D 0x0280; break;
> > +    case NVKM_ENGINE_MSENC:
> > +        ptr0 =3D 0x0290;
> > +        break;
> > +    case NVKM_ENGINE_NVDEC:
> >           ptr1 =3D 0x0270;
> >           ptr0 =3D 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 =3D 0; break;
> > -    case NVKM_MEM_TARGET_NCOH: target =3D 3; break;
> > +    case NVKM_MEM_TARGET_VRAM:
> > +        target =3D 0;
> > +        break;
> > +    case NVKM_MEM_TARGET_NCOH:
> > +        target =3D 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
>