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;
Subject: Re: [PATCH v11 2/2] spi: loongson: add bus driver for the loongson
 spi controller
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Mark Brown <broonie@kernel.org>, 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, zhuyinbo@loongson.cn
References: <20230522071030.5193-1-zhuyinbo@loongson.cn>
 <20230522071030.5193-3-zhuyinbo@loongson.cn> <ZGy3b7ZfNwWoGDTu@surfacebook>
 <35b0500c-d7fe-6479-eeff-d45bbf9a9426@loongson.cn>
 <CAHp75VdHPFDAd4iHdX5jXCM-tq0ZbFJDjvF9GCR_n7HVtd+obg@mail.gmail.com>
From:   zhuyinbo <zhuyinbo@loongson.cn>
Message-ID: <2a72a2c2-6fda-1ea8-3b27-5623cc1132aa@loongson.cn>
Date:   Thu, 25 May 2023 11:34:06 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <CAHp75VdHPFDAd4iHdX5jXCM-tq0ZbFJDjvF9GCR_n7HVtd+obg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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