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