lkml.org 
[lkml]   [2022]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
From

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

In qcom-pm8008-regulator.c I tried to get the regmap using


dev_get_regmap(pdev->dev.parent, "regulators");


\
 
 \ /
  Last update: 2022-07-01 08:48    [W:0.075 / U:3.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site