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 7/1/2022 1:24 PM, Lee Jones wrote:
> 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@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);
> 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"


I thought you asked whether we have to pass two regmaps to the child
regulator driver.


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


Okay, So I will leave the driver as is then?

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