2021-06-29 10:35:35

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: [PATCH v10 2/5] regulator: hi6421v600-regulator: fix platform drvdata

platform drvdata can't be used inside the regulator driver,
as this is already used by the MFD and SPMI drivers.

So, change the logic to allocate it inside the
struct hi6421_spmi_pmic.

While here, drop the now unused struct and add a missing dot
at the Huawei's copyrights.

Fixes: 50e629362e1f ("regulator: hi6421v600: Fix setting wrong driver_data")

Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
drivers/misc/hi6421v600-irq.c | 9 ++--
drivers/regulator/hi6421v600-regulator.c | 49 +++++++++++----------
drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 18 +++-----
include/linux/mfd/hi6421-spmi-pmic.h | 25 -----------
4 files changed, 35 insertions(+), 66 deletions(-)
delete mode 100644 include/linux/mfd/hi6421-spmi-pmic.h

diff --git a/drivers/misc/hi6421v600-irq.c b/drivers/misc/hi6421v600-irq.c
index 7c1468f0ea01..0c2477480450 100644
--- a/drivers/misc/hi6421v600-irq.c
+++ b/drivers/misc/hi6421v600-irq.c
@@ -10,7 +10,6 @@
#include <linux/bitops.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
-#include <linux/mfd/hi6421-spmi-pmic.h>
#include <linux/module.h>
#include <linux/of_gpio.h>
#include <linux/platform_device.h>
@@ -220,7 +219,7 @@ static int hi6421v600_irq_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = pmic_dev->of_node;
struct hi6421v600_irq *priv;
- struct hi6421_spmi_pmic *pmic;
+ struct regmap *regmap;
unsigned int virq;
int i, ret;

@@ -229,8 +228,8 @@ static int hi6421v600_irq_probe(struct platform_device *pdev)
* which should first set drvdata. If this doesn't happen, hit
* a warn on and return.
*/
- pmic = dev_get_drvdata(pmic_dev);
- if (WARN_ON(!pmic))
+ regmap = dev_get_drvdata(pmic_dev);
+ if (WARN_ON(!regmap))
return -ENODEV;

priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -238,7 +237,7 @@ static int hi6421v600_irq_probe(struct platform_device *pdev)
return -ENOMEM;

priv->dev = dev;
- priv->regmap = pmic->regmap;
+ priv->regmap = regmap;

spin_lock_init(&priv->lock);

diff --git a/drivers/regulator/hi6421v600-regulator.c b/drivers/regulator/hi6421v600-regulator.c
index 9b162c0555c3..0e78535eca62 100644
--- a/drivers/regulator/hi6421v600-regulator.c
+++ b/drivers/regulator/hi6421v600-regulator.c
@@ -4,27 +4,25 @@
//
// Copyright (c) 2013 Linaro Ltd.
// Copyright (c) 2011 HiSilicon Ltd.
-// Copyright (c) 2020-2021 Huawei Technologies Co., Ltd
+// Copyright (c) 2020-2021 Huawei Technologies Co., Ltd.
//
// Guodong Xu <[email protected]>

#include <linux/delay.h>
-#include <linux/mfd/hi6421-spmi-pmic.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/regulator/driver.h>
#include <linux/spmi.h>

-struct hi6421_spmi_reg_priv {
- /* Serialize regulator enable logic */
- struct mutex enable_mutex;
-};
-
struct hi6421_spmi_reg_info {
struct regulator_desc desc;
u8 eco_mode_mask;
u32 eco_uA;
+
+ /* Serialize regulator enable logic */
+ struct mutex *enable_mutex;
};

static const unsigned int ldo3_voltages[] = {
@@ -98,12 +96,11 @@ static const unsigned int ldo34_voltages[] = {

static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
{
- struct hi6421_spmi_reg_priv *priv;
+ struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev);
int ret;

- priv = dev_get_drvdata(rdev->dev.parent);
/* cannot enable more than one regulator at one time */
- mutex_lock(&priv->enable_mutex);
+ mutex_lock(sreg->enable_mutex);

ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
rdev->desc->enable_mask,
@@ -112,7 +109,7 @@ static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
/* Avoid powering up multiple devices at the same time */
usleep_range(rdev->desc->off_on_delay, rdev->desc->off_on_delay + 60);

- mutex_unlock(&priv->enable_mutex);
+ mutex_unlock(sreg->enable_mutex);

return ret;
}
@@ -231,11 +228,12 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
{
struct device *pmic_dev = pdev->dev.parent;
struct regulator_config config = { };
- struct hi6421_spmi_reg_priv *priv;
+ struct hi6421_spmi_reg_info *sreg;
struct hi6421_spmi_reg_info *info;
struct device *dev = &pdev->dev;
- struct hi6421_spmi_pmic *pmic;
+ struct regmap *regmap;
struct regulator_dev *rdev;
+ struct mutex *enable_mutex;
int i;

/*
@@ -243,23 +241,27 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
* which should first set drvdata. If this doesn't happen, hit
* a warn on and return.
*/
- pmic = dev_get_drvdata(pmic_dev);
- if (WARN_ON(!pmic))
+ regmap = dev_get_drvdata(pmic_dev);
+ if (WARN_ON(!regmap))
return -ENODEV;

- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- mutex_init(&priv->enable_mutex);
- platform_set_drvdata(pdev, priv);
+ enable_mutex = devm_kzalloc(dev, sizeof(*enable_mutex), GFP_KERNEL);
+ mutex_init(enable_mutex);

for (i = 0; i < ARRAY_SIZE(regulator_info); i++) {
info = &regulator_info[i];

+ sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
+ if (!sreg)
+ return -ENOMEM;
+
+ sreg->enable_mutex = enable_mutex;
+ sreg->eco_mode_mask = regulator_info[i].eco_mode_mask;
+ sreg->eco_uA = regulator_info[i].eco_uA;
+
config.dev = pdev->dev.parent;
- config.driver_data = info;
- config.regmap = pmic->regmap;
+ config.driver_data = sreg;
+ config.regmap = regmap;

rdev = devm_regulator_register(dev, &info->desc, &config);
if (IS_ERR(rdev)) {
@@ -289,4 +291,3 @@ module_platform_driver(hi6421_spmi_regulator_driver);

MODULE_DESCRIPTION("Hi6421v600 SPMI regulator driver");
MODULE_LICENSE("GPL v2");
-
diff --git a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
index 0b5686655954..f63ba73c9e33 100644
--- a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
+++ b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
@@ -4,11 +4,10 @@
*
* Copyright (c) 2013 Linaro Ltd.
* Copyright (c) 2011 Hisilicon.
- * Copyright (c) 2020-2021 Huawei Technologies Co., Ltd
+ * Copyright (c) 2020-2021 Huawei Technologies Co., Ltd.
*/

#include <linux/mfd/core.h>
-#include <linux/mfd/hi6421-spmi-pmic.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
@@ -30,19 +29,14 @@ static const struct regmap_config regmap_config = {
static int hi6421_spmi_pmic_probe(struct spmi_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct regmap *regmap;
int ret;
- struct hi6421_spmi_pmic *ddata;
- ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
- if (!ddata)
- return -ENOMEM;

- ddata->regmap = devm_regmap_init_spmi_ext(pdev, &regmap_config);
- if (IS_ERR(ddata->regmap))
- return PTR_ERR(ddata->regmap);
+ regmap = devm_regmap_init_spmi_ext(pdev, &regmap_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);

- ddata->dev = dev;
-
- dev_set_drvdata(&pdev->dev, ddata);
+ dev_set_drvdata(&pdev->dev, regmap);

ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
hi6421v600_devs, ARRAY_SIZE(hi6421v600_devs),
diff --git a/include/linux/mfd/hi6421-spmi-pmic.h b/include/linux/mfd/hi6421-spmi-pmic.h
deleted file mode 100644
index e5b8dbf828b6..000000000000
--- a/include/linux/mfd/hi6421-spmi-pmic.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Header file for device driver Hi6421 PMIC
- *
- * Copyright (c) 2013 Linaro Ltd.
- * Copyright (C) 2011 Hisilicon.
- * Copyright (c) 2020-2021 Huawei Technologies Co., Ltd
- *
- * Guodong Xu <[email protected]>
- */
-
-#ifndef __HISI_PMIC_H
-#define __HISI_PMIC_H
-
-#include <linux/irqdomain.h>
-#include <linux/regmap.h>
-
-struct hi6421_spmi_pmic {
- struct resource *res;
- struct device *dev;
- void __iomem *regs;
- struct regmap *regmap;
-};
-
-#endif /* __HISI_PMIC_H */
--
2.31.1


2021-06-29 11:11:42

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH v10 2/5] regulator: hi6421v600-regulator: fix platform drvdata

Em Tue, 29 Jun 2021 18:48:31 +0800
Axel Lin <[email protected]> escreveu:

> Mauro Carvalho Chehab <[email protected]> 於 2021年6月29日 週二 下午6:31寫道:
>
> > platform drvdata can't be used inside the regulator driver,
> > as this is already used by the MFD and SPMI drivers.
> >
>
> Could you point out which part of the code set the platform drvdata?
> My understanding is the mfd only set dev->platform_data rather than
> dev->driver_data.
> If you mean the dev_set_drvdata() call in hi6421_spmi_pmic_probe, it's the
> parent device of the regulator pdev.

It needs to be double-checked, but I guess the SPMI or the SPMI controller
driver already uses it.

See, there are 5 drivers involved, all of them using private data
and passing data to child drivers, called on the order below:

spmi
hisi-spmi-controller
hi6421-spmi-pmic
mfd drivers:
hi6421v600-irq
hi6421-regulator

If you're in doubt, try to apply this series, and then change the
driver again to use platform data. You'll see that it will stop
booting after initializing the first regulator.

Thanks,
Mauro

2021-06-29 15:39:51

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH v10 2/5] regulator: hi6421v600-regulator: fix platform drvdata

Em Tue, 29 Jun 2021 20:51:01 +0800
Axel Lin <[email protected]> escreveu:

> > If you're in doubt, try to apply this series, and then change the
> > driver again to use platform data. You'll see that it will stop
> > booting after initializing the first regulator.
> >
>
> Sorry I don't have this h/w to test.

Then, why do you care? Your patch broke the driver. Changes like
that should be tested on a real hardware, in order to avoid
regressions.

Thanks,
Mauro

2021-06-29 15:42:46

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v10 2/5] regulator: hi6421v600-regulator: fix platform drvdata

On Tue, Jun 29, 2021 at 12:31:28PM +0200, Mauro Carvalho Chehab wrote:

> platform drvdata can't be used inside the regulator driver,
> as this is already used by the MFD and SPMI drivers.

Can you clarify what exactly is using which platform drvdata already?
This all feels very confused and I can't tell what the problem that's
being fixed is, if it's a real issue or how this fixes it.

> drivers/misc/hi6421v600-irq.c | 9 ++--
> drivers/regulator/hi6421v600-regulator.c | 49 +++++++++++----------
> drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 18 +++-----
> include/linux/mfd/hi6421-spmi-pmic.h | 25 -----------

I'm especially nervous about the core driver still being in staging
perhaps meaning there's some issue with it doing odd and confusing
things.


Attachments:
(No filename) (807.00 B)
signature.asc (499.00 B)
Download all attachments

2021-06-29 19:35:34

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH v10 2/5] regulator: hi6421v600-regulator: fix platform drvdata

Em Tue, 29 Jun 2021 16:11:01 +0100
Mark Brown <[email protected]> escreveu:

> On Tue, Jun 29, 2021 at 12:31:28PM +0200, Mauro Carvalho Chehab wrote:
>
> > platform drvdata can't be used inside the regulator driver,
> > as this is already used by the MFD and SPMI drivers.
>
> Can you clarify what exactly is using which platform drvdata already?
> This all feels very confused and I can't tell what the problem that's
> being fixed is, if it's a real issue or how this fixes it.

I don't remember the dirty details anymore... It has been almost a year
since when I started doing that. The SPMI controller driver left staging
8 months ago.

I guess it is related with passing the parent's device to
devm_regulator_register() at the hi6421v600-regulator driver:

struct regulator_config config = { };
...
config.dev = pdev->dev.parent;
...
rdev = devm_regulator_register(dev, &info->desc, &config);

This is needed by SPMI bus and the SPMI controller in order to use the
right platform data when talking to the hardware.

> > drivers/misc/hi6421v600-irq.c | 9 ++--
> > drivers/regulator/hi6421v600-regulator.c | 49 +++++++++++----------
> > drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 18 +++-----
> > include/linux/mfd/hi6421-spmi-pmic.h | 25 -----------
>
> I'm especially nervous about the core driver still being in staging
> perhaps meaning there's some issue with it doing odd and confusing
> things.

The only missing part in staging is the MFD driver. At the current way,
it is very simple (71 lines in total): it just declares a regmap, and has
a single function on it:

static int hi6421_spmi_pmic_probe(struct spmi_device *pdev)
{
struct device *dev = &pdev->dev;
int ret;
struct hi6421_spmi_pmic *ddata;
ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
if (!ddata)
return -ENOMEM;

ddata->regmap = devm_regmap_init_spmi_ext(pdev, &regmap_config);
if (IS_ERR(ddata->regmap))
return PTR_ERR(ddata->regmap);

ddata->dev = dev;

dev_set_drvdata(&pdev->dev, ddata);

ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
hi6421v600_devs, ARRAY_SIZE(hi6421v600_devs),
NULL, 0, NULL);
if (ret < 0)
dev_err(dev, "Failed to add child devices: %d\n", ret);

return ret;
}

You can see the full driver's code at:
https://lore.kernel.org/lkml/8d871e2ccc544d11959c16d8312dbf03dd01b1c8.1624962269.git.mchehab+huawei@kernel.org/#Z30drivers:mfd:hi6421-spmi-pmic.c

I'm not aware of anything left preventing it to leave staging.

Thanks,
Mauro

2021-06-30 08:23:38

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH v10 2/5] regulator: hi6421v600-regulator: fix platform drvdata

Em Tue, 29 Jun 2021 16:11:01 +0100
Mark Brown <[email protected]> escreveu:

> On Tue, Jun 29, 2021 at 12:31:28PM +0200, Mauro Carvalho Chehab wrote:
>
> > platform drvdata can't be used inside the regulator driver,
> > as this is already used by the MFD and SPMI drivers.
>
> Can you clarify what exactly is using which platform drvdata already?
> This all feels very confused and I can't tell what the problem that's
> being fixed is, if it's a real issue or how this fixes it.

It turns that the problem was not related to any bad usage of
platform data by MFD/SPMI drivers. There used to have a need to access
the SPMI privdata before we added support for regmap, but this is
long gone.

The problem is just that the Axel's patch was setting the platform
drvdata, but the internal logic was addressing a different memory
region (from the SPMI driver), because hi6421v600-regulator driver sets
the config.dev as:

config.dev = pdev->dev.parent;

The config.dev needs to point to the SPMI driver, in order to ensure that
regulator_register() will access the proper OF data from the SPMI
devicetree descriptors here:

init_data = regulator_of_get_init_data(dev, regulator_desc, config,
&rdev->dev.of_node);

With the Axel's fixup patch:

https://lore.kernel.org/lkml/[email protected]/T/#t

the regression was solved.

Thanks,
Mauro