Messages in this thread | | | Date | Sun, 27 Jan 2013 14:00:15 +0800 | From | Mark Brown <> | Subject | Re: [PATCH v2 1/4] regulator-core: support shared enable GPIO concept |
| |
On Tue, Jan 15, 2013 at 04:35:41AM +0000, Kim, Milo wrote: > A Regulator can be enabled by external GPIO pin. > This is configurable in the regulator_config.
Please use subject lines matching the subsystem - not doing this makes it more likely that patches will be missed or responses delayed. For example, when looking at my patch queue for regulator patches I search for "regulator:" in my review pending queue, patches that don't have that won't turn up. This should be "regulator: core: ...".
Anyway, this series looks pretty close now...
> +/* Manage enable GPIO list. Same GPIO pin can be shared among regulators */ > +static int regulator_ena_gpio_request(struct regulator_dev *rdev, > + const struct regulator_config *config) > +{ > + struct regulator_enable_gpio *pin; > + int ret; > + > + list_for_each_entry(pin, ®ulator_ena_gpio_list, list) { > + if (pin->gpio == config->ena_gpio) { > + rdev_info(rdev, "GPIO %d is already used\n", > + config->ena_gpio); > + return 0;
This log is going to get noisy once the GPIOs are shared. A _dbg() would be OK though.
> + ret = gpio_request_one(config->ena_gpio, > + GPIOF_DIR_OUT | config->ena_gpio_flags, > + rdev_get_name(rdev)); > + if (ret) > + return ret; > + > + pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL); > + if (pin == NULL) > + return -ENOMEM;
Should free the GPIO if there's an error here.
> + pin->regulator = rdev;
Do we really want to keep track of the regulator here, again once we start sharing pins...
We also need some matching code in the release path to free the GPIO and struct when the regulator is removed. [unhandled content-type:application/pgp-signature] | |