linux-kernel - Re: [PATCH V4 4/6] spi: mediatek: add spi memory support for ipm design

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:   Tue, 15 Mar 2022 10:31:57 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Leilk Liu <leilk.liu@...iatek.com>, Mark Brown <broonie@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH V4 4/6] spi: mediatek: add spi memory support for ipm
 design

Il 15/03/22 04:24, Leilk Liu ha scritto:
> this patch add the support of spi-mem for ipm design.
> 
> Signed-off-by: Leilk Liu <leilk.liu@...iatek.com>
> ---
>   drivers/spi/spi-mt65xx.c | 349 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 348 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> index 1a0b3208dfca..8958c3fa4fea 100644
> --- a/drivers/spi/spi-mt65xx.c
> +++ b/drivers/spi/spi-mt65xx.c

...snip...

> +
> +static void of_mtk_spi_parse_dt(struct spi_master *master, struct device_node *nc)
> +{
> +	struct mtk_spi *mdata = spi_master_get_devdata(master);
> +	u32 value;
> +
> +	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {

Hello Leilk,

thanks for considering my advice about "spi-{tx,rx}-bus-width", but there's
something that you have misunderstood about it.

Simply, you don't need this function at all. Whatever you are doing here is
already being performed in the Linux SPI framework: at the end of the probe
function, this driver is calling the (legacy) devm_spi_register_master(),
which calls devm_spi_register_controller().

In drivers/spi/spi.c, function spi_register_controller(), will in turn call
of_register_spi_devices(ctlr) -> of_register_spi_device(ctlr, nc)... that
will end up finally calling function of_spi_parse_dt(ctlr, spi, nc).

The last mentioned function already contains the logic and setup to check
devicetree properties "spi-tx-bus-width" and "spi-rx-bus-width" (and some
others, as well).

This means that spi-mt65xx.c already probed these even before your IPM
implementation, hence ***function of_mtk_spi_parse_dt() is not needed***.

Simply drop it and don't check for these properties: that's already done.


Regards,
Angelo

> +		switch (value) {
> +		case 1:
> +			break;
> +		case 2:
> +			master->mode_bits |= SPI_TX_DUAL;
> +			break;
> +		case 4:
> +			master->mode_bits |= SPI_TX_QUAD;
> +			break;
> +		default:
> +			dev_warn(mdata->dev,
> +				 "spi-tx-bus-width %d not supported\n",
> +				value);
> +			break;
> +		}
> +	}
> +
> +	if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
> +		switch (value) {
> +		case 1:
> +			break;
> +		case 2:
> +			master->mode_bits |= SPI_RX_DUAL;
> +			break;
> +		case 4:
> +			master->mode_bits |= SPI_RX_QUAD;
> +			break;
> +		case 8:
> +			master->mode_bits |= SPI_RX_OCTAL;
> +			break;
> +		default:
> +			dev_warn(mdata->dev,
> +				 "spi-rx-bus-width %d not supported\n",
> +				value);
> +			break;
> +		}
> +	}
> +}
> +
>   static int mtk_spi_probe(struct platform_device *pdev)
>   {
>   	struct spi_master *master;
> @@ -830,6 +1170,13 @@ static int mtk_spi_probe(struct platform_device *pdev)
>   	if (mdata->dev_comp->ipm_design)
>   		master->mode_bits |= SPI_LOOP;
>   
> +	if (mdata->dev_comp->ipm_design) {
> +		mdata->dev = &pdev->dev;
> +		master->mem_ops = &mtk_spi_mem_ops;
> +		of_mtk_spi_parse_dt(master, pdev->dev.of_node);
> +		init_completion(&mdata->spimem_done);
> +	}
> +
>   	if (mdata->dev_comp->need_pad_sel) {
>   		mdata->pad_num = of_property_count_u32_elems(
>   			pdev->dev.of_node,