Messages in this thread | | | Date | Wed, 13 Mar 2024 10:24:13 +0100 | Subject | Re: [PATCH 2/2] mtd: core: Don't fail mtd_device_parse_register() if OTP is unsupported | From | "Michael Walle" <> |
| |
Hi,
On Mon Mar 11, 2024 at 5:20 PM CET, Aapo Vienamo wrote: > On Mon, Mar 11, 2024 at 03:38:17PM +0100, Michael Walle wrote: > > On Thu Mar 7, 2024 at 2:04 PM CET, Aapo Vienamo wrote: > > > Handle the case where -EOPNOTSUPP is returned from OTP driver. > > > + /* > > > + * Don't abort MTD init if OTP functionality is unsupported. The > > > + * cleanup of the OTP init is contained within mtd_otp_nvmem_add(). > > > + * Omitting goto out here is safe since the cleanup code there > > > + * should be no-ops. > > > + */ > > > > Only if that's true for both the factory and user OTP area. > > I'm not sure I follow. I'm not seeing a path in mtd_otp_nvmem_add() > that would result in either mtd->otp_user_nvmem or mtd->otp_factor_nvmem > needing to be cleaned up by the caller, if an error is returned, if > that's what you are referring to.
Yes you're right, sorry for the noise. > > > Also, you'll print an error message for EOPNOTSUPP, although that is > > not really an error. Is that intended? > > Well, when we hit this, the functionality of the SPI memory itself is > degraded in the sense that the OTP functionality is not available. What > would you suggest?
But it's not really an error, I mean, we are ignoring that one on purpose now :) I'd just guard it with "if (ret != -EOPNOTSUPP)".
> > > > > ret = mtd_otp_nvmem_add(mtd); > > > - if (ret) > > > + if (ret && ret != -EOPNOTSUPP) > > > > Maybe there is a better way to handle this, like controller > > capabilities instead of putting these EOPNOTSUPP checks > > everywhere? I'm not sure. > > Trying to come up with clear semantics for a capabilities flag to solve > this is difficult. The issue is that on the SPI controller side, the > limitation stems from the really strict set of opcodes that are allowed. > For example, we already hit an error with the 0x35 (read configuration > register) not being on the set of allowed opcodes. While this > instruction is used by the OTP code, it's not a strictly OTP specific > operation.
I see. It's just that due to this (very) restricted SPI contoller all this EOPNOTSUPP handling is creeping into more an more places in spi-nor core and now mtdcore :)
Anyway, I don't have any better idea right now. So I think this is fine.
-michael
> If there was a flag that would signal OTP support, I think it would have > be defined as "the controller supports all operations that are > performed by the OTP code", which sounds brittle. The other way around > would be to have a really fine-grained set of flags that the MTD core > would check, but that feels tedious and error prone as well.
[unhandled content-type:application/pgp-signature] | |