Re: [PATCH V10 5/9] mfd: pm8008: Use i2c_new_dummy_device() API

From: "Satya Priya Kakitapalli (Temp)" <quic_c_skakit@quicinc.com>
To: Stephen Boyd <swboyd@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rob Herring <robh+dt@kernel.org>
Cc: Lee Jones <lee.jones@linaro.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, <linux-arm-msm@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<quic_collinsd@quicinc.com>, <quic_subbaram@quicinc.com>,
	<quic_jprakash@quicinc.com>
Subject: Re: [PATCH V10 5/9] mfd: pm8008: Use i2c_new_dummy_device() API
Date: Mon, 18 Apr 2022 20:38:33 +0530	[thread overview]
Message-ID: <9d8543e1-4aa8-f430-322a-83d110586152@quicinc.com> (raw)
In-Reply-To: <CAE-0n531GVMuuR6HM2ezjOPJCg7S-rD3eaEKWzrYsnM-jwQHKw@mail.gmail.com>

On 4/15/2022 5:51 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-04-14 05:30:14)
>> Use i2c_new_dummy_device() to register clients along with
>> the main mfd device.
> Why?

As discussed on V9 of this series, I've done these changes.

By using this API we can register other clients at different address 
space without having separate DT node.

This avoids calling the probe twice for the same chip, once for each 
address space 0x8 and 0x9. I'll add this description in commit text.

>> Signed-off-by: Satya Priya<quic_c_skakit@quicinc.com>
>> ---
>> Changes in V10:
>>   - Implement i2c_new_dummy_device to register extra clients.
>>
>>   drivers/mfd/qcom-pm8008.c       | 54 +++++++++++++++++++++++++++++++++--------
>>   include/linux/mfd/qcom_pm8008.h | 13 ++++++++++
>>   2 files changed, 57 insertions(+), 10 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 97a72da..ca5240d 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -56,8 +57,10 @@ enum {
>>   #define PM8008_PERIPH_OFFSET(paddr)    (paddr - PM8008_PERIPH_0_BASE)
>>
>>   struct pm8008_data {
>> +       bool ready;
>>          struct device *dev;
>> -       struct regmap *regmap;
>> +       struct i2c_client *clients[PM8008_NUM_CLIENTS];
>> +       struct regmap *regmap[PM8008_NUM_CLIENTS];
>>          struct gpio_desc *reset_gpio;
>>          int irq;
>>          struct regmap_irq_chip_data *irq_data;
>> @@ -152,9 +155,20 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>>          .max_register   = 0xFFFF,
>>   };
>>
>> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid)
>> +{
>> +       if (!chip || !chip->ready) {
> Is it even possible?

I think no, I'll remove it.

>> +               pr_err("pm8008 chip not initialized\n");
>> +               return NULL;
>> +       }
>> +
>> +       return chip->regmap[sid];
>> +}
>> +
>>   static int pm8008_init(struct pm8008_data *chip)
>>   {
>>          int rc;
>> +       struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);
>>
>>          /*
>>           * Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework
>> @@ -162,19 +176,19 @@ 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,
>> +       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,
>> +       rc = regmap_write(regmap,
>>                           (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>          if (rc)
>>                  return rc;
>>
>> -       rc = regmap_write(chip->regmap,
>> +       rc = regmap_write(regmap,
>>                           (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>
>>          return rc;
>> @@ -186,6 +200,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>          int rc, i;
>>          struct regmap_irq_type *type;
>>          struct regmap_irq_chip_data *irq_data;
>> +       struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);
> Instead of calling pm8008_get_regmap() many times why not pass the
> regmap through from pm8008_probe_irq_peripherals() called in probe? At
> that point we could remove the regmap pointer from struct pm8008_data?

Okay.

>>          rc = pm8008_init(chip);
>>          if (rc) {
>> @@ -209,7 +224,7 @@ 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(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);
>> @@ -221,19 +236,38 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>
>>   static int pm8008_probe(struct i2c_client *client)
>>   {
>> -       int rc;
>> +       int rc, i;
>>          struct pm8008_data *chip;
>> +       struct device_node *node = client->dev.of_node;
>>
>>          chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>          if (!chip)
>>                  return -ENOMEM;
>>
>>          chip->dev = &client->dev;
>> -       chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>> -       if (!chip->regmap)
>> -               return -ENODEV;
>>
>> -       i2c_set_clientdata(client, chip);
>> +       for (i = 0; i < PM8008_NUM_CLIENTS; i++) {
> This is 2. Why do we have a loop? Just register the i2c client for
> pm8008_infra first and then make a dummy for the second address without
> the loop and the indentation. Are there going to be more i2c clients?

There wont be more than 2 clients, I can remove the loop, but then we 
will have repetitive code.. something like below

      chip->dev = &client->dev;
      i2c_set_clientdata(client, chip);

      regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
      if (!regmap)
          return -ENODEV;

      pm8008_client = i2c_new_dummy_device(client->adapter,
                              client->addr + 1);
      if (IS_ERR(pm8008_client)) {
          dev_err(&pm8008_client->dev, "can't attach client\n");
          return PTR_ERR(pm8008_client);
      }
      pm8008_client->dev.of_node = of_node_get(client->dev.of_node);
      i2c_set_clientdata(pm8008_client, chip);

      regulators_regmap = devm_regmap_init_i2c(pm8008_client, 
&qcom_mfd_regmap_cfg[1]);
      if (!regmap)
          return -ENODEV;

>> +               if (i == 0) {
>> +                       chip->clients[i] = client;
>> +               } else {
>> +                       chip->clients[i] = i2c_new_dummy_device(client->adapter,
>> +                                                               client->addr + i);
>> +                       if (IS_ERR(chip->clients[i])) {
>> +                               dev_err(&client->dev, "can't attach client %d\n", i);
>> +                               return PTR_ERR(chip->clients[i]);
>> +                       }
>> +                       chip->clients[i]->dev.of_node = of_node_get(node);
>> +               }
>> +
>> +               chip->regmap[i] = devm_regmap_init_i2c(chip->clients[i],
>> +                                                       &qcom_mfd_regmap_cfg);
>> +               if (!chip->regmap[i])
>> +                       return -ENODEV;
>> +
>> +               i2c_set_clientdata(chip->clients[i], chip);
>> +       }
>> +
>> +       chip->ready = true;
>>
>>          if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
>>                  rc = pm8008_probe_irq_peripherals(chip, client->irq);
>> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
>> new file mode 100644
>> index 0000000..bc64f01
>> --- /dev/null
>> +++ b/include/linux/mfd/qcom_pm8008.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __QCOM_PM8008_H__
>> +#define __QCOM_PM8008_H__
>> +
>> +#define PM8008_INFRA_SID       0
>> +#define PM8008_REGULATORS_SID  1
>> +
>> +#define PM8008_NUM_CLIENTS     2
>> +
>> +struct pm8008_data;
>> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid);
> Could this be avoided if the regulator driver used
> dev_get_regmap(&pdev->dev.parent, "regulator") to find the regmap named
> "regulator" of the parent device, i.e. pm8008-infra.

I gave it a try, it didn't work. I could not get the regmap for 
regulators using pm8008-infra i.e., 0x8 device pointer.

I checked the other drivers using i2c_new_dummy_device(), they are 
either getting the regmap by accessing the mfd struct through global 
variables or using headers.