MIME-Version: 1.0
References: <1638850665-9474-1-git-send-email-wellslutw@gmail.com>
 <1638850665-9474-3-git-send-email-wellslutw@gmail.com> <CACRpkdYO-hgEKsra-+qPq=apSqRBHDnzfP6YYLp6wGb++XEsNA@mail.gmail.com>
 <CAFnkrsnY=-9VuN5ek9e-wyU+udrc6zsm_ttupe0mkEkcDK5j5A@mail.gmail.com>
In-Reply-To: <CAFnkrsnY=-9VuN5ek9e-wyU+udrc6zsm_ttupe0mkEkcDK5j5A@mail.gmail.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Sat, 11 Dec 2021 00:45:30 +0100
Message-ID: <CACRpkdYYwHMh+DqoyP=XvWmnUMrDJngsJuH+7DUvYqjr48XJ2A@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] pinctrl: Add driver for Sunplus SP7021
To:     =?UTF-8?B?5ZGC6Iqz6aiw?= <wellslutw@gmail.com>
Cc:     linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        robh+dt@kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Wells Lu <wells.lu@sunplus.com>,
        Dvorkin Dmitry <dvorkin@tibbo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Wells,

I understand things better now!

On Fri, Dec 10, 2021 at 6:34 PM =E5=91=82=E8=8A=B3=E9=A8=B0 <wellslutw@gmai=
l.com> wrote:

> > #define SSPCTL_FUNC_FLAG BIT(0)
> >
> > if (func & SSPCTL_FUNC_FLAG)
> >
> > Use the name that bit has in your documentation for the
> > define so we know what is going on.
>
> Actually, 'if (func & 1)' is not used to test bit 0,
> but test 'func' is an odd number or not.
> If 'func' is even number, control-field is at bit 6 ~ 0.
> Its corresponding mask-field is at bit 22 ~ 16.
> If 'func' is odd number, control-field is at bit 14 ~ 8.
> Its corresponding mask-field is at bit 30 ~ 24.

Aha! That makes sense. Just put in exactly that comment in
a block comment in the code so people maintaining this
can see what is going on here and why you are checking
for an odd/even number.

> Control and mask fields of 'func' are arranged as shown
> below:
>
> func # | register control-field  mask-field
> -------+------------------------------------
>    0   | reg[0]     ( 6:0)        (22 :  6)
>    1   | reg[0]     (14:8)        (30 : 24)
>    2   | reg[1]     ( 6:0)        (22 :  6)
>    3   | reg[1]     (14:8)        (30 : 24)

This is also nice to actually have in the code. Be generous
with comments I like that.

> > This layout with "mask and value" in registers needs to be explained
> > somewhere it looks complex. I don't understand why a machine register
> > contains a mask for example.
>
> This is a hardware mechanism for protecting some important registers from
> being overwritten accidentally. The corresponding mask bit should be set
> first and then the control-bits or fields can be written. The design is
> originally requested from car makers.

Wow that is very very interesting! So the feature is there to stop a
program that goes astray and just write random numbers all over the
memory from doing something harmful.

Insert some comments about this at the top of the file or at the first
time you use a mask.

Thanks for keeping fixing this up despite my sometimes confused
comments. Most of my comment is about making the code easy
to read by people that will maintain it, so my rule of thumb is if I
have a hard time to understand it then others may have a hard time
with it too.

Yours,
Linus Walleij