Re: [PATCH 07/10] mtd: spi-nor: add mtd_is_locked() support

[Brian Norris <computersforpeace@gmail.com>]

On Thu, Sep 03, 2015 at 11:43:09AM +0200, Marek Vasut wrote:
> On Wednesday, September 02, 2015 at 10:30:41 PM, Brian Norris wrote:
> > On Wed, Sep 02, 2015 at 11:01:49AM +0200, Marek Vasut wrote:
> > > On Tuesday, September 01, 2015 at 09:57:12 PM, Brian Norris wrote:
> > > > This enables ioctl(MEMISLOCKED). Status can now be reported in the
> > > > mtdinfo or flash_lock utilities found in mtd-utils.
> > > > 
> > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/mtd/spi-nor/spi-nor.c | 37
> > > >  ++++++++++++++++++++++++++++++++++++- include/linux/mtd/spi-nor.h   |
> > > >   3 +++
> > > >  2 files changed, 39 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > b/drivers/mtd/spi-nor/spi-nor.c index 62fa1b4ff3c0..c4fb1205f1d3
> > > > 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > 
> > ...
> > 
> > > > @@ -549,6 +567,21 @@ static int spi_nor_unlock(struct mtd_info *mtd,
> > > > loff_t ofs, uint64_t len) return ret;
> > > > 
> > > >  }
> > > > 
> > > > +static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs,
> > > > uint64_t len) +{
> > > > +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> > > > +	int ret;
> > > > +
> > > > +	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_UNLOCK);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = nor->flash_is_locked(nor, ofs, len);
> > > 
> > > Is nor->flash_is_locked () always available or should you check this here
> > > ?
> > 
> > spi_nor_is_locked() is only used when all 3 of
> > flash_{lock,unlock,is_locked} are available:
> > 
> > 	if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
> > 		mtd->_lock = spi_nor_lock;
> > 		mtd->_unlock = spi_nor_unlock;
> > 		mtd->_is_locked = spi_nor_is_locked;
> > 	}
> > 
> > I think it's a reasonable condition to enforce, that we only provide
> > lock/unlock support if your flash also implements is_locked.
> 
> Yes, you're right. At least until some sort of flash which can only be locked
> and not unlocked (like OTP) comes around.

I suppose we could split this up into independent conditions if/when
needed:

	if (nor->flash_lock)
		mtd->_lock = spi_nor_lock;
 	if (nor->flash_unlock)
 		mtd->_unlock = spi_nor_unlock;
 	if (nor->flash_is_locked) {
 		mtd->_is_locked = spi_nor_is_locked;

But I kinda like enforcing all three, for as much as possible.

> But we shouldn't overengineer things
> from the getgo.

Sure.

Thanks for reviewing.

Brian