Message-ID: <Yr6oLlmfWRkiAZG7@google.com>
Date:   Fri, 1 Jul 2022 08:54:22 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     "Satya Priya Kakitapalli (Temp)" <quic_c_skakit@...cinc.com>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        swboyd@...omium.org, quic_collinsd@...cinc.com,
        quic_subbaram@...cinc.com, quic_jprakash@...cinc.com
Subject: Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API

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@...cinc.com>
> > > > > > > > > > > > > > Reviewed-by: Stephen Boyd <swboyd@...omium.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@...aro.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@...aro.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

Powered by blists - more mailing lists