Message ID | 1388990069-27066-1-git-send-email-b48286@freescale.com |
---|---|
State | Rejected |
Headers | show |
Hi Hou, On Mon, Jan 06, 2014 at 02:34:29PM +0800, Hou Zhiqiang wrote: > To give spi flash layout using "mtdparts=..." in cmdline, we must > give mtd_info a fixed name,because the cmdlinepart's parser will > match the name given in cmdline with the mtd_info. > > Now, if use OF node, mtd_info's name will be spi->dev->name. It > consists of spi_master->bus_num, and the spi_master->bus_num maybe > dynamically fetched. > So, give the mtd_info a new fiexd name "name.cs", "name" is name of > spi_device_id and "cs" is chip-select in spi_dev. > > Signed-off-by: Hou Zhiqiang <b48286@freescale.com> > --- > drivers/mtd/devices/m25p80.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index eb558e8..d1ed480 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -1012,7 +1012,8 @@ static int m25p_probe(struct spi_device *spi) > if (data && data->name) > flash->mtd.name = data->name; > else > - flash->mtd.name = dev_name(&spi->dev); > + flash->mtd.name = kasprintf(GFP_KERNEL, "%s.%d", > + id->name, spi->chip_select); Changing the mtd.name may have far-reaching consequences for users who already have mtdparts= command lines. But your concern is probably valid for dynamically-determined bus numbers. Perhaps you can edit this patch to only change the name when the busnum is dynamically-allocated? This also needs a NULL check (for OOM), and you leak memory on device removal. > > flash->mtd.type = MTD_NORFLASH; > flash->mtd.writesize = 1; Brian
Hi Brian, Thanks for your comments! > -----Original Message----- > From: Brian Norris [mailto:computersforpeace@gmail.com] > Sent: Thursday, January 23, 2014 10:12 AM > To: Hou Zhiqiang-B48286 > Cc: linux-mtd@lists.infradead.org; linuxppc-dev@ozlabs.org; Wood Scott- > B07421; Hu Mingkai-B21284; Ezequiel Garcia > Subject: Re: [PATCH] mtd: m25p80: Make the name of mtd_info fixed > > Hi Hou, > > On Mon, Jan 06, 2014 at 02:34:29PM +0800, Hou Zhiqiang wrote: > > To give spi flash layout using "mtdparts=..." in cmdline, we must give > > mtd_info a fixed name,because the cmdlinepart's parser will match the > > name given in cmdline with the mtd_info. > > > > Now, if use OF node, mtd_info's name will be spi->dev->name. It > > consists of spi_master->bus_num, and the spi_master->bus_num maybe > > dynamically fetched. > > So, give the mtd_info a new fiexd name "name.cs", "name" is name of > > spi_device_id and "cs" is chip-select in spi_dev. > > > > Signed-off-by: Hou Zhiqiang <b48286@freescale.com> > > --- > > drivers/mtd/devices/m25p80.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/devices/m25p80.c > > b/drivers/mtd/devices/m25p80.c index eb558e8..d1ed480 100644 > > --- a/drivers/mtd/devices/m25p80.c > > +++ b/drivers/mtd/devices/m25p80.c > > @@ -1012,7 +1012,8 @@ static int m25p_probe(struct spi_device *spi) > > if (data && data->name) > > flash->mtd.name = data->name; > > else > > - flash->mtd.name = dev_name(&spi->dev); > > + flash->mtd.name = kasprintf(GFP_KERNEL, "%s.%d", > > + id->name, spi->chip_select); > > Changing the mtd.name may have far-reaching consequences for users who > already have mtdparts= command lines. But your concern is probably valid > for dynamically-determined bus numbers. Perhaps you can edit this patch > to only change the name when the busnum is dynamically-allocated? > It's a good idea, but in the case of mtd_info's name dynamically-allocated using "mtdparts=..." in command lines is illegal obviously. Would you tell me what side-effect will be brought by the change of mtd_info's name. Thanks > This also needs a NULL check (for OOM), and you leak memory on device > removal. > Yes, it's necessary to check the return value of function kasprintf. > > > > flash->mtd.type = MTD_NORFLASH; > > flash->mtd.writesize = 1; > > Brian > Thanks, Hou Zhiqiang
Hi Hou, On Thu, Jan 23, 2014 at 03:29:41AM +0000, B48286@freescale.com wrote: > > -----Original Message----- > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > Sent: Thursday, January 23, 2014 10:12 AM > > To: Hou Zhiqiang-B48286 > > Cc: linux-mtd@lists.infradead.org; linuxppc-dev@ozlabs.org; Wood Scott- > > B07421; Hu Mingkai-B21284; Ezequiel Garcia > > Subject: Re: [PATCH] mtd: m25p80: Make the name of mtd_info fixed > > > > On Mon, Jan 06, 2014 at 02:34:29PM +0800, Hou Zhiqiang wrote: > > > --- a/drivers/mtd/devices/m25p80.c > > > +++ b/drivers/mtd/devices/m25p80.c > > > @@ -1012,7 +1012,8 @@ static int m25p_probe(struct spi_device *spi) > > > if (data && data->name) > > > flash->mtd.name = data->name; > > > else > > > - flash->mtd.name = dev_name(&spi->dev); > > > + flash->mtd.name = kasprintf(GFP_KERNEL, "%s.%d", > > > + id->name, spi->chip_select); > > > > Changing the mtd.name may have far-reaching consequences for users who > > already have mtdparts= command lines. But your concern is probably valid > > for dynamically-determined bus numbers. Perhaps you can edit this patch > > to only change the name when the busnum is dynamically-allocated? > > > It's a good idea, but in the case of mtd_info's name dynamically-allocated > using "mtdparts=..." in command lines is illegal obviously. I agree that users should never have relied on the dynamically-allocated name. But changing the name for non-dynamic schemes (e.g., where the SPI busnum is a fixed value) is not pleasant for users. > Would you tell > me what side-effect will be brought by the change of mtd_info's name. I can only think of the mtdparts= command line. Otherwise, I don't think the name is very important. Brian
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index eb558e8..d1ed480 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -1012,7 +1012,8 @@ static int m25p_probe(struct spi_device *spi) if (data && data->name) flash->mtd.name = data->name; else - flash->mtd.name = dev_name(&spi->dev); + flash->mtd.name = kasprintf(GFP_KERNEL, "%s.%d", + id->name, spi->chip_select); flash->mtd.type = MTD_NORFLASH; flash->mtd.writesize = 1;
To give spi flash layout using "mtdparts=..." in cmdline, we must give mtd_info a fixed name,because the cmdlinepart's parser will match the name given in cmdline with the mtd_info. Now, if use OF node, mtd_info's name will be spi->dev->name. It consists of spi_master->bus_num, and the spi_master->bus_num maybe dynamically fetched. So, give the mtd_info a new fiexd name "name.cs", "name" is name of spi_device_id and "cs" is chip-select in spi_dev. Signed-off-by: Hou Zhiqiang <b48286@freescale.com> --- drivers/mtd/devices/m25p80.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)