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

[zhuyinbo <zhuyinbo@loongson.cn>]

在 2023/5/24 下午4:42, Andy Shevchenko 写道:
> On Wed, May 24, 2023 at 10:52 AM zhuyinbo <zhuyinbo@loongson.cn> wrote:
>> 在 2023/5/23 下午8:54, andy.shevchenko@gmail.com 写道:
>>> 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