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

[AngeloGioacchino Del Regno]

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

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: