[PATCH 1/3] mtd: name the mtd device with an optional label property

[Cédric Le Goater]

On 01/25/2017 05:51 PM, Boris Brezillon wrote:
> On Wed, 25 Jan 2017 17:47:13 +0100
> Cédric Le Goater <clg at kaod.org> wrote:
> 
>> On 01/24/2017 03:13 PM, Boris Brezillon wrote:
>>> Hi Cédric,
>>>
>>> On Mon, 16 Jan 2017 14:27:03 +0100
>>> Cédric Le Goater <clg at kaod.org> wrote:
>>>   
>>>> This can be used to easily identify a specific chip on a system with
>>>> multiple chips.
>>>>
>>>> Suggested-by: Boris Brezillon <boris.brezillon at free-electrons.com>
>>>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>>>> ---
>>>>  drivers/mtd/mtdcore.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>>> index 052772f7caef..bf61557b599d 100644
>>>> --- a/drivers/mtd/mtdcore.c
>>>> +++ b/drivers/mtd/mtdcore.c
>>>> @@ -654,6 +654,13 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
>>>>   */
>>>>  static void mtd_set_dev_defaults(struct mtd_info *mtd)
>>>>  {
>>>> +	/*
>>>> +	 * If DT support is enabled and we have a valid of_node pointer, try to
>>>> +	 * extract the MTD name from the label property.
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_OF) && mtd->dev.of_node)
>>>> +		of_property_read_string(mtd->dev.of_node, "label", &mtd->name);
>>>> +  
>>>
>>> I realize this kind of thing would be better placed in mtd_set_of_node()
>>> (see the patch below). Modifying the mtd->name pointer in the back of
>>> MTD drivers is not such a good idea (suppose the driver allocated the
>>> memory with a regular kmalloc() and tries to free mtd->name in the remove
>>> path).
>>>
>>> If we move that to mtd_set_of_node(), drivers that wants to support this
>>> label property just have to check if mtd->name is NULL (after calling
>>> mtd_set_of_node() or nand_set_flash_node()) before assigning a default
>>> name.
>>> For unmodified drivers we keep the existing behavior: they'll just
>>> unconditionally override mtd->name with their own value (which might or
>>> might not be dynamically allocated).  
>>
>> ok. So the expected behavior looks correct to me, but adding a call to 
>> of_property_read_string() in the inline below feels a little hacky. 
>> Doesn't it ? 
>>
>> May be we need an extra check on IS_ENABLED(CONFIG_OF) also ? 
> 
> This is all safe, because the of.h header defines stubs if CONFIG_OF is
> not set. That just means the name will be unchanged, but you shouldn't
> have any problem (neither as compilation time nor at runtime).
> 

ok. I will cook a v2 with your proposal then.

Thanks,

C.