Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences

On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:

+		power-off-sequence {
+			step0 {
+				gpio = "enable";
+				disable;

I'd change the name to "reset" or something in the example - avoids any
confusion with the action.

+#ifdef CONFIG_REGULATOR
+	case POWER_SEQ_REGULATOR:
+		if (pdata->regulator.enable)
+			err = regulator_enable(step->resource->regulator);
+		else
+			err = regulator_disable(step->resource->regulator);
+		break;

The regulator and GPIO APIs should stub themselves out adequately to not
need the ifdefs at least for regulator I'd use the stubs since...

+	/*
+	 * should never happen unless the sequence includes a step which
+	 * type does not have support compiled in
+	 */
+	default:
+		return -EINVAL;

...this probably isn't what's meant.  It might also be nice to have
support for bulk_enable() but that's definitely something that could be
added later.

+		err = power_seq_step_run(&seq->steps[cpt]);
+		if (err) {
+			dev_err(dev, "error %d while running power sequence!\n",
+				err);
+			return err;

I'd also log at least the step number, it'll make diagnostics easier.