Re: [PATCH v2 1/4] regulator-core: support shared enable GPIO concept

2013-01-26 Thread Mark Brown 
On Tue, Jan 15, 2013 at 04:35:41AM +, 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, _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.


signature.asc
Description: Digital signature

Re: [PATCH v2 1/4] regulator-core: support shared enable GPIO concept

2013-01-26 Thread Mark Brown 
On Tue, Jan 15, 2013 at 04:35:41AM +, 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, regulator_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.


signature.asc
Description: Digital signature

[PATCH v2 1/4] regulator-core: support shared enable GPIO concept

2013-01-14 Thread Kim, Milo 
 A Regulator can be enabled by external GPIO pin.
 This is configurable in the regulator_config.
 At this moment, this enable GPIO is owned by only one regulator device.
 In some devices, multiple regulators are enabled by shared one GPIO pin.
 This patch extends this limitation, enabling shared enable GPIO of regulators.

 New list for enable GPIO: 'regulator_ena_gpio_list'
   This manages enable GPIO list.

 New structure for supporting shared enable GPIO: 'regulator_enable_gpio'
   The enable count is used for balancing GPIO control count.
   The count is incremented when GPIO is enabled.
   On the other hand, it's decremented when GPIO is disabled.

 How it works
   If the GPIO is already used, skip requesting new GPIO usage.
   The GPIO is new one, request GPIO function and add it to the list of
   enable GPIO.
   This list is used for balancing enable GPIO count and pin control.

Signed-off-by: Milo(Woogyom) Kim 
---
 drivers/regulator/core.c |   49 +++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index de47880..1273090 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -51,6 +51,7 @@
 static DEFINE_MUTEX(regulator_list_mutex);
 static LIST_HEAD(regulator_list);
 static LIST_HEAD(regulator_map_list);
+static LIST_HEAD(regulator_ena_gpio_list);
 static bool has_full_constraints;
 static bool board_wants_dummy_regulator;
 
@@ -69,6 +70,18 @@ struct regulator_map {
 };
 
 /*
+ * struct regulator_enable_gpio
+ *
+ * Management for shared enable GPIO pin
+ */
+struct regulator_enable_gpio {
+   struct list_head list;
+   int gpio;
+   u32 enable_count;
+   struct regulator_dev *regulator;
+};
+
+/*
  * struct regulator
  *
  * One for each consumer device.
@@ -1456,6 +1469,38 @@ void devm_regulator_put(struct regulator *regulator)
 }
 EXPORT_SYMBOL_GPL(devm_regulator_put);
 
+/* 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, _ena_gpio_list, list) {
+   if (pin->gpio == config->ena_gpio) {
+   rdev_info(rdev, "GPIO %d is already used\n",
+   config->ena_gpio);
+   return 0;
+   }
+   }
+
+   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;
+
+   pin->gpio = config->ena_gpio;
+   pin->regulator = rdev;
+   list_add(>list, _ena_gpio_list);
+
+   return 0;
+}
+
 static int _regulator_do_enable(struct regulator_dev *rdev)
 {
int ret, delay;
@@ -3420,9 +3465,7 @@ regulator_register(const struct regulator_desc 
*regulator_desc,
dev_set_drvdata(>dev, rdev);
 
if (config->ena_gpio && gpio_is_valid(config->ena_gpio)) {
-   ret = gpio_request_one(config->ena_gpio,
-  GPIOF_DIR_OUT | config->ena_gpio_flags,
-  rdev_get_name(rdev));
+   ret = regulator_ena_gpio_request(rdev, config);
if (ret != 0) {
rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 config->ena_gpio, ret);
-- 
1.7.9.5


Best Regards,
Milo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/4] regulator-core: support shared enable GPIO concept

2013-01-14 Thread Kim, Milo 
 A Regulator can be enabled by external GPIO pin.
 This is configurable in the regulator_config.
 At this moment, this enable GPIO is owned by only one regulator device.
 In some devices, multiple regulators are enabled by shared one GPIO pin.
 This patch extends this limitation, enabling shared enable GPIO of regulators.

 New list for enable GPIO: 'regulator_ena_gpio_list'
   This manages enable GPIO list.

 New structure for supporting shared enable GPIO: 'regulator_enable_gpio'
   The enable count is used for balancing GPIO control count.
   The count is incremented when GPIO is enabled.
   On the other hand, it's decremented when GPIO is disabled.

 How it works
   If the GPIO is already used, skip requesting new GPIO usage.
   The GPIO is new one, request GPIO function and add it to the list of
   enable GPIO.
   This list is used for balancing enable GPIO count and pin control.

Signed-off-by: Milo(Woogyom) Kim milo@ti.com
---
 drivers/regulator/core.c |   49 +++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index de47880..1273090 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -51,6 +51,7 @@
 static DEFINE_MUTEX(regulator_list_mutex);
 static LIST_HEAD(regulator_list);
 static LIST_HEAD(regulator_map_list);
+static LIST_HEAD(regulator_ena_gpio_list);
 static bool has_full_constraints;
 static bool board_wants_dummy_regulator;
 
@@ -69,6 +70,18 @@ struct regulator_map {
 };
 
 /*
+ * struct regulator_enable_gpio
+ *
+ * Management for shared enable GPIO pin
+ */
+struct regulator_enable_gpio {
+   struct list_head list;
+   int gpio;
+   u32 enable_count;
+   struct regulator_dev *regulator;
+};
+
+/*
  * struct regulator
  *
  * One for each consumer device.
@@ -1456,6 +1469,38 @@ void devm_regulator_put(struct regulator *regulator)
 }
 EXPORT_SYMBOL_GPL(devm_regulator_put);
 
+/* 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, regulator_ena_gpio_list, list) {
+   if (pin-gpio == config-ena_gpio) {
+   rdev_info(rdev, GPIO %d is already used\n,
+   config-ena_gpio);
+   return 0;
+   }
+   }
+
+   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;
+
+   pin-gpio = config-ena_gpio;
+   pin-regulator = rdev;
+   list_add(pin-list, regulator_ena_gpio_list);
+
+   return 0;
+}
+
 static int _regulator_do_enable(struct regulator_dev *rdev)
 {
int ret, delay;
@@ -3420,9 +3465,7 @@ regulator_register(const struct regulator_desc 
*regulator_desc,
dev_set_drvdata(rdev-dev, rdev);
 
if (config-ena_gpio  gpio_is_valid(config-ena_gpio)) {
-   ret = gpio_request_one(config-ena_gpio,
-  GPIOF_DIR_OUT | config-ena_gpio_flags,
-  rdev_get_name(rdev));
+   ret = regulator_ena_gpio_request(rdev, config);
if (ret != 0) {
rdev_err(rdev, Failed to request enable GPIO%d: %d\n,
 config-ena_gpio, ret);
-- 
1.7.9.5


Best Regards,
Milo

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/