From: Roger Quadros <[email protected]>
Now fixed regulators that have their enable pin connected to a GPIO line
can use the fixed regulator driver. The GPIO number and polarity
information is passed through platform data. GPIO enable control
is acheived using gpiolib.
Signed-off-by: Roger Quadros <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
Reviewed-by: Philipp Zabel <[email protected]>
---
drivers/regulator/fixed.c | 66 +++++++++++++++++++++++++++++++++++++-
include/linux/regulator/fixed.h | 17 ++++++++++
2 files changed, 81 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index cdc674f..bcebb0c 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -20,20 +20,50 @@
#include <linux/platform_device.h>
#include <linux/regulator/driver.h>
#include <linux/regulator/fixed.h>
+#include <linux/gpio.h>
struct fixed_voltage_data {
struct regulator_desc desc;
struct regulator_dev *dev;
int microvolts;
+ int gpio;
+ int enable_high;
+ int is_enabled;
};
static int fixed_voltage_is_enabled(struct regulator_dev *dev)
{
- return 1;
+ struct fixed_voltage_data *data = rdev_get_drvdata(dev);
+
+ if (data->gpio)
+ return data->is_enabled;
+ else
+ return 1;
}
static int fixed_voltage_enable(struct regulator_dev *dev)
{
+ struct fixed_voltage_data *data = rdev_get_drvdata(dev);
+
+ if (data->gpio) {
+ gpio_set_value(data->gpio,
+ data->enable_high ? 1 : 0);
+ data->is_enabled = 1;
+ }
+
+ return 0;
+}
+
+static int fixed_voltage_disable(struct regulator_dev *dev)
+{
+ struct fixed_voltage_data *data = rdev_get_drvdata(dev);
+
+ if (data->gpio) {
+ gpio_set_value(data->gpio,
+ data->enable_high ? 0 : 1);
+ data->is_enabled = 0;
+ }
+
return 0;
}
@@ -58,6 +88,7 @@ static int fixed_voltage_list_voltage(struct regulator_dev *dev,
static struct regulator_ops fixed_voltage_ops = {
.is_enabled = fixed_voltage_is_enabled,
.enable = fixed_voltage_enable,
+ .disable = fixed_voltage_disable,
.get_voltage = fixed_voltage_get_voltage,
.list_voltage = fixed_voltage_list_voltage,
};
@@ -85,12 +116,37 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
drvdata->desc.n_voltages = 1;
drvdata->microvolts = config->microvolts;
+ if (config->gpio) {
+ drvdata->gpio = config->gpio;
+ drvdata->enable_high = config->enable_high;
+
+ ret = gpio_request(config->gpio,
+ config->supply_name);
+ if (ret) {
+ dev_err(&pdev->dev, "Could not obtain regulator"
+ "enable GPIO %d: %d\n", config->gpio, ret);
+ goto err_name;
+ }
+
+ /* set output direction without changing state
+ * to prevent glitch
+ */
+ ret = gpio_get_value(config->gpio);
+ ret = gpio_direction_output(config->gpio, ret);
+ if (ret) {
+ dev_err(&pdev->dev, "Could not configure "
+ "enable GPIO %d direction: %d\n",
+ config->gpio, ret);
+ goto err_gpio;
+ }
+ } else
+ dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");
drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
config->init_data, drvdata);
if (IS_ERR(drvdata->dev)) {
ret = PTR_ERR(drvdata->dev);
- goto err_name;
+ goto err_gpio;
}
platform_set_drvdata(pdev, drvdata);
@@ -100,6 +156,9 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
return 0;
+err_gpio:
+ if (config->gpio)
+ gpio_free(config->gpio);
err_name:
kfree(drvdata->desc.name);
err:
@@ -115,6 +174,9 @@ static int regulator_fixed_voltage_remove(struct platform_device *pdev)
kfree(drvdata->desc.name);
kfree(drvdata);
+ if (drvdata->gpio)
+ gpio_free(drvdata->gpio);
+
return 0;
}
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index 91b4da3..b40e06b 100644
--- a/include/linux/regulator/fixed.h
+++ b/include/linux/regulator/fixed.h
@@ -16,9 +16,26 @@
struct regulator_init_data;
+/**
+ * struct fixed_voltage_config - fixed_voltage_config structure
+ * @supply_name: Name of the regulator supply
+ * @microvolts: Output voltage of regulator
+ * @use_gpio_control: Use GPIO enable control
+ * @gpio: GPIO to use for enable control
+ * @enable_high: Polarity of enable GPIO
+ * 1 = Active high, 0 = Active low
+ * @init_data: regulator_init_data
+ *
+ * This structure contains fixed voltage regulator configuration
+ * information that must be passed by platform code to the fixed
+ * voltage regulator driver.
+ */
struct fixed_voltage_config {
const char *supply_name;
int microvolts;
+ int use_gpio_control;
+ int gpio;
+ int enable_high;
struct regulator_init_data *init_data;
};
--
1.6.0.4
On Fri, Jul 31, 2009 at 06:19:49PM +0200, ext Roger Quadros wrote:
> From: Roger Quadros <[email protected]>
>
> Now fixed regulators that have their enable pin connected to a GPIO line
> can use the fixed regulator driver. The GPIO number and polarity
> information is passed through platform data. GPIO enable control
> is acheived using gpiolib.
little typo here.
> Signed-off-by: Roger Quadros <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
> Reviewed-by: Philipp Zabel <[email protected]>
>
> ---
> drivers/regulator/fixed.c | 66 +++++++++++++++++++++++++++++++++++++-
> include/linux/regulator/fixed.h | 17 ++++++++++
> 2 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index cdc674f..bcebb0c 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -20,20 +20,50 @@
> #include <linux/platform_device.h>
> #include <linux/regulator/driver.h>
> #include <linux/regulator/fixed.h>
> +#include <linux/gpio.h>
>
> struct fixed_voltage_data {
> struct regulator_desc desc;
> struct regulator_dev *dev;
> int microvolts;
> + int gpio;
> + int enable_high;
> + int is_enabled;
how about making these two:
unsigned enable_high:1;
unsigned is_enabled:1;
> };
>
> static int fixed_voltage_is_enabled(struct regulator_dev *dev)
> {
> - return 1;
> + struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> +
> + if (data->gpio)
> + return data->is_enabled;
> + else
> + return 1;
> }
>
> static int fixed_voltage_enable(struct regulator_dev *dev)
> {
> + struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> +
> + if (data->gpio) {
> + gpio_set_value(data->gpio,
> + data->enable_high ? 1 : 0);
gpio_set_value(data->gpio, data->enable_high);
should be enough, no ?
> + data->is_enabled = 1;
> + }
> +
> + return 0;
> +}
> +
> +static int fixed_voltage_disable(struct regulator_dev *dev)
> +{
> + struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> +
> + if (data->gpio) {
> + gpio_set_value(data->gpio,
> + data->enable_high ? 0 : 1);
gpio_set_value(data->gpio, !data->enable_high); ??
> @@ -85,12 +116,37 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
> drvdata->desc.n_voltages = 1;
>
> drvdata->microvolts = config->microvolts;
> + if (config->gpio) {
gpio_is_valid() since 0 is a valid gpio number.
> + drvdata->gpio = config->gpio;
> + drvdata->enable_high = config->enable_high;
> +
> + ret = gpio_request(config->gpio,
> + config->supply_name);
> + if (ret) {
> + dev_err(&pdev->dev, "Could not obtain regulator"
> + "enable GPIO %d: %d\n", config->gpio, ret);
> + goto err_name;
> + }
> +
> + /* set output direction without changing state
> + * to prevent glitch
> + */
> + ret = gpio_get_value(config->gpio);
and if this fails ??
> + ret = gpio_direction_output(config->gpio, ret);
you continue anyways...
> + if (ret) {
> + dev_err(&pdev->dev, "Could not configure "
> + "enable GPIO %d direction: %d\n",
> + config->gpio, ret);
> + goto err_gpio;
> + }
> + } else
this should have brackets.
> + dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");
>
> drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
> config->init_data, drvdata);
> if (IS_ERR(drvdata->dev)) {
> ret = PTR_ERR(drvdata->dev);
> - goto err_name;
> + goto err_gpio;
> }
>
> platform_set_drvdata(pdev, drvdata);
> @@ -100,6 +156,9 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
>
> return 0;
>
> +err_gpio:
> + if (config->gpio)
0 is a valid gpio number, you should initialize everyone using this to
-EINVAL, for example, and use gpio_is_valid()
> + gpio_free(config->gpio);
> err_name:
> kfree(drvdata->desc.name);
> err:
> @@ -115,6 +174,9 @@ static int regulator_fixed_voltage_remove(struct platform_device *pdev)
> kfree(drvdata->desc.name);
> kfree(drvdata);
>
> + if (drvdata->gpio)
likewise, use gpio_is_valid()
> + gpio_free(drvdata->gpio);
> +
> return 0;
> }
>
> diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
> index 91b4da3..b40e06b 100644
> --- a/include/linux/regulator/fixed.h
> +++ b/include/linux/regulator/fixed.h
> @@ -16,9 +16,26 @@
>
> struct regulator_init_data;
>
> +/**
> + * struct fixed_voltage_config - fixed_voltage_config structure
> + * @supply_name: Name of the regulator supply
> + * @microvolts: Output voltage of regulator
> + * @use_gpio_control: Use GPIO enable control
> + * @gpio: GPIO to use for enable control
> + * @enable_high: Polarity of enable GPIO
> + * 1 = Active high, 0 = Active low
> + * @init_data: regulator_init_data
> + *
> + * This structure contains fixed voltage regulator configuration
> + * information that must be passed by platform code to the fixed
> + * voltage regulator driver.
> + */
> struct fixed_voltage_config {
> const char *supply_name;
> int microvolts;
> + int use_gpio_control;
> + int gpio;
put a comment for this field asking people to intialized to -EINVAL if
they're not gonna use it.
> + int enable_high;
unsigned enable_high:1;
--
balbi
On Fri, Jul 31, 2009 at 6:19 PM, Roger Quadros<[email protected]> wrote:
> From: Roger Quadros <[email protected]>
>
> Now fixed regulators that have their enable pin connected to a GPIO line
> can use the fixed regulator driver. The GPIO number and polarity
> information is passed through platform data. GPIO enable control
> is acheived using gpiolib.
>
> Signed-off-by: Roger Quadros <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
> Reviewed-by: Philipp Zabel <[email protected]>
>
> ---
> ?drivers/regulator/fixed.c ? ? ? | ? 66 +++++++++++++++++++++++++++++++++++++-
> ?include/linux/regulator/fixed.h | ? 17 ++++++++++
> ?2 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index cdc674f..bcebb0c 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -20,20 +20,50 @@
> ?#include <linux/platform_device.h>
> ?#include <linux/regulator/driver.h>
> ?#include <linux/regulator/fixed.h>
> +#include <linux/gpio.h>
>
> ?struct fixed_voltage_data {
> ? ? ? ?struct regulator_desc desc;
> ? ? ? ?struct regulator_dev *dev;
> ? ? ? ?int microvolts;
> + ? ? ? int gpio;
> + ? ? ? int enable_high;
> + ? ? ? int is_enabled;
> ?};
>
> ?static int fixed_voltage_is_enabled(struct regulator_dev *dev)
> ?{
> - ? ? ? return 1;
> + ? ? ? struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> +
> + ? ? ? if (data->gpio)
if (gpio_is_valid(data->gpio))
> + ? ? ? ? ? ? ? return data->is_enabled;
> + ? ? ? else
> + ? ? ? ? ? ? ? return 1;
> ?}
Or this function could just unconditionally return data->is_enabled if
you set it to 1 in _probe for gpio-less regulators.
>
> ?static int fixed_voltage_enable(struct regulator_dev *dev)
> ?{
> + ? ? ? struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> +
> + ? ? ? if (data->gpio) {
if (gpio_is_valid(data->gpio)) {
> + ? ? ? ? ? ? ? gpio_set_value(data->gpio,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? data->enable_high ? 1 : 0);
> + ? ? ? ? ? ? ? data->is_enabled = 1;
> + ? ? ? }
> +
> + ? ? ? return 0;
> +}
> +
> +static int fixed_voltage_disable(struct regulator_dev *dev)
> +{
> + ? ? ? struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> +
> + ? ? ? if (data->gpio) {
> + ? ? ? ? ? ? ? gpio_set_value(data->gpio,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? data->enable_high ? 0 : 1);
> + ? ? ? ? ? ? ? data->is_enabled = 0;
> + ? ? ? }
> +
> ? ? ? ?return 0;
> ?}
>
> @@ -58,6 +88,7 @@ static int fixed_voltage_list_voltage(struct regulator_dev *dev,
> ?static struct regulator_ops fixed_voltage_ops = {
> ? ? ? ?.is_enabled = fixed_voltage_is_enabled,
> ? ? ? ?.enable = fixed_voltage_enable,
> + ? ? ? .disable = fixed_voltage_disable,
> ? ? ? ?.get_voltage = fixed_voltage_get_voltage,
> ? ? ? ?.list_voltage = fixed_voltage_list_voltage,
> ?};
> @@ -85,12 +116,37 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
> ? ? ? ?drvdata->desc.n_voltages = 1;
>
> ? ? ? ?drvdata->microvolts = config->microvolts;
> + ? ? ? if (config->gpio) {
That should be more like
if (gpio_is_valid(config->gpio) && config->gpio) {
People are going to set config->gpio to negative values to disable the gpio.
(And I think eventually GPIO0 should be allowed here.)
> + ? ? ? ? ? ? ? drvdata->gpio = config->gpio;
> + ? ? ? ? ? ? ? drvdata->enable_high = config->enable_high;
> +
> + ? ? ? ? ? ? ? ret = gpio_request(config->gpio,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? config->supply_name);
> + ? ? ? ? ? ? ? if (ret) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&pdev->dev, "Could not obtain regulator"
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "enable GPIO %d: %d\n", config->gpio, ret);
> + ? ? ? ? ? ? ? ? ? ? ? goto err_name;
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? /* set output direction without changing state
> + ? ? ? ? ? ? ? ?* to prevent glitch
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? ret = gpio_get_value(config->gpio);
> + ? ? ? ? ? ? ? ret = gpio_direction_output(config->gpio, ret);
> + ? ? ? ? ? ? ? if (ret) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&pdev->dev, "Could not configure "
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "enable GPIO %d direction: %d\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? config->gpio, ret);
> + ? ? ? ? ? ? ? ? ? ? ? goto err_gpio;
> + ? ? ? ? ? ? ? }
> + ? ? ? } else
> + ? ? ? ? ? ? ? dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");
>
> ? ? ? ?drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?config->init_data, drvdata);
> ? ? ? ?if (IS_ERR(drvdata->dev)) {
> ? ? ? ? ? ? ? ?ret = PTR_ERR(drvdata->dev);
> - ? ? ? ? ? ? ? goto err_name;
> + ? ? ? ? ? ? ? goto err_gpio;
> ? ? ? ?}
>
> ? ? ? ?platform_set_drvdata(pdev, drvdata);
> @@ -100,6 +156,9 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
>
> ? ? ? ?return 0;
>
> +err_gpio:
> + ? ? ? if (config->gpio)
> + ? ? ? ? ? ? ? gpio_free(config->gpio);
gpio_is_valid(config->gpio)
> ?err_name:
> ? ? ? ?kfree(drvdata->desc.name);
> ?err:
> @@ -115,6 +174,9 @@ static int regulator_fixed_voltage_remove(struct platform_device *pdev)
> ? ? ? ?kfree(drvdata->desc.name);
> ? ? ? ?kfree(drvdata);
>
> + ? ? ? if (drvdata->gpio)
> + ? ? ? ? ? ? ? gpio_free(drvdata->gpio);
> +
dito
> ? ? ? ?return 0;
> ?}
>
> diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
> index 91b4da3..b40e06b 100644
> --- a/include/linux/regulator/fixed.h
> +++ b/include/linux/regulator/fixed.h
> @@ -16,9 +16,26 @@
>
> ?struct regulator_init_data;
>
> +/**
> + * struct fixed_voltage_config - fixed_voltage_config structure
> + * @supply_name: ? ? ? Name of the regulator supply
> + * @microvolts: ? ? ? ? ? ? ? ?Output voltage of regulator
> + * @use_gpio_control: ?Use GPIO enable control
> + * @gpio: ? ? ? ? ? ? ?GPIO to use for enable control
> + * @enable_high: ? ? ? Polarity of enable GPIO
> + * ? ? ? ? ? ? ? ? ? ? 1 = Active high, 0 = Active low
> + * @init_data: ? ? ? ? regulator_init_data
> + *
> + * This structure contains fixed voltage regulator configuration
> + * information that must be passed by platform code to the fixed
> + * voltage regulator driver.
> + */
> ?struct fixed_voltage_config {
> ? ? ? ?const char *supply_name;
> ? ? ? ?int microvolts;
> + ? ? ? int use_gpio_control;
You wanted to drop that, it's not used anymore.
> + ? ? ? int gpio;
> + ? ? ? int enable_high;
> ? ? ? ?struct regulator_init_data *init_data;
> ?};
>
> --
> 1.6.0.4
>
>
regards
Philipp
On Fri, Jul 31, 2009 at 07:19:49PM +0300, Roger Quadros wrote:
> Signed-off-by: Roger Quadros <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
Err... no, it isn't. I reviewed a previous version of the patch but
there were some issues that needed fixing. I've not reviewed this
version.
> static int fixed_voltage_is_enabled(struct regulator_dev *dev)
> {
> - return 1;
> + struct fixed_voltage_data *data = rdev_get_drvdata(dev);
> +
> + if (data->gpio)
> + return data->is_enabled;
Please use gpio_is_valid() as Phillip previously suggested. For this
particular use you could simplify the code marginally by just setting
is_enabled at probe() time if there's no GPIO, not that it really
matters.
> + if (data->gpio) {
> + gpio_set_value(data->gpio,
> + data->enable_high ? 1 : 0);
_cansleep(), please, as previously requested.
> + ret = gpio_get_value(config->gpio);
Can this fail? It returns an int...
> + ret = gpio_direction_output(config->gpio, ret);
> + if (ret) {
> + dev_err(&pdev->dev, "Could not configure "
> + "enable GPIO %d direction: %d\n",
> + config->gpio, ret);
> + goto err_gpio;
As previously requested please don't split for log messages text over
lines unless you have to. It makes it easier to find the message from a
kernel log - grep for 'configure enable GPIO' won't match this source
file the way you've written it above.
> + }
> + } else
> + dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");
Probably better to soldier on and use the GPIO here but either way is
fine; the warning is for the benefit of existing out of tree users to
help them transition gracefully.
On Fri, Jul 31, 2009 at 7:20 PM, Mark
Brown<[email protected]> wrote:
> On Fri, Jul 31, 2009 at 07:19:49PM +0300, Roger Quadros wrote:
>
>> Signed-off-by: Roger Quadros <[email protected]>
>> Reviewed-by: Mark Brown <[email protected]>
>
> Err... ?no, it isn't. ?I reviewed a previous version of the patch but
> there were some issues that needed fixing. ?I've not reviewed this
> version.
>
>> ?static int fixed_voltage_is_enabled(struct regulator_dev *dev)
>> ?{
>> - ? ? return 1;
>> + ? ? struct fixed_voltage_data *data = rdev_get_drvdata(dev);
>> +
>> + ? ? if (data->gpio)
>> + ? ? ? ? ? ? return data->is_enabled;
>
> Please use gpio_is_valid() as Phillip previously suggested. ?For this
> particular use you could simplify the code marginally by just setting
> is_enabled at probe() time if there's no GPIO, not that it really
> matters.
>
>> + ? ? ? if (data->gpio) {
>> + ? ? ? ? ? ? ? gpio_set_value(data->gpio,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? data->enable_high ? 1 : 0);
>
> _cansleep(), please, as previously requested.
>
>> + ? ? ? ? ? ? ret = gpio_get_value(config->gpio);
>
> Can this fail? ?It returns an int...
No, gpio_get_value(_cansleep) return either zero or non-zero.
I think some I2C gpio chips just return zero when the read fails.
Also that's the return value if a gpiochip doesn't define the .get method.
Only problem: drivers/gpio/gpiolib.c contains this piece of wisdom:
/* Drivers MUST set GPIO direction before making get/set calls. In
* some cases this is done in early boot, before IRQs are enabled.
[...]
*/
>> + ? ? ? ? ? ? ret = gpio_direction_output(config->gpio, ret);
>> + ? ? ? ? ? ? if (ret) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(&pdev->dev, "Could not configure "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? "enable GPIO %d direction: %d\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? config->gpio, ret);
>> + ? ? ? ? ? ? ? ? ? ? goto err_gpio;
>
> As previously requested please don't split for log messages text over
> lines unless you have to. ?It makes it easier to find the message from a
> kernel log - grep for 'configure enable GPIO' won't match this source
> file the way you've written it above.
>
>> + ? ? ? ? ? ? }
>> + ? ? } else
>> + ? ? ? ? ? ? dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");
>
> Probably better to soldier on and use the GPIO here but either way is
> fine; the warning is for the benefit of existing out of tree users to
> help them transition gracefully.
>
regards
Philipp
On Fri, Jul 31, 2009 at 8:38 PM, pHilipp Zabel<[email protected]> wrote:
> On Fri, Jul 31, 2009 at 7:20 PM, Mark
> Brown<[email protected]> wrote:
>> On Fri, Jul 31, 2009 at 07:19:49PM +0300, Roger Quadros wrote:
>>
>>
>>> + ? ? ? ? ? ? ret = gpio_get_value(config->gpio);
>>
>> Can this fail? ?It returns an int...
>
> No, gpio_get_value(_cansleep) return either zero or non-zero.
> I think some I2C gpio chips just return zero when the read fails.
> Also that's the return value if a gpiochip doesn't define the .get method.
>
> Only problem: drivers/gpio/gpiolib.c contains this piece of wisdom:
>
> /* Drivers MUST set GPIO direction before making get/set calls. ?In
> ?* some cases this is done in early boot, before IRQs are enabled.
> ?[...]
> ?*/
>
Not having gpio_get_direction() in gpiolib is a limitation I think.
Can we just do the following for now ? (i.e. disable regulator by default)
Will there be any glitch? If yes, then how?
ret = gpio_request(config->gpio, config->supply_name);
...
- ret = gpio_get_value(config->gpio);
- ret = gpio_direction_output(config->gpio, ret);
+ ret = gpio_direction_output(config->gpio, !config->enable_high);
if (ret) {
dev_err(&pdev->dev, "Could not configure
enable GPIO %d direction: %d\n",
config->gpio, ret);
goto err_gpio;
}
On Fri, Jul 31, 2009 at 11:32:32PM +0300, Roger Quadros wrote:
> Not having gpio_get_direction() in gpiolib is a limitation I think.
> Can we just do the following for now ? (i.e. disable regulator by default)
> Will there be any glitch? If yes, then how?
If you're going to do this then like I say you should make the default
state controllable by platform data. If we unconditionally disable the
regulator then if, for example, the regulator is controlling the display
backlight and it's on when Linux get controls then the user will see the
backlight blink off then on during startup.