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]

Message-ID: <b6394c1a-28ee-f4bb-434f-afd311893fb8@collabora.com>
Date:   Thu, 17 Mar 2022 10:33:48 +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 17/03/22 10:27, Leilk Liu ha scritto:
> On Tue, 2022-03-15 at 10:31 +0100, AngeloGioacchino Del Regno wrote:
>> 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
>>
> Hi Angelo,
> 
> Thanks for your advice.
> 
> There are two spi controllor on MT7986. One supports single/dual mode,
> the other supports quad mode. Both of them can support spi memory
> framework(one's tx/rx bus width is 1/2, the other one's tx/rx bus width
> is 1/2/4).
> 
> Can I use of_mtk_spi_parse_dt() to parse the information? What's your
> suggestion?
> 
> Thanks!
> 

As I've already said, this does NOT require any devicetree handling in
spi-mt65xx.c, as setting the right mode_bits is already handled in
drivers/spi/spi.c - please follow the explaination that I have given
before to fully understand the situation.

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: