Messages in this thread | | | Date | Tue, 28 Jun 2022 10:23:19 +0530 | Subject | Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API | From | "Satya Priya Kakitapalli (Temp)" <> |
| |
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[] = { >
| |