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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 8 May 2024 17:12:51 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Johan Hovold <johan+linaro@...nel.org>, Lee Jones <lee@...nel.org>,
 Mark Brown <broonie@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
 Bjorn Andersson <andersson@...nel.org>
Cc: Konrad Dybcio <konrad.dybcio@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
 Das Srinagesh <quic_gurus@...cinc.com>,
 Satya Priya <quic_c_skakit@...cinc.com>, Stephen Boyd <swboyd@...omium.org>,
 linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 03/13] mfd: pm8008: deassert reset on probe

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@...nel.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@...aro.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@...aro.org>

---
bod