Messages in this thread |  | | Date | Thu, 9 Jan 2020 16:48:21 +0100 | From | Miquel Raynal <> | Subject | Re: [PATCH 1/1] mtd: spinand: Add support for new Micron SPI NAND devices |
| |
Hi Shiva,
shiva.linuxworks@gmail.com wrote on Mon, 9 Dec 2019 07:42:23 +0100:
> From: Shivamurthy Shastri <sshivamurthy@micron.com> > > Add device table for new Micron SPI NAND devices. While at it, add > support to the multi-die selection. Also, generalize the OOB layout > structure and function names.
Sorry for the delay. I am fine with this patch mostly, but could we split it please?
O/ Disable continuous read feature (one typo, see below). I think this might be considered as a fix.
1/ Generalize the OOB layout structure and function names. 2/ Add support for all the parts. 3/ Add multi-die support (one comment below about that).
As a general rule of thumb, small patches, doing one logic change are much easier and quick to review and accept.
> +static int micron_select_target(struct spinand_device *spinand, > + unsigned int target) > +{ > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0, > + spinand->scratchbuf); > + > + if (target == 1) > + *spinand->scratchbuf = 0x40;
Please define 0x40 and explain clearly with a comment that this is multi-die selection.
> + > + return spi_mem_exec_op(spinand->spimem, &op); > +} > +
[...]
> +static int micron_spinand_init(struct spinand_device *spinand) > +{ > + /* > + * M70A series device enables Continuos Read feature on Power-up, > + * which is not supported here. Making this BIT disable will avoid > + * any possible failure.
What about:
M70A device series enable Continuous Read feature at power-up, which is not supported. Disable this bit to avoid any possible failure.
> + */ > + return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0); > +} > + > static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = { > .detect = micron_spinand_detect, > + .init = micron_spinand_init, > }; > > const struct spinand_manufacturer micron_spinand_manufacturer = {
Thanks, Miquèl
|  |