Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxx>
Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
---
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@xxxxxxxxxx>

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@xxxxxxxxxx>
---
   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[] = {




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux