Re: [PATCH v2 22/35] mtd: spi-nor: core: Use common naming scheme for setting mtd_info fields

Michael Walle
2021-10-22T13:14:10Z

Am 2021-10-22 14:51, schrieb Tudor.Ambarus@microchip.com:
> On 10/22/21 2:57 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> Am 2021-07-27 06:52, schrieb Tudor Ambarus:
>>> The functions names are self explanatory, get rid of the comment
>>> for the OTP function.
>>
>> Mhh. I see, this partly addresses my comments to the previous patch.
>> Maybe it would have been better to have this squashed into one
>> commit :p
>
> one thing per patch

well it is really just one thing, you're moving the function. but then
the function name doesn't make sense anymore. So the patch on its own
is wrong or at least it makes things worse. Also, I'm reviewing that
just to see this function was renamed (correctly) in the next patch.

>> But my main concern remains: what if we need more that just
>> the mtd callbacks assigments. Basically we loose the otp_init()
>> call.
>
> if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor))))
> return;
>
> is there just to avoid setting the mtd function pointers when OTP
> region length is not power of 2. This is not an init, just a check,
> so for the moment I see it ok to have it in spi_nor_set_mtd_otp_ops().

It is a sanity check which actually has nothing to do with the mtd ops
registration. Just to see if there was a mistake in the otp info flags.

Of course, if that is not the case, we don't set the ops. But that
doesn't
mean it is part of the mtd op assigment.

I said stricly speaking, I'm ok with having that in the (now renamed)
"otp set mtd ops" function.

-michael