On Mon, May 26, 2014 at 01:12:43PM +0200, Hans de Goede wrote: > On 05/26/2014 12:38 PM, Mark Brown wrote: > > On Sun, May 25, 2014 at 09:20:52PM +0200, Hans de Goede wrote: > > until we've powered up and enumerated. The only time that there's a > > problem and would need to specify exactly what the device is in the DTS > > is if we need the custom sequence prior to being able to do that at > > which point I don't see much option. > Specified != driver available. Determining whether or not we should power > up based purely on there being a compatible string is not going to work, as > even when using simple powerup we still need compatible strings for non > powerup settings like sdio signal drive strength, oob irq, etc. If there are compatible strings but not one we've heard of then we're back in the situation where we may as well not bother powering up in the first place since we've no idea what to do with the device once it's powered. If we fall down a list of compatibles and decide that the only thing we know about the device is that we can power it up (this is distinct from the case where we probe the device) I'd expect the device to be turned off. > So the only way this will work is if we delay powerup until we've a driver > available, which may be never if the module is not build, or whatever, > and without powerup the user is never going to figure out he is missing > a module. Basically adding a driver flag for blacklisting from the simple > power up logic means inserting an unnecessary initialization ordering issue, > which we really don't need as each ordering dependency is usually one too many. I'm afraid I'm really not seeing a substantial problem either way here, powering up the device isn't going to help us find a driver for it and the UI around reporting that the device is there without a driver should hopefully be unaffected by its power state. > > I don't understand why not powering the device up would be a sensible > > default or why other OSs would also choose to implement things that way. > Because if we are not 100% sure that our simple power up logic will do the > job properly (i.e. in the right order), then the SAFE thing to do is to > not power up. So long as we've got a clear way of saying that we might need to do something special I don't think we should have an issue either way; it's mostly a case of how we specify. > What if a user takes an older distro kernel, where the driver does not > set the opt-out flag you're suggesting since at that time it did not > have its own power logic, then tries to boot that with a dtb file written > for a newer kernel (where the driver does have the opt out flag), and we > start powering up things in the wrong order and let the magic smoke out > of various components on the board ? Conversely what if someone fixes a bug in the standard power up sequence in a newer version of the kernel but then tries to run older software? There's plenty of ways things can go wrong with this stuff. > Also note that this is a perfectly standard use of compatibles, compatibles > are typically used to indicate which driver should be used, in this case > the compatible indicates that the simple powerup driver should be used, > where as if another powerup driver should be used another compatbile will > be there instead. We don't typically actually bind multiple compatibles for a single device. We've got a bunch of options we can choose from but we generally pick the one that matches best and ignore the others. > Where as what you're suggesting is to always pick driver foo, unless > driver bar is available and has a special flag saying to not use foo, this > is a whole new way to use compatibles really, and not one which I think > we want to introduce. I'm not sure I'm buying the idea that we have a powerup driver that's meaningfully not part of the main device driver. > > Well, if things aren't going to work either way for these devices > > without extra stuff it seems it doesn't much matter but it helps the > > simple case to have things default to working. > The simple case still needs a child node describing the needed resources, > adding a compatible = "simple-sdio-powerup" to that at the same as creating > the child node in the first place really is no extra effort at all. From where I'm sitting it's more effort since instead of just putting the device in there (and possibly also some other devices that are software compatible) we have to put in another compatible string which is really just a boolean flag to be used in conjunction with the others. That's harder to think about and we clearly don't want to go through the compatible list, decide that we don't know how to handle the device except power it up so go and do that. If it were done as something like "set boolean property X or powerup-method = Y in the card description" or whatever it'd seem a bit annoying but not a big deal, I think it's the fact that it's getting put into the compatible list that's really concerning me.
Attachment:
signature.asc
Description: Digital signature
