Re: [RFC PATCH 3/6] mtd: spi-nor: core: run calibration when initialization is done

[Pratyush Yadav <p.yadav@xxxxxx>]

+Cedric

On 17/05/22 04:02PM, Miquel Raynal wrote:
> Hi Pratyush,
> 
> p.yadav@xxxxxx wrote on Fri, 12 Mar 2021 00:42:13 +0530:
> 
> > Once the flash is initialized tell the controller it can run
> > calibration procedures if needed. This can be useful when calibration is
> > needed to run at higher clock speeds.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
> > ---
> >  drivers/mtd/spi-nor/core.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 88888df009f0..e0cbcaf1be89 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -3650,6 +3650,7 @@ static int spi_nor_probe(struct spi_mem *spimem)
> >  	 * checking what's really supported using spi_mem_supports_op().
> >  	 */
> >  	const struct spi_nor_hwcaps hwcaps = { .mask = SNOR_HWCAPS_ALL };
> > +	struct spi_mem_op op;
> >  	char *flash_name;
> >  	int ret;
> >  
> > @@ -3709,8 +3710,15 @@ static int spi_nor_probe(struct spi_mem *spimem)
> >  	if (ret)
> >  		return ret;
> >  
> > -	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> > -				   data ? data->nr_parts : 0);
> > +	ret = mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> > +				  data ? data->nr_parts : 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	op = spi_nor_spimem_get_read_op(nor);
> 
> Isn't this too specific? I really don't know much about spi-nors, but I
> find odd to have this op being created here, why not moving this into
> the _do_calibration() helper?

Maybe the naming confused you but this is a function in the SPI NOR 
core, not in SPI MEM. SPI NOR supports both SPI MEM based controllers 
and "legacy" controllers, so the convention is to add the "spimem" 
prefix before SPI MEM specific functions. So I don't get the comment 
about it being too specific. It is too specific to what?

And how can spi_mem_do_calibration() know what op the flash uses to read 
data? SPI NOR or SPI NAND would know it, but not SPI MEM. That is why we 
pass in that information to spi_mem_do_calibration().

> 
> > +	spi_mem_do_calibration(nor->spimem, &op);
> 
> A warning/info upon calibration error (not on the absence of the hook)
> would be nice?

Yes, agreed.

> 
> > +
> > +	return 0;
> >  }
> >  
> >  static int spi_nor_remove(struct spi_mem *spimem)
> 
> Otherwise I like the overall idea.

Thanks for reviewing.

> 
> Thanks,
> Miquèl

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.