| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <52c6ab15-1cd8-324e-4bcc-c449d8bceb19@quicinc.com>
Date: Fri, 1 Jul 2022 14:17:48 +0530
From: "Satya Priya Kakitapalli (Temp)" <quic_c_skakit@...cinc.com>
To: Lee Jones <lee.jones@...aro.org>
CC: Bjorn Andersson <bjorn.andersson@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <swboyd@...omium.org>,
<quic_collinsd@...cinc.com>, <quic_subbaram@...cinc.com>,
<quic_jprakash@...cinc.com>
Subject: Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
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@...cinc.com>
>>>>>>>>>>>>>>> Reviewed-by: Stephen Boyd <swboyd@...omium.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@...aro.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@...aro.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?
Powered by blists - more mailing lists