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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Wed, 25 Jan 2017 17:47:13 +0100
Cédric Le Goater <clg@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@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@free-electrons.com>
> >> Signed-off-by: Cédric Le Goater <clg@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).