Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Ronald,

ron minnich <rminnich@xxxxxxxxx> wrote on Fri, 27 Mar 2020 08:48:39
-0700:

> anyone? This is in our internal tree but I'd like to get it upstreamed
> if possible.

Maybe it is one of your fist contributions and I thank you very much
for trying to keep Google's tree aligned with mainline by upstreaming
this kind of change with enthusiasm, this is appreciated.

However, please note that a 2 days ping (and this is the second time you
do it) is a bit of an impatient move. Also, please have in mind the
maintainer's schedule: we freeze our branches about a week before
sending them to Linus. The SPI-NOR pull-request has been sent earlier
this week already so this patch will not reach mainline in v5.7 anyway.

Last generic comment, the subject prefix is wrong, so if you want the
SPI-NOR maintainer to care, better use the right one to catch his eye
;) "mtd: spi-nor: intel-spi:" seems more appropriate.

More comments below :)

> 
> On Wed, Mar 25, 2020 at 10:34 AM ron minnich <rminnich@xxxxxxxxx> wrote:
> >
> > Sorry, on that first one, I forgot the checkpatch. This one is clean.

When resending, please:
- increment the version with git-format-patch -v<x>
- write a changelog explaining what are the changes below the three
  dashes '---'.
- do not answer your previous version

> >
> > thanks
> >
> > On Wed, Mar 25, 2020 at 10:34 AM ron minnich <rminnich@xxxxxxxxx> wrote:  
> > >
> > > The MTD subsystem can support command-line defined partitions
> > > for one or more MTD devices.
> > >
> > > The format is:
> > >  * mtdparts=<mtddef>[;<mtddef]
> > >  * <mtddef>  := <mtd-id>:<partdef>[,<partdef>]
> > >
> > > The ':' currently separates the id from the partdef.

s/id/Linux MTD device ID/             ^^

s/partdef/partition definition/                   ^^^^^^^

> > >
> > > The mtdparts can define more than one part, in which case

What about:

"The mtdparts cmdline parameter can define the partitioning of several
MTD devices, in which case"...
                                            
> > > there will be more than one <mtd-id>:<partdef> component.
> > >
> > > On Intel spi devices, the name is set to the PCI slot name,

               SPI

> > > e.g. 0000:00:1f.5.  There are two : in the name alone.

                                        ':'

> > > Since strchr is used to find the <mtd-id>,
> > > in this case it will return the wrong
> > > result. Using strrchr is not an option, as there may

What about:
      result as ':' is part of the parameter syntax.
 
> > > be more than one mtddef in the argument.

Not sure the strrchr mentiont is relevant here. I'd drop it.

> > >
> > > This change modifies the name attached to the intel spi

      Change the name attached to the Intel SPI device, ...

> > > device, changing all ':' to '.', e.g. 0000:00:1f.5
> > > becomes 0000.00.1f.5. It also adds command line partition
> > > parsing registration to the intel_spi_probe function.
> > >
> > > Signed-off-by: Ronald G. Minnich <rminnich@xxxxxxxxxx>
> > > Change-Id: Ibfa5caba51b8cdd3c41c60b15f8f8c583ded82ff

Change-ID can be dropped too.

> > > ---
> > >  drivers/mtd/spi-nor/intel-spi-pci.c | 23 +++++++++++++++++++++++
> > >  drivers/mtd/spi-nor/intel-spi.c     |  5 ++++-
> > >  2 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
> > > b/drivers/mtd/spi-nor/intel-spi-pci.c
> > > index 81329f680bec..dc608d74e824 100644
> > > --- a/drivers/mtd/spi-nor/intel-spi-pci.c
> > > +++ b/drivers/mtd/spi-nor/intel-spi-pci.c
> > > @@ -24,6 +24,24 @@ static const struct intel_spi_boardinfo cnl_info = {
> > >      .type = INTEL_SPI_CNL,
> > >  };
> > >
> > > +/*
> > > + * PCI names use a ':' as a separator, which conflicts
> > > + * with the mtdparts cmdline parameter. Dup the name and

s/dup/duplicate/

> > > + * replace : with .
> > > + */
> > > +static int fixname(struct pci_dev *pdev)
> > > +{
> > > +    char *name;
> > > +
> > > +    name = kstrdup(pci_name(pdev), GFP_KERNEL);
> > > +    if (!name)
> > > +        return -ENOMEM;

new line

> > > +    strreplace(name, ':', '.');
> > > +    dev_set_name(&pdev->dev, name);
> > > +    kfree(name);

new line

> > > +    return 0;
> > > +}
> > > +
> > >  static int intel_spi_pci_probe(struct pci_dev *pdev,
> > >                     const struct pci_device_id *id)
> > >  {
> > > @@ -41,6 +59,11 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
> > >      if (!info)
> > >          return -ENOMEM;
> > >
> > > +    if (fixname(pdev)) {

	ret = fixname(pdev);
	if (ret) {
		kfree(info);
		return ret;
	}

> > > +        kfree(info);
> > > +        return -ENOMEM;
> > > +    }
> > > +
> > >      /* Try to make the chip read/write */
> > >      pci_read_config_dword(pdev, BCR, &bcr);
> > >      if (!(bcr & BCR_WPD)) {
> > > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> > > index 61d2a0ad2131..cb08ee4d2029 100644
> > > --- a/drivers/mtd/spi-nor/intel-spi.c
> > > +++ b/drivers/mtd/spi-nor/intel-spi.c
> > > @@ -894,6 +894,8 @@ static const struct spi_nor_controller_ops
> > > intel_spi_controller_ops = {
> > >      .erase = intel_spi_erase,
> > >  };
> > >
> > > +static const char * const part_probes[] = { "cmdlinepart", NULL };
> > > +
> > >  struct intel_spi *intel_spi_probe(struct device *dev,
> > >      struct resource *mem, const struct intel_spi_boardinfo *info)
> > >  {
> > > @@ -941,7 +943,8 @@ struct intel_spi *intel_spi_probe(struct device *dev,
> > >      if (!ispi->writeable || !writeable)
> > >          ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> > >
> > > -    ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
> > > +    ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
> > > +                    NULL, &part, 1);

Wrong alignment here I guess


Should these two changes be separated (changing the name, registering
the device with the cmdline parser)?

> > >
> > >      if (ret)
> > >          return ERR_PTR(ret);
> > >
> > > --
> > > 2.25.1.696.g5e7596f4ac-goog  

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/