Messages in this thread | | | Date | Thu, 30 Jun 2022 15:07:29 +0530 | Subject | Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API | From | "Satya Priya Kakitapalli (Temp)" <> |
| |
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
| |