linux-kernel - Re: [PATCH 1/2] spi: spi-fsl-lpspi: support multiple cs for lpspi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Dec 2022 12:08:43 +0530
From:   Dhruva Gole <d-gole@...com>
To:     Han Xu <han.xu@....com>, <broonie@...nel.org>,
        <linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH 1/2] spi: spi-fsl-lpspi: support multiple cs for lpspi

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@....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