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: <20230426071045.20753-1-zhuyinbo@loongson.cn> <20230426071045.20753-3-zhuyinbo@loongson.cn>
 <ZFkPZhF8QqScXAmH@surfacebook> <ZFnOZptCM7JDFTQz@finisterre.sirena.org.uk>
In-Reply-To: <ZFnOZptCM7JDFTQz@finisterre.sirena.org.uk>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 10 May 2023 10:03:21 +0300
Message-ID: <CAHp75VcqQR0fFdkWG2QgXG0+SnKDs6_Zze6GMt+pHHEdE+8hkg@mail.gmail.com>
Subject: Re: [PATCH v9 2/2] spi: loongson: add bus driver for the loongson spi controller
To:     Mark Brown <broonie@kernel.org>
Cc:     Yinbo Zhu <zhuyinbo@loongson.cn>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jianmin Lv <lvjianmin@loongson.cn>,
        wanghongliang@loongson.cn, Liu Peibao <liupeibao@loongson.cn>,
        loongson-kernel@lists.loongnix.cn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 9, 2023 at 7:39=E2=80=AFAM Mark Brown <broonie@kernel.org> wrot=
e:
> On Mon, May 08, 2023 at 06:04:06PM +0300, andy.shevchenko@gmail.com wrote=
:
> > Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu kirjoitti:

...

> > > +           loongson_spi_write_reg(loongson_spi,
> > > +                                  LOONGSON_SPI_SFCS_REG,
> > > +                                  (val ? (0x11 << spi->chip_select) =
:
> > > +                                  (0x1 << spi->chip_select)) | cs);
>
> > Too many parentheses.
>
> The code is absolutely fine, there is nothing wrong with adding explicit
> parentheses even where not strictly needed if it helps to make things
> clear (which is obviously always a problem wiht ternery operator use).

> Please, stop this sort of nitpicking.  It is at times actively unhelpful.

Okay, sorry for the noise.

...

> > > +           bit =3D fls(div) - 1;
> > > +           if ((1<<bit) =3D=3D div)
> > > +                   bit--;
> > > +           div_tmp =3D rdiv[bit];
>
> > I believe this can be optimized.
>
> This isn't constructive feedback, if there is a concrete optimisation
> you want to suggest please just suggest it.

It goes together with the other questions in this function. But if you
think about some code proposal, here we are:

_original_

       const char rdiv[12] =3D {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};

               div =3D DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz);
               if (div < 2)
                       div =3D 2;
               if (div > 4096)
                       div =3D 4096;

               bit =3D fls(div) - 1;
               if ((1<<bit) =3D=3D div)
                       bit--;
               div_tmp =3D rdiv[bit];

               loongson_spi->spcr =3D div_tmp & 3;
               loongson_spi->sper =3D (div_tmp >> 2) & 3;

_proposed_

       const char rdiv[12] =3D {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};

// as far as I understood the above table
00 00  [0]  <=3D 2
00 01  [1]  <=3D 4
01 00  [2]  <=3D 8
00 10  [3]  <=3D 16
00 11  [4]  <=3D 32
01 01  [5]  <=3D 64
01 10  [6]  <=3D 128
01 11  [7]  <=3D 256
10 00  [8]  <=3D 512
10 01  [9]  <=3D 1024
10 10  [10]  <=3D 2048
10 11  [11]  <=3D 4096

               div =3D
clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096);
               div_tmp =3D rdiv[fls(div - 1)];

               loongson_spi->spcr =3D (div_tmp & GENMASK(1, 0)) >> 0;
               loongson_spi->sper =3D (div_tmp & GENMASK(3, 2)) >> 2;

But TBH I would expect the author to think about it more.

Also the check can be modified to have less indented code:

       if (hz && loongson_spi->hz =3D=3D hz)
          return 0;

       if (!((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK))
          return 0;

...


> > > +EXPORT_SYMBOL_GPL(loongson_spi_init_master);
>
> > Please, use _NS variant.
>
> It really does not matter, the chances of any collisions is pretty much
> zero.

The point is in preventing us from using those symbols outside of the scope=
.

--
With Best Regards,
Andy Shevchenko