Re: [PATCH 03/13] mfd: pm8008: deassert reset on probe

Bryan O'Donoghue
2024-05-08T16:12:54Z

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 <johan+linaro@kernel.org>
> ---
> 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 <bryan.odonoghue@linaro.org>

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 <bryan.odonoghue@linaro.org>

---
bod