Re: [PATCH v11 2/2] spi: loongson: add bus driver for the loongson spi controller

[zhuyinbo]

在 2023/5/24 下午4:42, Andy Shevchenko 写道:
> On Wed, May 24, 2023 at 10:52 AM zhuyinbo <zhuyinbo@xxxxxxxxxxx> wrote:
> > 在 2023/5/23 下午8:54, andy.shevchenko@xxxxxxxxx 写道:
> > > Mon, May 22, 2023 at 03:10:30PM +0800, Yinbo Zhu kirjoitti:
>
> ...
>
> > > > +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
> > > > +                            struct spi_device *spi, struct spi_transfer *t)
> > > > +{
> > > > +    unsigned int hz;
> > > > +
> > > > +    if (t)
> > > > +            hz = t->speed_hz;
> > >
> > > And if t is NULL? hz will be uninitialized. Don't you get a compiler warning?
> > > (Always test your code with `make W=1 ...`)
> >
> > I always use `make W=1` and I don't find any warning, but that what you
> > said was right and I will initial hz.
>
> Note, if hz == 0 when t == NULL, you can unify that check with the below.
>
> > > > +    if (hz && loongson_spi->hz != hz)
>
> Something like
>
>    if (t && _spi->hz != t->speed_hz)
>      ...(..., t->speed_hz);
>
> In such a case you won't need a temporary variable.

okay, I got it.

> > > > +            loongson_spi_set_clk(loongson_spi, hz);
> > > > +
> > > > +    if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
> > > > +            loongson_spi_set_mode(loongson_spi, spi);
> > > > +
> > > > +    return 0;
> > > > +}
>
> ...
>
> > > Why do you use deprecated naming? Can you use spi_controller* instead of
> > > spi_master* in all cases?
> >
> > It seems was a personal code style issue and I don't find the
> > differences between spi_controller and spi_master, Using spi_controller*
> > is also acceptable to me and I will use spi_controller* instead of
> > spi_master* in all cases.
>
> Read this section (#4) in full
> https://kernel.org/doc/html/latest/process/coding-style.html#naming

okay, I got it.

> ...
>
> > > > +    clk = devm_clk_get_optional(dev, NULL);
> > > > +    if (!IS_ERR(clk))
> > > > +            spi->clk_rate = clk_get_rate(clk);
> > >
> > > > +    else
> > >
> > > Redundant. Just check for the error first as it's very usual pattern in the
> > > Linux kernel.
> >
> > Like below ?
> >
> >           clk = devm_clk_get_optional(dev, NULL);
> > -       if (!IS_ERR(clk))
> > -               spi->clk_rate = clk_get_rate(clk);
> > -       else
> > +       if (IS_ERR(clk))
> >                   return dev_err_probe(dev, PTR_ERR(clk), "unable to get
> > clock\n");
> >
> > +       spi->clk_rate = clk_get_rate(clk);
>
> Yes.

okay, I got it.
> >           loongson_spi_reginit(spi);
>
> > > > +            return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");
>
> ...
>
> > > > +    ret = loongson_spi_init_master(dev, reg_base);
> > > > +    if (ret)
> > > > +            return dev_err_probe(dev, ret, "failed to initialize master\n");
> > > > +
> > > > +    return ret;
> > >
> > >        return 0;
> >
> > It seems was more appropriate that initialize ret then return ret.
> > Do you think so ?
>
> What do you mean and how does it help here?

I'm sorry, I was wrong before and the ret varible seems not to be
initialized and it always record the return value for
loongson_spi_init_master.

It seems was appropriate that use "return ret" and I don't got your
point that in probe for use "return 0"

> ...
>
> > > > +#include <linux/spi/spi.h>
> > >
> > > This neither.
> >
> > That other .c file seems to need it and I will move it to other .c
> > code file.
>
> Yes, please do.

okay, I got it.

Thanks,
Yinbo