Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices | From | David Woodhouse <> | Date | Fri, 29 Aug 2008 13:19:50 +0100 |
| |
On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote: > On Wed, 27 August 2008 17:47:43 +0300, Artem Bityutskiy wrote: > > > > The plus of sysfs I see is that I can add more files to expose more > > information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I > > need to expose sub-page size to user-space, and I cannot do this with > > MEMGETINFO. > > sysfs makes adding new attributes easier, yes. But once added you > cannot remove the attribute again - ever. Which means that either way > you need to tread carefully and think twice before making a rash > decision.
That's not _necessarily_ true, although it should certainly be done with care. Attributes in sysfs can be optional (in a way that they can't really be optional if they're part of a binary ioctl payload), and userspace can cope with them being absent. The sub-page size attribute is something which wouldn't always be present, and we could happily just drop it and forget about it in future if we really wanted to.
> > > So what was the reason again why mtd needs two userspace interfaces > > > instead of just one? > > > > I would like to make udev creating MTD devices, instead of creating them > > by hands. Adding MTD to LDM would anyway introduce corresponding sysfs > > files, right? This means we would have one more interface anyway. > > Could be useful, I don't mind you sending a patch. However, does this > means that MEMGETINFO64 or some other ioctl should not be done? Should > flash_erase open, read and close 8 seperate files instead of doing a > single ioctl?
It's hardly a fast path. And we don't have to worry about the fact that it's non-atomic -- these things aren't exactly _changing_ over time.
> And should our support for large devices wait for the sysfs support > that has been talked about and not done for about two years already?
You whine too much, Jörn. It doesn't take very long, as a proof of concept, to add some attributes to the existing class support in mtdchar.c ...
--- drivers/mtd/mtdchar.c~ 2008-07-13 22:51:29.000000000 +0100 +++ drivers/mtd/mtdchar.c 2008-08-29 13:15:08.000000000 +0100 @@ -22,12 +22,32 @@ static struct class *mtd_class; +static ssize_t mtd_show_size(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct mtd_info *mtd = dev_get_drvdata(dev); + return sprintf(buf, "0x%x\n", mtd->size); +} +static ssize_t mtd_show_erasesize(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct mtd_info *mtd = dev_get_drvdata(dev); + return sprintf(buf, "0x%x\n", mtd->erasesize); +} + +static DEVICE_ATTR(size, S_IRUGO, mtd_show_size, NULL); +static DEVICE_ATTR(erasesize, S_IRUGO, mtd_show_erasesize, NULL); + static void mtd_notify_add(struct mtd_info* mtd) { + struct device *dev; if (!mtd) return; - device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index); + dev = device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index); + dev_set_drvdata(dev, mtd); + device_create_file(dev, &dev_attr_size); + device_create_file(dev, &dev_attr_erasesize); device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2+1), "mtd%dro", mtd->index);
Ok, so it shouldn't be only for mtdchar -- it should be generic, so we should shift some of that into the mtd core code. And we should let people hook up the 'parent' correctly, and there are a few other things we should do to tidy it up. But it isn't exactly hard.
-- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |