linux-kernel - Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 20 Jun 2022 10:58:24 +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/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.


>>   static int pm8008_init(struct regmap *regmap)
>>   {
>>   	int rc;
>> @@ -217,11 +225,25 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>   	return 0;
>>   }
>>   
>> +static struct regmap *pm8008_regmap_init(struct i2c_client *client,
>> +							struct pm8008_data *chip)
>> +{
>> +	struct regmap *regmap;
>> +
>> +	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>> +	if (!regmap)
>> +		return NULL;
>> +
>> +	i2c_set_clientdata(client, chip);
>> +	return regmap;
>> +}
> This function seems superfluous.
>
> It's only called once and it contains a single call.


No, It is being called twice. To avoid repetitive code, I've added this 
subroutine.


> Just pop the call directly into probe.
>
>>   static int pm8008_probe(struct i2c_client *client)
>>   {
>>   	int rc;
>>   	struct pm8008_data *chip;
>>   	struct gpio_desc *reset_gpio;
>> +	struct i2c_client *regulators_client;
>>   	struct regmap *regmap;
>>   
>>   	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>> @@ -229,11 +251,19 @@ static int pm8008_probe(struct i2c_client *client)
>>   		return -ENOMEM;
>>   
>>   	chip->dev = &client->dev;
>> -	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>> +	regmap = pm8008_regmap_init(client, chip);
>>   	if (!regmap)
>>   		return -ENODEV;
>>   
>> -	i2c_set_clientdata(client, chip);
>> +	regulators_client = i2c_new_dummy_device(client->adapter, client->addr + 1);
>> +	if (IS_ERR(regulators_client)) {
>> +		dev_err(&client->dev, "can't attach client\n");
>> +		return PTR_ERR(regulators_client);
>> +	}
>> +
>> +	chip->regulators_regmap = pm8008_regmap_init(regulators_client, chip);
>> +	if (!chip->regulators_regmap)
>> +		return -ENODEV;
>>   
>>   	reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
>>   	if (IS_ERR(reset_gpio))
>> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
>> new file mode 100644
>> index 0000000..3814bff
>> --- /dev/null
>> +++ b/include/linux/mfd/qcom_pm8008.h
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +// Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> +#ifndef __QCOM_PM8008_H__
>> +#define __QCOM_PM8008_H__
>> +
>> +struct pm8008_data;
>> +struct regmap *pm8008_get_regmap(const struct pm8008_data *chip);
>> +
>> +#endif