diff mbox series

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

Message ID 1649939418-19861-6-git-send-email-quic_c_skakit@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add Qualcomm Technologies, Inc. PM8008 regulator driver | expand

Commit Message

Satya Priya Kakitapalli (Temp) April 14, 2022, 12:30 p.m. UTC
Use i2c_new_dummy_device() to register clients along with
the main mfd device.

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

Comments

Stephen Boyd April 15, 2022, 12:21 a.m. UTC | #1
Quoting Satya Priya (2022-04-14 05:30:14)
> Use i2c_new_dummy_device() to register clients along with
> the main mfd device.

Why?

>
> 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?

> +               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?

>
>         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?

> +               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.
Satya Priya Kakitapalli (Temp) April 18, 2022, 3:08 p.m. UTC | #2
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.
Stephen Boyd April 18, 2022, 7:23 p.m. UTC | #3
Quoting Satya Priya Kakitapalli (Temp) (2022-04-18 08:08:33)
>
> 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.
>

Perfect, thanks.

>
> >> @@ -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

Repetitive code can be refactored into a subroutine.

>
>       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;
>
> >> 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.

Did it not work because the regmap config for the regulator config
didn't have a name?
Satya Priya Kakitapalli (Temp) April 19, 2022, 6:04 a.m. UTC | #4
On 4/19/2022 12:53 AM, Stephen Boyd wrote:
> Quoting Satya Priya Kakitapalli (Temp) (2022-04-18 08:08:33)
>> 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.
>>
> Perfect, thanks.
>
>>>> @@ -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
> Repetitive code can be refactored into a subroutine.
>
>>        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;
>>
>>>> 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.
> Did it not work because the regmap config for the regulator config
> didn't have a name?


No I specified the name "regulators" in the regmap_config struct for 
regulator config.
diff mbox series

Patch

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
@@ -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>
@@ -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) {
+		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);
 
 	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++) {
+		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);
+
+#endif