| 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: <4112b5af-15de-007c-fcc2-c31ce9f9e426@quicinc.com>
Date: Wed, 29 Jun 2022 16:06:54 +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 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.
>>> - if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
>>> - rc = pm8008_probe_irq_peripherals(chip, client->irq);
>>> + if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
>>> + rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
>>> if (rc)
>>> - dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>>> + dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
>>> }
>>> - return devm_of_platform_populate(chip->dev);
>>> + return devm_of_platform_populate(dev);
>>> }
>>> static const struct of_device_id pm8008_match[] = {
>>>
Powered by blists - more mailing lists