在 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
