| Message ID | 1303897191-14792-5-git-send-email-gg@slimlogic.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote: > Hi, > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote: > > The twl6025 uses a different regulator for USB than the 6030 so select > > the correct regulator name depending on the subclass of device. > > > > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk> > > I don't see the point of this patch. It's just a string. Use the same > name and add a comment saying that on datasheet/TRM/documentation the > name LDO is actually referred to as LDOUSB. It's the same functionality > anyway. > I think for the avoidance of any doubt, it's probably best to use the TWL6025 string name here as it will importantly match the TWL6025 TRM and any schematics using the TWL6025. Getting this wrong during TWL6025 board integration has the potential for hardware damage. Liam -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote: > On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote: > > Hi, > > > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote: > > > The twl6025 uses a different regulator for USB than the 6030 so select > > > the correct regulator name depending on the subclass of device. > > > > > > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk> > > > > I don't see the point of this patch. It's just a string. Use the same > > name and add a comment saying that on datasheet/TRM/documentation the > > name LDO is actually referred to as LDOUSB. It's the same functionality > > anyway. > > > > I think for the avoidance of any doubt, it's probably best to use the > TWL6025 string name here as it will importantly match the TWL6025 TRM > and any schematics using the TWL6025. Getting this wrong during TWL6025 > board integration has the potential for hardware damage. I would rather have something that doesn't depend on a correct string and matches based on the device pointer instead. I agree that having the correct string makes it easier to reference schematics/trm and the like, but making the SW depend on the correct spelling of a simple string, is too much for me :-( Specially when getting it wrong "has the potential for hardware damage" :-)
On Mon, 2011-05-09 at 12:03 +0300, Felipe Balbi wrote: > Hi, > > On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote: > > On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote: > > > Hi, > > > > > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote: > > > > The twl6025 uses a different regulator for USB than the 6030 so select > > > > the correct regulator name depending on the subclass of device. > > > > > > > > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk> > > > > > > I don't see the point of this patch. It's just a string. Use the same > > > name and add a comment saying that on datasheet/TRM/documentation the > > > name LDO is actually referred to as LDOUSB. It's the same functionality > > > anyway. > > > > > > > I think for the avoidance of any doubt, it's probably best to use the > > TWL6025 string name here as it will importantly match the TWL6025 TRM > > and any schematics using the TWL6025. Getting this wrong during TWL6025 > > board integration has the potential for hardware damage. > > I would rather have something that doesn't depend on a correct string > and matches based on the device pointer instead. I agree that having the > correct string makes it easier to reference schematics/trm and the like, > but making the SW depend on the correct spelling of a simple string, is > too much for me :-( > > Specially when getting it wrong "has the potential for hardware damage" > :-) > I think it's the lesser evil though, especially for device integrators. They will just match the regulator name from the schematics together with the TRM name when creating their regulator constraints and having different names here will definitely cause confusion. Liam -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote: > On Mon, 2011-05-09 at 12:03 +0300, Felipe Balbi wrote: > > Hi, > > > > On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote: > > > On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote: > > > > Hi, > > > > > > > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote: > > > > > The twl6025 uses a different regulator for USB than the 6030 so select > > > > > the correct regulator name depending on the subclass of device. > > > > > > > > > > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk> > > > > > > > > I don't see the point of this patch. It's just a string. Use the same > > > > name and add a comment saying that on datasheet/TRM/documentation the > > > > name LDO is actually referred to as LDOUSB. It's the same functionality > > > > anyway. > > > > > > > > > > I think for the avoidance of any doubt, it's probably best to use the > > > TWL6025 string name here as it will importantly match the TWL6025 TRM > > > and any schematics using the TWL6025. Getting this wrong during TWL6025 > > > board integration has the potential for hardware damage. > > > > I would rather have something that doesn't depend on a correct string > > and matches based on the device pointer instead. I agree that having the > > correct string makes it easier to reference schematics/trm and the like, > > but making the SW depend on the correct spelling of a simple string, is > > too much for me :-( > > > > Specially when getting it wrong "has the potential for hardware damage" > > :-) > > > > I think it's the lesser evil though, especially for device integrators. > They will just match the regulator name from the schematics together > with the TRM name when creating their regulator constraints and having > different names here will definitely cause confusion. Any chance Device Tree could be used to pass that data to kernel instead, together with regulator names and all needed data for each one of them ?
On Mon, May 09, 2011 at 03:16:03PM +0300, Felipe Balbi wrote: > On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote: > > I think it's the lesser evil though, especially for device integrators. > > They will just match the regulator name from the schematics together > > with the TRM name when creating their regulator constraints and having > > different names here will definitely cause confusion. > Any chance Device Tree could be used to pass that data to kernel > instead, together with regulator names and all needed data for each one > of them ? I'm pretty sure that as soon as we have viable device tree support for relevant platforms in mainline we'll have regulator support, though I'm not sure that this will help too much with the naming as you'll still have to figure out what the consumer requests. We shouldn't be passing in the consumer supply names from device tree (at least not a board specific one) as this breaks the model that the supply names correspond to the chip pins. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, May 09, 2011 at 02:29:41PM +0200, Mark Brown wrote: > On Mon, May 09, 2011 at 03:16:03PM +0300, Felipe Balbi wrote: > > On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote: > > > > I think it's the lesser evil though, especially for device integrators. > > > They will just match the regulator name from the schematics together > > > with the TRM name when creating their regulator constraints and having > > > different names here will definitely cause confusion. > > > Any chance Device Tree could be used to pass that data to kernel > > instead, together with regulator names and all needed data for each one > > of them ? > > I'm pretty sure that as soon as we have viable device tree support for > relevant platforms in mainline we'll have regulator support, though I'm > not sure that this will help too much with the naming as you'll still > have to figure out what the consumer requests. We shouldn't be passing > in the consumer supply names from device tree (at least not a board > specific one) as this breaks the model that the supply names correspond > to the chip pins. The thing is. We already had problems in the past with the Clock FW because drivers were passing clock names on platform_data and I really want to avoid the same on regulators. We need something clever. We pass in the data of how regulators are wired at the board level and drivers would regulator_get(my_dev->dev, "whatever fancy name I want as it doesn't matter at all"); If a driver has >1 regulator, you can distinguish them by which functionality they provide, no matter the name, they will be connected to a particular ball on the IC package which has a certain definition on the TRM. In other words, we should match on the functionality they provide, not on the name. Let's try to thing some 5 - 6 years ahead. With the current trend, we will be working on support for the PMIC on OMAP10, imagine if each one of those revisions decide to change the name of the regulator, because this new HW engineer working on the PMIC design doesn't like the old name. We will have X regulator pointers and X regulator name pointers in our platform_data. It will be really cumbersome and prone to errors if people continue on copy&pasting old code to "generate" a new driver. What I was expecting to see, was a mechanism where we define how those things are wired (it doesn't matter if the data uses DeviceTree or what) at the HW level and we ask the framework to give us the regulator connected to device X which provides a certain functionality. Just like clkdev has managed to get close to. Currently drivers will: clk_get(dev, "ick"); clk_get(dev, "fck"); etc... and the name really doesn't matter. Clkdev still isn't perfect as it uses device names, then when we want/need to change the device/driver name (due to some re-factoring for example) we end up breaking things. IMHO, struct device should point to its dependencies, be it a clock, be it a regulator.
On Mon, 2011-05-09 at 15:16 +0300, Felipe Balbi wrote: > Hi, > > On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote: > > On Mon, 2011-05-09 at 12:03 +0300, Felipe Balbi wrote: > > > Hi, > > > > > > On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote: > > > > On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote: > > > > > Hi, > > > > > > > > > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote: > > > > > > The twl6025 uses a different regulator for USB than the 6030 so select > > > > > > the correct regulator name depending on the subclass of device. > > > > > > > > > > > > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk> > > > > > > > > > > I don't see the point of this patch. It's just a string. Use the same > > > > > name and add a comment saying that on datasheet/TRM/documentation the > > > > > name LDO is actually referred to as LDOUSB. It's the same functionality > > > > > anyway. > > > > > > > > > > > > > I think for the avoidance of any doubt, it's probably best to use the > > > > TWL6025 string name here as it will importantly match the TWL6025 TRM > > > > and any schematics using the TWL6025. Getting this wrong during TWL6025 > > > > board integration has the potential for hardware damage. > > > > > > I would rather have something that doesn't depend on a correct string > > > and matches based on the device pointer instead. I agree that having the > > > correct string makes it easier to reference schematics/trm and the like, > > > but making the SW depend on the correct spelling of a simple string, is > > > too much for me :-( > > > > > > Specially when getting it wrong "has the potential for hardware damage" > > > :-) > > > > > > > I think it's the lesser evil though, especially for device integrators. > > They will just match the regulator name from the schematics together > > with the TRM name when creating their regulator constraints and having > > different names here will definitely cause confusion. > > Any chance Device Tree could be used to pass that data to kernel > instead, together with regulator names and all needed data for each one > of them ? > Yes, I think this should be the long term aim, but atm we will have to stick with current implementation until regulator has device tree support. Liam -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 09, 2011 at 04:07:07PM +0300, Felipe Balbi wrote: > The thing is. We already had problems in the past with the Clock FW > because drivers were passing clock names on platform_data and I really > want to avoid the same on regulators. We need something clever. Right, which is why you should *never* do that. Any time I see anyone requesting a regulator based on anything other than a fixed string in the driver and the struct device for the consumer I push back very hard, and so should everyone else. > We pass in the data of how regulators are wired at the board level and > drivers would regulator_get(my_dev->dev, "whatever fancy name I want as > it doesn't matter at all"); No, not at all. Consumer drivers should be hard coding the strings they request and the strings used should correspond to the pin names used on the device. > If a driver has >1 regulator, you can distinguish them by which > functionality they provide, no matter the name, they will be connected > to a particular ball on the IC package which has a certain definition Having more than one supply is the common case for devices; relatively few devices have a single supply. > on the TRM. In other words, we should match on the functionality they > provide, not on the name. As Liam said this will just make the situation worse. Users will have to figure out what the names the driver authors assigned to the various device supplies map onto in the physical system via an additional layer of indirection which will at best be written down in comments in the driver. > Let's try to thing some 5 - 6 years ahead. With the current trend, we > will be working on support for the PMIC on OMAP10, imagine if each one > of those revisions decide to change the name of the regulator, because > this new HW engineer working on the PMIC design doesn't like the old > name. We will have X regulator pointers and X regulator name pointers in > our platform_data. It will be really cumbersome and prone to errors if > people continue on copy&pasting old code to "generate" a new driver. Here you're apparently talking about something different - you're talking about how we pass in the regulator init data for the regulators supplied by the chip. The patch being discussed changes the consumer side for USB, not the regulator driver side. The naming on the driver side is a particular problem for TWL devices since unlike most PMICs the regulators aren't simply numbered but are instead assigned individual names so you can't just use a big array indexed by number. > What I was expecting to see, was a mechanism where we define how those > things are wired (it doesn't matter if the data uses DeviceTree or what) > at the HW level and we ask the framework to give us the regulator > connected to device X which provides a certain functionality. Just like > clkdev has managed to get close to. Currently drivers will: This is preceisely what is happening. A well written consumer driver asks the regulator core for the regulator supplying a given supply, with the supplies named in the same way as they are named in the datasheet. You appear to be arging strongly for us to adopt the model which is already in use. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 09, 2011 at 03:35:43PM +0200, Mark Brown wrote: > On Mon, May 09, 2011 at 04:07:07PM +0300, Felipe Balbi wrote: > > > The thing is. We already had problems in the past with the Clock FW > > because drivers were passing clock names on platform_data and I really > > want to avoid the same on regulators. We need something clever. > > Right, which is why you should *never* do that. Any time I see anyone > requesting a regulator based on anything other than a fixed string in > the driver and the struct device for the consumer I push back very hard, > and so should everyone else. ok, so let's go back to the original patch: > @@ -190,6 +190,12 @@ static int twl6030_phy_suspend(struct otg_transceiver *x, int suspend) > > static int twl6030_usb_ldo_init(struct twl6030_usb *twl) > { > + char *regulator_name; > + > + if (twl_features() & TWL6025_SUBCLASS) > + regulator_name = "ldousb"; > + else > + regulator_name = "vusb"; > > /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */ > twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG); > @@ -200,7 +206,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb *twl) > /* Program MISC2 register and set bit VUSB_IN_VBAT */ > twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2); > > - twl->usb3v3 = regulator_get(twl->dev, "vusb"); > + twl->usb3v3 = regulator_get(twl->dev, regulator_name); > if (IS_ERR(twl->usb3v3)) > return -ENODEV; then, imagine 5 years from now how that branch will look. How long do you think it'll take until someone decides to pass that name via platform_data to avoid growing that conditional ? > > We pass in the data of how regulators are wired at the board level and > > drivers would regulator_get(my_dev->dev, "whatever fancy name I want as > > it doesn't matter at all"); > > No, not at all. Consumer drivers should be hard coding the strings they > request and the strings used should correspond to the pin names used on > the device. but that's the thing. Why do they need to depend on that string ? > > on the TRM. In other words, we should match on the functionality they > > provide, not on the name. > > As Liam said this will just make the situation worse. Users will have > to figure out what the names the driver authors assigned to the various > device supplies map onto in the physical system via an additional layer > of indirection which will at best be written down in comments in the > driver. My point is that the "name" shouldn't matter. Instead of matching a "name" match a "reference". Have something earlier in the boot assing regulators to devices, or something like that, so that drivers don't need to care about "names". > > Let's try to thing some 5 - 6 years ahead. With the current trend, we > > will be working on support for the PMIC on OMAP10, imagine if each one > > of those revisions decide to change the name of the regulator, because > > this new HW engineer working on the PMIC design doesn't like the old > > name. We will have X regulator pointers and X regulator name pointers in > > our platform_data. It will be really cumbersome and prone to errors if > > people continue on copy&pasting old code to "generate" a new driver. > > Here you're apparently talking about something different - you're > talking about how we pass in the regulator init data for the regulators > supplied by the chip. The patch being discussed changes the consumer > side for USB, not the regulator driver side. The naming on the driver see above, how long do you think it'll take for someone to try and pass that name via pdata ? On the next name change, this is already likely to happen to prevent that conditional from growing. > side is a particular problem for TWL devices since unlike most PMICs the > regulators aren't simply numbered but are instead assigned individual > names so you can't just use a big array indexed by number. why not ? The name shouldn't matter. In anycase, if you look into /sys/class/regulator/ all directories are regulator.<index>: dev_set_name(&rdev->dev, "regulator.%d", atomic_inc_return(®ulator_no) - 1); > > What I was expecting to see, was a mechanism where we define how those > > things are wired (it doesn't matter if the data uses DeviceTree or what) > > at the HW level and we ask the framework to give us the regulator > > connected to device X which provides a certain functionality. Just like > > clkdev has managed to get close to. Currently drivers will: > > This is preceisely what is happening. A well written consumer driver > asks the regulator core for the regulator supplying a given supply, with > the supplies named in the same way as they are named in the datasheet. > > You appear to be arging strongly for us to adopt the model which is > already in use. I guess I hasn't been clear enough then, my whole point is that drivers shouldn't depend on strings at all in order to fetch the correct regulator. How many: if (xxx_features() & XXX_CLASS_IS_YYYY) regulator_name = "my_first_name"; else regulator_name = "my_second_name"; will have to pop in drivers until we figure that it won't scale ?
On Mon, May 09, 2011 at 04:51:26PM +0300, Felipe Balbi wrote: > On Mon, May 09, 2011 at 03:35:43PM +0200, Mark Brown wrote: > > - twl->usb3v3 = regulator_get(twl->dev, "vusb"); > > + twl->usb3v3 = regulator_get(twl->dev, regulator_name); > > if (IS_ERR(twl->usb3v3)) > > return -ENODEV; > then, imagine 5 years from now how that branch will look. How long do > you think it'll take until someone decides to pass that name via > platform_data to avoid growing that conditional ? Probably way more than five years; frankly someone needs to yell at whoever wrote the documentation and point out to them that VBUS is a well known name that came from the spec. In this particular case we actually shouldn't be worry about this at all as it turns out that the consumer and supply are both internal to the chip and hard wired together with no external users (the regulator_init_data is part of the twl core not the board) so we shouldn't be making this visible outside the drivers in the first place, the name is totally irrelevant to anything except the twl drivers. The name is much more important if someone mapping out the regulators on a board has to worry about it. > > > We pass in the data of how regulators are wired at the board level and > > > drivers would regulator_get(my_dev->dev, "whatever fancy name I want as > > > it doesn't matter at all"); > > No, not at all. Consumer drivers should be hard coding the strings they > > request and the strings used should correspond to the pin names used on > > the device. > but that's the thing. Why do they need to depend on that string ? They have to depend on *something* textual. Even if you change it into program code it's still going to have to be written down somewhere. > > As Liam said this will just make the situation worse. Users will have > > to figure out what the names the driver authors assigned to the various > > device supplies map onto in the physical system via an additional layer > > of indirection which will at best be written down in comments in the > > driver. > My point is that the "name" shouldn't matter. Instead of matching a > "name" match a "reference". Have something earlier in the boot assing > regulators to devices, or something like that, so that drivers don't > need to care about "names". Unfortunately we write computer programs using text and that does rather mean that things need names eventually, and that's going to have to include the driver since whatever else does the mapping is going to need to figure out what the driver is using to refer to a given regulator. At some point these magic references are going to have to come from a human in some form the human has a hope of comprehending. The names aren't really for the benefit of the driver, they're there so that a human reading a schematic can work out which regulator to attach to which supply and produce a mapping which other people can read and understand. > > Here you're apparently talking about something different - you're > > talking about how we pass in the regulator init data for the regulators > > supplied by the chip. The patch being discussed changes the consumer > > side for USB, not the regulator driver side. The naming on the driver > see above, how long do you think it'll take for someone to try and pass > that name via pdata ? On the next name change, this is already likely to > happen to prevent that conditional from growing. People try to pass things as platform data all the time, usually as the first thing they think of because they are having a hard time distinguishing between the label the rail was given on the board and the label the chip uses for the supply. This isn't a problem, we just tell them not to do that so that people can use the driver on other boards. > > side is a particular problem for TWL devices since unlike most PMICs the > > regulators aren't simply numbered but are instead assigned individual > > names so you can't just use a big array indexed by number. > why not ? The name shouldn't matter. In anycase, if you look into > /sys/class/regulator/ all directories are regulator.<index>: > dev_set_name(&rdev->dev, "regulator.%d", > atomic_inc_return(®ulator_no) - 1); That's not terribly useful for describing the relationships between devices as the names come on a first come first served basis and are therefore not at all stable. We could try assiging base numbers to the various regulator drivers but then you end up with a massive legibility problem as nobody numbers the supplies on devices. > > > What I was expecting to see, was a mechanism where we define how those > shouldn't depend on strings at all in order to fetch the correct > regulator. How many: > if (xxx_features() & XXX_CLASS_IS_YYYY) > regulator_name = "my_first_name"; > else > regulator_name = "my_second_name"; > will have to pop in drivers until we figure that it won't scale ? Note that the combination of _features and _CLASS tests here is already fairly unusual for chips, even before you add on the random name changes. I really think you're generalising too much from a rare case here, and that it's more likely that for most cases where you get a name change it'll be but one of many changes that need to be accounted for or you'll just be able to do something fairly straightforward and table based. The upshot is that I'm not terribly concerned about this for the chips we're actually seeing (this is the first time I've seen a device which just randomly renames one supply at all). Besides, it's also not like there's any alternatives on offer here. Having said all that I do have a patch in the works which should mean that noddy consumers (not this one) don't need any code or data at all and can just key off device lifetime and PM events transparently -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c index 6e920de..3d82419 100644 --- a/drivers/usb/otg/twl6030-usb.c +++ b/drivers/usb/otg/twl6030-usb.c @@ -190,6 +190,12 @@ static int twl6030_phy_suspend(struct otg_transceiver *x, int suspend) static int twl6030_usb_ldo_init(struct twl6030_usb *twl) { + char *regulator_name; + + if (twl_features() & TWL6025_SUBCLASS) + regulator_name = "ldousb"; + else + regulator_name = "vusb"; /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */ twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG); @@ -200,7 +206,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb *twl) /* Program MISC2 register and set bit VUSB_IN_VBAT */ twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2); - twl->usb3v3 = regulator_get(twl->dev, "vusb"); + twl->usb3v3 = regulator_get(twl->dev, regulator_name); if (IS_ERR(twl->usb3v3)) return -ENODEV;
The twl6025 uses a different regulator for USB than the 6030 so select the correct regulator name depending on the subclass of device. Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk> --- drivers/usb/otg/twl6030-usb.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)