On Mon, Mar 19, 2012 at 02:46:20PM +0200, Mika Westerberg wrote: > It is possible that some of the i2c busses are supplied by a separate > regulator which needs to be enabled before we can access the bus. Thus add > support for an optional regulator. No, support for critical supplies should not be optional. I'm getting rather fed up with having to point this out. There's always a regulator there supplying the bus, the driver should just assume that the system provides it. > We also implement enabling/disabling the regulator on system and runtime PM > hooks. This is suspicious too... If this is just the external buffer/reference voltage for the bus then I'd really not expect the controller to have to worry about it, the drivers for the target devices ought to be making sure they've powered up the devices prior to trying to talk to them anyway (and most of the time will need the supplies on for *much* longer than just the time spent doing I2C transactions). There's no harm in having support in the controller driver but I'd be surprised if the clients were happy with having their I/O voltage bounced on them... > + if (i2c->controller->regulator) > + regulator_enable(i2c->controller->regulator); > + controller->regulator = regulator_get(&pdev->dev, "v-i2c"); > + if (IS_ERR(controller->regulator)) > + controller->regulator = NULL; > + else > + regulator_enable(controller->regulator); There's no error checking at all in this ccode - if it fails to request the supply the driver silently ignores the error and if the regulator is there but fails to enable the error will also be silently ignored. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
