* [PATCH V15 1/9] dt-bindings: mfd: pm8008: Add reset-gpios
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-16 20:58 ` Lee Jones
2022-06-14 9:48 ` [PATCH V15 2/9] dt-bindings: mfd: pm8008: Change the address cells Satya Priya
` (8 subsequent siblings)
9 siblings, 1 reply; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Add reset-gpios property for pm8008.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in V15:
- None.
Changes in V14:
- None.
Changes in V13:
- None.
Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index ec3138c..a89649c 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -44,6 +44,9 @@ properties:
"#size-cells":
const: 0
+ reset-gpios:
+ maxItems: 1
+
patternProperties:
"^gpio@[0-9a-f]+$":
type: object
@@ -92,6 +95,7 @@ required:
- "#address-cells"
- "#size-cells"
- "#interrupt-cells"
+ - reset-gpios
additionalProperties: false
@@ -99,6 +103,7 @@ examples:
- |
#include <dt-bindings/mfd/qcom-pm8008.h>
#include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
qupv3_se13_i2c {
#address-cells = <1>;
#size-cells = <0>;
@@ -113,6 +118,8 @@ examples:
interrupt-parent = <&tlmm>;
interrupts = <32 IRQ_TYPE_EDGE_RISING>;
+ reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_LOW>;
+
pm8008_gpios: gpio@c000 {
compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
reg = <0xc000>;
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 1/9] dt-bindings: mfd: pm8008: Add reset-gpios
2022-06-14 9:48 ` [PATCH V15 1/9] dt-bindings: mfd: pm8008: Add reset-gpios Satya Priya
@ 2022-06-16 20:58 ` Lee Jones
0 siblings, 0 replies; 54+ messages in thread
From: Lee Jones @ 2022-06-16 20:58 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Tue, 14 Jun 2022, Satya Priya wrote:
> Add reset-gpios property for pm8008.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes in V15:
> - None.
>
> Changes in V14:
> - None.
>
> Changes in V13:
> - None.
>
> Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 7 +++++++
> 1 file changed, 7 insertions(+)
For my own reference (apply this as-is to your sign-off block):
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH V15 2/9] dt-bindings: mfd: pm8008: Change the address cells
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2022-06-14 9:48 ` [PATCH V15 1/9] dt-bindings: mfd: pm8008: Add reset-gpios Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-16 20:59 ` Lee Jones
2022-06-14 9:48 ` [PATCH V15 3/9] dt-bindings: mfd: pm8008: Add regulators for pm8008 Satya Priya
` (7 subsequent siblings)
9 siblings, 1 reply; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Change the address cells as '2' so that the first cell
describes the i2c address offset of the clients.
This helps us to define the child nodes of all
clients under the same parent mfd node, instead of
adding separate mfd DT nodes.
Change the gpios reg value accordingly.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in V15:
- None.
Changes in V14:
- None.
Changes in V13:
- Fixed nit.
Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index a89649c..a54d1ce0 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -39,7 +39,7 @@ properties:
interrupt-controller: true
"#address-cells":
- const: 1
+ const: 2
"#size-cells":
const: 0
@@ -48,7 +48,7 @@ properties:
maxItems: 1
patternProperties:
- "^gpio@[0-9a-f]+$":
+ "^gpio@0,[0-9a-f]+$":
type: object
description: |
@@ -61,7 +61,7 @@ patternProperties:
- const: qcom,spmi-gpio
reg:
- description: Peripheral address of one of the two GPIO peripherals.
+ description: Peripheral offset and address of one of the two GPIO peripherals.
maxItems: 1
gpio-controller: true
@@ -110,7 +110,7 @@ examples:
pm8008i@8 {
compatible = "qcom,pm8008";
reg = <0x8>;
- #address-cells = <1>;
+ #address-cells = <2>;
#size-cells = <0>;
interrupt-controller;
#interrupt-cells = <2>;
@@ -120,9 +120,9 @@ examples:
reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_LOW>;
- pm8008_gpios: gpio@c000 {
+ pm8008_gpios: gpio@0,c000 {
compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
- reg = <0xc000>;
+ reg = <0x0 0xc000>;
gpio-controller;
gpio-ranges = <&pm8008_gpios 0 0 2>;
#gpio-cells = <2>;
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 2/9] dt-bindings: mfd: pm8008: Change the address cells
2022-06-14 9:48 ` [PATCH V15 2/9] dt-bindings: mfd: pm8008: Change the address cells Satya Priya
@ 2022-06-16 20:59 ` Lee Jones
2022-06-17 16:34 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-16 20:59 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Tue, 14 Jun 2022, Satya Priya wrote:
> Change the address cells as '2' so that the first cell
> describes the i2c address offset of the clients.
> This helps us to define the child nodes of all
> clients under the same parent mfd node, instead of
> adding separate mfd DT nodes.
>
> Change the gpios reg value accordingly.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes in V15:
> - None.
>
> Changes in V14:
> - None.
>
> Changes in V13:
> - Fixed nit.
>
> Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
Applied, thanks.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 2/9] dt-bindings: mfd: pm8008: Change the address cells
2022-06-16 20:59 ` Lee Jones
@ 2022-06-17 16:34 ` Lee Jones
0 siblings, 0 replies; 54+ messages in thread
From: Lee Jones @ 2022-06-17 16:34 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Thu, 16 Jun 2022, Lee Jones wrote:
> On Tue, 14 Jun 2022, Satya Priya wrote:
>
> > Change the address cells as '2' so that the first cell
> > describes the i2c address offset of the clients.
> > This helps us to define the child nodes of all
> > clients under the same parent mfd node, instead of
> > adding separate mfd DT nodes.
> >
> > Change the gpios reg value accordingly.
> >
> > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> > Changes in V15:
> > - None.
> >
> > Changes in V14:
> > - None.
> >
> > Changes in V13:
> > - Fixed nit.
> >
> > Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
>
> Applied, thanks.
Sorry, wrong key-combo:
For my own reference (apply this as-is to your sign-off block):
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH V15 3/9] dt-bindings: mfd: pm8008: Add regulators for pm8008
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2022-06-14 9:48 ` [PATCH V15 1/9] dt-bindings: mfd: pm8008: Add reset-gpios Satya Priya
2022-06-14 9:48 ` [PATCH V15 2/9] dt-bindings: mfd: pm8008: Change the address cells Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-16 20:58 ` Lee Jones
2022-06-14 9:48 ` [PATCH V15 4/9] mfd: pm8008: Add reset-gpios Satya Priya
` (6 subsequent siblings)
9 siblings, 1 reply; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Add regulators and their parent supplies along with example.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in V15:
- None.
Changes in V14:
- None.
Changes in V13:
- None.
.../devicetree/bindings/mfd/qcom,pm8008.yaml | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index a54d1ce0..fd3c51e 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -47,6 +47,21 @@ properties:
reset-gpios:
maxItems: 1
+ vdd_l1_l2-supply:
+ description: Input supply phandle of ldo1 and ldo2 regulators.
+
+ vdd_l3_l4-supply:
+ description: Input supply phandle of ldo3 and ldo4 regulators.
+
+ vdd_l5-supply:
+ description: Input supply phandle of ldo5 regulator.
+
+ vdd_l6-supply:
+ description: Input supply phandle of ldo6 regulator.
+
+ vdd_l7-supply:
+ description: Input supply phandle of ldo7 regulator.
+
patternProperties:
"^gpio@0,[0-9a-f]+$":
type: object
@@ -88,6 +103,27 @@ patternProperties:
additionalProperties: false
+ "^ldo[1-7]@[1],[0-9a-f]+$":
+ type: object
+
+ $ref: "/schemas/regulator/regulator.yaml#"
+
+ description: PM8008 regulator peripherals of PM8008 regulator device.
+
+ properties:
+ compatible:
+ const: qcom,pm8008-regulator
+
+ reg:
+ description: Peripheral offset and address of the ldo regulator.
+ maxItems: 1
+
+ required:
+ - compatible
+ - reg
+
+ unevaluatedProperties: false
+
required:
- compatible
- reg
@@ -120,6 +156,12 @@ examples:
reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_LOW>;
+ vdd_l1_l2-supply = <&vreg_s8b_1p2>;
+ vdd_l3_l4-supply = <&vreg_s1b_1p8>;
+ vdd_l5-supply = <&vreg_bob>;
+ vdd_l6-supply = <&vreg_bob>;
+ vdd_l7-supply = <&vreg_bob>;
+
pm8008_gpios: gpio@0,c000 {
compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
reg = <0x0 0xc000>;
@@ -129,6 +171,14 @@ examples:
interrupt-controller;
#interrupt-cells = <2>;
};
+
+ ldo1@1,4000 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4000>;
+ regulator-name = "pm8008_ldo1";
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1300000>;
+ };
};
};
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 3/9] dt-bindings: mfd: pm8008: Add regulators for pm8008
2022-06-14 9:48 ` [PATCH V15 3/9] dt-bindings: mfd: pm8008: Add regulators for pm8008 Satya Priya
@ 2022-06-16 20:58 ` Lee Jones
0 siblings, 0 replies; 54+ messages in thread
From: Lee Jones @ 2022-06-16 20:58 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Tue, 14 Jun 2022, Satya Priya wrote:
> Add regulators and their parent supplies along with example.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes in V15:
> - None.
>
> Changes in V14:
> - None.
>
> Changes in V13:
> - None.
>
> .../devicetree/bindings/mfd/qcom,pm8008.yaml | 50 ++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
For my own reference (apply this as-is to your sign-off block):
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH V15 4/9] mfd: pm8008: Add reset-gpios
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (2 preceding siblings ...)
2022-06-14 9:48 ` [PATCH V15 3/9] dt-bindings: mfd: pm8008: Add regulators for pm8008 Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-16 20:58 ` Lee Jones
2022-06-29 18:36 ` Guru Das Srinagesh
2022-06-14 9:48 ` [PATCH V15 5/9] mfd: pm8008: Remove the regmap member from pm8008_data struct Satya Priya
` (5 subsequent siblings)
9 siblings, 2 replies; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
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 <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V15:
- None.
Changes in V14:
- None.
Changes in V13:
- None.
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 c472d7f..5a670b0 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>
@@ -221,6 +222,7 @@ static int pm8008_probe(struct i2c_client *client)
{
int rc;
struct pm8008_data *chip;
+ struct gpio_desc *reset_gpio;
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
@@ -233,6 +235,10 @@ static int pm8008_probe(struct i2c_client *client)
i2c_set_clientdata(client, chip);
+ reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(reset_gpio))
+ return PTR_ERR(reset_gpio);
+
if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
rc = pm8008_probe_irq_peripherals(chip, client->irq);
if (rc)
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 4/9] mfd: pm8008: Add reset-gpios
2022-06-14 9:48 ` [PATCH V15 4/9] mfd: pm8008: Add reset-gpios Satya Priya
@ 2022-06-16 20:58 ` Lee Jones
2022-06-17 16:34 ` Lee Jones
2022-06-29 18:36 ` Guru Das Srinagesh
1 sibling, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-16 20:58 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Tue, 14 Jun 2022, Satya Priya wrote:
> 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 <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> ---
> Changes in V15:
> - None.
>
> Changes in V14:
> - None.
>
> Changes in V13:
> - None.
>
> drivers/mfd/qcom-pm8008.c | 6 ++++++
> 1 file changed, 6 insertions(+)
Applied, thanks.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 4/9] mfd: pm8008: Add reset-gpios
2022-06-16 20:58 ` Lee Jones
@ 2022-06-17 16:34 ` Lee Jones
0 siblings, 0 replies; 54+ messages in thread
From: Lee Jones @ 2022-06-17 16:34 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Thu, 16 Jun 2022, Lee Jones wrote:
> On Tue, 14 Jun 2022, Satya Priya wrote:
>
> > 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 <quic_c_skakit@quicinc.com>
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> > Changes in V15:
> > - None.
> >
> > Changes in V14:
> > - None.
> >
> > Changes in V13:
> > - None.
> >
> > drivers/mfd/qcom-pm8008.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
>
> Applied, thanks.
Sorry, wrong key-combo:
For my own reference (apply this as-is to your sign-off block):
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 4/9] mfd: pm8008: Add reset-gpios
2022-06-14 9:48 ` [PATCH V15 4/9] mfd: pm8008: Add reset-gpios Satya Priya
2022-06-16 20:58 ` Lee Jones
@ 2022-06-29 18:36 ` Guru Das Srinagesh
1 sibling, 0 replies; 54+ messages in thread
From: Guru Das Srinagesh @ 2022-06-29 18:36 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Lee Jones, Liam Girdwood,
Mark Brown, linux-arm-msm, devicetree, linux-kernel, swboyd,
quic_collinsd, quic_subbaram, quic_jprakash
On Jun 14 2022 15:18, Satya Priya wrote:
> 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 <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Acked-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH V15 5/9] mfd: pm8008: Remove the regmap member from pm8008_data struct
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (3 preceding siblings ...)
2022-06-14 9:48 ` [PATCH V15 4/9] mfd: pm8008: Add reset-gpios Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-16 20:58 ` Lee Jones
2022-06-29 18:35 ` Guru Das Srinagesh
2022-06-14 9:48 ` [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API Satya Priya
` (4 subsequent siblings)
9 siblings, 2 replies; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Remove the regmap member from pm8008_data struct as it is
not used outside of probe. Add a local variable for regmap
and pass it to the pm8008_probe_irq_peripherals()
API in pm8008_probe.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V15:
- None.
Changes in V14:
- None.
Changes in V13:
- None.
drivers/mfd/qcom-pm8008.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 5a670b0..569ffd50 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -57,7 +57,6 @@ enum {
struct pm8008_data {
struct device *dev;
- struct regmap *regmap;
int irq;
struct regmap_irq_chip_data *irq_data;
};
@@ -151,7 +150,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
.max_register = 0xFFFF,
};
-static int pm8008_init(struct pm8008_data *chip)
+static int pm8008_init(struct regmap *regmap)
{
int rc;
@@ -161,32 +160,32 @@ static int pm8008_init(struct pm8008_data *chip)
* This is required to enable the writing of TYPE registers in
* regmap_irq_sync_unlock().
*/
- rc = regmap_write(chip->regmap,
+ rc = regmap_write(regmap,
(PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
BIT(0));
if (rc)
return rc;
/* Do the same for GPIO1 and GPIO2 peripherals */
- rc = regmap_write(chip->regmap,
+ rc = regmap_write(regmap,
(PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
if (rc)
return rc;
- rc = regmap_write(chip->regmap,
+ rc = regmap_write(regmap,
(PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
return rc;
}
static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
- int client_irq)
+ struct regmap *regmap, int client_irq)
{
int rc, i;
struct regmap_irq_type *type;
struct regmap_irq_chip_data *irq_data;
- rc = pm8008_init(chip);
+ rc = pm8008_init(regmap);
if (rc) {
dev_err(chip->dev, "Init failed: %d\n", rc);
return rc;
@@ -208,7 +207,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
}
- rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
+ rc = devm_regmap_add_irq_chip(chip->dev, regmap, client_irq,
IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
if (rc) {
dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
@@ -223,14 +222,15 @@ static int pm8008_probe(struct i2c_client *client)
int rc;
struct pm8008_data *chip;
struct gpio_desc *reset_gpio;
+ struct regmap *regmap;
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
return -ENOMEM;
chip->dev = &client->dev;
- chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
- if (!chip->regmap)
+ regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
+ if (!regmap)
return -ENODEV;
i2c_set_clientdata(client, chip);
@@ -240,7 +240,7 @@ static int pm8008_probe(struct i2c_client *client)
return PTR_ERR(reset_gpio);
if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
- rc = pm8008_probe_irq_peripherals(chip, client->irq);
+ rc = pm8008_probe_irq_peripherals(chip, regmap, client->irq);
if (rc)
dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 5/9] mfd: pm8008: Remove the regmap member from pm8008_data struct
2022-06-14 9:48 ` [PATCH V15 5/9] mfd: pm8008: Remove the regmap member from pm8008_data struct Satya Priya
@ 2022-06-16 20:58 ` Lee Jones
2022-06-29 18:35 ` Guru Das Srinagesh
1 sibling, 0 replies; 54+ messages in thread
From: Lee Jones @ 2022-06-16 20:58 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Tue, 14 Jun 2022, Satya Priya wrote:
> Remove the regmap member from pm8008_data struct as it is
> not used outside of probe. Add a local variable for regmap
> and pass it to the pm8008_probe_irq_peripherals()
> API in pm8008_probe.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> ---
> Changes in V15:
> - None.
>
> Changes in V14:
> - None.
>
> Changes in V13:
> - None.
>
> drivers/mfd/qcom-pm8008.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
For my own reference (apply this as-is to your sign-off block):
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 5/9] mfd: pm8008: Remove the regmap member from pm8008_data struct
2022-06-14 9:48 ` [PATCH V15 5/9] mfd: pm8008: Remove the regmap member from pm8008_data struct Satya Priya
2022-06-16 20:58 ` Lee Jones
@ 2022-06-29 18:35 ` Guru Das Srinagesh
1 sibling, 0 replies; 54+ messages in thread
From: Guru Das Srinagesh @ 2022-06-29 18:35 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Lee Jones, Liam Girdwood,
Mark Brown, linux-arm-msm, devicetree, linux-kernel, swboyd,
quic_collinsd, quic_subbaram, quic_jprakash
On Jun 14 2022 15:18, Satya Priya wrote:
> Remove the regmap member from pm8008_data struct as it is
> not used outside of probe. Add a local variable for regmap
> and pass it to the pm8008_probe_irq_peripherals()
> API in pm8008_probe.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Acked-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (4 preceding siblings ...)
2022-06-14 9:48 ` [PATCH V15 5/9] mfd: pm8008: Remove the regmap member from pm8008_data struct Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-16 20:57 ` Lee Jones
2022-06-14 9:48 ` [PATCH V15 7/9] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
` (3 subsequent siblings)
9 siblings, 1 reply; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Use i2c_new_dummy_device() to register pm8008-regulator
client present at a different address space, instead of
defining a separate DT node. This avoids calling the probe
twice for the same chip, once for each client pm8008-infra
and pm8008-regulator.
As a part of this define pm8008_regmap_init() to do regmap
init for both the clients and define pm8008_get_regmap() to
pass the regmap to the regulator driver.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V15:
- None.
Changes in V14:
- None.
Changes in V13:
- None.
drivers/mfd/qcom-pm8008.c | 34 ++++++++++++++++++++++++++++++++--
include/linux/mfd/qcom_pm8008.h | 9 +++++++++
2 files changed, 41 insertions(+), 2 deletions(-)
create mode 100644 include/linux/mfd/qcom_pm8008.h
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 569ffd50..55e2a8e 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -9,6 +9,7 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
+#include <linux/mfd/qcom_pm8008.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/of_platform.h>
@@ -57,6 +58,7 @@ enum {
struct pm8008_data {
struct device *dev;
+ struct regmap *regulators_regmap;
int irq;
struct regmap_irq_chip_data *irq_data;
};
@@ -150,6 +152,12 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
.max_register = 0xFFFF,
};
+struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
+{
+ return chip->regulators_regmap;
+}
+EXPORT_SYMBOL_GPL(pm8008_get_regmap);
+
static int pm8008_init(struct regmap *regmap)
{
int rc;
@@ -217,11 +225,25 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
return 0;
}
+static struct regmap *pm8008_regmap_init(struct i2c_client *client,
+ struct pm8008_data *chip)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
+ if (!regmap)
+ return NULL;
+
+ i2c_set_clientdata(client, chip);
+ return regmap;
+}
+
static int pm8008_probe(struct i2c_client *client)
{
int rc;
struct pm8008_data *chip;
struct gpio_desc *reset_gpio;
+ struct i2c_client *regulators_client;
struct regmap *regmap;
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
@@ -229,11 +251,19 @@ static int pm8008_probe(struct i2c_client *client)
return -ENOMEM;
chip->dev = &client->dev;
- regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
+ regmap = pm8008_regmap_init(client, chip);
if (!regmap)
return -ENODEV;
- i2c_set_clientdata(client, chip);
+ regulators_client = i2c_new_dummy_device(client->adapter, client->addr + 1);
+ if (IS_ERR(regulators_client)) {
+ dev_err(&client->dev, "can't attach client\n");
+ return PTR_ERR(regulators_client);
+ }
+
+ chip->regulators_regmap = pm8008_regmap_init(regulators_client, chip);
+ if (!chip->regulators_regmap)
+ return -ENODEV;
reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(reset_gpio))
diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
new file mode 100644
index 0000000..3814bff
--- /dev/null
+++ b/include/linux/mfd/qcom_pm8008.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+// Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+#ifndef __QCOM_PM8008_H__
+#define __QCOM_PM8008_H__
+
+struct pm8008_data;
+struct regmap *pm8008_get_regmap(const struct pm8008_data *chip);
+
+#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-14 9:48 ` [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API Satya Priya
@ 2022-06-16 20:57 ` Lee Jones
2022-06-20 5:28 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-16 20:57 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Tue, 14 Jun 2022, Satya Priya wrote:
> Use i2c_new_dummy_device() to register pm8008-regulator
> client present at a different address space, instead of
> defining a separate DT node. This avoids calling the probe
> twice for the same chip, once for each client pm8008-infra
> and pm8008-regulator.
>
> As a part of this define pm8008_regmap_init() to do regmap
> init for both the clients and define pm8008_get_regmap() to
> pass the regmap to the regulator driver.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> ---
> Changes in V15:
> - None.
>
> Changes in V14:
> - None.
>
> Changes in V13:
> - None.
>
> drivers/mfd/qcom-pm8008.c | 34 ++++++++++++++++++++++++++++++++--
> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> 2 files changed, 41 insertions(+), 2 deletions(-)
> create mode 100644 include/linux/mfd/qcom_pm8008.h
>
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index 569ffd50..55e2a8e 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -9,6 +9,7 @@
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/irqdomain.h>
> +#include <linux/mfd/qcom_pm8008.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/of_platform.h>
> @@ -57,6 +58,7 @@ enum {
>
> struct pm8008_data {
> struct device *dev;
> + struct regmap *regulators_regmap;
> int irq;
> struct regmap_irq_chip_data *irq_data;
> };
> @@ -150,6 +152,12 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> .max_register = 0xFFFF,
> };
>
> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> +{
> + return chip->regulators_regmap;
> +}
> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
Seems like abstraction for the sake of abstraction.
Why not do the dereference inside the regulator driver?
> static int pm8008_init(struct regmap *regmap)
> {
> int rc;
> @@ -217,11 +225,25 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> return 0;
> }
>
> +static struct regmap *pm8008_regmap_init(struct i2c_client *client,
> + struct pm8008_data *chip)
> +{
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> + if (!regmap)
> + return NULL;
> +
> + i2c_set_clientdata(client, chip);
> + return regmap;
> +}
This function seems superfluous.
It's only called once and it contains a single call.
Just pop the call directly into probe.
> static int pm8008_probe(struct i2c_client *client)
> {
> int rc;
> struct pm8008_data *chip;
> struct gpio_desc *reset_gpio;
> + struct i2c_client *regulators_client;
> struct regmap *regmap;
>
> chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> @@ -229,11 +251,19 @@ static int pm8008_probe(struct i2c_client *client)
> return -ENOMEM;
>
> chip->dev = &client->dev;
> - regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> + regmap = pm8008_regmap_init(client, chip);
> if (!regmap)
> return -ENODEV;
>
> - i2c_set_clientdata(client, chip);
> + regulators_client = i2c_new_dummy_device(client->adapter, client->addr + 1);
> + if (IS_ERR(regulators_client)) {
> + dev_err(&client->dev, "can't attach client\n");
> + return PTR_ERR(regulators_client);
> + }
> +
> + chip->regulators_regmap = pm8008_regmap_init(regulators_client, chip);
> + if (!chip->regulators_regmap)
> + return -ENODEV;
>
> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(reset_gpio))
> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
> new file mode 100644
> index 0000000..3814bff
> --- /dev/null
> +++ b/include/linux/mfd/qcom_pm8008.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +// Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> +#ifndef __QCOM_PM8008_H__
> +#define __QCOM_PM8008_H__
> +
> +struct pm8008_data;
> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip);
> +
> +#endif
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-16 20:57 ` Lee Jones
@ 2022-06-20 5:28 ` Satya Priya Kakitapalli (Temp)
2022-06-20 8:20 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-06-20 5:28 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On 6/17/2022 2:27 AM, Lee Jones wrote:
> On Tue, 14 Jun 2022, Satya Priya wrote:
>
>> Use i2c_new_dummy_device() to register pm8008-regulator
>> client present at a different address space, instead of
>> defining a separate DT node. This avoids calling the probe
>> twice for the same chip, once for each client pm8008-infra
>> and pm8008-regulator.
>>
>> As a part of this define pm8008_regmap_init() to do regmap
>> init for both the clients and define pm8008_get_regmap() to
>> pass the regmap to the regulator driver.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>> ---
>> Changes in V15:
>> - None.
>>
>> Changes in V14:
>> - None.
>>
>> Changes in V13:
>> - None.
>>
>> drivers/mfd/qcom-pm8008.c | 34 ++++++++++++++++++++++++++++++++--
>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>> 2 files changed, 41 insertions(+), 2 deletions(-)
>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index 569ffd50..55e2a8e 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -9,6 +9,7 @@
>> #include <linux/interrupt.h>
>> #include <linux/irq.h>
>> #include <linux/irqdomain.h>
>> +#include <linux/mfd/qcom_pm8008.h>
>> #include <linux/module.h>
>> #include <linux/of_device.h>
>> #include <linux/of_platform.h>
>> @@ -57,6 +58,7 @@ enum {
>>
>> struct pm8008_data {
>> struct device *dev;
>> + struct regmap *regulators_regmap;
>> int irq;
>> struct regmap_irq_chip_data *irq_data;
>> };
>> @@ -150,6 +152,12 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>> .max_register = 0xFFFF,
>> };
>>
>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>> +{
>> + return chip->regulators_regmap;
>> +}
>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> Seems like abstraction for the sake of abstraction.
>
> Why not do the dereference inside the regulator driver?
To derefer this in the regulator driver, we need to have the pm8008_data
struct definition in the qcom_pm8008 header file.
I think it doesn't look great to have only that structure in header and
all other structs and enum in the mfd driver.
>> static int pm8008_init(struct regmap *regmap)
>> {
>> int rc;
>> @@ -217,11 +225,25 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>> return 0;
>> }
>>
>> +static struct regmap *pm8008_regmap_init(struct i2c_client *client,
>> + struct pm8008_data *chip)
>> +{
>> + struct regmap *regmap;
>> +
>> + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>> + if (!regmap)
>> + return NULL;
>> +
>> + i2c_set_clientdata(client, chip);
>> + return regmap;
>> +}
> This function seems superfluous.
>
> It's only called once and it contains a single call.
No, It is being called twice. To avoid repetitive code, I've added this
subroutine.
> Just pop the call directly into probe.
>
>> static int pm8008_probe(struct i2c_client *client)
>> {
>> int rc;
>> struct pm8008_data *chip;
>> struct gpio_desc *reset_gpio;
>> + struct i2c_client *regulators_client;
>> struct regmap *regmap;
>>
>> chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>> @@ -229,11 +251,19 @@ static int pm8008_probe(struct i2c_client *client)
>> return -ENOMEM;
>>
>> chip->dev = &client->dev;
>> - regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>> + regmap = pm8008_regmap_init(client, chip);
>> if (!regmap)
>> return -ENODEV;
>>
>> - i2c_set_clientdata(client, chip);
>> + regulators_client = i2c_new_dummy_device(client->adapter, client->addr + 1);
>> + if (IS_ERR(regulators_client)) {
>> + dev_err(&client->dev, "can't attach client\n");
>> + return PTR_ERR(regulators_client);
>> + }
>> +
>> + chip->regulators_regmap = pm8008_regmap_init(regulators_client, chip);
>> + if (!chip->regulators_regmap)
>> + return -ENODEV;
>>
>> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
>> if (IS_ERR(reset_gpio))
>> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
>> new file mode 100644
>> index 0000000..3814bff
>> --- /dev/null
>> +++ b/include/linux/mfd/qcom_pm8008.h
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +// Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> +#ifndef __QCOM_PM8008_H__
>> +#define __QCOM_PM8008_H__
>> +
>> +struct pm8008_data;
>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip);
>> +
>> +#endif
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-20 5:28 ` Satya Priya Kakitapalli (Temp)
@ 2022-06-20 8:20 ` Lee Jones
2022-06-20 11:07 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-20 8:20 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> On 6/17/2022 2:27 AM, Lee Jones wrote:
> > On Tue, 14 Jun 2022, Satya Priya wrote:
> >
> > > Use i2c_new_dummy_device() to register pm8008-regulator
> > > client present at a different address space, instead of
> > > defining a separate DT node. This avoids calling the probe
> > > twice for the same chip, once for each client pm8008-infra
> > > and pm8008-regulator.
> > >
> > > As a part of this define pm8008_regmap_init() to do regmap
> > > init for both the clients and define pm8008_get_regmap() to
> > > pass the regmap to the regulator driver.
> > >
> > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > > Changes in V15:
> > > - None.
> > >
> > > Changes in V14:
> > > - None.
> > >
> > > Changes in V13:
> > > - None.
> > >
> > > drivers/mfd/qcom-pm8008.c | 34 ++++++++++++++++++++++++++++++++--
> > > include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> > > 2 files changed, 41 insertions(+), 2 deletions(-)
> > > create mode 100644 include/linux/mfd/qcom_pm8008.h
> > >
> > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > index 569ffd50..55e2a8e 100644
> > > --- a/drivers/mfd/qcom-pm8008.c
> > > +++ b/drivers/mfd/qcom-pm8008.c
> > > @@ -9,6 +9,7 @@
> > > #include <linux/interrupt.h>
> > > #include <linux/irq.h>
> > > #include <linux/irqdomain.h>
> > > +#include <linux/mfd/qcom_pm8008.h>
> > > #include <linux/module.h>
> > > #include <linux/of_device.h>
> > > #include <linux/of_platform.h>
> > > @@ -57,6 +58,7 @@ enum {
> > > struct pm8008_data {
> > > struct device *dev;
> > > + struct regmap *regulators_regmap;
> > > int irq;
> > > struct regmap_irq_chip_data *irq_data;
> > > };
> > > @@ -150,6 +152,12 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> > > .max_register = 0xFFFF,
> > > };
> > > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > > +{
> > > + return chip->regulators_regmap;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > Seems like abstraction for the sake of abstraction.
> >
> > Why not do the dereference inside the regulator driver?
>
> To derefer this in the regulator driver, we need to have the pm8008_data
> struct definition in the qcom_pm8008 header file.
>
> I think it doesn't look great to have only that structure in header and all
> other structs and enum in the mfd driver.
Then why pass 'pm8008_data' at all?
What's preventing you from passing 'regmap'?
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-20 8:20 ` Lee Jones
@ 2022-06-20 11:07 ` Satya Priya Kakitapalli (Temp)
2022-06-27 5:07 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-06-20 11:07 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On 6/20/2022 1:50 PM, Lee Jones wrote:
> On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> On 6/17/2022 2:27 AM, Lee Jones wrote:
>>> On Tue, 14 Jun 2022, Satya Priya wrote:
>>>
>>>> Use i2c_new_dummy_device() to register pm8008-regulator
>>>> client present at a different address space, instead of
>>>> defining a separate DT node. This avoids calling the probe
>>>> twice for the same chip, once for each client pm8008-infra
>>>> and pm8008-regulator.
>>>>
>>>> As a part of this define pm8008_regmap_init() to do regmap
>>>> init for both the clients and define pm8008_get_regmap() to
>>>> pass the regmap to the regulator driver.
>>>>
>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>> ---
>>>> Changes in V15:
>>>> - None.
>>>>
>>>> Changes in V14:
>>>> - None.
>>>>
>>>> Changes in V13:
>>>> - None.
>>>>
>>>> drivers/mfd/qcom-pm8008.c | 34 ++++++++++++++++++++++++++++++++--
>>>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>>>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>>>
>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>> index 569ffd50..55e2a8e 100644
>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>> @@ -9,6 +9,7 @@
>>>> #include <linux/interrupt.h>
>>>> #include <linux/irq.h>
>>>> #include <linux/irqdomain.h>
>>>> +#include <linux/mfd/qcom_pm8008.h>
>>>> #include <linux/module.h>
>>>> #include <linux/of_device.h>
>>>> #include <linux/of_platform.h>
>>>> @@ -57,6 +58,7 @@ enum {
>>>> struct pm8008_data {
>>>> struct device *dev;
>>>> + struct regmap *regulators_regmap;
>>>> int irq;
>>>> struct regmap_irq_chip_data *irq_data;
>>>> };
>>>> @@ -150,6 +152,12 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>>>> .max_register = 0xFFFF,
>>>> };
>>>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>>>> +{
>>>> + return chip->regulators_regmap;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>> Seems like abstraction for the sake of abstraction.
>>>
>>> Why not do the dereference inside the regulator driver?
>> To derefer this in the regulator driver, we need to have the pm8008_data
>> struct definition in the qcom_pm8008 header file.
>>
>> I think it doesn't look great to have only that structure in header and all
>> other structs and enum in the mfd driver.
> Then why pass 'pm8008_data' at all?
There is one more option, instead of passing the pm8008_data, we could
pass the pdev->dev.parent and get the pm8008 chip data directly in the
pm8008_get_regmap() like below
struct regmap *pm8008_get_regmap(const struct device *dev)
{
const struct pm8008_data *chip = dev_get_drvdata(dev);
return chip->regulators_regmap;
}
EXPORT_SYMBOL_GPL(pm8008_get_regmap);
By doing this we can avoid having declaration of pm8008_data also in the
header. Please let me know if this looks good.
> What's preventing you from passing 'regmap'?
I didn't get what you meant here, could you please elaborate a bit?
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-20 11:07 ` Satya Priya Kakitapalli (Temp)
@ 2022-06-27 5:07 ` Satya Priya Kakitapalli (Temp)
2022-06-27 7:41 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-06-27 5:07 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
Hi Lee,
On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
>
> On 6/20/2022 1:50 PM, Lee Jones wrote:
>> On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>
>>> On 6/17/2022 2:27 AM, Lee Jones wrote:
>>>> On Tue, 14 Jun 2022, Satya Priya wrote:
>>>>
>>>>> Use i2c_new_dummy_device() to register pm8008-regulator
>>>>> client present at a different address space, instead of
>>>>> defining a separate DT node. This avoids calling the probe
>>>>> twice for the same chip, once for each client pm8008-infra
>>>>> and pm8008-regulator.
>>>>>
>>>>> As a part of this define pm8008_regmap_init() to do regmap
>>>>> init for both the clients and define pm8008_get_regmap() to
>>>>> pass the regmap to the regulator driver.
>>>>>
>>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>> ---
>>>>> Changes in V15:
>>>>> - None.
>>>>>
>>>>> Changes in V14:
>>>>> - None.
>>>>>
>>>>> Changes in V13:
>>>>> - None.
>>>>>
>>>>> drivers/mfd/qcom-pm8008.c | 34
>>>>> ++++++++++++++++++++++++++++++++--
>>>>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>>>>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>>>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>>>>
>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>> index 569ffd50..55e2a8e 100644
>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>> @@ -9,6 +9,7 @@
>>>>> #include <linux/interrupt.h>
>>>>> #include <linux/irq.h>
>>>>> #include <linux/irqdomain.h>
>>>>> +#include <linux/mfd/qcom_pm8008.h>
>>>>> #include <linux/module.h>
>>>>> #include <linux/of_device.h>
>>>>> #include <linux/of_platform.h>
>>>>> @@ -57,6 +58,7 @@ enum {
>>>>> struct pm8008_data {
>>>>> struct device *dev;
>>>>> + struct regmap *regulators_regmap;
>>>>> int irq;
>>>>> struct regmap_irq_chip_data *irq_data;
>>>>> };
>>>>> @@ -150,6 +152,12 @@ static struct regmap_config
>>>>> qcom_mfd_regmap_cfg = {
>>>>> .max_register = 0xFFFF,
>>>>> };
>>>>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>>>>> +{
>>>>> + return chip->regulators_regmap;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>> Seems like abstraction for the sake of abstraction.
>>>>
>>>> Why not do the dereference inside the regulator driver?
>>> To derefer this in the regulator driver, we need to have the
>>> pm8008_data
>>> struct definition in the qcom_pm8008 header file.
>>>
>>> I think it doesn't look great to have only that structure in header
>>> and all
>>> other structs and enum in the mfd driver.
>> Then why pass 'pm8008_data' at all?
>
>
> There is one more option, instead of passing the pm8008_data, we could
> pass the pdev->dev.parent and get the pm8008 chip data directly in the
> pm8008_get_regmap() like below
>
>
> struct regmap *pm8008_get_regmap(const struct device *dev)
> {
> const struct pm8008_data *chip = dev_get_drvdata(dev);
>
> return chip->regulators_regmap;
> }
> EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>
>
> By doing this we can avoid having declaration of pm8008_data also in
> the header. Please let me know if this looks good.
>
Could you please confirm on this?
>> What's preventing you from passing 'regmap'?
>
>
> I didn't get what you meant here, could you please elaborate a bit?
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-27 5:07 ` Satya Priya Kakitapalli (Temp)
@ 2022-06-27 7:41 ` Lee Jones
2022-06-28 4:53 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-27 7:41 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> Hi Lee,
>
>
> On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
> >
> > On 6/20/2022 1:50 PM, Lee Jones wrote:
> > > On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > >
> > > > On 6/17/2022 2:27 AM, Lee Jones wrote:
> > > > > On Tue, 14 Jun 2022, Satya Priya wrote:
> > > > >
> > > > > > Use i2c_new_dummy_device() to register pm8008-regulator
> > > > > > client present at a different address space, instead of
> > > > > > defining a separate DT node. This avoids calling the probe
> > > > > > twice for the same chip, once for each client pm8008-infra
> > > > > > and pm8008-regulator.
> > > > > >
> > > > > > As a part of this define pm8008_regmap_init() to do regmap
> > > > > > init for both the clients and define pm8008_get_regmap() to
> > > > > > pass the regmap to the regulator driver.
> > > > > >
> > > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > > ---
> > > > > > Changes in V15:
> > > > > > - None.
> > > > > >
> > > > > > Changes in V14:
> > > > > > - None.
> > > > > >
> > > > > > Changes in V13:
> > > > > > - None.
> > > > > >
> > > > > > drivers/mfd/qcom-pm8008.c | 34
> > > > > > ++++++++++++++++++++++++++++++++--
> > > > > > include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> > > > > > 2 files changed, 41 insertions(+), 2 deletions(-)
> > > > > > create mode 100644 include/linux/mfd/qcom_pm8008.h
> > > > > >
> > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > index 569ffd50..55e2a8e 100644
> > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > @@ -9,6 +9,7 @@
> > > > > > #include <linux/interrupt.h>
> > > > > > #include <linux/irq.h>
> > > > > > #include <linux/irqdomain.h>
> > > > > > +#include <linux/mfd/qcom_pm8008.h>
> > > > > > #include <linux/module.h>
> > > > > > #include <linux/of_device.h>
> > > > > > #include <linux/of_platform.h>
> > > > > > @@ -57,6 +58,7 @@ enum {
> > > > > > struct pm8008_data {
> > > > > > struct device *dev;
> > > > > > + struct regmap *regulators_regmap;
> > > > > > int irq;
> > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > };
> > > > > > @@ -150,6 +152,12 @@ static struct regmap_config
> > > > > > qcom_mfd_regmap_cfg = {
> > > > > > .max_register = 0xFFFF,
> > > > > > };
> > > > > > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > > > > > +{
> > > > > > + return chip->regulators_regmap;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > Seems like abstraction for the sake of abstraction.
> > > > >
> > > > > Why not do the dereference inside the regulator driver?
> > > > To derefer this in the regulator driver, we need to have the
> > > > pm8008_data
> > > > struct definition in the qcom_pm8008 header file.
> > > >
> > > > I think it doesn't look great to have only that structure in
> > > > header and all
> > > > other structs and enum in the mfd driver.
> > > Then why pass 'pm8008_data' at all?
> >
> >
> > There is one more option, instead of passing the pm8008_data, we could
> > pass the pdev->dev.parent and get the pm8008 chip data directly in the
> > pm8008_get_regmap() like below
> >
> >
> > struct regmap *pm8008_get_regmap(const struct device *dev)
> > {
> > const struct pm8008_data *chip = dev_get_drvdata(dev);
> >
> > return chip->regulators_regmap;
> > }
> > EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> >
> >
> > By doing this we can avoid having declaration of pm8008_data also in the
> > header. Please let me know if this looks good.
> >
>
> Could you please confirm on this?
>
> > > What's preventing you from passing 'regmap'?
> >
> >
> > I didn't get what you meant here, could you please elaborate a bit?
Ah yes. I authored you a patch, but became distracted. Here:
-----8<--------------------8<-------
From: Lee Jones <lee.jones@linaro.org>
mfd: pm8008: Remove driver data structure pm8008_data
Maintaining a local driver data structure that is never shared
outside of the core device is an unnecessary complexity. Half of the
attributes were not used outside of a single function, one of which
was not used at all. The remaining 2 are generic and can be passed
around as required.
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
1 file changed, 20 insertions(+), 33 deletions(-)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index c472d7f8103c4..4b8ff947762f2 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -54,13 +54,6 @@ enum {
#define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
-struct pm8008_data {
- struct device *dev;
- struct regmap *regmap;
- int irq;
- struct regmap_irq_chip_data *irq_data;
-};
-
static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
@@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
.max_register = 0xFFFF,
};
-static int pm8008_init(struct pm8008_data *chip)
+static int pm8008_init(struct regmap *regmap)
{
int rc;
@@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
* This is required to enable the writing of TYPE registers in
* regmap_irq_sync_unlock().
*/
- rc = regmap_write(chip->regmap,
- (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
- BIT(0));
+ rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
if (rc)
return rc;
/* Do the same for GPIO1 and GPIO2 peripherals */
- rc = regmap_write(chip->regmap,
- (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
+ rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
if (rc)
return rc;
- rc = regmap_write(chip->regmap,
- (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
+ rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
return rc;
}
-static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
+static int pm8008_probe_irq_peripherals(struct device *dev,
+ struct regmap *regmap,
int client_irq)
{
int rc, i;
struct regmap_irq_type *type;
struct regmap_irq_chip_data *irq_data;
- rc = pm8008_init(chip);
+ rc = pm8008_init(regmap);
if (rc) {
- dev_err(chip->dev, "Init failed: %d\n", rc);
+ dev_err(dev, "Init failed: %d\n", rc);
return rc;
}
@@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
}
- rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
+ rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
if (rc) {
- dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
+ dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
return rc;
}
@@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
static int pm8008_probe(struct i2c_client *client)
{
int rc;
- struct pm8008_data *chip;
-
- chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
- if (!chip)
- return -ENOMEM;
+ struct device *dev;
+ struct regmap *regmap;
- chip->dev = &client->dev;
- chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
- if (!chip->regmap)
+ dev = &client->dev;
+ regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
+ if (!regmap)
return -ENODEV;
- i2c_set_clientdata(client, chip);
+ i2c_set_clientdata(client, regmap);
- if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
- rc = pm8008_probe_irq_peripherals(chip, client->irq);
+ if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
+ rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
if (rc)
- dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
+ dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
}
- return devm_of_platform_populate(chip->dev);
+ return devm_of_platform_populate(dev);
}
static const struct of_device_id pm8008_match[] = {
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-27 7:41 ` Lee Jones
@ 2022-06-28 4:53 ` Satya Priya Kakitapalli (Temp)
2022-06-28 7:42 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-06-28 4:53 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On 6/27/2022 1:11 PM, Lee Jones wrote:
> On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> Hi Lee,
>>
>>
>> On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
>>> On 6/20/2022 1:50 PM, Lee Jones wrote:
>>>> On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>
>>>>> On 6/17/2022 2:27 AM, Lee Jones wrote:
>>>>>> On Tue, 14 Jun 2022, Satya Priya wrote:
>>>>>>
>>>>>>> Use i2c_new_dummy_device() to register pm8008-regulator
>>>>>>> client present at a different address space, instead of
>>>>>>> defining a separate DT node. This avoids calling the probe
>>>>>>> twice for the same chip, once for each client pm8008-infra
>>>>>>> and pm8008-regulator.
>>>>>>>
>>>>>>> As a part of this define pm8008_regmap_init() to do regmap
>>>>>>> init for both the clients and define pm8008_get_regmap() to
>>>>>>> pass the regmap to the regulator driver.
>>>>>>>
>>>>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>>>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>>>> ---
>>>>>>> Changes in V15:
>>>>>>> - None.
>>>>>>>
>>>>>>> Changes in V14:
>>>>>>> - None.
>>>>>>>
>>>>>>> Changes in V13:
>>>>>>> - None.
>>>>>>>
>>>>>>> drivers/mfd/qcom-pm8008.c | 34
>>>>>>> ++++++++++++++++++++++++++++++++--
>>>>>>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>>>>>>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>>>>>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>>>>>>
>>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>>> index 569ffd50..55e2a8e 100644
>>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>>> @@ -9,6 +9,7 @@
>>>>>>> #include <linux/interrupt.h>
>>>>>>> #include <linux/irq.h>
>>>>>>> #include <linux/irqdomain.h>
>>>>>>> +#include <linux/mfd/qcom_pm8008.h>
>>>>>>> #include <linux/module.h>
>>>>>>> #include <linux/of_device.h>
>>>>>>> #include <linux/of_platform.h>
>>>>>>> @@ -57,6 +58,7 @@ enum {
>>>>>>> struct pm8008_data {
>>>>>>> struct device *dev;
>>>>>>> + struct regmap *regulators_regmap;
>>>>>>> int irq;
>>>>>>> struct regmap_irq_chip_data *irq_data;
>>>>>>> };
>>>>>>> @@ -150,6 +152,12 @@ static struct regmap_config
>>>>>>> qcom_mfd_regmap_cfg = {
>>>>>>> .max_register = 0xFFFF,
>>>>>>> };
>>>>>>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>>>>>>> +{
>>>>>>> + return chip->regulators_regmap;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>> Seems like abstraction for the sake of abstraction.
>>>>>>
>>>>>> Why not do the dereference inside the regulator driver?
>>>>> To derefer this in the regulator driver, we need to have the
>>>>> pm8008_data
>>>>> struct definition in the qcom_pm8008 header file.
>>>>>
>>>>> I think it doesn't look great to have only that structure in
>>>>> header and all
>>>>> other structs and enum in the mfd driver.
>>>> Then why pass 'pm8008_data' at all?
>>>
>>> There is one more option, instead of passing the pm8008_data, we could
>>> pass the pdev->dev.parent and get the pm8008 chip data directly in the
>>> pm8008_get_regmap() like below
>>>
>>>
>>> struct regmap *pm8008_get_regmap(const struct device *dev)
>>> {
>>> const struct pm8008_data *chip = dev_get_drvdata(dev);
>>>
>>> return chip->regulators_regmap;
>>> }
>>> EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>
>>>
>>> By doing this we can avoid having declaration of pm8008_data also in the
>>> header. Please let me know if this looks good.
>>>
>> Could you please confirm on this?
>>
>>>> What's preventing you from passing 'regmap'?
>>>
>>> I didn't get what you meant here, could you please elaborate a bit?
> Ah yes. I authored you a patch, but became distracted. Here:
>
> -----8<--------------------8<-------
>
> From: Lee Jones <lee.jones@linaro.org>
>
> mfd: pm8008: Remove driver data structure pm8008_data
>
> Maintaining a local driver data structure that is never shared
> outside of the core device is an unnecessary complexity. Half of the
> attributes were not used outside of a single function, one of which
> was not used at all. The remaining 2 are generic and can be passed
> around as required.
Okay, but we still need to store the regulators_regmap, which is
required in the pm8008 regulator driver. Could we use a global variable
for it?
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
> 1 file changed, 20 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index c472d7f8103c4..4b8ff947762f2 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -54,13 +54,6 @@ enum {
>
> #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
>
> -struct pm8008_data {
> - struct device *dev;
> - struct regmap *regmap;
> - int irq;
> - struct regmap_irq_chip_data *irq_data;
> -};
> -
> static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
> static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
> static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
> @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> .max_register = 0xFFFF,
> };
>
> -static int pm8008_init(struct pm8008_data *chip)
> +static int pm8008_init(struct regmap *regmap)
> {
> int rc;
>
> @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
> * This is required to enable the writing of TYPE registers in
> * regmap_irq_sync_unlock().
> */
> - rc = regmap_write(chip->regmap,
> - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
> - BIT(0));
> + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> if (rc)
> return rc;
>
> /* Do the same for GPIO1 and GPIO2 peripherals */
> - rc = regmap_write(chip->regmap,
> - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> if (rc)
> return rc;
>
> - rc = regmap_write(chip->regmap,
> - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>
> return rc;
> }
>
> -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> +static int pm8008_probe_irq_peripherals(struct device *dev,
> + struct regmap *regmap,
> int client_irq)
> {
> int rc, i;
> struct regmap_irq_type *type;
> struct regmap_irq_chip_data *irq_data;
>
> - rc = pm8008_init(chip);
> + rc = pm8008_init(regmap);
> if (rc) {
> - dev_err(chip->dev, "Init failed: %d\n", rc);
> + dev_err(dev, "Init failed: %d\n", rc);
> return rc;
> }
>
> @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> }
>
> - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
> + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> if (rc) {
> - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
> + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> return rc;
> }
>
> @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> static int pm8008_probe(struct i2c_client *client)
> {
> int rc;
> - struct pm8008_data *chip;
> -
> - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> - if (!chip)
> - return -ENOMEM;
> + struct device *dev;
> + struct regmap *regmap;
>
> - chip->dev = &client->dev;
> - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> - if (!chip->regmap)
> + dev = &client->dev;
> + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> + if (!regmap)
> return -ENODEV;
>
> - i2c_set_clientdata(client, chip);
> + i2c_set_clientdata(client, regmap);
>
> - if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
> - rc = pm8008_probe_irq_peripherals(chip, client->irq);
> + if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> + rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
> if (rc)
> - dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
> + dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> }
>
> - return devm_of_platform_populate(chip->dev);
> + return devm_of_platform_populate(dev);
> }
>
> static const struct of_device_id pm8008_match[] = {
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-28 4:53 ` Satya Priya Kakitapalli (Temp)
@ 2022-06-28 7:42 ` Lee Jones
2022-06-29 10:36 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-28 7:42 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> On 6/27/2022 1:11 PM, Lee Jones wrote:
> > On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> >
> > > Hi Lee,
> > >
> > >
> > > On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
> > > > On 6/20/2022 1:50 PM, Lee Jones wrote:
> > > > > On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > >
> > > > > > On 6/17/2022 2:27 AM, Lee Jones wrote:
> > > > > > > On Tue, 14 Jun 2022, Satya Priya wrote:
> > > > > > >
> > > > > > > > Use i2c_new_dummy_device() to register pm8008-regulator
> > > > > > > > client present at a different address space, instead of
> > > > > > > > defining a separate DT node. This avoids calling the probe
> > > > > > > > twice for the same chip, once for each client pm8008-infra
> > > > > > > > and pm8008-regulator.
> > > > > > > >
> > > > > > > > As a part of this define pm8008_regmap_init() to do regmap
> > > > > > > > init for both the clients and define pm8008_get_regmap() to
> > > > > > > > pass the regmap to the regulator driver.
> > > > > > > >
> > > > > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > > > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > > > > ---
> > > > > > > > Changes in V15:
> > > > > > > > - None.
> > > > > > > >
> > > > > > > > Changes in V14:
> > > > > > > > - None.
> > > > > > > >
> > > > > > > > Changes in V13:
> > > > > > > > - None.
> > > > > > > >
> > > > > > > > drivers/mfd/qcom-pm8008.c | 34
> > > > > > > > ++++++++++++++++++++++++++++++++--
> > > > > > > > include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> > > > > > > > 2 files changed, 41 insertions(+), 2 deletions(-)
> > > > > > > > create mode 100644 include/linux/mfd/qcom_pm8008.h
> > > > > > > >
> > > > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > > > index 569ffd50..55e2a8e 100644
> > > > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > > #include <linux/interrupt.h>
> > > > > > > > #include <linux/irq.h>
> > > > > > > > #include <linux/irqdomain.h>
> > > > > > > > +#include <linux/mfd/qcom_pm8008.h>
> > > > > > > > #include <linux/module.h>
> > > > > > > > #include <linux/of_device.h>
> > > > > > > > #include <linux/of_platform.h>
> > > > > > > > @@ -57,6 +58,7 @@ enum {
> > > > > > > > struct pm8008_data {
> > > > > > > > struct device *dev;
> > > > > > > > + struct regmap *regulators_regmap;
> > > > > > > > int irq;
> > > > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > > > };
> > > > > > > > @@ -150,6 +152,12 @@ static struct regmap_config
> > > > > > > > qcom_mfd_regmap_cfg = {
> > > > > > > > .max_register = 0xFFFF,
> > > > > > > > };
> > > > > > > > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > > > > > > > +{
> > > > > > > > + return chip->regulators_regmap;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > Seems like abstraction for the sake of abstraction.
> > > > > > >
> > > > > > > Why not do the dereference inside the regulator driver?
> > > > > > To derefer this in the regulator driver, we need to have the
> > > > > > pm8008_data
> > > > > > struct definition in the qcom_pm8008 header file.
> > > > > >
> > > > > > I think it doesn't look great to have only that structure in
> > > > > > header and all
> > > > > > other structs and enum in the mfd driver.
> > > > > Then why pass 'pm8008_data' at all?
> > > >
> > > > There is one more option, instead of passing the pm8008_data, we could
> > > > pass the pdev->dev.parent and get the pm8008 chip data directly in the
> > > > pm8008_get_regmap() like below
> > > >
> > > >
> > > > struct regmap *pm8008_get_regmap(const struct device *dev)
> > > > {
> > > > const struct pm8008_data *chip = dev_get_drvdata(dev);
> > > >
> > > > return chip->regulators_regmap;
> > > > }
> > > > EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > >
> > > >
> > > > By doing this we can avoid having declaration of pm8008_data also in the
> > > > header. Please let me know if this looks good.
> > > >
> > > Could you please confirm on this?
> > >
> > > > > What's preventing you from passing 'regmap'?
> > > >
> > > > I didn't get what you meant here, could you please elaborate a bit?
> > Ah yes. I authored you a patch, but became distracted. Here:
> >
> > -----8<--------------------8<-------
> >
> > From: Lee Jones <lee.jones@linaro.org>
> >
> > mfd: pm8008: Remove driver data structure pm8008_data
> > Maintaining a local driver data structure that is never shared
> > outside of the core device is an unnecessary complexity. Half of the
> > attributes were not used outside of a single function, one of which
> > was not used at all. The remaining 2 are generic and can be passed
> > around as required.
>
>
> Okay, but we still need to store the regulators_regmap, which is required in
> the pm8008 regulator driver. Could we use a global variable for it?
Look down ...
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
> > 1 file changed, 20 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > index c472d7f8103c4..4b8ff947762f2 100644
> > --- a/drivers/mfd/qcom-pm8008.c
> > +++ b/drivers/mfd/qcom-pm8008.c
> > @@ -54,13 +54,6 @@ enum {
> > #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
> > -struct pm8008_data {
> > - struct device *dev;
> > - struct regmap *regmap;
> > - int irq;
> > - struct regmap_irq_chip_data *irq_data;
> > -};
> > -
> > static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
> > static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
> > static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
> > @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> > .max_register = 0xFFFF,
> > };
> > -static int pm8008_init(struct pm8008_data *chip)
> > +static int pm8008_init(struct regmap *regmap)
> > {
> > int rc;
> > @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
> > * This is required to enable the writing of TYPE registers in
> > * regmap_irq_sync_unlock().
> > */
> > - rc = regmap_write(chip->regmap,
> > - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
> > - BIT(0));
> > + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > if (rc)
> > return rc;
> > /* Do the same for GPIO1 and GPIO2 peripherals */
> > - rc = regmap_write(chip->regmap,
> > - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > if (rc)
> > return rc;
> > - rc = regmap_write(chip->regmap,
> > - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > return rc;
> > }
> > -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > +static int pm8008_probe_irq_peripherals(struct device *dev,
> > + struct regmap *regmap,
> > int client_irq)
> > {
> > int rc, i;
> > struct regmap_irq_type *type;
> > struct regmap_irq_chip_data *irq_data;
> > - rc = pm8008_init(chip);
> > + rc = pm8008_init(regmap);
> > if (rc) {
> > - dev_err(chip->dev, "Init failed: %d\n", rc);
> > + dev_err(dev, "Init failed: %d\n", rc);
> > return rc;
> > }
> > @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> > }
> > - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
> > + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> > if (rc) {
> > - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
> > + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> > return rc;
> > }
> > @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > static int pm8008_probe(struct i2c_client *client)
> > {
> > int rc;
> > - struct pm8008_data *chip;
> > -
> > - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > - if (!chip)
> > - return -ENOMEM;
> > + struct device *dev;
> > + struct regmap *regmap;
> > - chip->dev = &client->dev;
> > - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > - if (!chip->regmap)
> > + dev = &client->dev;
> > + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > + if (!regmap)
> > return -ENODEV;
> > - i2c_set_clientdata(client, chip);
> > + i2c_set_clientdata(client, regmap);
Here ^
> > - if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
> > - rc = pm8008_probe_irq_peripherals(chip, client->irq);
> > + if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> > + rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
> > if (rc)
> > - dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
> > + dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > }
> > - return devm_of_platform_populate(chip->dev);
> > + return devm_of_platform_populate(dev);
> > }
> > static const struct of_device_id pm8008_match[] = {
> >
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-28 7:42 ` Lee Jones
@ 2022-06-29 10:36 ` Satya Priya Kakitapalli (Temp)
2022-06-29 15:18 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-06-29 10:36 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On 6/28/2022 1:12 PM, Lee Jones wrote:
> On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> On 6/27/2022 1:11 PM, Lee Jones wrote:
>>> On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>
>>>> Hi Lee,
>>>>
>>>>
>>>> On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
>>>>> On 6/20/2022 1:50 PM, Lee Jones wrote:
>>>>>> On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>
>>>>>>> On 6/17/2022 2:27 AM, Lee Jones wrote:
>>>>>>>> On Tue, 14 Jun 2022, Satya Priya wrote:
>>>>>>>>
>>>>>>>>> Use i2c_new_dummy_device() to register pm8008-regulator
>>>>>>>>> client present at a different address space, instead of
>>>>>>>>> defining a separate DT node. This avoids calling the probe
>>>>>>>>> twice for the same chip, once for each client pm8008-infra
>>>>>>>>> and pm8008-regulator.
>>>>>>>>>
>>>>>>>>> As a part of this define pm8008_regmap_init() to do regmap
>>>>>>>>> init for both the clients and define pm8008_get_regmap() to
>>>>>>>>> pass the regmap to the regulator driver.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>>>>>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>>>>>> ---
>>>>>>>>> Changes in V15:
>>>>>>>>> - None.
>>>>>>>>>
>>>>>>>>> Changes in V14:
>>>>>>>>> - None.
>>>>>>>>>
>>>>>>>>> Changes in V13:
>>>>>>>>> - None.
>>>>>>>>>
>>>>>>>>> drivers/mfd/qcom-pm8008.c | 34
>>>>>>>>> ++++++++++++++++++++++++++++++++--
>>>>>>>>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>>>>>>>>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>>>>>>>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>>>>> index 569ffd50..55e2a8e 100644
>>>>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>>>>> @@ -9,6 +9,7 @@
>>>>>>>>> #include <linux/interrupt.h>
>>>>>>>>> #include <linux/irq.h>
>>>>>>>>> #include <linux/irqdomain.h>
>>>>>>>>> +#include <linux/mfd/qcom_pm8008.h>
>>>>>>>>> #include <linux/module.h>
>>>>>>>>> #include <linux/of_device.h>
>>>>>>>>> #include <linux/of_platform.h>
>>>>>>>>> @@ -57,6 +58,7 @@ enum {
>>>>>>>>> struct pm8008_data {
>>>>>>>>> struct device *dev;
>>>>>>>>> + struct regmap *regulators_regmap;
>>>>>>>>> int irq;
>>>>>>>>> struct regmap_irq_chip_data *irq_data;
>>>>>>>>> };
>>>>>>>>> @@ -150,6 +152,12 @@ static struct regmap_config
>>>>>>>>> qcom_mfd_regmap_cfg = {
>>>>>>>>> .max_register = 0xFFFF,
>>>>>>>>> };
>>>>>>>>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>>>>>>>>> +{
>>>>>>>>> + return chip->regulators_regmap;
>>>>>>>>> +}
>>>>>>>>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>>>> Seems like abstraction for the sake of abstraction.
>>>>>>>>
>>>>>>>> Why not do the dereference inside the regulator driver?
>>>>>>> To derefer this in the regulator driver, we need to have the
>>>>>>> pm8008_data
>>>>>>> struct definition in the qcom_pm8008 header file.
>>>>>>>
>>>>>>> I think it doesn't look great to have only that structure in
>>>>>>> header and all
>>>>>>> other structs and enum in the mfd driver.
>>>>>> Then why pass 'pm8008_data' at all?
>>>>> There is one more option, instead of passing the pm8008_data, we could
>>>>> pass the pdev->dev.parent and get the pm8008 chip data directly in the
>>>>> pm8008_get_regmap() like below
>>>>>
>>>>>
>>>>> struct regmap *pm8008_get_regmap(const struct device *dev)
>>>>> {
>>>>> const struct pm8008_data *chip = dev_get_drvdata(dev);
>>>>>
>>>>> return chip->regulators_regmap;
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>
>>>>>
>>>>> By doing this we can avoid having declaration of pm8008_data also in the
>>>>> header. Please let me know if this looks good.
>>>>>
>>>> Could you please confirm on this?
>>>>
>>>>>> What's preventing you from passing 'regmap'?
>>>>> I didn't get what you meant here, could you please elaborate a bit?
>>> Ah yes. I authored you a patch, but became distracted. Here:
>>>
>>> -----8<--------------------8<-------
>>>
>>> From: Lee Jones <lee.jones@linaro.org>
>>>
>>> mfd: pm8008: Remove driver data structure pm8008_data
>>> Maintaining a local driver data structure that is never shared
>>> outside of the core device is an unnecessary complexity. Half of the
>>> attributes were not used outside of a single function, one of which
>>> was not used at all. The remaining 2 are generic and can be passed
>>> around as required.
>>
>> Okay, but we still need to store the regulators_regmap, which is required in
>> the pm8008 regulator driver. Could we use a global variable for it?
> Look down ...
>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>> drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
>>> 1 file changed, 20 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>> index c472d7f8103c4..4b8ff947762f2 100644
>>> --- a/drivers/mfd/qcom-pm8008.c
>>> +++ b/drivers/mfd/qcom-pm8008.c
>>> @@ -54,13 +54,6 @@ enum {
>>> #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
>>> -struct pm8008_data {
>>> - struct device *dev;
>>> - struct regmap *regmap;
>>> - int irq;
>>> - struct regmap_irq_chip_data *irq_data;
>>> -};
>>> -
>>> static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
>>> static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
>>> static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
>>> @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>>> .max_register = 0xFFFF,
>>> };
>>> -static int pm8008_init(struct pm8008_data *chip)
>>> +static int pm8008_init(struct regmap *regmap)
>>> {
>>> int rc;
>>> @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
>>> * This is required to enable the writing of TYPE registers in
>>> * regmap_irq_sync_unlock().
>>> */
>>> - rc = regmap_write(chip->regmap,
>>> - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
>>> - BIT(0));
>>> + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>> if (rc)
>>> return rc;
>>> /* Do the same for GPIO1 and GPIO2 peripherals */
>>> - rc = regmap_write(chip->regmap,
>>> - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>> + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>> if (rc)
>>> return rc;
>>> - rc = regmap_write(chip->regmap,
>>> - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>> + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>> return rc;
>>> }
>>> -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>> +static int pm8008_probe_irq_peripherals(struct device *dev,
>>> + struct regmap *regmap,
>>> int client_irq)
>>> {
>>> int rc, i;
>>> struct regmap_irq_type *type;
>>> struct regmap_irq_chip_data *irq_data;
>>> - rc = pm8008_init(chip);
>>> + rc = pm8008_init(regmap);
>>> if (rc) {
>>> - dev_err(chip->dev, "Init failed: %d\n", rc);
>>> + dev_err(dev, "Init failed: %d\n", rc);
>>> return rc;
>>> }
>>> @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>> IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>>> }
>>> - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
>>> + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
>>> IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>>> if (rc) {
>>> - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
>>> + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
>>> return rc;
>>> }
>>> @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>> static int pm8008_probe(struct i2c_client *client)
>>> {
>>> int rc;
>>> - struct pm8008_data *chip;
>>> -
>>> - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>> - if (!chip)
>>> - return -ENOMEM;
>>> + struct device *dev;
>>> + struct regmap *regmap;
>>> - chip->dev = &client->dev;
>>> - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>> - if (!chip->regmap)
>>> + dev = &client->dev;
>>> + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>> + if (!regmap)
>>> return -ENODEV;
>>> - i2c_set_clientdata(client, chip);
>>> + i2c_set_clientdata(client, regmap);
> Here ^
I have added a dummy device and set the client data by passing regmap,
see below:
+ regulators_client = i2c_new_dummy_device(client->adapter,
client->addr + 1);
+ if (IS_ERR(regulators_client)) {
+ dev_err(dev, "can't attach client\n");
+ return PTR_ERR(regulators_client);
+ }
+
+ regulators_regmap = devm_regmap_init_i2c(regulators_client,
&qcom_mfd_regmap_cfg[1]);
+ if (!regmap)
+ return -ENODEV;
+
+ i2c_set_clientdata(client, regulators_regmap);
Now if i try to get this regmap from regulator driver by doing
struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
it still gets me the regmap of pm8008@8 device and not the regulator
device regmap (0x9). Not sure if I'm missing something here.
>>> - if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
>>> - rc = pm8008_probe_irq_peripherals(chip, client->irq);
>>> + if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
>>> + rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
>>> if (rc)
>>> - dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>>> + dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
>>> }
>>> - return devm_of_platform_populate(chip->dev);
>>> + return devm_of_platform_populate(dev);
>>> }
>>> static const struct of_device_id pm8008_match[] = {
>>>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-29 10:36 ` Satya Priya Kakitapalli (Temp)
@ 2022-06-29 15:18 ` Lee Jones
2022-06-30 9:37 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-29 15:18 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Wed, 29 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> On 6/28/2022 1:12 PM, Lee Jones wrote:
> > On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> >
> > > On 6/27/2022 1:11 PM, Lee Jones wrote:
> > > > On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > >
> > > > > Hi Lee,
> > > > >
> > > > >
> > > > > On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > On 6/20/2022 1:50 PM, Lee Jones wrote:
> > > > > > > On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > >
> > > > > > > > On 6/17/2022 2:27 AM, Lee Jones wrote:
> > > > > > > > > On Tue, 14 Jun 2022, Satya Priya wrote:
> > > > > > > > >
> > > > > > > > > > Use i2c_new_dummy_device() to register pm8008-regulator
> > > > > > > > > > client present at a different address space, instead of
> > > > > > > > > > defining a separate DT node. This avoids calling the probe
> > > > > > > > > > twice for the same chip, once for each client pm8008-infra
> > > > > > > > > > and pm8008-regulator.
> > > > > > > > > >
> > > > > > > > > > As a part of this define pm8008_regmap_init() to do regmap
> > > > > > > > > > init for both the clients and define pm8008_get_regmap() to
> > > > > > > > > > pass the regmap to the regulator driver.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > > > > > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > > > > > > ---
> > > > > > > > > > Changes in V15:
> > > > > > > > > > - None.
> > > > > > > > > >
> > > > > > > > > > Changes in V14:
> > > > > > > > > > - None.
> > > > > > > > > >
> > > > > > > > > > Changes in V13:
> > > > > > > > > > - None.
> > > > > > > > > >
> > > > > > > > > > drivers/mfd/qcom-pm8008.c | 34
> > > > > > > > > > ++++++++++++++++++++++++++++++++--
> > > > > > > > > > include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> > > > > > > > > > 2 files changed, 41 insertions(+), 2 deletions(-)
> > > > > > > > > > create mode 100644 include/linux/mfd/qcom_pm8008.h
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > index 569ffd50..55e2a8e 100644
> > > > > > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > > > > #include <linux/interrupt.h>
> > > > > > > > > > #include <linux/irq.h>
> > > > > > > > > > #include <linux/irqdomain.h>
> > > > > > > > > > +#include <linux/mfd/qcom_pm8008.h>
> > > > > > > > > > #include <linux/module.h>
> > > > > > > > > > #include <linux/of_device.h>
> > > > > > > > > > #include <linux/of_platform.h>
> > > > > > > > > > @@ -57,6 +58,7 @@ enum {
> > > > > > > > > > struct pm8008_data {
> > > > > > > > > > struct device *dev;
> > > > > > > > > > + struct regmap *regulators_regmap;
> > > > > > > > > > int irq;
> > > > > > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > > > > > };
> > > > > > > > > > @@ -150,6 +152,12 @@ static struct regmap_config
> > > > > > > > > > qcom_mfd_regmap_cfg = {
> > > > > > > > > > .max_register = 0xFFFF,
> > > > > > > > > > };
> > > > > > > > > > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > > > > > > > > > +{
> > > > > > > > > > + return chip->regulators_regmap;
> > > > > > > > > > +}
> > > > > > > > > > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > > > Seems like abstraction for the sake of abstraction.
> > > > > > > > >
> > > > > > > > > Why not do the dereference inside the regulator driver?
> > > > > > > > To derefer this in the regulator driver, we need to have the
> > > > > > > > pm8008_data
> > > > > > > > struct definition in the qcom_pm8008 header file.
> > > > > > > >
> > > > > > > > I think it doesn't look great to have only that structure in
> > > > > > > > header and all
> > > > > > > > other structs and enum in the mfd driver.
> > > > > > > Then why pass 'pm8008_data' at all?
> > > > > > There is one more option, instead of passing the pm8008_data, we could
> > > > > > pass the pdev->dev.parent and get the pm8008 chip data directly in the
> > > > > > pm8008_get_regmap() like below
> > > > > >
> > > > > >
> > > > > > struct regmap *pm8008_get_regmap(const struct device *dev)
> > > > > > {
> > > > > > const struct pm8008_data *chip = dev_get_drvdata(dev);
> > > > > >
> > > > > > return chip->regulators_regmap;
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > >
> > > > > >
> > > > > > By doing this we can avoid having declaration of pm8008_data also in the
> > > > > > header. Please let me know if this looks good.
> > > > > >
> > > > > Could you please confirm on this?
> > > > >
> > > > > > > What's preventing you from passing 'regmap'?
> > > > > > I didn't get what you meant here, could you please elaborate a bit?
> > > > Ah yes. I authored you a patch, but became distracted. Here:
> > > >
> > > > -----8<--------------------8<-------
> > > >
> > > > From: Lee Jones <lee.jones@linaro.org>
> > > >
> > > > mfd: pm8008: Remove driver data structure pm8008_data
> > > > Maintaining a local driver data structure that is never shared
> > > > outside of the core device is an unnecessary complexity. Half of the
> > > > attributes were not used outside of a single function, one of which
> > > > was not used at all. The remaining 2 are generic and can be passed
> > > > around as required.
> > >
> > > Okay, but we still need to store the regulators_regmap, which is required in
> > > the pm8008 regulator driver. Could we use a global variable for it?
> > Look down ...
> >
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > > drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
> > > > 1 file changed, 20 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > index c472d7f8103c4..4b8ff947762f2 100644
> > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > @@ -54,13 +54,6 @@ enum {
> > > > #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
> > > > -struct pm8008_data {
> > > > - struct device *dev;
> > > > - struct regmap *regmap;
> > > > - int irq;
> > > > - struct regmap_irq_chip_data *irq_data;
> > > > -};
> > > > -
> > > > static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
> > > > static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
> > > > static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
> > > > @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> > > > .max_register = 0xFFFF,
> > > > };
> > > > -static int pm8008_init(struct pm8008_data *chip)
> > > > +static int pm8008_init(struct regmap *regmap)
> > > > {
> > > > int rc;
> > > > @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
> > > > * This is required to enable the writing of TYPE registers in
> > > > * regmap_irq_sync_unlock().
> > > > */
> > > > - rc = regmap_write(chip->regmap,
> > > > - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
> > > > - BIT(0));
> > > > + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > if (rc)
> > > > return rc;
> > > > /* Do the same for GPIO1 and GPIO2 peripherals */
> > > > - rc = regmap_write(chip->regmap,
> > > > - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > if (rc)
> > > > return rc;
> > > > - rc = regmap_write(chip->regmap,
> > > > - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > return rc;
> > > > }
> > > > -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > +static int pm8008_probe_irq_peripherals(struct device *dev,
> > > > + struct regmap *regmap,
> > > > int client_irq)
> > > > {
> > > > int rc, i;
> > > > struct regmap_irq_type *type;
> > > > struct regmap_irq_chip_data *irq_data;
> > > > - rc = pm8008_init(chip);
> > > > + rc = pm8008_init(regmap);
> > > > if (rc) {
> > > > - dev_err(chip->dev, "Init failed: %d\n", rc);
> > > > + dev_err(dev, "Init failed: %d\n", rc);
> > > > return rc;
> > > > }
> > > > @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> > > > }
> > > > - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
> > > > + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> > > > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> > > > if (rc) {
> > > > - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
> > > > + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> > > > return rc;
> > > > }
> > > > @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > static int pm8008_probe(struct i2c_client *client)
> > > > {
> > > > int rc;
> > > > - struct pm8008_data *chip;
> > > > -
> > > > - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > > > - if (!chip)
> > > > - return -ENOMEM;
> > > > + struct device *dev;
> > > > + struct regmap *regmap;
> > > > - chip->dev = &client->dev;
> > > > - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > - if (!chip->regmap)
> > > > + dev = &client->dev;
> > > > + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > + if (!regmap)
> > > > return -ENODEV;
> > > > - i2c_set_clientdata(client, chip);
> > > > + i2c_set_clientdata(client, regmap);
> > Here ^
>
>
> I have added a dummy device and set the client data by passing regmap, see
> below:
>
> + regulators_client = i2c_new_dummy_device(client->adapter,
> client->addr + 1);
> + if (IS_ERR(regulators_client)) {
> + dev_err(dev, "can't attach client\n");
> + return PTR_ERR(regulators_client);
> + }
> +
> + regulators_regmap = devm_regmap_init_i2c(regulators_client,
> &qcom_mfd_regmap_cfg[1]);
> + if (!regmap)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, regulators_regmap);
>
> Now if i try to get this regmap from regulator driver by doing
>
> struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
>
> it still gets me the regmap of pm8008@8 device and not the regulator device
> regmap (0x9). Not sure if I'm missing something here.
So you need to pass 2 regmap pointers?
If you need to pass more than one item to the child devices, you do
need to use a struct for that.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-29 15:18 ` Lee Jones
@ 2022-06-30 9:37 ` Satya Priya Kakitapalli (Temp)
2022-06-30 10:34 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-06-30 9:37 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On 6/29/2022 8:48 PM, Lee Jones wrote:
> On Wed, 29 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> On 6/28/2022 1:12 PM, Lee Jones wrote:
>>> On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>
>>>> On 6/27/2022 1:11 PM, Lee Jones wrote:
>>>>> On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>
>>>>>> Hi Lee,
>>>>>>
>>>>>>
>>>>>> On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>> On 6/20/2022 1:50 PM, Lee Jones wrote:
>>>>>>>> On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>>
>>>>>>>>> On 6/17/2022 2:27 AM, Lee Jones wrote:
>>>>>>>>>> On Tue, 14 Jun 2022, Satya Priya wrote:
>>>>>>>>>>
>>>>>>>>>>> Use i2c_new_dummy_device() to register pm8008-regulator
>>>>>>>>>>> client present at a different address space, instead of
>>>>>>>>>>> defining a separate DT node. This avoids calling the probe
>>>>>>>>>>> twice for the same chip, once for each client pm8008-infra
>>>>>>>>>>> and pm8008-regulator.
>>>>>>>>>>>
>>>>>>>>>>> As a part of this define pm8008_regmap_init() to do regmap
>>>>>>>>>>> init for both the clients and define pm8008_get_regmap() to
>>>>>>>>>>> pass the regmap to the regulator driver.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>>>>>>>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>>>>>>>> ---
>>>>>>>>>>> Changes in V15:
>>>>>>>>>>> - None.
>>>>>>>>>>>
>>>>>>>>>>> Changes in V14:
>>>>>>>>>>> - None.
>>>>>>>>>>>
>>>>>>>>>>> Changes in V13:
>>>>>>>>>>> - None.
>>>>>>>>>>>
>>>>>>>>>>> drivers/mfd/qcom-pm8008.c | 34
>>>>>>>>>>> ++++++++++++++++++++++++++++++++--
>>>>>>>>>>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>>>>>>>>>>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>>>>>>>>>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>> index 569ffd50..55e2a8e 100644
>>>>>>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>> @@ -9,6 +9,7 @@
>>>>>>>>>>> #include <linux/interrupt.h>
>>>>>>>>>>> #include <linux/irq.h>
>>>>>>>>>>> #include <linux/irqdomain.h>
>>>>>>>>>>> +#include <linux/mfd/qcom_pm8008.h>
>>>>>>>>>>> #include <linux/module.h>
>>>>>>>>>>> #include <linux/of_device.h>
>>>>>>>>>>> #include <linux/of_platform.h>
>>>>>>>>>>> @@ -57,6 +58,7 @@ enum {
>>>>>>>>>>> struct pm8008_data {
>>>>>>>>>>> struct device *dev;
>>>>>>>>>>> + struct regmap *regulators_regmap;
>>>>>>>>>>> int irq;
>>>>>>>>>>> struct regmap_irq_chip_data *irq_data;
>>>>>>>>>>> };
>>>>>>>>>>> @@ -150,6 +152,12 @@ static struct regmap_config
>>>>>>>>>>> qcom_mfd_regmap_cfg = {
>>>>>>>>>>> .max_register = 0xFFFF,
>>>>>>>>>>> };
>>>>>>>>>>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>>>>>>>>>>> +{
>>>>>>>>>>> + return chip->regulators_regmap;
>>>>>>>>>>> +}
>>>>>>>>>>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>>>>>> Seems like abstraction for the sake of abstraction.
>>>>>>>>>>
>>>>>>>>>> Why not do the dereference inside the regulator driver?
>>>>>>>>> To derefer this in the regulator driver, we need to have the
>>>>>>>>> pm8008_data
>>>>>>>>> struct definition in the qcom_pm8008 header file.
>>>>>>>>>
>>>>>>>>> I think it doesn't look great to have only that structure in
>>>>>>>>> header and all
>>>>>>>>> other structs and enum in the mfd driver.
>>>>>>>> Then why pass 'pm8008_data' at all?
>>>>>>> There is one more option, instead of passing the pm8008_data, we could
>>>>>>> pass the pdev->dev.parent and get the pm8008 chip data directly in the
>>>>>>> pm8008_get_regmap() like below
>>>>>>>
>>>>>>>
>>>>>>> struct regmap *pm8008_get_regmap(const struct device *dev)
>>>>>>> {
>>>>>>> const struct pm8008_data *chip = dev_get_drvdata(dev);
>>>>>>>
>>>>>>> return chip->regulators_regmap;
>>>>>>> }
>>>>>>> EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>>>
>>>>>>>
>>>>>>> By doing this we can avoid having declaration of pm8008_data also in the
>>>>>>> header. Please let me know if this looks good.
>>>>>>>
>>>>>> Could you please confirm on this?
>>>>>>
>>>>>>>> What's preventing you from passing 'regmap'?
>>>>>>> I didn't get what you meant here, could you please elaborate a bit?
>>>>> Ah yes. I authored you a patch, but became distracted. Here:
>>>>>
>>>>> -----8<--------------------8<-------
>>>>>
>>>>> From: Lee Jones <lee.jones@linaro.org>
>>>>>
>>>>> mfd: pm8008: Remove driver data structure pm8008_data
>>>>> Maintaining a local driver data structure that is never shared
>>>>> outside of the core device is an unnecessary complexity. Half of the
>>>>> attributes were not used outside of a single function, one of which
>>>>> was not used at all. The remaining 2 are generic and can be passed
>>>>> around as required.
>>>> Okay, but we still need to store the regulators_regmap, which is required in
>>>> the pm8008 regulator driver. Could we use a global variable for it?
>>> Look down ...
>>>
>>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>>> ---
>>>>> drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
>>>>> 1 file changed, 20 insertions(+), 33 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>> index c472d7f8103c4..4b8ff947762f2 100644
>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>> @@ -54,13 +54,6 @@ enum {
>>>>> #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
>>>>> -struct pm8008_data {
>>>>> - struct device *dev;
>>>>> - struct regmap *regmap;
>>>>> - int irq;
>>>>> - struct regmap_irq_chip_data *irq_data;
>>>>> -};
>>>>> -
>>>>> static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
>>>>> static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
>>>>> static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
>>>>> @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>>>>> .max_register = 0xFFFF,
>>>>> };
>>>>> -static int pm8008_init(struct pm8008_data *chip)
>>>>> +static int pm8008_init(struct regmap *regmap)
>>>>> {
>>>>> int rc;
>>>>> @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
>>>>> * This is required to enable the writing of TYPE registers in
>>>>> * regmap_irq_sync_unlock().
>>>>> */
>>>>> - rc = regmap_write(chip->regmap,
>>>>> - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
>>>>> - BIT(0));
>>>>> + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>> if (rc)
>>>>> return rc;
>>>>> /* Do the same for GPIO1 and GPIO2 peripherals */
>>>>> - rc = regmap_write(chip->regmap,
>>>>> - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>> + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>> if (rc)
>>>>> return rc;
>>>>> - rc = regmap_write(chip->regmap,
>>>>> - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>> + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>> return rc;
>>>>> }
>>>>> -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>> +static int pm8008_probe_irq_peripherals(struct device *dev,
>>>>> + struct regmap *regmap,
>>>>> int client_irq)
>>>>> {
>>>>> int rc, i;
>>>>> struct regmap_irq_type *type;
>>>>> struct regmap_irq_chip_data *irq_data;
>>>>> - rc = pm8008_init(chip);
>>>>> + rc = pm8008_init(regmap);
>>>>> if (rc) {
>>>>> - dev_err(chip->dev, "Init failed: %d\n", rc);
>>>>> + dev_err(dev, "Init failed: %d\n", rc);
>>>>> return rc;
>>>>> }
>>>>> @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>> IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>>>>> }
>>>>> - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
>>>>> + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
>>>>> IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>>>>> if (rc) {
>>>>> - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
>>>>> + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
>>>>> return rc;
>>>>> }
>>>>> @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>> static int pm8008_probe(struct i2c_client *client)
>>>>> {
>>>>> int rc;
>>>>> - struct pm8008_data *chip;
>>>>> -
>>>>> - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>>>> - if (!chip)
>>>>> - return -ENOMEM;
>>>>> + struct device *dev;
>>>>> + struct regmap *regmap;
>>>>> - chip->dev = &client->dev;
>>>>> - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>>>> - if (!chip->regmap)
>>>>> + dev = &client->dev;
>>>>> + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>>>> + if (!regmap)
>>>>> return -ENODEV;
>>>>> - i2c_set_clientdata(client, chip);
>>>>> + i2c_set_clientdata(client, regmap);
>>> Here ^
>>
>> I have added a dummy device and set the client data by passing regmap, see
>> below:
>>
>> + regulators_client = i2c_new_dummy_device(client->adapter,
>> client->addr + 1);
>> + if (IS_ERR(regulators_client)) {
>> + dev_err(dev, "can't attach client\n");
>> + return PTR_ERR(regulators_client);
>> + }
>> +
>> + regulators_regmap = devm_regmap_init_i2c(regulators_client,
>> &qcom_mfd_regmap_cfg[1]);
>> + if (!regmap)
>> + return -ENODEV;
>> +
>> + i2c_set_clientdata(client, regulators_regmap);
>>
>> Now if i try to get this regmap from regulator driver by doing
>>
>> struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
>>
>> it still gets me the regmap of pm8008@8 device and not the regulator device
>> regmap (0x9). Not sure if I'm missing something here.
> So you need to pass 2 regmap pointers?
>
> If you need to pass more than one item to the child devices, you do
> need to use a struct for that.
I need to pass only one regmap out of the two, but i am not able to
retrieve the correct regmap simply by doing i2c_set_clientdata
probably because we are having all the child nodes under same DT node
and thus not able to distinguish based on the dev pointer
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-30 9:37 ` Satya Priya Kakitapalli (Temp)
@ 2022-06-30 10:34 ` Lee Jones
2022-07-01 6:46 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-06-30 10:34 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Thu, 30 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> On 6/29/2022 8:48 PM, Lee Jones wrote:
> > On Wed, 29 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> >
> > > On 6/28/2022 1:12 PM, Lee Jones wrote:
> > > > On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > >
> > > > > On 6/27/2022 1:11 PM, Lee Jones wrote:
> > > > > > On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > >
> > > > > > > Hi Lee,
> > > > > > >
> > > > > > >
> > > > > > > On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > > On 6/20/2022 1:50 PM, Lee Jones wrote:
> > > > > > > > > On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > > >
> > > > > > > > > > On 6/17/2022 2:27 AM, Lee Jones wrote:
> > > > > > > > > > > On Tue, 14 Jun 2022, Satya Priya wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Use i2c_new_dummy_device() to register pm8008-regulator
> > > > > > > > > > > > client present at a different address space, instead of
> > > > > > > > > > > > defining a separate DT node. This avoids calling the probe
> > > > > > > > > > > > twice for the same chip, once for each client pm8008-infra
> > > > > > > > > > > > and pm8008-regulator.
> > > > > > > > > > > >
> > > > > > > > > > > > As a part of this define pm8008_regmap_init() to do regmap
> > > > > > > > > > > > init for both the clients and define pm8008_get_regmap() to
> > > > > > > > > > > > pass the regmap to the regulator driver.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > > > > > > > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > > > > > > > > ---
> > > > > > > > > > > > Changes in V15:
> > > > > > > > > > > > - None.
> > > > > > > > > > > >
> > > > > > > > > > > > Changes in V14:
> > > > > > > > > > > > - None.
> > > > > > > > > > > >
> > > > > > > > > > > > Changes in V13:
> > > > > > > > > > > > - None.
> > > > > > > > > > > >
> > > > > > > > > > > > drivers/mfd/qcom-pm8008.c | 34
> > > > > > > > > > > > ++++++++++++++++++++++++++++++++--
> > > > > > > > > > > > include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> > > > > > > > > > > > 2 files changed, 41 insertions(+), 2 deletions(-)
> > > > > > > > > > > > create mode 100644 include/linux/mfd/qcom_pm8008.h
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > index 569ffd50..55e2a8e 100644
> > > > > > > > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > > > > > > #include <linux/interrupt.h>
> > > > > > > > > > > > #include <linux/irq.h>
> > > > > > > > > > > > #include <linux/irqdomain.h>
> > > > > > > > > > > > +#include <linux/mfd/qcom_pm8008.h>
> > > > > > > > > > > > #include <linux/module.h>
> > > > > > > > > > > > #include <linux/of_device.h>
> > > > > > > > > > > > #include <linux/of_platform.h>
> > > > > > > > > > > > @@ -57,6 +58,7 @@ enum {
> > > > > > > > > > > > struct pm8008_data {
> > > > > > > > > > > > struct device *dev;
> > > > > > > > > > > > + struct regmap *regulators_regmap;
> > > > > > > > > > > > int irq;
> > > > > > > > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > > > > > > > };
> > > > > > > > > > > > @@ -150,6 +152,12 @@ static struct regmap_config
> > > > > > > > > > > > qcom_mfd_regmap_cfg = {
> > > > > > > > > > > > .max_register = 0xFFFF,
> > > > > > > > > > > > };
> > > > > > > > > > > > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + return chip->regulators_regmap;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > > > > > Seems like abstraction for the sake of abstraction.
> > > > > > > > > > >
> > > > > > > > > > > Why not do the dereference inside the regulator driver?
> > > > > > > > > > To derefer this in the regulator driver, we need to have the
> > > > > > > > > > pm8008_data
> > > > > > > > > > struct definition in the qcom_pm8008 header file.
> > > > > > > > > >
> > > > > > > > > > I think it doesn't look great to have only that structure in
> > > > > > > > > > header and all
> > > > > > > > > > other structs and enum in the mfd driver.
> > > > > > > > > Then why pass 'pm8008_data' at all?
> > > > > > > > There is one more option, instead of passing the pm8008_data, we could
> > > > > > > > pass the pdev->dev.parent and get the pm8008 chip data directly in the
> > > > > > > > pm8008_get_regmap() like below
> > > > > > > >
> > > > > > > >
> > > > > > > > struct regmap *pm8008_get_regmap(const struct device *dev)
> > > > > > > > {
> > > > > > > > const struct pm8008_data *chip = dev_get_drvdata(dev);
> > > > > > > >
> > > > > > > > return chip->regulators_regmap;
> > > > > > > > }
> > > > > > > > EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > >
> > > > > > > >
> > > > > > > > By doing this we can avoid having declaration of pm8008_data also in the
> > > > > > > > header. Please let me know if this looks good.
> > > > > > > >
> > > > > > > Could you please confirm on this?
> > > > > > >
> > > > > > > > > What's preventing you from passing 'regmap'?
> > > > > > > > I didn't get what you meant here, could you please elaborate a bit?
> > > > > > Ah yes. I authored you a patch, but became distracted. Here:
> > > > > >
> > > > > > -----8<--------------------8<-------
> > > > > >
> > > > > > From: Lee Jones <lee.jones@linaro.org>
> > > > > >
> > > > > > mfd: pm8008: Remove driver data structure pm8008_data
> > > > > > Maintaining a local driver data structure that is never shared
> > > > > > outside of the core device is an unnecessary complexity. Half of the
> > > > > > attributes were not used outside of a single function, one of which
> > > > > > was not used at all. The remaining 2 are generic and can be passed
> > > > > > around as required.
> > > > > Okay, but we still need to store the regulators_regmap, which is required in
> > > > > the pm8008 regulator driver. Could we use a global variable for it?
> > > > Look down ...
> > > >
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > ---
> > > > > > drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
> > > > > > 1 file changed, 20 insertions(+), 33 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > index c472d7f8103c4..4b8ff947762f2 100644
> > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > @@ -54,13 +54,6 @@ enum {
> > > > > > #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
> > > > > > -struct pm8008_data {
> > > > > > - struct device *dev;
> > > > > > - struct regmap *regmap;
> > > > > > - int irq;
> > > > > > - struct regmap_irq_chip_data *irq_data;
> > > > > > -};
> > > > > > -
> > > > > > static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
> > > > > > static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
> > > > > > static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
> > > > > > @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> > > > > > .max_register = 0xFFFF,
> > > > > > };
> > > > > > -static int pm8008_init(struct pm8008_data *chip)
> > > > > > +static int pm8008_init(struct regmap *regmap)
> > > > > > {
> > > > > > int rc;
> > > > > > @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
> > > > > > * This is required to enable the writing of TYPE registers in
> > > > > > * regmap_irq_sync_unlock().
> > > > > > */
> > > > > > - rc = regmap_write(chip->regmap,
> > > > > > - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
> > > > > > - BIT(0));
> > > > > > + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > if (rc)
> > > > > > return rc;
> > > > > > /* Do the same for GPIO1 and GPIO2 peripherals */
> > > > > > - rc = regmap_write(chip->regmap,
> > > > > > - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > if (rc)
> > > > > > return rc;
> > > > > > - rc = regmap_write(chip->regmap,
> > > > > > - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > return rc;
> > > > > > }
> > > > > > -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > +static int pm8008_probe_irq_peripherals(struct device *dev,
> > > > > > + struct regmap *regmap,
> > > > > > int client_irq)
> > > > > > {
> > > > > > int rc, i;
> > > > > > struct regmap_irq_type *type;
> > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > - rc = pm8008_init(chip);
> > > > > > + rc = pm8008_init(regmap);
> > > > > > if (rc) {
> > > > > > - dev_err(chip->dev, "Init failed: %d\n", rc);
> > > > > > + dev_err(dev, "Init failed: %d\n", rc);
> > > > > > return rc;
> > > > > > }
> > > > > > @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> > > > > > }
> > > > > > - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
> > > > > > + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> > > > > > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> > > > > > if (rc) {
> > > > > > - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
> > > > > > + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> > > > > > return rc;
> > > > > > }
> > > > > > @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > static int pm8008_probe(struct i2c_client *client)
> > > > > > {
> > > > > > int rc;
> > > > > > - struct pm8008_data *chip;
> > > > > > -
> > > > > > - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > > > > > - if (!chip)
> > > > > > - return -ENOMEM;
> > > > > > + struct device *dev;
> > > > > > + struct regmap *regmap;
> > > > > > - chip->dev = &client->dev;
> > > > > > - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > > > - if (!chip->regmap)
> > > > > > + dev = &client->dev;
> > > > > > + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > > > + if (!regmap)
> > > > > > return -ENODEV;
> > > > > > - i2c_set_clientdata(client, chip);
> > > > > > + i2c_set_clientdata(client, regmap);
> > > > Here ^
> > >
> > > I have added a dummy device and set the client data by passing regmap, see
> > > below:
> > >
> > > + regulators_client = i2c_new_dummy_device(client->adapter,
> > > client->addr + 1);
> > > + if (IS_ERR(regulators_client)) {
> > > + dev_err(dev, "can't attach client\n");
> > > + return PTR_ERR(regulators_client);
> > > + }
> > > +
> > > + regulators_regmap = devm_regmap_init_i2c(regulators_client,
> > > &qcom_mfd_regmap_cfg[1]);
> > > + if (!regmap)
> > > + return -ENODEV;
> > > +
> > > + i2c_set_clientdata(client, regulators_regmap);
> > >
> > > Now if i try to get this regmap from regulator driver by doing
> > >
> > > struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
> > >
> > > it still gets me the regmap of pm8008@8 device and not the regulator device
> > > regmap (0x9). Not sure if I'm missing something here.
> > So you need to pass 2 regmap pointers?
> >
> > If you need to pass more than one item to the child devices, you do
> > need to use a struct for that.
>
> I need to pass only one regmap out of the two, but i am not able to retrieve
> the correct regmap simply by doing i2c_set_clientdata
>
> probably because we are having all the child nodes under same DT node and
> thus not able to distinguish based on the dev pointer
You can only pull out (get) the pointer that you put in (set).
Unless you over-wrote it later in the thread of execution, you are
pulling out whatever regulators_regmap happens to be.
Is qcom_mfd_regmap_cfg[1] definitely the one you want?
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-06-30 10:34 ` Lee Jones
@ 2022-07-01 6:46 ` Satya Priya Kakitapalli (Temp)
2022-07-01 7:54 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-07-01 6:46 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On 6/30/2022 4:04 PM, Lee Jones wrote:
> On Thu, 30 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> On 6/29/2022 8:48 PM, Lee Jones wrote:
>>> On Wed, 29 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>
>>>> On 6/28/2022 1:12 PM, Lee Jones wrote:
>>>>> On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>
>>>>>> On 6/27/2022 1:11 PM, Lee Jones wrote:
>>>>>>> On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>
>>>>>>>> Hi Lee,
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>>> On 6/20/2022 1:50 PM, Lee Jones wrote:
>>>>>>>>>> On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>>>>
>>>>>>>>>>> On 6/17/2022 2:27 AM, Lee Jones wrote:
>>>>>>>>>>>> On Tue, 14 Jun 2022, Satya Priya wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Use i2c_new_dummy_device() to register pm8008-regulator
>>>>>>>>>>>>> client present at a different address space, instead of
>>>>>>>>>>>>> defining a separate DT node. This avoids calling the probe
>>>>>>>>>>>>> twice for the same chip, once for each client pm8008-infra
>>>>>>>>>>>>> and pm8008-regulator.
>>>>>>>>>>>>>
>>>>>>>>>>>>> As a part of this define pm8008_regmap_init() to do regmap
>>>>>>>>>>>>> init for both the clients and define pm8008_get_regmap() to
>>>>>>>>>>>>> pass the regmap to the regulator driver.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>>>>>>>>>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> Changes in V15:
>>>>>>>>>>>>> - None.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes in V14:
>>>>>>>>>>>>> - None.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes in V13:
>>>>>>>>>>>>> - None.
>>>>>>>>>>>>>
>>>>>>>>>>>>> drivers/mfd/qcom-pm8008.c | 34
>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++--
>>>>>>>>>>>>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>>>>>>>>>>>>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>>>>>>>>>>>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>>>> index 569ffd50..55e2a8e 100644
>>>>>>>>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>>>> @@ -9,6 +9,7 @@
>>>>>>>>>>>>> #include <linux/interrupt.h>
>>>>>>>>>>>>> #include <linux/irq.h>
>>>>>>>>>>>>> #include <linux/irqdomain.h>
>>>>>>>>>>>>> +#include <linux/mfd/qcom_pm8008.h>
>>>>>>>>>>>>> #include <linux/module.h>
>>>>>>>>>>>>> #include <linux/of_device.h>
>>>>>>>>>>>>> #include <linux/of_platform.h>
>>>>>>>>>>>>> @@ -57,6 +58,7 @@ enum {
>>>>>>>>>>>>> struct pm8008_data {
>>>>>>>>>>>>> struct device *dev;
>>>>>>>>>>>>> + struct regmap *regulators_regmap;
>>>>>>>>>>>>> int irq;
>>>>>>>>>>>>> struct regmap_irq_chip_data *irq_data;
>>>>>>>>>>>>> };
>>>>>>>>>>>>> @@ -150,6 +152,12 @@ static struct regmap_config
>>>>>>>>>>>>> qcom_mfd_regmap_cfg = {
>>>>>>>>>>>>> .max_register = 0xFFFF,
>>>>>>>>>>>>> };
>>>>>>>>>>>>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + return chip->regulators_regmap;
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>>>>>>>> Seems like abstraction for the sake of abstraction.
>>>>>>>>>>>>
>>>>>>>>>>>> Why not do the dereference inside the regulator driver?
>>>>>>>>>>> To derefer this in the regulator driver, we need to have the
>>>>>>>>>>> pm8008_data
>>>>>>>>>>> struct definition in the qcom_pm8008 header file.
>>>>>>>>>>>
>>>>>>>>>>> I think it doesn't look great to have only that structure in
>>>>>>>>>>> header and all
>>>>>>>>>>> other structs and enum in the mfd driver.
>>>>>>>>>> Then why pass 'pm8008_data' at all?
>>>>>>>>> There is one more option, instead of passing the pm8008_data, we could
>>>>>>>>> pass the pdev->dev.parent and get the pm8008 chip data directly in the
>>>>>>>>> pm8008_get_regmap() like below
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> struct regmap *pm8008_get_regmap(const struct device *dev)
>>>>>>>>> {
>>>>>>>>> const struct pm8008_data *chip = dev_get_drvdata(dev);
>>>>>>>>>
>>>>>>>>> return chip->regulators_regmap;
>>>>>>>>> }
>>>>>>>>> EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> By doing this we can avoid having declaration of pm8008_data also in the
>>>>>>>>> header. Please let me know if this looks good.
>>>>>>>>>
>>>>>>>> Could you please confirm on this?
>>>>>>>>
>>>>>>>>>> What's preventing you from passing 'regmap'?
>>>>>>>>> I didn't get what you meant here, could you please elaborate a bit?
>>>>>>> Ah yes. I authored you a patch, but became distracted. Here:
>>>>>>>
>>>>>>> -----8<--------------------8<-------
>>>>>>>
>>>>>>> From: Lee Jones <lee.jones@linaro.org>
>>>>>>>
>>>>>>> mfd: pm8008: Remove driver data structure pm8008_data
>>>>>>> Maintaining a local driver data structure that is never shared
>>>>>>> outside of the core device is an unnecessary complexity. Half of the
>>>>>>> attributes were not used outside of a single function, one of which
>>>>>>> was not used at all. The remaining 2 are generic and can be passed
>>>>>>> around as required.
>>>>>> Okay, but we still need to store the regulators_regmap, which is required in
>>>>>> the pm8008 regulator driver. Could we use a global variable for it?
>>>>> Look down ...
>>>>>
>>>>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>>>>> ---
>>>>>>> drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
>>>>>>> 1 file changed, 20 insertions(+), 33 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>>> index c472d7f8103c4..4b8ff947762f2 100644
>>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>>> @@ -54,13 +54,6 @@ enum {
>>>>>>> #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
>>>>>>> -struct pm8008_data {
>>>>>>> - struct device *dev;
>>>>>>> - struct regmap *regmap;
>>>>>>> - int irq;
>>>>>>> - struct regmap_irq_chip_data *irq_data;
>>>>>>> -};
>>>>>>> -
>>>>>>> static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
>>>>>>> static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
>>>>>>> static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
>>>>>>> @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>>>>>>> .max_register = 0xFFFF,
>>>>>>> };
>>>>>>> -static int pm8008_init(struct pm8008_data *chip)
>>>>>>> +static int pm8008_init(struct regmap *regmap)
>>>>>>> {
>>>>>>> int rc;
>>>>>>> @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
>>>>>>> * This is required to enable the writing of TYPE registers in
>>>>>>> * regmap_irq_sync_unlock().
>>>>>>> */
>>>>>>> - rc = regmap_write(chip->regmap,
>>>>>>> - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
>>>>>>> - BIT(0));
>>>>>>> + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>> if (rc)
>>>>>>> return rc;
>>>>>>> /* Do the same for GPIO1 and GPIO2 peripherals */
>>>>>>> - rc = regmap_write(chip->regmap,
>>>>>>> - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>> + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>> if (rc)
>>>>>>> return rc;
>>>>>>> - rc = regmap_write(chip->regmap,
>>>>>>> - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>> + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>> return rc;
>>>>>>> }
>>>>>>> -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>>>> +static int pm8008_probe_irq_peripherals(struct device *dev,
>>>>>>> + struct regmap *regmap,
>>>>>>> int client_irq)
>>>>>>> {
>>>>>>> int rc, i;
>>>>>>> struct regmap_irq_type *type;
>>>>>>> struct regmap_irq_chip_data *irq_data;
>>>>>>> - rc = pm8008_init(chip);
>>>>>>> + rc = pm8008_init(regmap);
>>>>>>> if (rc) {
>>>>>>> - dev_err(chip->dev, "Init failed: %d\n", rc);
>>>>>>> + dev_err(dev, "Init failed: %d\n", rc);
>>>>>>> return rc;
>>>>>>> }
>>>>>>> @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>>>> IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>>>>>>> }
>>>>>>> - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
>>>>>>> + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
>>>>>>> IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>>>>>>> if (rc) {
>>>>>>> - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
>>>>>>> + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
>>>>>>> return rc;
>>>>>>> }
>>>>>>> @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>>>> static int pm8008_probe(struct i2c_client *client)
>>>>>>> {
>>>>>>> int rc;
>>>>>>> - struct pm8008_data *chip;
>>>>>>> -
>>>>>>> - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>>>>>> - if (!chip)
>>>>>>> - return -ENOMEM;
>>>>>>> + struct device *dev;
>>>>>>> + struct regmap *regmap;
>>>>>>> - chip->dev = &client->dev;
>>>>>>> - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>>>>>> - if (!chip->regmap)
>>>>>>> + dev = &client->dev;
>>>>>>> + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>>>>>> + if (!regmap)
>>>>>>> return -ENODEV;
>>>>>>> - i2c_set_clientdata(client, chip);
>>>>>>> + i2c_set_clientdata(client, regmap);
>>>>> Here ^
>>>> I have added a dummy device and set the client data by passing regmap, see
>>>> below:
>>>>
>>>> + regulators_client = i2c_new_dummy_device(client->adapter,
>>>> client->addr + 1);
>>>> + if (IS_ERR(regulators_client)) {
>>>> + dev_err(dev, "can't attach client\n");
>>>> + return PTR_ERR(regulators_client);
>>>> + }
>>>> +
>>>> + regulators_regmap = devm_regmap_init_i2c(regulators_client,
>>>> &qcom_mfd_regmap_cfg[1]);
>>>> + if (!regmap)
>>>> + return -ENODEV;
>>>> +
>>>> + i2c_set_clientdata(client, regulators_regmap);
>>>>
>>>> Now if i try to get this regmap from regulator driver by doing
>>>>
>>>> struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
>>>>
>>>> it still gets me the regmap of pm8008@8 device and not the regulator device
>>>> regmap (0x9). Not sure if I'm missing something here.
>>> So you need to pass 2 regmap pointers?
>>>
>>> If you need to pass more than one item to the child devices, you do
>>> need to use a struct for that.
>> I need to pass only one regmap out of the two, but i am not able to retrieve
>> the correct regmap simply by doing i2c_set_clientdata
>>
>> probably because we are having all the child nodes under same DT node and
>> thus not able to distinguish based on the dev pointer
> You can only pull out (get) the pointer that you put in (set).
>
> Unless you over-wrote it later in the thread of execution, you are
> pulling out whatever regulators_regmap happens to be.
>
> Is qcom_mfd_regmap_cfg[1] definitely the one you want?
Yes, I need qcom_mfd_regmap_cfg[1]
Pasting code snippet for reference:
static struct regmap_config qcom_mfd_regmap_cfg[2] = {
{
.name = "infra",
.reg_bits = 16,
.val_bits = 8,
.max_register = 0xFFFF,
},
{
.name = "regulators",
.reg_bits = 16,
.val_bits = 8,
.max_register = 0xFFFF,
},
};
Inside pm8008_probe:
regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
if (!regmap)
return -ENODEV;
i2c_set_clientdata(client, regmap);
regulators_client = i2c_new_dummy_device(client->adapter,
client->addr + 1);
if (IS_ERR(regulators_client)) {
dev_err(dev, "can't attach client\n");
return PTR_ERR(regulators_client);
}
regulators_regmap = devm_regmap_init_i2c(regulators_client,
&qcom_mfd_regmap_cfg[1]);
if (!regmap)
return -ENODEV;
i2c_set_clientdata(regulators_client, regulators_regmap);
In qcom-pm8008-regulator.c I tried to get the regmap using
dev_get_regmap(pdev->dev.parent, "regulators");
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-07-01 6:46 ` Satya Priya Kakitapalli (Temp)
@ 2022-07-01 7:54 ` Lee Jones
2022-07-01 8:47 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-07-01 7:54 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Fri, 01 Jul 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> On 6/30/2022 4:04 PM, Lee Jones wrote:
> > On Thu, 30 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> >
> > > On 6/29/2022 8:48 PM, Lee Jones wrote:
> > > > On Wed, 29 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > >
> > > > > On 6/28/2022 1:12 PM, Lee Jones wrote:
> > > > > > On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > >
> > > > > > > On 6/27/2022 1:11 PM, Lee Jones wrote:
> > > > > > > > On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > >
> > > > > > > > > Hi Lee,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > > > > On 6/20/2022 1:50 PM, Lee Jones wrote:
> > > > > > > > > > > On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > > > > >
> > > > > > > > > > > > On 6/17/2022 2:27 AM, Lee Jones wrote:
> > > > > > > > > > > > > On Tue, 14 Jun 2022, Satya Priya wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Use i2c_new_dummy_device() to register pm8008-regulator
> > > > > > > > > > > > > > client present at a different address space, instead of
> > > > > > > > > > > > > > defining a separate DT node. This avoids calling the probe
> > > > > > > > > > > > > > twice for the same chip, once for each client pm8008-infra
> > > > > > > > > > > > > > and pm8008-regulator.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > As a part of this define pm8008_regmap_init() to do regmap
> > > > > > > > > > > > > > init for both the clients and define pm8008_get_regmap() to
> > > > > > > > > > > > > > pass the regmap to the regulator driver.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > > > > > > > > > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > Changes in V15:
> > > > > > > > > > > > > > - None.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Changes in V14:
> > > > > > > > > > > > > > - None.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Changes in V13:
> > > > > > > > > > > > > > - None.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > drivers/mfd/qcom-pm8008.c | 34
> > > > > > > > > > > > > > ++++++++++++++++++++++++++++++++--
> > > > > > > > > > > > > > include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> > > > > > > > > > > > > > 2 files changed, 41 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > create mode 100644 include/linux/mfd/qcom_pm8008.h
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > > > index 569ffd50..55e2a8e 100644
> > > > > > > > > > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > > > > > > > > #include <linux/interrupt.h>
> > > > > > > > > > > > > > #include <linux/irq.h>
> > > > > > > > > > > > > > #include <linux/irqdomain.h>
> > > > > > > > > > > > > > +#include <linux/mfd/qcom_pm8008.h>
> > > > > > > > > > > > > > #include <linux/module.h>
> > > > > > > > > > > > > > #include <linux/of_device.h>
> > > > > > > > > > > > > > #include <linux/of_platform.h>
> > > > > > > > > > > > > > @@ -57,6 +58,7 @@ enum {
> > > > > > > > > > > > > > struct pm8008_data {
> > > > > > > > > > > > > > struct device *dev;
> > > > > > > > > > > > > > + struct regmap *regulators_regmap;
> > > > > > > > > > > > > > int irq;
> > > > > > > > > > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > > > > > > > > > };
> > > > > > > > > > > > > > @@ -150,6 +152,12 @@ static struct regmap_config
> > > > > > > > > > > > > > qcom_mfd_regmap_cfg = {
> > > > > > > > > > > > > > .max_register = 0xFFFF,
> > > > > > > > > > > > > > };
> > > > > > > > > > > > > > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > + return chip->regulators_regmap;
> > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > > > > > > > Seems like abstraction for the sake of abstraction.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Why not do the dereference inside the regulator driver?
> > > > > > > > > > > > To derefer this in the regulator driver, we need to have the
> > > > > > > > > > > > pm8008_data
> > > > > > > > > > > > struct definition in the qcom_pm8008 header file.
> > > > > > > > > > > >
> > > > > > > > > > > > I think it doesn't look great to have only that structure in
> > > > > > > > > > > > header and all
> > > > > > > > > > > > other structs and enum in the mfd driver.
> > > > > > > > > > > Then why pass 'pm8008_data' at all?
> > > > > > > > > > There is one more option, instead of passing the pm8008_data, we could
> > > > > > > > > > pass the pdev->dev.parent and get the pm8008 chip data directly in the
> > > > > > > > > > pm8008_get_regmap() like below
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > struct regmap *pm8008_get_regmap(const struct device *dev)
> > > > > > > > > > {
> > > > > > > > > > const struct pm8008_data *chip = dev_get_drvdata(dev);
> > > > > > > > > >
> > > > > > > > > > return chip->regulators_regmap;
> > > > > > > > > > }
> > > > > > > > > > EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > By doing this we can avoid having declaration of pm8008_data also in the
> > > > > > > > > > header. Please let me know if this looks good.
> > > > > > > > > >
> > > > > > > > > Could you please confirm on this?
> > > > > > > > >
> > > > > > > > > > > What's preventing you from passing 'regmap'?
> > > > > > > > > > I didn't get what you meant here, could you please elaborate a bit?
> > > > > > > > Ah yes. I authored you a patch, but became distracted. Here:
> > > > > > > >
> > > > > > > > -----8<--------------------8<-------
> > > > > > > >
> > > > > > > > From: Lee Jones <lee.jones@linaro.org>
> > > > > > > >
> > > > > > > > mfd: pm8008: Remove driver data structure pm8008_data
> > > > > > > > Maintaining a local driver data structure that is never shared
> > > > > > > > outside of the core device is an unnecessary complexity. Half of the
> > > > > > > > attributes were not used outside of a single function, one of which
> > > > > > > > was not used at all. The remaining 2 are generic and can be passed
> > > > > > > > around as required.
> > > > > > > Okay, but we still need to store the regulators_regmap, which is required in
> > > > > > > the pm8008 regulator driver. Could we use a global variable for it?
> > > > > > Look down ...
> > > > > >
> > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > ---
> > > > > > > > drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
> > > > > > > > 1 file changed, 20 insertions(+), 33 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > > > index c472d7f8103c4..4b8ff947762f2 100644
> > > > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > > > @@ -54,13 +54,6 @@ enum {
> > > > > > > > #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
> > > > > > > > -struct pm8008_data {
> > > > > > > > - struct device *dev;
> > > > > > > > - struct regmap *regmap;
> > > > > > > > - int irq;
> > > > > > > > - struct regmap_irq_chip_data *irq_data;
> > > > > > > > -};
> > > > > > > > -
> > > > > > > > static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
> > > > > > > > static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
> > > > > > > > static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
> > > > > > > > @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> > > > > > > > .max_register = 0xFFFF,
> > > > > > > > };
> > > > > > > > -static int pm8008_init(struct pm8008_data *chip)
> > > > > > > > +static int pm8008_init(struct regmap *regmap)
> > > > > > > > {
> > > > > > > > int rc;
> > > > > > > > @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
> > > > > > > > * This is required to enable the writing of TYPE registers in
> > > > > > > > * regmap_irq_sync_unlock().
> > > > > > > > */
> > > > > > > > - rc = regmap_write(chip->regmap,
> > > > > > > > - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
> > > > > > > > - BIT(0));
> > > > > > > > + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > if (rc)
> > > > > > > > return rc;
> > > > > > > > /* Do the same for GPIO1 and GPIO2 peripherals */
> > > > > > > > - rc = regmap_write(chip->regmap,
> > > > > > > > - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > if (rc)
> > > > > > > > return rc;
> > > > > > > > - rc = regmap_write(chip->regmap,
> > > > > > > > - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > return rc;
> > > > > > > > }
> > > > > > > > -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > > > +static int pm8008_probe_irq_peripherals(struct device *dev,
> > > > > > > > + struct regmap *regmap,
> > > > > > > > int client_irq)
> > > > > > > > {
> > > > > > > > int rc, i;
> > > > > > > > struct regmap_irq_type *type;
> > > > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > > > - rc = pm8008_init(chip);
> > > > > > > > + rc = pm8008_init(regmap);
> > > > > > > > if (rc) {
> > > > > > > > - dev_err(chip->dev, "Init failed: %d\n", rc);
> > > > > > > > + dev_err(dev, "Init failed: %d\n", rc);
> > > > > > > > return rc;
> > > > > > > > }
> > > > > > > > @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > > > IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> > > > > > > > }
> > > > > > > > - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
> > > > > > > > + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> > > > > > > > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> > > > > > > > if (rc) {
> > > > > > > > - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
> > > > > > > > + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> > > > > > > > return rc;
> > > > > > > > }
> > > > > > > > @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > > > static int pm8008_probe(struct i2c_client *client)
> > > > > > > > {
> > > > > > > > int rc;
> > > > > > > > - struct pm8008_data *chip;
> > > > > > > > -
> > > > > > > > - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > > > > > > > - if (!chip)
> > > > > > > > - return -ENOMEM;
> > > > > > > > + struct device *dev;
> > > > > > > > + struct regmap *regmap;
> > > > > > > > - chip->dev = &client->dev;
> > > > > > > > - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > > > > > - if (!chip->regmap)
> > > > > > > > + dev = &client->dev;
> > > > > > > > + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > > > > > + if (!regmap)
> > > > > > > > return -ENODEV;
> > > > > > > > - i2c_set_clientdata(client, chip);
> > > > > > > > + i2c_set_clientdata(client, regmap);
> > > > > > Here ^
> > > > > I have added a dummy device and set the client data by passing regmap, see
> > > > > below:
> > > > >
> > > > > + regulators_client = i2c_new_dummy_device(client->adapter,
> > > > > client->addr + 1);
> > > > > + if (IS_ERR(regulators_client)) {
> > > > > + dev_err(dev, "can't attach client\n");
> > > > > + return PTR_ERR(regulators_client);
> > > > > + }
> > > > > +
> > > > > + regulators_regmap = devm_regmap_init_i2c(regulators_client,
> > > > > &qcom_mfd_regmap_cfg[1]);
> > > > > + if (!regmap)
> > > > > + return -ENODEV;
> > > > > +
> > > > > + i2c_set_clientdata(client, regulators_regmap);
> > > > >
> > > > > Now if i try to get this regmap from regulator driver by doing
> > > > >
> > > > > struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
> > > > >
> > > > > it still gets me the regmap of pm8008@8 device and not the regulator device
> > > > > regmap (0x9). Not sure if I'm missing something here.
> > > > So you need to pass 2 regmap pointers?
> > > >
> > > > If you need to pass more than one item to the child devices, you do
> > > > need to use a struct for that.
> > > I need to pass only one regmap out of the two, but i am not able to retrieve
> > > the correct regmap simply by doing i2c_set_clientdata
> > >
> > > probably because we are having all the child nodes under same DT node and
> > > thus not able to distinguish based on the dev pointer
> > You can only pull out (get) the pointer that you put in (set).
> >
> > Unless you over-wrote it later in the thread of execution, you are
> > pulling out whatever regulators_regmap happens to be.
> >
> > Is qcom_mfd_regmap_cfg[1] definitely the one you want?
>
>
> Yes, I need qcom_mfd_regmap_cfg[1]
>
> Pasting code snippet for reference:
>
> static struct regmap_config qcom_mfd_regmap_cfg[2] = {
> {
>
> .name = "infra",
> .reg_bits = 16,
> .val_bits = 8,
> .max_register = 0xFFFF,
> },
> {
> .name = "regulators",
> .reg_bits = 16,
> .val_bits = 8,
> .max_register = 0xFFFF,
> },
>
> };
>
>
> Inside pm8008_probe:
>
>
> regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
> if (!regmap)
> return -ENODEV;
>
> i2c_set_clientdata(client, regmap);
>
>
> regulators_client = i2c_new_dummy_device(client->adapter, client->addr
> + 1);
> if (IS_ERR(regulators_client)) {
> dev_err(dev, "can't attach client\n");
> return PTR_ERR(regulators_client);
> }
>
> regulators_regmap = devm_regmap_init_i2c(regulators_client,
> &qcom_mfd_regmap_cfg[1]);
> if (!regmap)
> return -ENODEV;
>
> i2c_set_clientdata(regulators_client, regulators_regmap);
You can't call this twice.
Doing so with over-write regmap with regulators_regmap.
You said you only needed one?
"I need to pass only one regmap out of the two, but i am not able to retrieve"
> In qcom-pm8008-regulator.c I tried to get the regmap using
>
> dev_get_regmap(pdev->dev.parent, "regulators");
I haven't looked at this API before. I suggest that this would be
used *instead* of passing the regmap pointer via driver_data.
It looks like you're using different devices to init your regmaps;
'client' and 'regulator_client' (derived from client->adapter).
"regulators" is registered using regulators_regmap which was *not*
init'ed with pdev->dev.parent (same as client->dev), so trying to
dev_get_regmap() with that device pointer will not work.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-07-01 7:54 ` Lee Jones
@ 2022-07-01 8:47 ` Satya Priya Kakitapalli (Temp)
2022-07-01 9:12 ` Lee Jones
0 siblings, 1 reply; 54+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-07-01 8:47 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On 7/1/2022 1:24 PM, Lee Jones wrote:
> On Fri, 01 Jul 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> On 6/30/2022 4:04 PM, Lee Jones wrote:
>>> On Thu, 30 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>
>>>> On 6/29/2022 8:48 PM, Lee Jones wrote:
>>>>> On Wed, 29 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>
>>>>>> On 6/28/2022 1:12 PM, Lee Jones wrote:
>>>>>>> On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>
>>>>>>>> On 6/27/2022 1:11 PM, Lee Jones wrote:
>>>>>>>>> On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>>>
>>>>>>>>>> Hi Lee,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>>>>> On 6/20/2022 1:50 PM, Lee Jones wrote:
>>>>>>>>>>>> On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> On 6/17/2022 2:27 AM, Lee Jones wrote:
>>>>>>>>>>>>>> On Tue, 14 Jun 2022, Satya Priya wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Use i2c_new_dummy_device() to register pm8008-regulator
>>>>>>>>>>>>>>> client present at a different address space, instead of
>>>>>>>>>>>>>>> defining a separate DT node. This avoids calling the probe
>>>>>>>>>>>>>>> twice for the same chip, once for each client pm8008-infra
>>>>>>>>>>>>>>> and pm8008-regulator.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> As a part of this define pm8008_regmap_init() to do regmap
>>>>>>>>>>>>>>> init for both the clients and define pm8008_get_regmap() to
>>>>>>>>>>>>>>> pass the regmap to the regulator driver.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>>>>>>>>>>>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> Changes in V15:
>>>>>>>>>>>>>>> - None.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Changes in V14:
>>>>>>>>>>>>>>> - None.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Changes in V13:
>>>>>>>>>>>>>>> - None.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> drivers/mfd/qcom-pm8008.c | 34
>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++--
>>>>>>>>>>>>>>> include/linux/mfd/qcom_pm8008.h | 9 +++++++++
>>>>>>>>>>>>>>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>>>>>>>>>>>>>> create mode 100644 include/linux/mfd/qcom_pm8008.h
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>>>>>> index 569ffd50..55e2a8e 100644
>>>>>>>>>>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>>>>>>>>>>> @@ -9,6 +9,7 @@
>>>>>>>>>>>>>>> #include <linux/interrupt.h>
>>>>>>>>>>>>>>> #include <linux/irq.h>
>>>>>>>>>>>>>>> #include <linux/irqdomain.h>
>>>>>>>>>>>>>>> +#include <linux/mfd/qcom_pm8008.h>
>>>>>>>>>>>>>>> #include <linux/module.h>
>>>>>>>>>>>>>>> #include <linux/of_device.h>
>>>>>>>>>>>>>>> #include <linux/of_platform.h>
>>>>>>>>>>>>>>> @@ -57,6 +58,7 @@ enum {
>>>>>>>>>>>>>>> struct pm8008_data {
>>>>>>>>>>>>>>> struct device *dev;
>>>>>>>>>>>>>>> + struct regmap *regulators_regmap;
>>>>>>>>>>>>>>> int irq;
>>>>>>>>>>>>>>> struct regmap_irq_chip_data *irq_data;
>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>> @@ -150,6 +152,12 @@ static struct regmap_config
>>>>>>>>>>>>>>> qcom_mfd_regmap_cfg = {
>>>>>>>>>>>>>>> .max_register = 0xFFFF,
>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + return chip->regulators_regmap;
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>>>>>>>>>> Seems like abstraction for the sake of abstraction.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Why not do the dereference inside the regulator driver?
>>>>>>>>>>>>> To derefer this in the regulator driver, we need to have the
>>>>>>>>>>>>> pm8008_data
>>>>>>>>>>>>> struct definition in the qcom_pm8008 header file.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think it doesn't look great to have only that structure in
>>>>>>>>>>>>> header and all
>>>>>>>>>>>>> other structs and enum in the mfd driver.
>>>>>>>>>>>> Then why pass 'pm8008_data' at all?
>>>>>>>>>>> There is one more option, instead of passing the pm8008_data, we could
>>>>>>>>>>> pass the pdev->dev.parent and get the pm8008 chip data directly in the
>>>>>>>>>>> pm8008_get_regmap() like below
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> struct regmap *pm8008_get_regmap(const struct device *dev)
>>>>>>>>>>> {
>>>>>>>>>>> const struct pm8008_data *chip = dev_get_drvdata(dev);
>>>>>>>>>>>
>>>>>>>>>>> return chip->regulators_regmap;
>>>>>>>>>>> }
>>>>>>>>>>> EXPORT_SYMBOL_GPL(pm8008_get_regmap);
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> By doing this we can avoid having declaration of pm8008_data also in the
>>>>>>>>>>> header. Please let me know if this looks good.
>>>>>>>>>>>
>>>>>>>>>> Could you please confirm on this?
>>>>>>>>>>
>>>>>>>>>>>> What's preventing you from passing 'regmap'?
>>>>>>>>>>> I didn't get what you meant here, could you please elaborate a bit?
>>>>>>>>> Ah yes. I authored you a patch, but became distracted. Here:
>>>>>>>>>
>>>>>>>>> -----8<--------------------8<-------
>>>>>>>>>
>>>>>>>>> From: Lee Jones <lee.jones@linaro.org>
>>>>>>>>>
>>>>>>>>> mfd: pm8008: Remove driver data structure pm8008_data
>>>>>>>>> Maintaining a local driver data structure that is never shared
>>>>>>>>> outside of the core device is an unnecessary complexity. Half of the
>>>>>>>>> attributes were not used outside of a single function, one of which
>>>>>>>>> was not used at all. The remaining 2 are generic and can be passed
>>>>>>>>> around as required.
>>>>>>>> Okay, but we still need to store the regulators_regmap, which is required in
>>>>>>>> the pm8008 regulator driver. Could we use a global variable for it?
>>>>>>> Look down ...
>>>>>>>
>>>>>>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>>>>>>> ---
>>>>>>>>> drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
>>>>>>>>> 1 file changed, 20 insertions(+), 33 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>>>>> index c472d7f8103c4..4b8ff947762f2 100644
>>>>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>>>>> @@ -54,13 +54,6 @@ enum {
>>>>>>>>> #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
>>>>>>>>> -struct pm8008_data {
>>>>>>>>> - struct device *dev;
>>>>>>>>> - struct regmap *regmap;
>>>>>>>>> - int irq;
>>>>>>>>> - struct regmap_irq_chip_data *irq_data;
>>>>>>>>> -};
>>>>>>>>> -
>>>>>>>>> static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
>>>>>>>>> static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
>>>>>>>>> static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
>>>>>>>>> @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>>>>>>>>> .max_register = 0xFFFF,
>>>>>>>>> };
>>>>>>>>> -static int pm8008_init(struct pm8008_data *chip)
>>>>>>>>> +static int pm8008_init(struct regmap *regmap)
>>>>>>>>> {
>>>>>>>>> int rc;
>>>>>>>>> @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
>>>>>>>>> * This is required to enable the writing of TYPE registers in
>>>>>>>>> * regmap_irq_sync_unlock().
>>>>>>>>> */
>>>>>>>>> - rc = regmap_write(chip->regmap,
>>>>>>>>> - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
>>>>>>>>> - BIT(0));
>>>>>>>>> + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>>>> if (rc)
>>>>>>>>> return rc;
>>>>>>>>> /* Do the same for GPIO1 and GPIO2 peripherals */
>>>>>>>>> - rc = regmap_write(chip->regmap,
>>>>>>>>> - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>>>> + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>>>> if (rc)
>>>>>>>>> return rc;
>>>>>>>>> - rc = regmap_write(chip->regmap,
>>>>>>>>> - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>>>> + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>>>>>>>> return rc;
>>>>>>>>> }
>>>>>>>>> -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>>>>>> +static int pm8008_probe_irq_peripherals(struct device *dev,
>>>>>>>>> + struct regmap *regmap,
>>>>>>>>> int client_irq)
>>>>>>>>> {
>>>>>>>>> int rc, i;
>>>>>>>>> struct regmap_irq_type *type;
>>>>>>>>> struct regmap_irq_chip_data *irq_data;
>>>>>>>>> - rc = pm8008_init(chip);
>>>>>>>>> + rc = pm8008_init(regmap);
>>>>>>>>> if (rc) {
>>>>>>>>> - dev_err(chip->dev, "Init failed: %d\n", rc);
>>>>>>>>> + dev_err(dev, "Init failed: %d\n", rc);
>>>>>>>>> return rc;
>>>>>>>>> }
>>>>>>>>> @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>>>>>> IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>>>>>>>>> }
>>>>>>>>> - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
>>>>>>>>> + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
>>>>>>>>> IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>>>>>>>>> if (rc) {
>>>>>>>>> - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
>>>>>>>>> + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
>>>>>>>>> return rc;
>>>>>>>>> }
>>>>>>>>> @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>>>>>> static int pm8008_probe(struct i2c_client *client)
>>>>>>>>> {
>>>>>>>>> int rc;
>>>>>>>>> - struct pm8008_data *chip;
>>>>>>>>> -
>>>>>>>>> - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>>>>>>>> - if (!chip)
>>>>>>>>> - return -ENOMEM;
>>>>>>>>> + struct device *dev;
>>>>>>>>> + struct regmap *regmap;
>>>>>>>>> - chip->dev = &client->dev;
>>>>>>>>> - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>>>>>>>> - if (!chip->regmap)
>>>>>>>>> + dev = &client->dev;
>>>>>>>>> + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>>>>>>>> + if (!regmap)
>>>>>>>>> return -ENODEV;
>>>>>>>>> - i2c_set_clientdata(client, chip);
>>>>>>>>> + i2c_set_clientdata(client, regmap);
>>>>>>> Here ^
>>>>>> I have added a dummy device and set the client data by passing regmap, see
>>>>>> below:
>>>>>>
>>>>>> + regulators_client = i2c_new_dummy_device(client->adapter,
>>>>>> client->addr + 1);
>>>>>> + if (IS_ERR(regulators_client)) {
>>>>>> + dev_err(dev, "can't attach client\n");
>>>>>> + return PTR_ERR(regulators_client);
>>>>>> + }
>>>>>> +
>>>>>> + regulators_regmap = devm_regmap_init_i2c(regulators_client,
>>>>>> &qcom_mfd_regmap_cfg[1]);
>>>>>> + if (!regmap)
>>>>>> + return -ENODEV;
>>>>>> +
>>>>>> + i2c_set_clientdata(client, regulators_regmap);
>>>>>>
>>>>>> Now if i try to get this regmap from regulator driver by doing
>>>>>>
>>>>>> struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
>>>>>>
>>>>>> it still gets me the regmap of pm8008@8 device and not the regulator device
>>>>>> regmap (0x9). Not sure if I'm missing something here.
>>>>> So you need to pass 2 regmap pointers?
>>>>>
>>>>> If you need to pass more than one item to the child devices, you do
>>>>> need to use a struct for that.
>>>> I need to pass only one regmap out of the two, but i am not able to retrieve
>>>> the correct regmap simply by doing i2c_set_clientdata
>>>>
>>>> probably because we are having all the child nodes under same DT node and
>>>> thus not able to distinguish based on the dev pointer
>>> You can only pull out (get) the pointer that you put in (set).
>>>
>>> Unless you over-wrote it later in the thread of execution, you are
>>> pulling out whatever regulators_regmap happens to be.
>>>
>>> Is qcom_mfd_regmap_cfg[1] definitely the one you want?
>>
>> Yes, I need qcom_mfd_regmap_cfg[1]
>>
>> Pasting code snippet for reference:
>>
>> static struct regmap_config qcom_mfd_regmap_cfg[2] = {
>> {
>>
>> .name = "infra",
>> .reg_bits = 16,
>> .val_bits = 8,
>> .max_register = 0xFFFF,
>> },
>> {
>> .name = "regulators",
>> .reg_bits = 16,
>> .val_bits = 8,
>> .max_register = 0xFFFF,
>> },
>>
>> };
>>
>>
>> Inside pm8008_probe:
>>
>>
>> regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
>> if (!regmap)
>> return -ENODEV;
>>
>> i2c_set_clientdata(client, regmap);
>>
>>
>> regulators_client = i2c_new_dummy_device(client->adapter, client->addr
>> + 1);
>> if (IS_ERR(regulators_client)) {
>> dev_err(dev, "can't attach client\n");
>> return PTR_ERR(regulators_client);
>> }
>>
>> regulators_regmap = devm_regmap_init_i2c(regulators_client,
>> &qcom_mfd_regmap_cfg[1]);
>> if (!regmap)
>> return -ENODEV;
>>
>> i2c_set_clientdata(regulators_client, regulators_regmap);
> You can't call this twice.
>
> Doing so with over-write regmap with regulators_regmap.
>
> You said you only needed one?
>
> "I need to pass only one regmap out of the two, but i am not able to retrieve"
I thought you asked whether we have to pass two regmaps to the child
regulator driver.
>> In qcom-pm8008-regulator.c I tried to get the regmap using
>>
>> dev_get_regmap(pdev->dev.parent, "regulators");
> I haven't looked at this API before. I suggest that this would be
> used *instead* of passing the regmap pointer via driver_data.
>
> It looks like you're using different devices to init your regmaps;
> 'client' and 'regulator_client' (derived from client->adapter).
>
> "regulators" is registered using regulators_regmap which was *not*
> init'ed with pdev->dev.parent (same as client->dev), so trying to
> dev_get_regmap() with that device pointer will not work.
Okay, So I will leave the driver as is then?
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
2022-07-01 8:47 ` Satya Priya Kakitapalli (Temp)
@ 2022-07-01 9:12 ` Lee Jones
[not found] ` <0481d3cc-4bb9-4969-0232-76ba57ff260d@quicinc.com>
0 siblings, 1 reply; 54+ messages in thread
From: Lee Jones @ 2022-07-01 9:12 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Bjorn Andersson, Rob Herring, Liam Girdwood, Mark Brown,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
On Fri, 01 Jul 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> On 7/1/2022 1:24 PM, Lee Jones wrote:
> > On Fri, 01 Jul 2022, Satya Priya Kakitapalli (Temp) wrote:
> >
> > > On 6/30/2022 4:04 PM, Lee Jones wrote:
> > > > On Thu, 30 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > >
> > > > > On 6/29/2022 8:48 PM, Lee Jones wrote:
> > > > > > On Wed, 29 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > >
> > > > > > > On 6/28/2022 1:12 PM, Lee Jones wrote:
> > > > > > > > On Tue, 28 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > >
> > > > > > > > > On 6/27/2022 1:11 PM, Lee Jones wrote:
> > > > > > > > > > On Mon, 27 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi Lee,
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On 6/20/2022 4:37 PM, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > > > > > > On 6/20/2022 1:50 PM, Lee Jones wrote:
> > > > > > > > > > > > > On Mon, 20 Jun 2022, Satya Priya Kakitapalli (Temp) wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > On 6/17/2022 2:27 AM, Lee Jones wrote:
> > > > > > > > > > > > > > > On Tue, 14 Jun 2022, Satya Priya wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Use i2c_new_dummy_device() to register pm8008-regulator
> > > > > > > > > > > > > > > > client present at a different address space, instead of
> > > > > > > > > > > > > > > > defining a separate DT node. This avoids calling the probe
> > > > > > > > > > > > > > > > twice for the same chip, once for each client pm8008-infra
> > > > > > > > > > > > > > > > and pm8008-regulator.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > As a part of this define pm8008_regmap_init() to do regmap
> > > > > > > > > > > > > > > > init for both the clients and define pm8008_get_regmap() to
> > > > > > > > > > > > > > > > pass the regmap to the regulator driver.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > > > > > > > > > > > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > > Changes in V15:
> > > > > > > > > > > > > > > > - None.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Changes in V14:
> > > > > > > > > > > > > > > > - None.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Changes in V13:
> > > > > > > > > > > > > > > > - None.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > drivers/mfd/qcom-pm8008.c | 34
> > > > > > > > > > > > > > > > ++++++++++++++++++++++++++++++++--
> > > > > > > > > > > > > > > > include/linux/mfd/qcom_pm8008.h | 9 +++++++++
> > > > > > > > > > > > > > > > 2 files changed, 41 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > > > create mode 100644 include/linux/mfd/qcom_pm8008.h
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > > > > > index 569ffd50..55e2a8e 100644
> > > > > > > > > > > > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > > > > > > > > > > #include <linux/interrupt.h>
> > > > > > > > > > > > > > > > #include <linux/irq.h>
> > > > > > > > > > > > > > > > #include <linux/irqdomain.h>
> > > > > > > > > > > > > > > > +#include <linux/mfd/qcom_pm8008.h>
> > > > > > > > > > > > > > > > #include <linux/module.h>
> > > > > > > > > > > > > > > > #include <linux/of_device.h>
> > > > > > > > > > > > > > > > #include <linux/of_platform.h>
> > > > > > > > > > > > > > > > @@ -57,6 +58,7 @@ enum {
> > > > > > > > > > > > > > > > struct pm8008_data {
> > > > > > > > > > > > > > > > struct device *dev;
> > > > > > > > > > > > > > > > + struct regmap *regulators_regmap;
> > > > > > > > > > > > > > > > int irq;
> > > > > > > > > > > > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > > > > > > > > > > > };
> > > > > > > > > > > > > > > > @@ -150,6 +152,12 @@ static struct regmap_config
> > > > > > > > > > > > > > > > qcom_mfd_regmap_cfg = {
> > > > > > > > > > > > > > > > .max_register = 0xFFFF,
> > > > > > > > > > > > > > > > };
> > > > > > > > > > > > > > > > +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip)
> > > > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > > > + return chip->regulators_regmap;
> > > > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > > > +EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > > > > > > > > > Seems like abstraction for the sake of abstraction.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Why not do the dereference inside the regulator driver?
> > > > > > > > > > > > > > To derefer this in the regulator driver, we need to have the
> > > > > > > > > > > > > > pm8008_data
> > > > > > > > > > > > > > struct definition in the qcom_pm8008 header file.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think it doesn't look great to have only that structure in
> > > > > > > > > > > > > > header and all
> > > > > > > > > > > > > > other structs and enum in the mfd driver.
> > > > > > > > > > > > > Then why pass 'pm8008_data' at all?
> > > > > > > > > > > > There is one more option, instead of passing the pm8008_data, we could
> > > > > > > > > > > > pass the pdev->dev.parent and get the pm8008 chip data directly in the
> > > > > > > > > > > > pm8008_get_regmap() like below
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > struct regmap *pm8008_get_regmap(const struct device *dev)
> > > > > > > > > > > > {
> > > > > > > > > > > > const struct pm8008_data *chip = dev_get_drvdata(dev);
> > > > > > > > > > > >
> > > > > > > > > > > > return chip->regulators_regmap;
> > > > > > > > > > > > }
> > > > > > > > > > > > EXPORT_SYMBOL_GPL(pm8008_get_regmap);
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > By doing this we can avoid having declaration of pm8008_data also in the
> > > > > > > > > > > > header. Please let me know if this looks good.
> > > > > > > > > > > >
> > > > > > > > > > > Could you please confirm on this?
> > > > > > > > > > >
> > > > > > > > > > > > > What's preventing you from passing 'regmap'?
> > > > > > > > > > > > I didn't get what you meant here, could you please elaborate a bit?
> > > > > > > > > > Ah yes. I authored you a patch, but became distracted. Here:
> > > > > > > > > >
> > > > > > > > > > -----8<--------------------8<-------
> > > > > > > > > >
> > > > > > > > > > From: Lee Jones <lee.jones@linaro.org>
> > > > > > > > > >
> > > > > > > > > > mfd: pm8008: Remove driver data structure pm8008_data
> > > > > > > > > > Maintaining a local driver data structure that is never shared
> > > > > > > > > > outside of the core device is an unnecessary complexity. Half of the
> > > > > > > > > > attributes were not used outside of a single function, one of which
> > > > > > > > > > was not used at all. The remaining 2 are generic and can be passed
> > > > > > > > > > around as required.
> > > > > > > > > Okay, but we still need to store the regulators_regmap, which is required in
> > > > > > > > > the pm8008 regulator driver. Could we use a global variable for it?
> > > > > > > > Look down ...
> > > > > > > >
> > > > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > > drivers/mfd/qcom-pm8008.c | 53 ++++++++++++++++++-----------------------------
> > > > > > > > > > 1 file changed, 20 insertions(+), 33 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > index c472d7f8103c4..4b8ff947762f2 100644
> > > > > > > > > > --- a/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > +++ b/drivers/mfd/qcom-pm8008.c
> > > > > > > > > > @@ -54,13 +54,6 @@ enum {
> > > > > > > > > > #define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE)
> > > > > > > > > > -struct pm8008_data {
> > > > > > > > > > - struct device *dev;
> > > > > > > > > > - struct regmap *regmap;
> > > > > > > > > > - int irq;
> > > > > > > > > > - struct regmap_irq_chip_data *irq_data;
> > > > > > > > > > -};
> > > > > > > > > > -
> > > > > > > > > > static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)};
> > > > > > > > > > static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)};
> > > > > > > > > > static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)};
> > > > > > > > > > @@ -150,7 +143,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
> > > > > > > > > > .max_register = 0xFFFF,
> > > > > > > > > > };
> > > > > > > > > > -static int pm8008_init(struct pm8008_data *chip)
> > > > > > > > > > +static int pm8008_init(struct regmap *regmap)
> > > > > > > > > > {
> > > > > > > > > > int rc;
> > > > > > > > > > @@ -160,34 +153,31 @@ static int pm8008_init(struct pm8008_data *chip)
> > > > > > > > > > * This is required to enable the writing of TYPE registers in
> > > > > > > > > > * regmap_irq_sync_unlock().
> > > > > > > > > > */
> > > > > > > > > > - rc = regmap_write(chip->regmap,
> > > > > > > > > > - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
> > > > > > > > > > - BIT(0));
> > > > > > > > > > + rc = regmap_write(regmap, (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > > > if (rc)
> > > > > > > > > > return rc;
> > > > > > > > > > /* Do the same for GPIO1 and GPIO2 peripherals */
> > > > > > > > > > - rc = regmap_write(chip->regmap,
> > > > > > > > > > - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > > > + rc = regmap_write(regmap, (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > > > if (rc)
> > > > > > > > > > return rc;
> > > > > > > > > > - rc = regmap_write(chip->regmap,
> > > > > > > > > > - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > > > + rc = regmap_write(regmap, (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
> > > > > > > > > > return rc;
> > > > > > > > > > }
> > > > > > > > > > -static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > > > > > +static int pm8008_probe_irq_peripherals(struct device *dev,
> > > > > > > > > > + struct regmap *regmap,
> > > > > > > > > > int client_irq)
> > > > > > > > > > {
> > > > > > > > > > int rc, i;
> > > > > > > > > > struct regmap_irq_type *type;
> > > > > > > > > > struct regmap_irq_chip_data *irq_data;
> > > > > > > > > > - rc = pm8008_init(chip);
> > > > > > > > > > + rc = pm8008_init(regmap);
> > > > > > > > > > if (rc) {
> > > > > > > > > > - dev_err(chip->dev, "Init failed: %d\n", rc);
> > > > > > > > > > + dev_err(dev, "Init failed: %d\n", rc);
> > > > > > > > > > return rc;
> > > > > > > > > > }
> > > > > > > > > > @@ -207,10 +197,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > > > > > IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> > > > > > > > > > }
> > > > > > > > > > - rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
> > > > > > > > > > + rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> > > > > > > > > > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> > > > > > > > > > if (rc) {
> > > > > > > > > > - dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
> > > > > > > > > > + dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> > > > > > > > > > return rc;
> > > > > > > > > > }
> > > > > > > > > > @@ -220,26 +210,23 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> > > > > > > > > > static int pm8008_probe(struct i2c_client *client)
> > > > > > > > > > {
> > > > > > > > > > int rc;
> > > > > > > > > > - struct pm8008_data *chip;
> > > > > > > > > > -
> > > > > > > > > > - chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > > > > > > > > > - if (!chip)
> > > > > > > > > > - return -ENOMEM;
> > > > > > > > > > + struct device *dev;
> > > > > > > > > > + struct regmap *regmap;
> > > > > > > > > > - chip->dev = &client->dev;
> > > > > > > > > > - chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > > > > > > > - if (!chip->regmap)
> > > > > > > > > > + dev = &client->dev;
> > > > > > > > > > + regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> > > > > > > > > > + if (!regmap)
> > > > > > > > > > return -ENODEV;
> > > > > > > > > > - i2c_set_clientdata(client, chip);
> > > > > > > > > > + i2c_set_clientdata(client, regmap);
> > > > > > > > Here ^
> > > > > > > I have added a dummy device and set the client data by passing regmap, see
> > > > > > > below:
> > > > > > >
> > > > > > > + regulators_client = i2c_new_dummy_device(client->adapter,
> > > > > > > client->addr + 1);
> > > > > > > + if (IS_ERR(regulators_client)) {
> > > > > > > + dev_err(dev, "can't attach client\n");
> > > > > > > + return PTR_ERR(regulators_client);
> > > > > > > + }
> > > > > > > +
> > > > > > > + regulators_regmap = devm_regmap_init_i2c(regulators_client,
> > > > > > > &qcom_mfd_regmap_cfg[1]);
> > > > > > > + if (!regmap)
> > > > > > > + return -ENODEV;
> > > > > > > +
> > > > > > > + i2c_set_clientdata(client, regulators_regmap);
> > > > > > >
> > > > > > > Now if i try to get this regmap from regulator driver by doing
> > > > > > >
> > > > > > > struct regmap *regmap = dev_get_drvdata(pdev->dev.parent);
> > > > > > >
> > > > > > > it still gets me the regmap of pm8008@8 device and not the regulator device
> > > > > > > regmap (0x9). Not sure if I'm missing something here.
> > > > > > So you need to pass 2 regmap pointers?
> > > > > >
> > > > > > If you need to pass more than one item to the child devices, you do
> > > > > > need to use a struct for that.
> > > > > I need to pass only one regmap out of the two, but i am not able to retrieve
> > > > > the correct regmap simply by doing i2c_set_clientdata
> > > > >
> > > > > probably because we are having all the child nodes under same DT node and
> > > > > thus not able to distinguish based on the dev pointer
> > > > You can only pull out (get) the pointer that you put in (set).
> > > >
> > > > Unless you over-wrote it later in the thread of execution, you are
> > > > pulling out whatever regulators_regmap happens to be.
> > > >
> > > > Is qcom_mfd_regmap_cfg[1] definitely the one you want?
> > >
> > > Yes, I need qcom_mfd_regmap_cfg[1]
> > >
> > > Pasting code snippet for reference:
> > >
> > > static struct regmap_config qcom_mfd_regmap_cfg[2] = {
> > > {
> > >
> > > .name = "infra",
> > > .reg_bits = 16,
> > > .val_bits = 8,
> > > .max_register = 0xFFFF,
> > > },
> > > {
> > > .name = "regulators",
> > > .reg_bits = 16,
> > > .val_bits = 8,
> > > .max_register = 0xFFFF,
> > > },
> > >
> > > };
> > >
> > >
> > > Inside pm8008_probe:
> > >
> > >
> > > regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
> > > if (!regmap)
> > > return -ENODEV;
> > >
> > > i2c_set_clientdata(client, regmap);
> > >
> > >
> > > regulators_client = i2c_new_dummy_device(client->adapter, client->addr
> > > + 1);
> > > if (IS_ERR(regulators_client)) {
> > > dev_err(dev, "can't attach client\n");
> > > return PTR_ERR(regulators_client);
> > > }
> > >
> > > regulators_regmap = devm_regmap_init_i2c(regulators_client,
> > > &qcom_mfd_regmap_cfg[1]);
> > > if (!regmap)
> > > return -ENODEV;
> > >
> > > i2c_set_clientdata(regulators_client, regulators_regmap);
> > You can't call this twice.
> >
> > Doing so with over-write regmap with regulators_regmap.
> >
> > You said you only needed one?
> >
> > "I need to pass only one regmap out of the two, but i am not able to retrieve"
>
> I thought you asked whether we have to pass two regmaps to the child
> regulator driver.
Yes, that's what I was asking.
So you only need to pass 'regulators_regmap' (derived from
"regulators") right?
In that case, keep:
i2c_set_clientdata(regulators_client, regulators_regmap);
... and drop:
i2c_set_clientdata(client, regmap);
> > > In qcom-pm8008-regulator.c I tried to get the regmap using
> > >
> > > dev_get_regmap(pdev->dev.parent, "regulators");
> > I haven't looked at this API before. I suggest that this would be
> > used *instead* of passing the regmap pointer via driver_data.
> >
> > It looks like you're using different devices to init your regmaps;
> > 'client' and 'regulator_client' (derived from client->adapter).
> >
> > "regulators" is registered using regulators_regmap which was *not*
> > init'ed with pdev->dev.parent (same as client->dev), so trying to
> > dev_get_regmap() with that device pointer will not work.
>
> Okay, So I will leave the driver as is then?
Right, let's take a step back and try to clarify a few things here.
What is the purpose of the two regmaps that you're creating here?
Where will each of them be used?
Regmaps created in MFD are usually either used only locally, here in
the parent driver or shared amongst *multiple* children. If that is
not the case for regulators_regmap, which looks suspiciously like it's
only used in the Regulator driver, then why not initialise the regmap
there instead? Rather than pointlessly creating it here and passing
it via the driver_data pointer.
Once I know more about your intentions, I can help you devise a plan.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH V15 7/9] regulator: Add a regulator driver for the PM8008 PMIC
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (5 preceding siblings ...)
2022-06-14 9:48 ` [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-14 20:36 ` Stephen Boyd
2022-06-14 9:48 ` [PATCH V15 8/9] arm64: dts: qcom: pm8008: Add base dts file Satya Priya
` (2 subsequent siblings)
9 siblings, 1 reply; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
containing 7 LDO regulators. Add a PM8008 regulator driver to
support PMIC regulator management via the regulator framework.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Changes in V15:
- Removed unused variables from pm8008_regulator struct.
- Replaced static_assert with BUILD_BUG_ON.
Changes in V14:
- Remove unused headers and debug prints.
- Remove min_uv and max_uv from reg_data[] and set the min/max based
on the voltage_range pointer.
- In get_voltage_sel read voltage from hw and calculate selector instead
of using selector from set_voltage.
- Add errro check after regulator_list_voltage_linear_range().
- Use static_assert to make sure nldo_ranges & pldo_ranges doesn't become
larger and we forget to update the pm8008_reg->rdesc.n_linear_ranges
Changes in V13:
- Added if check to avoid buffer overflow warnings.
for (i = 0; i < ARRAY_SIZE(reg_data); i++)
if (strstr(name, reg_data[i].name))
break;
if (i == ARRAY_SIZE(reg_data)) {
dev_err(dev, "Invalid regulator name %s\n", name);
return -EINVAL;
}
- Removed unused headers.
drivers/regulator/Kconfig | 9 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-pm8008-regulator.c | 242 ++++++++++++++++++++++++++++++
3 files changed, 252 insertions(+)
create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index cbe0f96..2c6d9c2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -925,6 +925,15 @@ config REGULATOR_PWM
This driver supports PWM controlled voltage regulators. PWM
duty cycle can increase or decrease the voltage.
+config REGULATOR_QCOM_PM8008
+ tristate "Qualcomm Technologies, Inc. PM8008 PMIC regulators"
+ depends on MFD_QCOM_PM8008
+ help
+ Select this option to get support for the voltage regulators
+ of Qualcomm Technologies, Inc. PM8008 PMIC chip. PM8008 has 7 LDO
+ regulators. This driver provides support for basic operations like
+ set/get voltage and enable/disable.
+
config REGULATOR_QCOM_RPM
tristate "Qualcomm RPM regulator driver"
depends on MFD_QCOM_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 8d3ee8b..169e686 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_PM8008) += qcom-pm8008-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
new file mode 100644
index 0000000..c999a7c
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mfd/qcom_pm8008.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+#define VSET_STEP_MV 8
+#define VSET_STEP_UV (VSET_STEP_MV * 1000)
+
+#define LDO_ENABLE_REG(base) ((base) + 0x46)
+#define ENABLE_BIT BIT(7)
+
+#define LDO_VSET_LB_REG(base) ((base) + 0x40)
+
+#define LDO_STEPPER_CTL_REG(base) ((base) + 0x3b)
+#define DEFAULT_VOLTAGE_STEPPER_RATE 38400
+#define STEP_RATE_MASK GENMASK(1, 0)
+
+#define NLDO_MIN_UV 528000
+#define NLDO_MAX_UV 1504000
+
+#define PLDO_MIN_UV 1504000
+#define PLDO_MAX_UV 3400000
+
+struct pm8008_regulator_data {
+ const char *name;
+ const char *supply_name;
+ int min_dropout_uv;
+ const struct linear_range *voltage_range;
+};
+
+struct pm8008_regulator {
+ struct regmap *regmap;
+ struct regulator_desc rdesc;
+ u16 base;
+ int step_rate;
+};
+
+static const struct linear_range nldo_ranges[] = {
+ REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
+};
+
+static const struct linear_range pldo_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
+};
+
+static const struct pm8008_regulator_data reg_data[] = {
+ /* name parent headroom_uv voltage_range */
+ { "ldo1", "vdd_l1_l2", 225000, nldo_ranges, },
+ { "ldo2", "vdd_l1_l2", 225000, nldo_ranges, },
+ { "ldo3", "vdd_l3_l4", 300000, pldo_ranges, },
+ { "ldo4", "vdd_l3_l4", 300000, pldo_ranges, },
+ { "ldo5", "vdd_l5", 200000, pldo_ranges, },
+ { "ldo6", "vdd_l6", 200000, pldo_ranges, },
+ { "ldo7", "vdd_l7", 200000, pldo_ranges, },
+};
+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+ struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+ __le16 mV;
+ int uV;
+
+ regmap_bulk_read(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
+
+ uV = le16_to_cpu(mV) * 1000;
+ return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
+}
+
+static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
+ int mV)
+{
+ __le16 vset_raw;
+
+ vset_raw = cpu_to_le16(mV);
+
+ return regmap_bulk_write(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base),
+ (const void *)&vset_raw, sizeof(vset_raw));
+}
+
+static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
+ int old_uV, int new_uv)
+{
+ struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+
+ return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
+}
+
+static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
+ unsigned int selector)
+{
+ struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+ int rc, mV;
+
+ rc = regulator_list_voltage_linear_range(rdev, selector);
+ if (rc < 0)
+ return rc;
+
+ /* voltage control register is set with voltage in millivolts */
+ mV = DIV_ROUND_UP(rc, 1000);
+
+ rc = pm8008_write_voltage(pm8008_reg, mV);
+ if (rc < 0)
+ return rc;
+
+ return 0;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .set_voltage_sel = pm8008_regulator_set_voltage,
+ .get_voltage_sel = pm8008_regulator_get_voltage,
+ .list_voltage = regulator_list_voltage_linear,
+ .set_voltage_time = pm8008_regulator_set_voltage_time,
+};
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+ int rc, i;
+ u32 base;
+ unsigned int reg;
+ const char *name;
+ struct device *dev = &pdev->dev;
+ struct regulator_config reg_config = {};
+ struct regulator_dev *rdev;
+ const struct pm8008_data *chip = dev_get_drvdata(pdev->dev.parent);
+ struct pm8008_regulator *pm8008_reg;
+
+ pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
+ if (!pm8008_reg)
+ return -ENOMEM;
+
+ pm8008_reg->regmap = pm8008_get_regmap(chip);
+ if (!pm8008_reg->regmap) {
+ dev_err(dev, "parent regmap is missing\n");
+ return -EINVAL;
+ }
+
+ rc = of_property_read_string(dev->of_node, "regulator-name", &name);
+ if (rc)
+ return rc;
+
+ /* get the required regulator data */
+ for (i = 0; i < ARRAY_SIZE(reg_data); i++)
+ if (strstr(name, reg_data[i].name))
+ break;
+
+ if (i == ARRAY_SIZE(reg_data)) {
+ dev_err(dev, "Invalid regulator name %s\n", name);
+ return -EINVAL;
+ }
+
+ rc = of_property_read_u32_index(dev->of_node, "reg", 1, &base);
+ if (rc < 0) {
+ dev_err(dev, "%s: failed to get regulator base rc=%d\n", name, rc);
+ return rc;
+ }
+ pm8008_reg->base = base;
+
+ /* get slew rate */
+ rc = regmap_bulk_read(pm8008_reg->regmap,
+ LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
+ if (rc < 0) {
+ dev_err(dev, "failed to read step rate configuration rc=%d\n", rc);
+ return rc;
+ }
+ reg &= STEP_RATE_MASK;
+ pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
+
+ pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
+ pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
+ pm8008_reg->rdesc.name = reg_data[i].name;
+ pm8008_reg->rdesc.supply_name = reg_data[i].supply_name;
+ pm8008_reg->rdesc.of_match = reg_data[i].name;
+ pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
+ pm8008_reg->rdesc.linear_ranges = reg_data[i].voltage_range;
+ pm8008_reg->rdesc.n_linear_ranges = 1;
+ BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
+ (ARRAY_SIZE(nldo_ranges) != 1));
+
+ if (reg_data[i].voltage_range == nldo_ranges) {
+ pm8008_reg->rdesc.min_uV = NLDO_MIN_UV;
+ pm8008_reg->rdesc.n_voltages
+ = ((NLDO_MAX_UV - NLDO_MIN_UV)
+ / pm8008_reg->rdesc.uV_step) + 1;
+ } else {
+ pm8008_reg->rdesc.min_uV = PLDO_MIN_UV;
+ pm8008_reg->rdesc.n_voltages
+ = ((PLDO_MAX_UV - PLDO_MIN_UV)
+ / pm8008_reg->rdesc.uV_step) + 1;
+ }
+
+ pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
+ pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
+ pm8008_reg->rdesc.min_dropout_uV = reg_data[i].min_dropout_uv;
+
+ reg_config.dev = dev->parent;
+ reg_config.driver_data = pm8008_reg;
+ reg_config.regmap = pm8008_reg->regmap;
+
+ rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, ®_config);
+ if (IS_ERR(rdev)) {
+ rc = PTR_ERR(rdev);
+ dev_err(dev, "%s: failed to register regulator rc=%d\n",
+ reg_data[i].name, rc);
+ return rc;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id pm8008_regulator_match_table[] = {
+ { .compatible = "qcom,pm8008-regulator", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, pm8008_regulator_match_table);
+
+static struct platform_driver pm8008_regulator_driver = {
+ .driver = {
+ .name = "qcom-pm8008-regulator",
+ .of_match_table = pm8008_regulator_match_table,
+ },
+ .probe = pm8008_regulator_probe,
+};
+
+module_platform_driver(pm8008_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. PM8008 PMIC Regulator Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 7/9] regulator: Add a regulator driver for the PM8008 PMIC
2022-06-14 9:48 ` [PATCH V15 7/9] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
@ 2022-06-14 20:36 ` Stephen Boyd
0 siblings, 0 replies; 54+ messages in thread
From: Stephen Boyd @ 2022-06-14 20:36 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Satya Priya
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, quic_collinsd, quic_subbaram, quic_jprakash
Quoting Satya Priya (2022-06-14 02:48:29)
> Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
> containing 7 LDO regulators. Add a PM8008 regulator driver to
> support PMIC regulator management via the regulator framework.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH V15 8/9] arm64: dts: qcom: pm8008: Add base dts file
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (6 preceding siblings ...)
2022-06-14 9:48 ` [PATCH V15 7/9] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2022-06-14 9:48 ` [PATCH V15 9/9] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp Satya Priya
2023-03-17 8:06 ` [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Luca Weiss
9 siblings, 0 replies; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Add base DTS file for pm8008.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V15:
- Changed copyright from GPL to BSD.
Changes in V14:
- Changed copyright from BSD to GPL.
Changes in V13:
- None.
arch/arm64/boot/dts/qcom/pm8008.dtsi | 54 ++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
new file mode 100644
index 0000000..0ffef2f
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: BSD-3-Clause
+// Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+
+&pm8008_bus {
+ pm8008: pmic@8 {
+ compatible = "qcom,pm8008";
+ reg = <0x8>;
+ #address-cells = <2>;
+ #size-cells = <0>;
+ #interrupt-cells = <2>;
+
+ pm8008_l1: ldo1@1,4000 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4000>;
+ regulator-name = "pm8008_ldo1";
+ };
+
+ pm8008_l2: ldo2@1,4100 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4100>;
+ regulator-name = "pm8008_ldo2";
+ };
+
+ pm8008_l3: ldo3@1,4200 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4200>;
+ regulator-name = "pm8008_ldo3";
+ };
+
+ pm8008_l4: ldo4@1,4300 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4300>;
+ regulator-name = "pm8008_ldo4";
+ };
+
+ pm8008_l5: ldo5@1,4400 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4400>;
+ regulator-name = "pm8008_ldo5";
+ };
+
+ pm8008_l6: ldo6@1,4500 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4500>;
+ regulator-name = "pm8008_ldo6";
+ };
+
+ pm8008_l7: ldo7@1,4600 {
+ compatible = "qcom,pm8008-regulator";
+ reg = <0x1 0x4600>;
+ regulator-name = "pm8008_ldo7";
+ };
+ };
+};
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH V15 9/9] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (7 preceding siblings ...)
2022-06-14 9:48 ` [PATCH V15 8/9] arm64: dts: qcom: pm8008: Add base dts file Satya Priya
@ 2022-06-14 9:48 ` Satya Priya
2023-03-17 8:06 ` [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Luca Weiss
9 siblings, 0 replies; 54+ messages in thread
From: Satya Priya @ 2022-06-14 9:48 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash, Satya Priya
Add pm8008 infra and regulators support for sc7280 idp.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V15:
- None.
Changes in V14:
- None.
Changes in V13:
- None.
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 5eb6689..166812e 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -271,6 +271,63 @@
};
};
+pm8008_bus: &i2c1 {
+ status = "okay";
+};
+
+#include "pm8008.dtsi"
+
+&pm8008 {
+ interrupt-parent = <&tlmm>;
+ interrupts = <24 IRQ_TYPE_EDGE_RISING>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pm8008_active>;
+
+ reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_LOW>;
+
+ vdd_l1_l2-supply = <&vreg_s8b_1p2>;
+ vdd_l3_l4-supply = <&vreg_s1b_1p8>;
+ vdd_l5-supply = <&vreg_bob>;
+ vdd_l6-supply = <&vreg_bob>;
+ vdd_l7-supply = <&vreg_bob>;
+};
+
+&pm8008_l1 {
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1300000>;
+};
+
+&pm8008_l2 {
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1250000>;
+};
+
+&pm8008_l3 {
+ regulator-min-microvolt = <1650000>;
+ regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l4 {
+ regulator-min-microvolt = <1504000>;
+ regulator-max-microvolt = <1600000>;
+};
+
+&pm8008_l5 {
+ regulator-min-microvolt = <2600000>;
+ regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l6 {
+ regulator-min-microvolt = <2600000>;
+ regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l7 {
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3544000>;
+};
+
&qfprom {
vcc-supply = <&vreg_l1c_1p8>;
};
@@ -383,6 +440,15 @@
drive-strength = <2>;
};
+&pm8350c_gpios {
+ pm8008_active: pm8008-active {
+ pins = "gpio4";
+ function = "normal";
+ bias-disable;
+ power-source = <0>;
+ };
+};
+
&qspi_cs0 {
bias-disable;
};
--
2.7.4
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver
2022-06-14 9:48 [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (8 preceding siblings ...)
2022-06-14 9:48 ` [PATCH V15 9/9] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp Satya Priya
@ 2023-03-17 8:06 ` Luca Weiss
2023-07-07 8:54 ` Luca Weiss
9 siblings, 1 reply; 54+ messages in thread
From: Luca Weiss @ 2023-03-17 8:06 UTC (permalink / raw)
To: Satya Priya, Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
Hi Satya,
On Tue Jun 14, 2022 at 11:48 AM CEST, Satya Priya wrote:
> Satya Priya (9):
> dt-bindings: mfd: pm8008: Add reset-gpios
> dt-bindings: mfd: pm8008: Change the address cells
> dt-bindings: mfd: pm8008: Add regulators for pm8008
> mfd: pm8008: Add reset-gpios
> mfd: pm8008: Remove the regmap member from pm8008_data struct
> mfd: pm8008: Use i2c_new_dummy_device() API
> regulator: Add a regulator driver for the PM8008 PMIC
> arm64: dts: qcom: pm8008: Add base dts file
> arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp
Is there any activity on this patch series? It's been a few months since
this revision. Would be interested in this for the pm8008 found on
sm7225-fairphone-fp4.
Regards
Luca
>
> .../devicetree/bindings/mfd/qcom,pm8008.yaml | 69 +++++-
> arch/arm64/boot/dts/qcom/pm8008.dtsi | 54 +++++
> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++
> drivers/mfd/qcom-pm8008.c | 60 ++++-
> drivers/regulator/Kconfig | 9 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/qcom-pm8008-regulator.c | 242 +++++++++++++++++++++
> include/linux/mfd/qcom_pm8008.h | 9 +
> 8 files changed, 492 insertions(+), 18 deletions(-)
> create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
> create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
> create mode 100644 include/linux/mfd/qcom_pm8008.h
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver
2023-03-17 8:06 ` [PATCH V15 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver Luca Weiss
@ 2023-07-07 8:54 ` Luca Weiss
0 siblings, 0 replies; 54+ messages in thread
From: Luca Weiss @ 2023-07-07 8:54 UTC (permalink / raw)
To: Luca Weiss, Satya Priya, Satya Priya Kakitapalli (Temp),
Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
On Fri Mar 17, 2023 at 9:06 AM CET, Luca Weiss wrote:
> Hi Satya,
>
> On Tue Jun 14, 2022 at 11:48 AM CEST, Satya Priya wrote:
> > Satya Priya (9):
> > dt-bindings: mfd: pm8008: Add reset-gpios
> > dt-bindings: mfd: pm8008: Change the address cells
> > dt-bindings: mfd: pm8008: Add regulators for pm8008
> > mfd: pm8008: Add reset-gpios
> > mfd: pm8008: Remove the regmap member from pm8008_data struct
> > mfd: pm8008: Use i2c_new_dummy_device() API
> > regulator: Add a regulator driver for the PM8008 PMIC
> > arm64: dts: qcom: pm8008: Add base dts file
> > arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp
>
> Is there any activity on this patch series? It's been a few months since
> this revision. Would be interested in this for the pm8008 found on
> sm7225-fairphone-fp4.
Hi all,
Quick ping again, I'd really like to see this PM8008 series land.
Regards
Luca
>
> Regards
> Luca
>
> >
> > .../devicetree/bindings/mfd/qcom,pm8008.yaml | 69 +++++-
> > arch/arm64/boot/dts/qcom/pm8008.dtsi | 54 +++++
> > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++
> > drivers/mfd/qcom-pm8008.c | 60 ++++-
> > drivers/regulator/Kconfig | 9 +
> > drivers/regulator/Makefile | 1 +
> > drivers/regulator/qcom-pm8008-regulator.c | 242 +++++++++++++++++++++
> > include/linux/mfd/qcom_pm8008.h | 9 +
> > 8 files changed, 492 insertions(+), 18 deletions(-)
> > create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
> > create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
> > create mode 100644 include/linux/mfd/qcom_pm8008.h
> >
> > --
> > 2.7.4
^ permalink raw reply [flat|nested] 54+ messages in thread