Re: [PATCH v3 03/25] mtd: spi-nor: Introduce spi_nor_set_mtd_info()

[<Tudor.Ambarus@microchip.com>]

Hi, Pratyush,

On 11/19/21 8:23 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 17/11/21 02:36PM, Tudor.Ambarus@microchip.com wrote:
>> On 11/16/21 8:11 PM, Pratyush Yadav wrote:
>>
>>>>
>>>>> - spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't
>>>>>   think it actually uses any values you initialize here but still worth
>>>>>   pointing out.
>>>>
>>>> we are safe here, the pointer to mtd is used just to get the pointer to
>>>> nor.
>>>
>>> Yeah, but who knows if that might change some time later. I would prefer
>>> we don't use a member we haven't initialized yet.
>>
>> If it weren't for the SPI NOR controller drivers that use
>> spi_nor_scan(), I would put the spi_nor_set_mtd_info() just
>> above the mtd_device_register(). It will indicate that no mtd_info
>> field is used up to that point, less things to worry about.
>> spi_nor_try_unlock_all() calls
>>       spi_nor_unlock(&nor->mtd, 0, nor->params->size);
>> I can't see for now if we will ever need some specific mtd_info
>> parameter. I would say that we won't, we're just unlocking the full
>> flash, every info we would need we can obtain from NOR. The discussion
>> would be different if it were about mtd partitions, but it isn't, we're
>> dealing with the entire flash.
>>
>> Would you accept the place where I put spi_nor_set_mtd_info() if I add
>> a comment before calling it? Something like:
>> /* No mtd_info fields are used up to this point. */
>> spi_nor_set_mtd_info();
> 
> I see that everything that spi_nor_set_mtd_info() needs is set by the
> time spi_nor_init_params() is finished. Everything after that is
> concerned about selecting the protocol and sending the init commands to
> the flash. So why can't you call it right after spi_nor_init_params()?

Because I would like to move it just above mtd_device_register() in the future.
If unlock_all() will need some mtd fields in the future, we can introduce a
spi_nor_prepare_mtd_for_unlock_all(). I don't want the mtd fields init to be
scattered through the SPI NOR core. They shouldn't be used in the NOR's
probe sequence of calls anyway, keeping them closer to mtd_device_register()
makes the code easier to grasp I think.

I will respin the series soon and wanted to let you know why I kept
spi_nor_set_mtd_info() where it is in this patch set.

Cheers,
ta

> That and updating spi_nor_spimem_check_op() and spi_nor_set_addr_width()
> to use nor->params->size instead of nor->mtd.size should do the trick.
> 
> I think that it is implied that mtd_info fields are not being used until
> they are initialized so I don't think the comment itself is of much use,
> but I don't care much about it either way.
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel