Messages in this thread | | | Date | Fri, 25 Jun 2021 18:39:35 +0200 | From | Mauro Carvalho Chehab <> | Subject | Re: [PATCH] regulator: hi6421v600: Fix setting wrong driver_data |
| |
Em Tue, 22 Jun 2021 12:33:29 +0800 Axel Lin <axel.lin@ingics.com> escreveu:
> Current code set "config.driver_data = sreg" but sreg only init the mutex, > the othere fields are just zero. Fix it by pass *info to config.driver_data > so each regulator can get corresponding data by rdev_get_drvdata(). > > Separate enable_mutex from struct hi6421_spmi_reg_info since only need one > mutex for the driver. > > Fixes: d2dfd50a0b57 ("staging: hikey9xx: hi6421v600-regulator: move LDO config from DT") > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > drivers/regulator/hi6421v600-regulator.c | 26 ++++++++++++++---------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/regulator/hi6421v600-regulator.c b/drivers/regulator/hi6421v600-regulator.c > index b5a19938fd3a..21153ee03a3f 100644 > --- a/drivers/regulator/hi6421v600-regulator.c > +++ b/drivers/regulator/hi6421v600-regulator.c > @@ -16,13 +16,15 @@ > #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[] = { > @@ -96,11 +98,12 @@ static const unsigned int ldo34_voltages[] = { > > static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev) > { > - struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev); > + struct hi6421_spmi_reg_priv *priv; > int ret; > > + priv = dev_get_drvdata(rdev->dev.parent); > /* cannot enable more than one regulator at one time */ > - mutex_lock(&sreg->enable_mutex); > + mutex_lock(&priv->enable_mutex); > > ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > rdev->desc->enable_mask, > @@ -109,7 +112,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(&sreg->enable_mutex); > + mutex_unlock(&priv->enable_mutex); > > return ret; > } > @@ -225,7 +228,7 @@ 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_info *sreg; > + struct hi6421_spmi_reg_priv *priv; > struct hi6421_spmi_reg_info *info; > struct device *dev = &pdev->dev; > struct hi6421_spmi_pmic *pmic; > @@ -241,17 +244,18 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev) > if (WARN_ON(!pmic)) > return -ENODEV; > > - sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL); > - if (!sreg) > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > return -ENOMEM; > > - mutex_init(&sreg->enable_mutex); > + mutex_init(&priv->enable_mutex);
> + platform_set_drvdata(pdev, priv);
This won't work, as platform data was already set, as it is already used as part of the MFD/SPMI binding logic. With the current struct hi6421_spmi_pmic, this is not a big problem, as, due to some code churns, there are enough space for a mutex at the beginning of the struct, but the code breaks when those are removed.
I'll send a separate patch addressing this issue.
Thanks, Mauro
| |