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

[Leilk Liu]

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@xxxxxxxxxxxx>
> > ---
> >   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!


> > +		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,