| Message ID | 1407515691.31897.26.camel@hornet |
|---|---|
| State | New |
| Headers | show |
On Fri, 2014-08-08 at 17:34 +0100, Pawel Moll wrote: > On Tue, 2014-08-05 at 21:08 +0100, Chris Metcalf wrote: > > >> In addition, we also have user binaries > > >> in the wild that know to look for /sys/devices/platform/srom/ paths, > > >> so I'm pretty reluctant to change this path without good reason. > > > So what is the srom class for then if not for device discovery? And why > > > do they look for them in the first place? To get relevant character > > > device's data, if I understand it right? > > > > > > Maybe you could just register a simple "proper" platform device for all > > > the sroms and then hang the class devices from it? I can type some code > > > doing this if it sound reasonably? > > > > I'm not sure exactly what you mean by device discovery here. (sorry, sent too early...) By "device discovery" I meant the way you find the way in your devices in /sysfs. You seem to be traversing /sys/devices/... tree, while you've got almost direct access to them through /sys/class/srom and you can (I believe, correct me if I'm wrong, Greg) rely on this path being stable. Paweł -- 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/
Thanks for the code and comments. I'm out on vacation till mid next week but I'll look at this when I get back. Chris > On Aug 8, 2014, at 12:35 PM, "Pawel Moll" <pawel.moll@arm.com> wrote: > > On Tue, 2014-08-05 at 21:08 +0100, Chris Metcalf wrote: >>>> In addition, we also have user binaries >>>> in the wild that know to look for /sys/devices/platform/srom/ paths, >>>> so I'm pretty reluctant to change this path without good reason. >>> So what is the srom class for then if not for device discovery? And why >>> do they look for them in the first place? To get relevant character >>> device's data, if I understand it right? >>> >>> Maybe you could just register a simple "proper" platform device for all >>> the sroms and then hang the class devices from it? I can type some code >>> doing this if it sound reasonably? >> >> I'm not sure exactly what you mean by device discovery here. > > > >> The >> subdirectories under /sys/devices/platform/srom/ correspond to partitions >> in the SPI-ROM, which are software constructs created by the Tilera hypervisor. >> By default we have three, where the first holds boot data that the chip >> can use to boot out of hardware, and the other two are smaller partitions >> for boot- and user-specific data. We use the /sys files primarily to get the >> page size and sector size for the sroms, and also export other interesting >> information like the total size of the particular srom device. >> >> Thank you for volunteering to write a bit of code; if that's the best >> way to clarify this for us, fantastic, or else pointing us at existing >> good practices or documentation would be great too. > > I was thinking about something like the following (warning, untested) > > 8<------------------------------------------- > From c53f0a2492d6cd38d1f82d57916a6528b071e8a8 Mon Sep 17 00:00:00 2001 > From: Pawel Moll <pawel.moll@arm.com> > Date: Fri, 8 Aug 2014 16:32:58 +0100 > Subject: [PATCH] char: tile-srom: Add real platform bus parent > > Add a real platform bus device as a parent for > the srom class devices, to prevent non-platform > devices hanging from the bus root. > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > --- > drivers/char/tile-srom.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c > index bd37747..7fb0fd5 100644 > --- a/drivers/char/tile-srom.c > +++ b/drivers/char/tile-srom.c > @@ -76,6 +76,7 @@ MODULE_LICENSE("GPL"); > > static int srom_devs; /* Number of SROM partitions */ > static struct cdev srom_cdev; > +static struct platform_device *srom_parent; > static struct class *srom_class; > static struct srom_dev *srom_devices; > > @@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index) > SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0) > return -EIO; > > - dev = device_create(srom_class, &platform_bus, > + dev = device_create(srom_class, srom_parent, > MKDEV(srom_major, index), srom, "%d", index); > return PTR_ERR_OR_ZERO(dev); > } > @@ -415,6 +416,13 @@ static int srom_init(void) > if (result < 0) > goto fail_chrdev; > > + /* Create a parent device */ > + srom_parent = platform_device_register_simple("srom", -1, NULL, 0); > + if (IS_ERR(srom_parent)) { > + result = PTR_ERR(srom_parent); > + goto fail_pdev; > + } > + > /* Create a sysfs class. */ > srom_class = class_create(THIS_MODULE, "srom"); > if (IS_ERR(srom_class)) { > @@ -438,6 +446,8 @@ fail_class: > device_destroy(srom_class, MKDEV(srom_major, i)); > class_destroy(srom_class); > fail_cdev: > + platform_device_unregister(srom_parent); > +fail_pdev: > cdev_del(&srom_cdev); > fail_chrdev: > unregister_chrdev_region(dev, srom_devs); > @@ -454,6 +464,7 @@ static void srom_cleanup(void) > device_destroy(srom_class, MKDEV(srom_major, i)); > class_destroy(srom_class); > cdev_del(&srom_cdev); > + platform_device_unregister(srom_parent); > unregister_chrdev_region(MKDEV(srom_major, 0), srom_devs); > kfree(srom_devices); > } > -- > 1.9.1 > 8<------------------------------------------- > > Would that work for you? Note that it will move the srom class devices > one level deeper in /sys/devices/... hierarchy. > > Paweł > -- 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/
(Resending with text/plain.) First, sorry for the delayed response, with summer vacation and then trying to catch up. :-) On 8/8/2014 12:34 PM, Pawel Moll wrote: > On Tue, 2014-08-05 at 21:08 +0100, Chris Metcalf wrote: >>>> In addition, we also have user binaries >>>> in the wild that know to look for /sys/devices/platform/srom/ paths, >>>> so I'm pretty reluctant to change this path without good reason. >>> So what is the srom class for then if not for device discovery? And why >>> do they look for them in the first place? To get relevant character >>> device's data, if I understand it right? >>> >>> Maybe you could just register a simple "proper" platform device for all >>> the sroms and then hang the class devices from it? I can type some code >>> doing this if it sound reasonably? >> By "device discovery" I meant the way you find the way in your devices >> in /sysfs. You seem to be traversing /sys/devices/... tree, while you've >> got almost direct access to them through /sys/class/srom and you can (I >> believe, correct me if I'm wrong, Greg) rely on this path being stable. Yes, this is an excellent point. I will change the user tool to use /sys/class instead and then it will work with the existing kernel as well as with future kernels that incorporate your suggested change. >> The >> subdirectories under /sys/devices/platform/srom/ correspond to partitions >> in the SPI-ROM, which are software constructs created by the Tilera hypervisor. >> By default we have three, where the first holds boot data that the chip >> can use to boot out of hardware, and the other two are smaller partitions >> for boot- and user-specific data. We use the /sys files primarily to get the >> page size and sector size for the sroms, and also export other interesting >> information like the total size of the particular srom device. >> >> Thank you for volunteering to write a bit of code; if that's the best >> way to clarify this for us, fantastic, or else pointing us at existing >> good practices or documentation would be great too. > [...] > @@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index) > SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0) > return -EIO; > > - dev = device_create(srom_class, &platform_bus, > + dev = device_create(srom_class, srom_parent, > MKDEV(srom_major, index), srom, "%d", index); > return PTR_ERR_OR_ZERO(dev); > } The second argument should be &srom_parent.dev though, I think. Right? > Would that work for you? Note that it will move the srom class devices > one level deeper in /sys/devices/... hierarchy. Yes, that seems slightly unfortunately but not too problematic. If the consensus is that this is the way to go, I can certainly take this change into the Tile tree.
On Fri, 2014-08-29 at 19:43 +0100, Chris Metcalf wrote: > >> Thank you for volunteering to write a bit of code; if that's the best > >> way to clarify this for us, fantastic, or else pointing us at existing > >> good practices or documentation would be great too. > > [...] > > @@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index) > > SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0) > > return -EIO; > > > > - dev = device_create(srom_class, &platform_bus, > > + dev = device_create(srom_class, srom_parent, > > MKDEV(srom_major, index), srom, "%d", index); > > return PTR_ERR_OR_ZERO(dev); > > } > > The second argument should be &srom_parent.dev though, I think. Right? Yes, sure - as I said, I haven't really tested this code, sorry! > If the > consensus is that this is the way to go, I can certainly take this change > into the Tile tree. That would be cool, and left us only with the scsi/DMA as the last user of platform_bus. But this is a completely different story ;-) Paweł -- 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/
On 9/1/2014 8:27 AM, Pawel Moll wrote: > On Fri, 2014-08-29 at 19:43 +0100, Chris Metcalf wrote: >> If the >> consensus is that this is the way to go, I can certainly take this change >> into the Tile tree. > That would be cool, and left us only with the scsi/DMA as the last user > of platform_bus. But this is a completely different story ;-) OK, sounds good. It's in the tile tree at linux-next now.
diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c index bd37747..7fb0fd5 100644 --- a/drivers/char/tile-srom.c +++ b/drivers/char/tile-srom.c @@ -76,6 +76,7 @@ MODULE_LICENSE("GPL"); static int srom_devs; /* Number of SROM partitions */ static struct cdev srom_cdev; +static struct platform_device *srom_parent; static struct class *srom_class; static struct srom_dev *srom_devices; @@ -350,7 +351,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index) SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0) return -EIO; - dev = device_create(srom_class, &platform_bus, + dev = device_create(srom_class, srom_parent, MKDEV(srom_major, index), srom, "%d", index); return PTR_ERR_OR_ZERO(dev); } @@ -415,6 +416,13 @@ static int srom_init(void) if (result < 0) goto fail_chrdev; + /* Create a parent device */ + srom_parent = platform_device_register_simple("srom", -1, NULL, 0); + if (IS_ERR(srom_parent)) { + result = PTR_ERR(srom_parent); + goto fail_pdev; + } + /* Create a sysfs class. */ srom_class = class_create(THIS_MODULE, "srom"); if (IS_ERR(srom_class)) { @@ -438,6 +446,8 @@ fail_class: device_destroy(srom_class, MKDEV(srom_major, i)); class_destroy(srom_class); fail_cdev: + platform_device_unregister(srom_parent); +fail_pdev: cdev_del(&srom_cdev); fail_chrdev: unregister_chrdev_region(dev, srom_devs); @@ -454,6 +464,7 @@ static void srom_cleanup(void) device_destroy(srom_class, MKDEV(srom_major, i)); class_destroy(srom_class); cdev_del(&srom_cdev); + platform_device_unregister(srom_parent); unregister_chrdev_region(MKDEV(srom_major, 0), srom_devs); kfree(srom_devices); }