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

[Leilk Liu]

On Thu, 2022-03-17 at 10:33 +0100, AngeloGioacchino Del Regno wrote:
> 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
> 
OK, I'll fix it, 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: