Hi Pratyush, p.yadav@xxxxxx wrote on Wed, 18 May 2022 11:37:05 +0530: > +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? Mmh right, it's fine then. > > 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(). But here the op is "spi-nor wide", I would have expected a per-device op. But that is not a big deal, that is something that can also be updated later if needed I guess. One last question, is there something that mtd_device_register() does that is really needed for the calibration to work? Otherwise I would rather prefer to have that calibration happening before the user gets access to the device. > > > > > > + 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 > Thanks, Miquèl