Re: [Patch v2 2/7] Regulator: DA9055 Regulator driver

[Mark Brown]

On Tue, Oct 09, 2012 at 04:30:16PM +0530, Ashish Jangam wrote:
> On Tue, 2012-10-09 at 15:37 +0530, Mark Brown wrote:
> > On Mon, Oct 08, 2012 at 07:00:39PM +0530, Ashish Jangam wrote:

> > > +		/* Set the GPIO I/P pin for controlling the regulator state. */
> > > +		ret = devm_gpio_request_one(config->dev, gpio, GPIOF_DIR_IN,
> > > +					    name);
> > > +		if (ret < 0)
> > > +			goto err;

> > We never actually appear to use this GPIO anywhere...  why are we
> > requesting it?

> DA9055 regulator changes its state by detecting the rising/failing edge at
> GPI DA9055. Therefore we just need to set the DA9055 GPIO direction to input.

Right, so there's several problems here.  One is that this code is very
obscure - you're really doing pinmux here rather than actually using it
as a GPIO, a better comment would clarify this.  The other is that
you're requiring a defined gpio_base in platform data, it would be
better to allow this to be dynamically assigned as the driver can find
it's own GPIOs easily enough.

> >   Also, why is the ability to read the regulator state via
> > a GPIO associated with controlling it via a GPIO, it's unusual for these
> > things to be tied together.

> There is no connection between state just to differentiate between two strings/labels.
> If required I can change the string.

It's nothing to do with the name, it's that it looks due to the above
like the input GPIO is used by the CPU to read the state of the device.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/