Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API

["Satya Priya Kakitapalli (Temp)" <quic_c_skakit@quicinc.com>]

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?