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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





在 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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux