| 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: <YrlfF+DMlGFsVBdk@google.com>
Date: Mon, 27 Jun 2022 08:41:11 +0100
From: Lee Jones <lee.jones@...aro.org>
To: "Satya Priya Kakitapalli (Temp)" <quic_c_skakit@...cinc.com>
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 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.
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);
- 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[] = {
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists