Add the reset-gpio toggling in the pm8008_probe() to bring
pm8008 chip out of reset instead of doing it in DT node using
"output-high" property.
Signed-off-by: Satya Priya <[email protected]>
---
Changes in V10:
- This has been split from [V9,3/6] as per comments here [1]
[1] https://patchwork.kernel.org/project/linux-arm-msm/patch/[email protected]/#24803409
drivers/mfd/qcom-pm8008.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index c472d7f..97a72da 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -4,6 +4,7 @@
*/
#include <linux/bitops.h>
+#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
@@ -57,6 +58,7 @@ enum {
struct pm8008_data {
struct device *dev;
struct regmap *regmap;
+ struct gpio_desc *reset_gpio;
int irq;
struct regmap_irq_chip_data *irq_data;
};
@@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client *client)
dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
}
+ chip->reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(chip->reset_gpio)) {
+ dev_err(chip->dev, "failed to acquire reset gpio\n");
+ return PTR_ERR(chip->reset_gpio);
+ }
+ gpiod_set_value(chip->reset_gpio, 1);
+
return devm_of_platform_populate(chip->dev);
}
--
2.7.4
Quoting Satya Priya (2022-04-14 05:30:13)
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index c472d7f..97a72da 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client *client)
> dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
> }
>
> + chip->reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(chip->reset_gpio)) {
> + dev_err(chip->dev, "failed to acquire reset gpio\n");
The API looks to print debug messages. This print doesn't look required.
> + return PTR_ERR(chip->reset_gpio);
> + }
> + gpiod_set_value(chip->reset_gpio, 1);
Does this do anything? Does this work just as well?
reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(reset_gpio))
return PTR_ERR(reset_gpio);
Note that there's no point to store the reset gpio in the structure if
it won't be used outside of probe. This should work fine? I used
GPIOD_OUT_LOW to indicate that the reset should be returned to this
function deasserted, i.e. taking the PMIC out of reset.
> +
> return devm_of_platform_populate(chip->dev);
On 4/15/2022 5:40 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-04-14 05:30:13)
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index c472d7f..97a72da 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client *client)
>> dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>> }
>>
>> + chip->reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
>> + if (IS_ERR(chip->reset_gpio)) {
>> + dev_err(chip->dev, "failed to acquire reset gpio\n");
> The API looks to print debug messages. This print doesn't look required.
Okay.
>> + return PTR_ERR(chip->reset_gpio);
>> + }
>> + gpiod_set_value(chip->reset_gpio, 1);
> Does this do anything? Does this work just as well?
>
> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(reset_gpio))
> return PTR_ERR(reset_gpio);
>
> Note that there's no point to store the reset gpio in the structure if
> it won't be used outside of probe.
Okay, I'll use a local variable.
> This should work fine? I used
> GPIOD_OUT_LOW to indicate that the reset should be returned to this
> function deasserted, i.e. taking the PMIC out of reset.
I'll try this out.
On 4/27/2022 10:58 AM, Satya Priya Kakitapalli (Temp) wrote:
>
> On 4/18/2022 10:34 AM, Satya Priya Kakitapalli (Temp) wrote:
>>
>> On 4/15/2022 5:40 AM, Stephen Boyd wrote:
>>> Quoting Satya Priya (2022-04-14 05:30:13)
>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>> index c472d7f..97a72da 100644
>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>> @@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client
>>>> *client)
>>>> dev_err(chip->dev, "Failed to probe irq
>>>> periphs: %d\n", rc);
>>>> }
>>>>
>>>> + chip->reset_gpio = devm_gpiod_get(chip->dev, "reset",
>>>> GPIOD_OUT_HIGH);
>>>> + if (IS_ERR(chip->reset_gpio)) {
>>>> + dev_err(chip->dev, "failed to acquire reset gpio\n");
>>> The API looks to print debug messages. This print doesn't look
>>> required.
>>
>>
>> Okay.
>>
>>
>>>> + return PTR_ERR(chip->reset_gpio);
>>>> + }
>>>> + gpiod_set_value(chip->reset_gpio, 1);
>>> Does this do anything? Does this work just as well?
>>>
>>> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
>>> if (IS_ERR(reset_gpio))
>>> return PTR_ERR(reset_gpio);
>>>
>
> This is not working as expected. We need to add
> "gpiod_set_value(chip->reset_gpio, 1);" to actually toggle the line.
>
I checked again and it is working after using GPIOD_OUT_HIGH instead of LOW.
reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(reset_gpio))
return PTR_ERR(reset_gpio);
>
>>> Note that there's no point to store the reset gpio in the structure if
>>> it won't be used outside of probe.
>>
>>
>> Okay, I'll use a local variable.
>>
>>
>>> This should work fine? I used
>>> GPIOD_OUT_LOW to indicate that the reset should be returned to this
>>> function deasserted, i.e. taking the PMIC out of reset.
>>
>>
>> I'll try this out.
>>
>>
Quoting Satya Priya Kakitapalli (Temp) (2022-04-26 23:03:08)
>
> On 4/27/2022 10:58 AM, Satya Priya Kakitapalli (Temp) wrote:
> >
> > On 4/18/2022 10:34 AM, Satya Priya Kakitapalli (Temp) wrote:
> >>
> >> On 4/15/2022 5:40 AM, Stephen Boyd wrote:
> >>> Quoting Satya Priya (2022-04-14 05:30:13)
> >>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> >>>> index c472d7f..97a72da 100644
> >>>> --- a/drivers/mfd/qcom-pm8008.c
> >>>> +++ b/drivers/mfd/qcom-pm8008.c
> >>
> >>>> + return PTR_ERR(chip->reset_gpio);
> >>>> + }
> >>>> + gpiod_set_value(chip->reset_gpio, 1);
> >>> Does this do anything? Does this work just as well?
> >>>
> >>> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
> >>> if (IS_ERR(reset_gpio))
> >>> return PTR_ERR(reset_gpio);
> >>>
> >
> > This is not working as expected. We need to add
> > "gpiod_set_value(chip->reset_gpio, 1);" to actually toggle the line.
> >
>
> I checked again and it is working after using GPIOD_OUT_HIGH instead of LOW.
>
> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(reset_gpio))
> return PTR_ERR(reset_gpio);
>
What do you have in DT for the 'reset-gpios' property? GPIOD_OUT_HIGH
means the reset line is asserted, which presumably you don't want to be
the case because you typically deassert a reset to "take it out of
reset". Using GPIOD_OUT_HIGH probably works because DT has the wrong
GPIO flag, i.e. GPIO_ACTIVE_HIGH, for an active low reset, or vice
versa.
On 4/27/2022 12:00 PM, Stephen Boyd wrote:
> Quoting Satya Priya Kakitapalli (Temp) (2022-04-26 23:03:08)
>> On 4/27/2022 10:58 AM, Satya Priya Kakitapalli (Temp) wrote:
>>> On 4/18/2022 10:34 AM, Satya Priya Kakitapalli (Temp) wrote:
>>>> On 4/15/2022 5:40 AM, Stephen Boyd wrote:
>>>>> Quoting Satya Priya (2022-04-14 05:30:13)
>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>> index c472d7f..97a72da 100644
>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>> + return PTR_ERR(chip->reset_gpio);
>>>>>> + }
>>>>>> + gpiod_set_value(chip->reset_gpio, 1);
>>>>> Does this do anything? Does this work just as well?
>>>>>
>>>>> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
>>>>> if (IS_ERR(reset_gpio))
>>>>> return PTR_ERR(reset_gpio);
>>>>>
>>> This is not working as expected. We need to add
>>> "gpiod_set_value(chip->reset_gpio, 1);" to actually toggle the line.
>>>
>> I checked again and it is working after using GPIOD_OUT_HIGH instead of LOW.
>>
>> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
>> if (IS_ERR(reset_gpio))
>> return PTR_ERR(reset_gpio);
>>
> What do you have in DT for the 'reset-gpios' property? GPIOD_OUT_HIGH
> means the reset line is asserted, which presumably you don't want to be
> the case because you typically deassert a reset to "take it out of
> reset". Using GPIOD_OUT_HIGH probably works because DT has the wrong
> GPIO flag, i.e. GPIO_ACTIVE_HIGH, for an active low reset, or vice
> versa.
Yeah, I had GPIOD_OUT_HIGH in DT, now I changed and it is working fine
with GPIOD_OUT_LOW.