From: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> To: Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> Cc: eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Haojian Zhuang <haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org Subject: Re: [PATCH v2 6/7] i2c: pxa: support i2c controller from DT Date: Sat, 30 Jul 2011 18:38:46 -0600 [thread overview] Message-ID: <20110731003846.GA21433@ponder.secretlab.ca> (raw) In-Reply-To: <20110730153728.GA17570-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> On Sat, Jul 30, 2011 at 04:37:28PM +0100, Russell King - ARM Linux wrote: > On Sat, Jul 30, 2011 at 04:29:50AM -1000, Mitch Bradley wrote: > > On 7/29/2011 6:55 AM, Russell King - ARM Linux wrote: > >> On Fri, Jul 29, 2011 at 10:52:22AM -0600, Grant Likely wrote: > >>> On Thu, Jul 28, 2011 at 02:41:32PM +0800, Haojian Zhuang wrote: > >>>> - /* > >>>> - * If "dev->id" is negative we consider it as zero. > >>>> - * The reason to do so is to avoid sysfs names that only make > >>>> - * sense when there are multiple adapters. > >>>> - */ > >>>> - i2c->adap.nr = dev->id; > >>>> - snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u", > >>>> - i2c->adap.nr); > >>>> > >>>> - i2c->clk = clk_get(&dev->dev, NULL); > >>>> + if (np) { > >>>> + i2c->adap.nr = idx++; > >>> > >>> Use this so that a bus number gets dynamically assigned: > >>> i2c->adap.nr = -1; > >>> > >>>> + snprintf(i2c->adap.name, sizeof(i2c->adap.name), > >>>> + "pxa2xx-i2c.%u", i2c->adap.nr); > >>>> + i2c->clk = clk_get_sys(i2c->adap.name, NULL); > >>> > >>> Missing i2c->adap.dev.of_node = dev->dev.of_node; > >> > >> And here we go again. Is it really the case that this DT stuff doesn't > >> have stable device names? The "full_name" is indeed stable for a device_node, so that isn't actually the problem, but the full_name is the entire path from the root of the tree to the device. The problem is that it is far too long for to be used as (struct device*)->kobj.name, and it has '/' characters in it which aren't particularly friendly. The short node name is unique among its siblings, but not globally. Currently the kernel constructs a 'short' name when creating a device with a heuristic that tries to use the physical address of the device and the node name. If that isn't available (ie. non-memory mapped devices), then it uses a globally incremented integer to assign each device a unique name so that sysfs doesn't freak out. See of_device_make_bus_id() in drivers/of/platform.c The /real/ problem is that I don't much like the heuristic; but I haven't been able to think of anything better. There is enough uncertainty (very tiny, but uncertainty never the less) on device names, and the fact that I'm hoping to improve the method of creating device names, that I don't want to rely on them for matching up resources. Instead, the dev_resdata array that is passed into of_platform_populate() has a field to override the heuristic generated name which neatly solves the problem entirely without tracking down all the references that need to be added, and has the added advantage of DT and non-DT platforms using the same names. The clk_get() changes in this patch are absolutely the wrong solution. > > Device tree names are completely stable, based on hardware addresses > > that don't change from boot to boot. Even for buses where access > > addresses are dynamically assigned, the device tree "reg property" > > address is based on a stable addressing form. For example, PCI devices > > use the (stable) configuration address as the reg property and USB > > devices use the (stable) hub port number. > > > > People's tendency to want to assign sequential small integers in Linux > > has nothing to do with the device tree. I suspect that it's a carryover > > from the historical Unix major/minor device numbering model, but in any > > case, there's nothing unstable about the device tree naming model. Quite > > the opposite - stable naming was a fundamental criterion when I designed > > Open Firmware. > > Which means - if Grant is accepting the conversion of ARM to DT and > upstreaming it, he needs to keep an eye on this madness and reject > stuff which tries to do as per this patch. Correct, I will nack/reject any patches messing about with clock attachment or similar changes. I was distracted by the other issues, so I didn't comment on it in this patch. g.
WARNING: multiple messages have this Message-ID (diff)
From: grant.likely@secretlab.ca (Grant Likely) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 6/7] i2c: pxa: support i2c controller from DT Date: Sat, 30 Jul 2011 18:38:46 -0600 [thread overview] Message-ID: <20110731003846.GA21433@ponder.secretlab.ca> (raw) In-Reply-To: <20110730153728.GA17570@n2100.arm.linux.org.uk> On Sat, Jul 30, 2011 at 04:37:28PM +0100, Russell King - ARM Linux wrote: > On Sat, Jul 30, 2011 at 04:29:50AM -1000, Mitch Bradley wrote: > > On 7/29/2011 6:55 AM, Russell King - ARM Linux wrote: > >> On Fri, Jul 29, 2011 at 10:52:22AM -0600, Grant Likely wrote: > >>> On Thu, Jul 28, 2011 at 02:41:32PM +0800, Haojian Zhuang wrote: > >>>> - /* > >>>> - * If "dev->id" is negative we consider it as zero. > >>>> - * The reason to do so is to avoid sysfs names that only make > >>>> - * sense when there are multiple adapters. > >>>> - */ > >>>> - i2c->adap.nr = dev->id; > >>>> - snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u", > >>>> - i2c->adap.nr); > >>>> > >>>> - i2c->clk = clk_get(&dev->dev, NULL); > >>>> + if (np) { > >>>> + i2c->adap.nr = idx++; > >>> > >>> Use this so that a bus number gets dynamically assigned: > >>> i2c->adap.nr = -1; > >>> > >>>> + snprintf(i2c->adap.name, sizeof(i2c->adap.name), > >>>> + "pxa2xx-i2c.%u", i2c->adap.nr); > >>>> + i2c->clk = clk_get_sys(i2c->adap.name, NULL); > >>> > >>> Missing i2c->adap.dev.of_node = dev->dev.of_node; > >> > >> And here we go again. Is it really the case that this DT stuff doesn't > >> have stable device names? The "full_name" is indeed stable for a device_node, so that isn't actually the problem, but the full_name is the entire path from the root of the tree to the device. The problem is that it is far too long for to be used as (struct device*)->kobj.name, and it has '/' characters in it which aren't particularly friendly. The short node name is unique among its siblings, but not globally. Currently the kernel constructs a 'short' name when creating a device with a heuristic that tries to use the physical address of the device and the node name. If that isn't available (ie. non-memory mapped devices), then it uses a globally incremented integer to assign each device a unique name so that sysfs doesn't freak out. See of_device_make_bus_id() in drivers/of/platform.c The /real/ problem is that I don't much like the heuristic; but I haven't been able to think of anything better. There is enough uncertainty (very tiny, but uncertainty never the less) on device names, and the fact that I'm hoping to improve the method of creating device names, that I don't want to rely on them for matching up resources. Instead, the dev_resdata array that is passed into of_platform_populate() has a field to override the heuristic generated name which neatly solves the problem entirely without tracking down all the references that need to be added, and has the added advantage of DT and non-DT platforms using the same names. The clk_get() changes in this patch are absolutely the wrong solution. > > Device tree names are completely stable, based on hardware addresses > > that don't change from boot to boot. Even for buses where access > > addresses are dynamically assigned, the device tree "reg property" > > address is based on a stable addressing form. For example, PCI devices > > use the (stable) configuration address as the reg property and USB > > devices use the (stable) hub port number. > > > > People's tendency to want to assign sequential small integers in Linux > > has nothing to do with the device tree. I suspect that it's a carryover > > from the historical Unix major/minor device numbering model, but in any > > case, there's nothing unstable about the device tree naming model. Quite > > the opposite - stable naming was a fundamental criterion when I designed > > Open Firmware. > > Which means - if Grant is accepting the conversion of ARM to DT and > upstreaming it, he needs to keep an eye on this madness and reject > stuff which tries to do as per this patch. Correct, I will nack/reject any patches messing about with clock attachment or similar changes. I was distracted by the other issues, so I didn't comment on it in this patch. g.
next prev parent reply other threads:[~2011-07-31 0:38 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-07-28 6:41 [PATCH v2 00/07] enable devicetree on arch-mmp Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang [not found] ` <1311835293-18125-1-git-send-email-haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 2011-07-28 6:41 ` [PATCH v2 1/7] ARM: mmp: parse irq from DT Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang [not found] ` <1311835293-18125-2-git-send-email-haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 2011-07-28 6:41 ` [PATCH v2 2/7] ARM: mmp: parse timer configuration " Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang [not found] ` <1311835293-18125-3-git-send-email-haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 2011-07-28 6:41 ` [PATCH v2 3/7] ARM: mmp: support DT on both dkb and brownstone Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang [not found] ` <1311835293-18125-4-git-send-email-haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 2011-07-28 6:41 ` [PATCH v2 4/7] tty: serial: support device tree in pxa Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang [not found] ` <1311835293-18125-5-git-send-email-haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 2011-07-28 6:41 ` [PATCH v2 5/7] tty: serial: check ops before registering console Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang [not found] ` <1311835293-18125-6-git-send-email-haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 2011-07-28 6:41 ` [PATCH v2 6/7] i2c: pxa: support i2c controller from DT Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang [not found] ` <1311835293-18125-7-git-send-email-haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 2011-07-28 6:41 ` [PATCH v2 7/7] i2c: pxa: support to parse property Haojian Zhuang 2011-07-28 6:41 ` Haojian Zhuang 2011-07-29 16:52 ` [PATCH v2 6/7] i2c: pxa: support i2c controller from DT Grant Likely 2011-07-29 16:52 ` Grant Likely [not found] ` <20110729165222.GN11164-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2011-07-29 16:55 ` Russell King - ARM Linux 2011-07-29 16:55 ` Russell King - ARM Linux 2011-07-30 14:29 ` Mitch Bradley 2011-07-30 14:29 ` Mitch Bradley [not found] ` <4E34155E.8010606-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> 2011-07-30 15:37 ` Russell King - ARM Linux 2011-07-30 15:37 ` Russell King - ARM Linux [not found] ` <20110730153728.GA17570-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2011-07-31 0:38 ` Grant Likely [this message] 2011-07-31 0:38 ` Grant Likely 2011-07-29 16:45 ` [PATCH v2 4/7] tty: serial: support device tree in pxa Grant Likely 2011-07-29 16:45 ` Grant Likely 2011-07-29 16:49 ` Russell King - ARM Linux 2011-07-29 16:49 ` Russell King - ARM Linux [not found] ` <20110729164922.GA30298-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2011-07-29 16:53 ` Grant Likely 2011-07-29 16:53 ` Grant Likely 2011-08-01 2:50 ` Haojian Zhuang 2011-08-01 2:50 ` Haojian Zhuang [not found] ` <CAN1soZwYjQp5c=4f=CbqjrXFxzSURSv6i9QHMyjyPszs=kzh9w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-08-01 8:27 ` Russell King - ARM Linux 2011-08-01 8:27 ` Russell King - ARM Linux 2011-07-29 16:42 ` [PATCH v2 3/7] ARM: mmp: support DT on both dkb and brownstone Grant Likely 2011-07-29 16:42 ` Grant Likely [not found] ` <20110729164218.GL11164-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2011-08-01 2:48 ` Haojian Zhuang 2011-08-01 2:48 ` Haojian Zhuang 2011-07-29 16:40 ` [PATCH v2 2/7] ARM: mmp: parse timer configuration from DT Grant Likely 2011-07-29 16:40 ` Grant Likely 2011-07-29 16:36 ` [PATCH v2 1/7] ARM: mmp: parse irq " Grant Likely 2011-07-29 16:36 ` Grant Likely 2011-08-01 2:47 ` Haojian Zhuang 2011-08-01 2:47 ` Haojian Zhuang [not found] ` <CAN1soZxX6EBRPvzV_00njioBn1NfiAbLBoD7EYc-wV-uQM_VoQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-08-01 14:10 ` Grant Likely 2011-08-01 14:10 ` Grant Likely [not found] ` <20110801141036.GA21627-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2011-08-01 14:42 ` Haojian Zhuang 2011-08-01 14:42 ` Haojian Zhuang 2011-08-01 14:43 ` Grant Likely 2011-08-01 14:43 ` Grant Likely 2011-08-01 8:26 ` Russell King - ARM Linux 2011-08-01 8:26 ` Russell King - ARM Linux
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110731003846.GA21433@ponder.secretlab.ca \ --to=grant.likely-s3s/wqlpoipyb63q8fvjnq@public.gmane.org \ --cc=alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org \ --cc=devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org \ --cc=eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.