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;
Message-ID: <4ed39c8f-3736-30d6-8d8c-92a4882b72e7@ti.com>
Date:   Wed, 7 Dec 2022 12:08:43 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH 1/2] spi: spi-fsl-lpspi: support multiple cs for lpspi
Content-Language: en-US
To:     Han Xu <han.xu@nxp.com>, <broonie@kernel.org>,
        <linux-spi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>
References: <20221206225410.604482-1-han.xu@nxp.com>
From:   Dhruva Gole <d-gole@ti.com>
In-Reply-To: <20221206225410.604482-1-han.xu@nxp.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 07/12/22 04:24, Han Xu wrote:
> support to get chip select number from DT file.

In my humble opinion, a  more elaborate commit message would help.
You can add perhaps which DT node is to be set, like you might want
to say,

support to get chip select number by Setting the value of num-cs in DT

or something on those lines.
> 
> Signed-off-by: Han Xu <han.xu@nxp.com>
> ---
>   drivers/spi/spi-fsl-lpspi.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index 6454b88c31fe..7f0562ed4d09 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -98,6 +98,7 @@ struct fsl_lpspi_data {
>   	struct clk *clk_ipg;
>   	struct clk *clk_per;
>   	bool is_slave;
> +	u32 num_cs;
>   	bool is_only_cs1;
>   	bool is_first_byte;
>   
> @@ -850,6 +851,9 @@ static int fsl_lpspi_probe(struct platform_device *pdev)
>   	fsl_lpspi->is_slave = is_slave;
>   	fsl_lpspi->is_only_cs1 = of_property_read_bool((&pdev->dev)->of_node,
>   						"fsl,spi-only-use-cs1-sel");
> +	if (of_property_read_u32((&pdev->dev)->of_node, "num-cs",
Running a checkpatch on this patch gave me the following,

CHECK: Unnecessary parentheses around '&pdev->dev'
#36: FILE: drivers/spi/spi-fsl-lpspi.c:854:
+       if (of_property_read_u32((&pdev->dev)->of_node, "num-cs",
+                                &fsl_lpspi->num_cs))

You might want to just do &pdev->dev->of_node instead

> +				 &fsl_lpspi->num_cs))
> +		fsl_lpspi->num_cs = 1;
I am not sure I understand why you are setting this to 1 here?
I am assuming it is because you want num_cs to default to 1 if
it is not specified in DT.

Please can you also add a dev_err and be sure to warn about this?

Also adding a comment or even a message inside dev err that you are
setting this to 1 if it fails to get from DT would be helpful.

>   
>   	controller->bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 32);
>   	controller->transfer_one = fsl_lpspi_transfer_one;
> @@ -859,6 +863,7 @@ static int fsl_lpspi_probe(struct platform_device *pdev)
>   	controller->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
>   	controller->dev.of_node = pdev->dev.of_node;
>   	controller->bus_num = pdev->id;
> +	controller->num_chipselect = fsl_lpspi->num_cs;
>   	controller->slave_abort = fsl_lpspi_slave_abort;
>   	if (!fsl_lpspi->is_slave)
>   		controller->use_gpio_descriptors = true;

-- 
Thanks and Regards,
Dhruva Gole