Request and deassert any (optional) reset gpio during probe in case it
has been left asserted by the boot firmware.
Note the reset line is not asserted to avoid reverting to the default
I2C address in case the firmware has configured an alternate address.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/mfd/qcom-pm8008.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index d53c987b0d49..d0f190c2ea2b 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>
@@ -158,6 +159,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
static int pm8008_probe(struct i2c_client *client)
{
struct regmap_irq_chip_data *irq_data;
+ struct gpio_desc *reset;
int rc;
struct device *dev;
struct regmap *regmap;
@@ -169,6 +171,10 @@ static int pm8008_probe(struct i2c_client *client)
i2c_set_clientdata(client, regmap);
+ reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(reset))
+ return PTR_ERR(reset);
+
if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
--
2.43.2
Mon, May 06, 2024 at 05:08:20PM +0200, Johan Hovold kirjoitti:
> Request and deassert any (optional) reset gpio during probe in case it
> has been left asserted by the boot firmware.
>
> Note the reset line is not asserted to avoid reverting to the default
> I2C address in case the firmware has configured an alternate address.
..
> + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(reset))
> + return PTR_ERR(reset);
Shouldn't you wait a bit to make chip settle down?
--
With Best Regards,
Andy Shevchenko
On Mon, May 06, 2024 at 09:57:07PM +0300, Andy Shevchenko wrote:
> Mon, May 06, 2024 at 05:08:20PM +0200, Johan Hovold kirjoitti:
> > Request and deassert any (optional) reset gpio during probe in case it
> > has been left asserted by the boot firmware.
> >
> > Note the reset line is not asserted to avoid reverting to the default
> > I2C address in case the firmware has configured an alternate address.
>
> ...
>
> > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > + if (IS_ERR(reset))
> > + return PTR_ERR(reset);
>
> Shouldn't you wait a bit to make chip settle down?
Yeah, probably. I actually asserted reset here for a while (e.g. to
reset the address), but didn't have to use a post-reset delay then.
I'll see if I can find someone with access to a datasheet or maybe add
some small delay here anyway.
Johan
On 06/05/2024 16:08, Johan Hovold wrote:
> Request and deassert any (optional) reset gpio during probe in case it
> has been left asserted by the boot firmware.
>
> Note the reset line is not asserted to avoid reverting to the default
> I2C address in case the firmware has configured an alternate address.
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/mfd/qcom-pm8008.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index d53c987b0d49..d0f190c2ea2b 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>
> @@ -158,6 +159,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> static int pm8008_probe(struct i2c_client *client)
> {
> struct regmap_irq_chip_data *irq_data;
> + struct gpio_desc *reset;
> int rc;
> struct device *dev;
> struct regmap *regmap;
> @@ -169,6 +171,10 @@ static int pm8008_probe(struct i2c_client *client)
>
> i2c_set_clientdata(client, regmap);
>
> + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(reset))
> + return PTR_ERR(reset);
> +
> if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
> IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
So not resetting is fine and I understand you want to retain the address
given by the firmware, I think that's the right thing to do.
You can add this now
Tested-by: Bryan O'Donoghue <[email protected]>
In addition to adding a small delay suggested by Andy - a few
microseconds pick a number, I think you should verify the chip is out of
reset as we would do with many other i2c devices.
A common pattern with i2c devices is take the device out of reset then
read back an identity register as a smoke test.
Take drivers/media/i2c/ov8856.c::ov8856_identify_module() as an example.
In this case, suggest reading REVID_PERPH_TYPE @ 0x104 and
REVID_PERPH_SUBTYPE @ 0x105
REVID_PERPH_TYPE @ 0x104 == 0x51 (PMIC)
REVID_PERPH_SUBYTE @ 0x105 == 0x2C (PM8008)
You can then add my
Reviewed-by: Bryan O'Donoghue <[email protected]>
---
bod
On Wed, May 08, 2024 at 05:12:51PM +0100, Bryan O'Donoghue wrote:
> On 06/05/2024 16:08, Johan Hovold wrote:
> > Request and deassert any (optional) reset gpio during probe in case it
> > has been left asserted by the boot firmware.
> >
> > Note the reset line is not asserted to avoid reverting to the default
> > I2C address in case the firmware has configured an alternate address.
> > @@ -169,6 +171,10 @@ static int pm8008_probe(struct i2c_client *client)
> >
> > i2c_set_clientdata(client, regmap);
> >
> > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > + if (IS_ERR(reset))
> > + return PTR_ERR(reset);
> > +
> > if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> > rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
> > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>
> So not resetting is fine and I understand you want to retain the address
> given by the firmware, I think that's the right thing to do.
> In addition to adding a small delay suggested by Andy - a few
> microseconds pick a number, I think you should verify the chip is out of
> reset as we would do with many other i2c devices.
> In this case, suggest reading REVID_PERPH_TYPE @ 0x104 and
> REVID_PERPH_SUBTYPE @ 0x105
>
> REVID_PERPH_TYPE @ 0x104 == 0x51 (PMIC)
> REVID_PERPH_SUBYTE @ 0x105 == 0x2C (PM8008)
I'll consider it for v2.
Johan
On Mon, May 6, 2024 at 5:10 PM Johan Hovold <[email protected]> wrote:
> Request and deassert any (optional) reset gpio during probe in case it
> has been left asserted by the boot firmware.
>
> Note the reset line is not asserted to avoid reverting to the default
> I2C address in case the firmware has configured an alternate address.
>
> Signed-off-by: Johan Hovold <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, May 09, 2024 at 11:31:22AM +0200, Johan Hovold wrote:
> On Wed, May 08, 2024 at 05:12:51PM +0100, Bryan O'Donoghue wrote:
> > On 06/05/2024 16:08, Johan Hovold wrote:
> > > Request and deassert any (optional) reset gpio during probe in case it
> > > has been left asserted by the boot firmware.
> > >
> > > Note the reset line is not asserted to avoid reverting to the default
> > > I2C address in case the firmware has configured an alternate address.
>
> > > @@ -169,6 +171,10 @@ static int pm8008_probe(struct i2c_client *client)
> > >
> > > i2c_set_clientdata(client, regmap);
> > >
> > > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > > + if (IS_ERR(reset))
> > > + return PTR_ERR(reset);
> > > +
> > > if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> > > rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
> > > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> >
> > So not resetting is fine and I understand you want to retain the address
> > given by the firmware, I think that's the right thing to do.
>
> > In addition to adding a small delay suggested by Andy - a few
> > microseconds pick a number, I think you should verify the chip is out of
> > reset as we would do with many other i2c devices.
>
> > In this case, suggest reading REVID_PERPH_TYPE @ 0x104 and
> > REVID_PERPH_SUBTYPE @ 0x105
> >
> > REVID_PERPH_TYPE @ 0x104 == 0x51 (PMIC)
> > REVID_PERPH_SUBYTE @ 0x105 == 0x2C (PM8008)
>
> I'll consider it for v2.
I decided to not add any revid checks for v2 as it's not strictly
needed. This is something can be added later when/if needed.
The irqchip registration will also fail if there's no from reply from
this address.
Johan
On 29/05/2024 17:17, Johan Hovold wrote:
> The irqchip registration will also fail if there's no from reply from
> this address.
That's acceptable too.
---
bod