diff mbox

[V2] mtd: basic (read only) driver for BCMA serial flash

Message ID 1346699237-414-1-git-send-email-zajec5@gmail.com
State New, archived
Headers show

Commit Message

Rafał Miłecki Sept. 3, 2012, 7:07 p.m. UTC
This registers MTD driver for serial flash platform device. Right now it
supports reading only, writing still has to be implemented.
At this point it's marked as BROKEN, because it requires code present in
net tree that wasn't mainlined yet.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: fix mem leak (add failure path in init)
    signed-off-by
---
 drivers/mtd/devices/Kconfig         |    8 ++
 drivers/mtd/devices/Makefile        |    1 +
 drivers/mtd/devices/bcm47xxsflash.c |  132 +++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/devices/bcm47xxsflash.c

Comments

Artem Bityutskiy Sept. 11, 2012, 10:20 a.m. UTC | #1
On Mon, 2012-09-03 at 21:07 +0200, Rafał Miłecki wrote:
> This registers MTD driver for serial flash platform device. Right now it
> supports reading only, writing still has to be implemented.
> At this point it's marked as BROKEN, because it requires code present in
> net tree that wasn't mainlined yet.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>

What are exactly the changes it depends on? URL? Are they staged for
3.7?

> +static void bcm47xxsflash_fill_mtd(struct bcma_sflash *sflash,
> +				   struct mtd_info *mtd)
> +{
> +	mtd->priv = sflash;
> +	mtd->name = "bcm47xxsflash";
> +	mtd->owner = THIS_MODULE;
> +	mtd->type = MTD_ROM;
> +	mtd->size = sflash->size;
> +	mtd->_read = bcm47xxsflash_read;

I guess also
mtd->flags = MTD_CAP_ROM;
would be good to add.

> +	mtd->writesize = 1; /* FIXME */

Is "FIXME" needed? Why? If needed, write a more descriptive comment.

Also I guess
mtd->writebufsize = mtd->writesize;

> +static int __devexit bcm47xxsflash_remove(struct platform_device *pdev)
> +{
> +	struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
> +
> +	if (!sflash->mtd) {
> +		WARN_ON(1);
> +		return -ENODEV;
> +	}

Why do you need this check with a warning? Can this happen, in which
cases?
Rafał Miłecki Sept. 11, 2012, 10:35 a.m. UTC | #2
2012/9/11 Artem Bityutskiy <dedekind1@gmail.com>:
> On Mon, 2012-09-03 at 21:07 +0200, Rafał Miłecki wrote:
>> This registers MTD driver for serial flash platform device. Right now it
>> supports reading only, writing still has to be implemented.
>> At this point it's marked as BROKEN, because it requires code present in
>> net tree that wasn't mainlined yet.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>
> What are exactly the changes it depends on? URL? Are they staged for
> 3.7?

http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=23cb3b2121323443834296a8ecb582b8aeb78d75
http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=d57ef3a6a2eeb88df47e892c66692e3f59722ffe

(Yes, there are queued to be pulled during 3.7 merge window)


>> +static void bcm47xxsflash_fill_mtd(struct bcma_sflash *sflash,
>> +                                struct mtd_info *mtd)
>> +{
>> +     mtd->priv = sflash;
>> +     mtd->name = "bcm47xxsflash";
>> +     mtd->owner = THIS_MODULE;
>> +     mtd->type = MTD_ROM;
>> +     mtd->size = sflash->size;
>> +     mtd->_read = bcm47xxsflash_read;
>
> I guess also
> mtd->flags = MTD_CAP_ROM;
> would be good to add.

Thanks.


>> +     mtd->writesize = 1; /* FIXME */
>
> Is "FIXME" needed? Why? If needed, write a more descriptive comment.
>
> Also I guess
> mtd->writebufsize = mtd->writesize;

Well, I just didn't implement writing yet (it's more complicated than
that trivial reads) and I didn't know the correct values for the above
ones. I just set writesize to 1 to avoid some WARN/BUG (not remember
right now).


>> +static int __devexit bcm47xxsflash_remove(struct platform_device *pdev)
>> +{
>> +     struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
>> +
>> +     if (!sflash->mtd) {
>> +             WARN_ON(1);
>> +             return -ENODEV;
>> +     }
>
> Why do you need this check with a warning? Can this happen, in which
> cases?

In case some other driver handling platform device was loaded earlier.
Is that wrong?
Artem Bityutskiy Sept. 11, 2012, 10:59 a.m. UTC | #3
On Tue, 2012-09-11 at 12:35 +0200, Rafał Miłecki wrote:
> 2012/9/11 Artem Bityutskiy <dedekind1@gmail.com>:
> > On Mon, 2012-09-03 at 21:07 +0200, Rafał Miłecki wrote:
> >> This registers MTD driver for serial flash platform device. Right now it
> >> supports reading only, writing still has to be implemented.
> >> At this point it's marked as BROKEN, because it requires code present in
> >> net tree that wasn't mainlined yet.
> >>
> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> >
> > What are exactly the changes it depends on? URL? Are they staged for
> > 3.7?
> 
> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=23cb3b2121323443834296a8ecb582b8aeb78d75
> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=d57ef3a6a2eeb88df47e892c66692e3f59722ffe
> 
> (Yes, there are queued to be pulled during 3.7 merge window)

CCing John. I am not 100% sure how the net drivers are handled, but if
John does not rebase and just sends a pull request to David Miller, who
also does not rebase, then we could pull John's tree into the MTD tree.
John W. Linville Sept. 11, 2012, 1:21 p.m. UTC | #4
On Tue, Sep 11, 2012 at 01:59:46PM +0300, Artem Bityutskiy wrote:
> On Tue, 2012-09-11 at 12:35 +0200, Rafał Miłecki wrote:
> > 2012/9/11 Artem Bityutskiy <dedekind1@gmail.com>:
> > > On Mon, 2012-09-03 at 21:07 +0200, Rafał Miłecki wrote:
> > >> This registers MTD driver for serial flash platform device. Right now it
> > >> supports reading only, writing still has to be implemented.
> > >> At this point it's marked as BROKEN, because it requires code present in
> > >> net tree that wasn't mainlined yet.
> > >>
> > >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> > >
> > > What are exactly the changes it depends on? URL? Are they staged for
> > > 3.7?
> > 
> > http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=23cb3b2121323443834296a8ecb582b8aeb78d75
> > http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=d57ef3a6a2eeb88df47e892c66692e3f59722ffe
> > 
> > (Yes, there are queued to be pulled during 3.7 merge window)
> 
> CCing John. I am not 100% sure how the net drivers are handled, but if
> John does not rebase and just sends a pull request to David Miller, who
> also does not rebase, then we could pull John's tree into the MTD tree.

OK, so...never, _ever_ pull from wireless-testing into another tree.
The wireless-testing tree is a community sandbox, and sometimes gets
messy and sometimes gets rebased.

OTOH, those exact commits are already in wireless-next (and in
net-next).  That tree will not be rebased, so you can pull from it
if you so choose.  But, it would generally be better if you only pull
from head commits that are already in net-next "just in case". :-)

Let me know if I can be helpful...

John
Rafał Miłecki Sept. 12, 2012, 9:19 a.m. UTC | #5
2012/9/11 Artem Bityutskiy <dedekind1@gmail.com>:
> On Tue, 2012-09-11 at 12:35 +0200, Rafał Miłecki wrote:
>> 2012/9/11 Artem Bityutskiy <dedekind1@gmail.com>:
>> > On Mon, 2012-09-03 at 21:07 +0200, Rafał Miłecki wrote:
>> >> This registers MTD driver for serial flash platform device. Right now it
>> >> supports reading only, writing still has to be implemented.
>> >> At this point it's marked as BROKEN, because it requires code present in
>> >> net tree that wasn't mainlined yet.
>> >>
>> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> >
>> > What are exactly the changes it depends on? URL? Are they staged for
>> > 3.7?
>>
>> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=23cb3b2121323443834296a8ecb582b8aeb78d75
>> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=d57ef3a6a2eeb88df47e892c66692e3f59722ffe
>>
>> (Yes, there are queued to be pulled during 3.7 merge window)
>
> CCing John. I am not 100% sure how the net drivers are handled, but if
> John does not rebase and just sends a pull request to David Miller, who
> also does not rebase, then we could pull John's tree into the MTD tree.

Not sure if my plans were clear since beginning so just for sure,
order I imagined:
1) Me adding mtd drivers
2) Linus releasing 3.7-rc1 with disabled mtd driver and changes from
wireless tree
3) Me sending trivial patch enabling mtd driver for -fixes tree
Artem Bityutskiy Sept. 16, 2012, 11:43 a.m. UTC | #6
On Tue, 2012-09-11 at 09:21 -0400, John W. Linville wrote:
> OTOH, those exact commits are already in wireless-next (and in
> net-next).  That tree will not be rebased, so you can pull from it
> if you so choose.  But, it would generally be better if you only pull
> from head commits that are already in net-next "just in case". :-)

Thanks, I've pulled 'net-next' to my 'l2-mtd' tree instead, up to the
commit Rafał needs:

commit 70f389f0fbbde0d00a5ec4ba4817c09e80a3ae47
Merge: 44e42b9 371a004
Author: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date:   Sun Sep 16 14:42:38 2012 +0300

    Merge commit '371a00448f95adaa612cf1a0b31a11e7093bc706' of 'git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git'
    
    We need the following 2 patches from the 'net-next' tree for the BCMA flash
    driver:
    
    371a004 bcma: detect and register NAND flash device
    d57ef3a bcma: detect and register serial flash device
    
    and this is why we are merging the net-next tree (presumably persistent)
    up to commit '371a004'.
    
    Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Artem Bityutskiy Sept. 16, 2012, 11:49 a.m. UTC | #7
On Tue, 2012-09-11 at 12:35 +0200, Rafał Miłecki wrote:
> 2012/9/11 Artem Bityutskiy <dedekind1@gmail.com>:
> > On Mon, 2012-09-03 at 21:07 +0200, Rafał Miłecki wrote:
> >> This registers MTD driver for serial flash platform device. Right now it
> >> supports reading only, writing still has to be implemented.
> >> At this point it's marked as BROKEN, because it requires code present in
> >> net tree that wasn't mainlined yet.
> >>
> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> >
> > What are exactly the changes it depends on? URL? Are they staged for
> > 3.7?
> 
> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=23cb3b2121323443834296a8ecb582b8aeb78d75
> http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commitdiff;h=d57ef3a6a2eeb88df47e892c66692e3f59722ffe

Please, remove "BROKEN" and re-send the compilable patch against the
l2-mtd.git tree. I've pulled Dave Miller's tree with your patches into
l2-mtd.git. Thanks!

> Well, I just didn't implement writing yet (it's more complicated than
> that trivial reads) and I didn't know the correct values for the above
> ones. I just set writesize to 1 to avoid some WARN/BUG (not remember
> right now).

OK. Just add 'mtd->writebufsize = mtd->writesize = 1; for now.

> In case some other driver handling platform device was loaded earlier.
> Is that wrong?

I do not really understand why you need this - can the general code
really call the removal method and the platform data be NULL? If not,
kill this check - if this happens, this is a bug and we'll end up with
an oops. If it can, please, explain in more details how this can
happen.
diff mbox

Patch

diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index 4cdb2af..4b4f7d2 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -120,6 +120,14 @@  config MTD_SST25L
 	  Set up your spi devices with the right board-specific platform data,
 	  if you want to specify device partitioning.
 
+config MTD_BCM47XXSFLASH
+	tristate "Support for serial flash on BCMA bus"
+	depends on BCMA_SFLASH && BROKEN
+	help
+	  BCMA bus can have various flash memories attached, they are
+	  registered by bcma as platform devices. This enables driver for
+	  serial flash memories.
+
 config MTD_SLRAM
 	tristate "Uncached system RAM"
 	help
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index a4dd1d8..395733a 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -19,5 +19,6 @@  obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
 obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
 obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
 obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
+obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 
 CFLAGS_docg3.o			+= -I$(src)
\ No newline at end of file
diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
new file mode 100644
index 0000000..0283138
--- /dev/null
+++ b/drivers/mtd/devices/bcm47xxsflash.c
@@ -0,0 +1,132 @@ 
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/mtd/mtd.h>
+#include <linux/platform_device.h>
+#include <linux/bcma/bcma.h>
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Serial flash driver for BCMA bus");
+
+static const char *probes[] = { "bcm47xxpart", NULL };
+
+static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
+			      size_t *retlen, u_char *buf)
+{
+	struct bcma_sflash *sflash = mtd->priv;
+	size_t bytes_read = 0;
+	u8 *src = (u8 *)KSEG0ADDR(sflash->window + from);
+	int i;
+	size_t unaligned_before, unaligned_after;
+
+	/* Check address range */
+	if ((from + len) > mtd->size)
+		return -EINVAL;
+
+	unaligned_before = from & 0x3;
+	unaligned_after = (from + len) & 0x3;
+
+	for (i = 0; i < unaligned_before; i++) {
+		*buf = readb(src);
+		buf++;
+		src++;
+		bytes_read++;
+	}
+	for (i = from - unaligned_before; i < from + len - unaligned_after;
+	     i += 4) {
+		*(u32 *)buf = readl((u32 *)src);
+		buf += 4;
+		src += 4;
+		bytes_read += 4;
+	}
+	for (i = 0; i < unaligned_after; i++) {
+		*buf = readb(src);
+		buf++;
+		src++;
+		bytes_read++;
+	}
+
+	*retlen = bytes_read;
+
+	return 0;
+}
+
+static void bcm47xxsflash_fill_mtd(struct bcma_sflash *sflash,
+				   struct mtd_info *mtd)
+{
+	mtd->priv = sflash;
+	mtd->name = "bcm47xxsflash";
+	mtd->owner = THIS_MODULE;
+	mtd->type = MTD_ROM;
+	mtd->size = sflash->size;
+	mtd->_read = bcm47xxsflash_read;
+	mtd->writesize = 1; /* FIXME */
+}
+
+static int bcm47xxsflash_probe(struct platform_device *pdev)
+{
+	struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
+	int err;
+
+	sflash->mtd = kzalloc(sizeof(struct mtd_info), GFP_KERNEL);
+	if (!sflash->mtd) {
+		err = -ENOMEM;
+		goto out;
+	}
+	bcm47xxsflash_fill_mtd(sflash, sflash->mtd);
+
+	err = mtd_device_parse_register(sflash->mtd, probes, NULL, NULL, 0);
+	if (err) {
+		pr_err("Failed to register MTD device: %d\n", err);
+		goto err_dev_reg;
+	}
+
+	return 0;
+
+err_dev_reg:
+	kfree(sflash->mtd);
+out:
+	return err;
+}
+
+static int __devexit bcm47xxsflash_remove(struct platform_device *pdev)
+{
+	struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
+
+	if (!sflash->mtd) {
+		WARN_ON(1);
+		return -ENODEV;
+	}
+
+	mtd_device_unregister(sflash->mtd);
+	kfree(sflash->mtd);
+
+	return 0;
+}
+
+static struct platform_driver bcma_sflash_driver = {
+	.remove = __devexit_p(bcm47xxsflash_remove),
+	.driver = {
+		.name = "bcma_sflash",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init bcm47xxsflash_init(void)
+{
+	int err;
+
+	err = platform_driver_probe(&bcma_sflash_driver, bcm47xxsflash_probe);
+	if (err)
+		pr_err("Failed to register BCMA serial flash driver: %d\n", err);
+
+	return err;
+}
+
+static void __exit bcm47xxsflash_exit(void)
+{
+	platform_driver_unregister(&bcma_sflash_driver);
+}
+
+module_init(bcm47xxsflash_init);
+module_exit(bcm47xxsflash_exit);