| Message ID | 1407515691.31897.26.camel@hornet (mailing list archive) |
|---|---|
| State | New, archived |
| 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?
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? >
(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?
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); }