| 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: <20200812094353.00006311@huawei.com>
Date: Wed, 12 Aug 2020 09:43:53 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
CC: Rob Herring <robh@...nel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>, Stephen Boyd <sboyd@...nel.org>,
<linux-arm-msm@...r.kernel.org>, Mark Brown <broonie@...nel.org>,
Mayulong <mayulong1@...wei.com>, <linuxarm@...wei.com>,
Liam Girdwood <lgirdwood@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Lee Jones <lee.jones@...aro.org>,
"David S. Miller" <davem@...emloft.net>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 00/33] Add driver for HiSilicon SPMI PMIC for Hikey 970
On Wed, 12 Aug 2020 09:45:40 +0200
Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:
....
>
> >
> > > menus in order to enable them.
> > > We communicate with the Hi6421 via memory-mapped I/O.
> > >
> > > +config MFD_HI6421_SPMI
> > > + tristate "HiSilicon Hi6421v600 SPMI PMU/Codec IC"
> > > + depends on OF
> > > + select MFD_CORE
> > > + select REGMAP_MMIO
> >
> > Nice thought, but it doesn't use it yet!
>
> What do you mean? Makefile should be using it:
>
> $ git grep MFD_HI6421_SPMI
> drivers/mfd/Kconfig:config MFD_HI6421_SPMI
> drivers/mfd/Makefile:obj-$(CONFIG_MFD_HI6421_SPMI) += hi6421-spmi-pmic.o
Just the REGMAP bit. Driver doesn't seem to be using regmap.
>
>
> > > + *
> > > + */
> > > +
> > > +#include <linux/slab.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/err.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_gpio.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/mfd/hi6421-spmi-pmic.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/spmi.h>
> > > +#ifndef NO_IRQ
> > > +#define NO_IRQ 0
> >
> > Drop
>
> Ok! I was in doubt about that, as there are a few other drivers with
> the same pattern:
>
> drivers/ata/sata_dwc_460ex.c:#define NO_IRQ 0
> drivers/input/touchscreen/ucb1400_ts.c:#define NO_IRQ 0
> drivers/mmc/host/of_mmc_spi.c:#define NO_IRQ 0
> drivers/pcmcia/pd6729.c:#define NO_IRQ ((unsigned int)(0))
> drivers/rtc/rtc-m48t59.c:#define NO_IRQ (-1)
>
> Btw, this definition at the rtc driver sounds weird ;-)
History I guess. IIRC, a long time back, 0 was a valid IRQ for some architectures.
>
> > > + /*SOC_PMIC_IRQ0_ADDR*/
> >
> > These superficially feel like things that would come from
> > the compatible, but maybe I'm missing something.
>
> My guess is that this is decided by the board manufacturer. I mean,
> from the schematics:
>
> https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
>
> This is a separate chipset, connected at the Kirin 970 SPMI bus. The SPMI bus
> allows up to 16 PMICs. So, I would assume that a chip designed to support
> SPMI would have the flexibility of using different IRQ lines, being
> programmable either via some firmware or via some wiring.
>
> Without knowing more, IMO the best would be to keep those settings
> at DT.
Fair enough. Would be nice if they could be established from a bus ID of
some type and the compatible but if we don't have the info to be able to
do that I guess we will have to do it this way.
> >
> > > + ret = get_pmic_device_tree_data(np, pmic);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "Error reading hisi pmic dts\n");
> > > + return ret;
> > > + }
> > > +
> > > + /* TODO: get and enable clk request */
> > > + spin_lock_init(&pmic->lock);
> > > +
> > > + pmic->dev = dev;
> > > +
> > > + pmic->gpio = of_get_gpio_flags(np, 0, &flags);
> >
> > Do we need flags for something?
> >
> > Can we use the gpio/consumer.h interfaces for all this?
> >
> > Though I'm not sure we need a gpio at all. Looks like we just
> > use it as an interrupt. Get an interrupt directly instead.
>
> Yeah, from what I saw, the GPIOs supported right now by this driver
> are just for IRQs.
Guess they all need switching to just being interrupt lines then.
>
> > > + if (pmic->gpio < 0)
> > > + return pmic->gpio;
> > > +
> > > + if (!gpio_is_valid(pmic->gpio))
> > > + return -EINVAL;
> > > +
> > > + ret = gpio_request_one(pmic->gpio, GPIOF_IN, "pmic");
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to request gpio%d\n", pmic->gpio);
> > > + return ret;
> > > + }
> > > +
> > > + ret = request_threaded_irq(pmic->irq, hi6421_spmi_irq_handler, NULL,
> > > + IRQF_TRIGGER_LOW | IRQF_SHARED | IRQF_NO_SUSPEND,
> > > + "pmic", pmic);
> > > + if (ret < 0) {
> > > + dev_err(dev, "could not claim pmic %d\n", ret);
> > > + ret = -ENODEV;
> > > + goto request_theaded_irq;
> > > + }
> > > +
> > > + dev_set_drvdata(&pdev->dev, pmic);
> > > +
> > > + /*
> > > + * The logic below will rely that the pmic is already stored at
> > > + * drvdata.
> > > + */
> > > + dev_dbg(&pdev->dev, "SPMI-PMIC: adding children for %pOF\n",
> > > + pdev->dev.of_node);
> > > + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
> > > + hi6421v600_devs, ARRAY_SIZE(hi6421v600_devs),
> > > + NULL, 0, NULL);
> >
> > This is mixing and matching managed an unmanaged. Should be one or the other
> > or we might be hiding some race conditions.
I intended this as a more localized comment, though the following answers
another question I had.
request_threaded_irq is not using devm_ whilst the add devices is.
As a result we might have a path in which the irq goes away before
the device cleanup happens.
>
> Actually, it is just the opposite. It took me a lot of time to
> figure out a good solution that will prevent all race conditions at
> probe time.
>
> See, the SPMI variant of HiSilicon 6421 requires the drivers to be
> probed on a very specific order:
>
> - The SPMI controller should be loaded first, as it provides
> the low-level I/O access to the serial bus used to talk with the
> PMICs. This bus is somewhat similar to the I2C bus.
>
> Once the controller is registered, the SPMI bus probes the PMIC
> driver.
>
> - Then, the MFD PMIC driver should be loaded. This adds support for
> a high level set of I/O operations, which are used by the regulator
> driver. Again, this approach is similar to the one taken by the
> I2C Kernel drivers.
>
> - Finally, the regulator drivers should come, as they rely on the
> MFD I/O operations in order to talk with the SPMI bus.
>
> The OOT driver probing was based on a some dirty hacks: it had an
> empty SPMI entry at the SoC, carrying on just the "compatible" line.
>
> Then, another entry at DT with the real SPMI settings.
>
> With such dirty hack, on Kernel 4.9, the PMIC driver were always
> loading before the regulator ones, as the SPMI bus code were
> serializing the probe there.
>
> However, such settings were too fragile and broke after porting to
> upstream Kernels, because the regulator drivers were probed on
> a random order, typically before the MFD one (and sometimes even
> before the SPMI controller driver). Adding EPROBE_DEFER didn't
> solve all the issues, and made a complex and hard to debug scenario.
> Also, regulators were probed on a random order, making harder to
> debug issues there.
There are no ordering guarantees IIRC in mfd children coming up even
in the normal path. It might currently happen in a particular order
but relying on that seems fragile to me.
>
> So, I ended using the same solution used by the already-existing
> drivers/mfd/hi6421-pmic-core.c driver[1].
>
> [1] This variant of the 6421 chipset is a lot simpler, as it
> doesn't use the SPMI bus.
>
> With such approach, the probing is warranted to happen the
> way it is expected by the driver:
>
> SPMI controller code starts:
> [ 0.416862] spmi_controller fff24000.spmi: HISI SPMI probe
> [ 0.422419] spmi spmi-0: allocated controller 0x(____ptrval____) id 0
> [ 0.428929] spmi_controller fff24000.spmi: spmi_add_controller base addr=0xffff800012055000!
> [ 0.437480] spmi spmi-0: adding child /spmi@...24000/pmic@0
> [ 0.443109] spmi spmi-0: read usid 00
> [ 0.446821] spmi 2-00: device 2-00 registered
> [ 0.451220] spmi spmi-0: spmi-2 registered: dev:(____ptrval____)
> [ 0.457286] spmi_controller fff24000.spmi: spmi_add_controller initialized
>
> The PMIC probe happens sometime after spmi_controller registers itself
> at the SPMI bus:
>
> [ 1.955838] [hi6421_spmi_pmic_probe]. pmic->irqs[0] = 43
> ...
> [ 2.036298] [hi6421_spmi_pmic_probe]. pmic->irqs[15] = 58
>
> After being ready to handle I/O requests, it starts probing the
> regulators:
>
> [ 2.057815] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo3@16
> [ 2.199827] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo4@17
> [ 2.336284] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo9@1C
> [ 2.472675] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo15@21
> [ 2.609402] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo16@22
> [ 2.746378] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo17@23
> [ 2.846707] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo33@32
> [ 2.988646] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@...24000/pmic@...egulators/ldo34@33
>
> As the current code serializes the regulator probing, it ensured that
> they'll happen at the right order, avoiding race conditions at
> probe time.
Why do we need the regulators to come up in a particular order?
That sounds suspicious as any relationships between different ones should be expressed
either in DT or in the order they are enabled in the drivers using them.
>
> >
> >
> > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> > > index edb1c4f8b496..de8a78487bb9 100644
> > > --- a/drivers/regulator/Kconfig
> > > +++ b/drivers/regulator/Kconfig
> > > @@ -356,6 +356,14 @@ config REGULATOR_HI6421V530
> > > provides 5 general purpose LDOs, and all of them come with support
> > > to either ECO (idle) or sleep mode.
> > >
> > > +config REGULATOR_HI6421V600
> > > + tristate "HiSilicon Hi6421v600 PMIC voltage regulator support"
> > > + depends on MFD_HI6421_PMIC && OF
> >
> > Can we do a COMPILE_TEST here?
>
> Neither REGULATOR_HI6421V600 nor MFD_HI6421_PMIC depends on ARM or
> ARM64, so they should build fine on any arch. So, I'm failing to see
> why adding COMPILE_TEST would make any difference.
cool. Good to get the extra build coverage.
>
> > > +
> > > +#include <linux/slab.h>
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/driver.h>
> > > +#include <linux/regulator/machine.h>
> > > +#include <linux/regulator/of_regulator.h>
> > > +#include <linux/mfd/hi6421-spmi-pmic.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/time.h>
> > > +#include <linux/version.h>
> > > +#include <linux/seq_file.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/spmi.h>
> > > +
> > > +#define rdev_dbg(rdev, fmt, arg...) \
> > > + pr_debug("%s: %s: " fmt, (rdev)->desc->name, __func__, ##arg)
> >
> > Not worth defining in my view.
>
> That was my first approach: to drop any existing printk macro, replacing
> them by dev_dbg(). However, with dev_dbg(), the logs were like:
>
> [ 2.069301] platform ldo3: LDO doesn't support economy mode.
> [ 2.074969] platform ldo3: voltage selector settings: reg: 0x51, mask: 0xf
> [ 2.094593] regulator.3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV
> [ 2.105530] regulator.3: hi6421_spmi_regulator_enable: off_on_delay=20000 us, enable_reg=0x16, enable_mask=0x1
> [ 2.153484] regulator.3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV
> [ 2.163409] ldo3: 1500 <--> 2000 mV at 1800 mV normal
> [ 2.181305] regulator.3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV
> [ 2.191289] regulator.3: hi6421_spmi_regulator_probe_ldo: valid_modes_mask: 0x2, valid_ops_mask: 0x9
>
> E. g. the messages printed by the regulator's core were using
> (rdev)->desc->name on their printks, producing a nice debug
> (when enabled at core level), while dev_dbg() were using a different name.
>
> In this specific case, one might assume that "regulator.3" is "ldo3",
> but this is by coincidence, as it actually matches the sysfs entries:
>
> $ ls -l /sys/class/regulator/
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.0 -> ../../devices/platform/reg-dummy/regulator/regulator.0
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.1 -> ../../devices/platform/regulator-1v8/regulator/regulator.1
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.10 -> ../../devices/platform/spmi-0/2-00/ldo34/regulator/regulator.10
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.2 -> ../../devices/platform/wlan-en-1-8v/regulator/regulator.2
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.3 -> ../../devices/platform/spmi-0/2-00/ldo3/regulator/regulator.3
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.4 -> ../../devices/platform/spmi-0/2-00/ldo4/regulator/regulator.4
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.5 -> ../../devices/platform/spmi-0/2-00/ldo9/regulator/regulator.5
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.6 -> ../../devices/platform/spmi-0/2-00/ldo15/regulator/regulator.6
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.7 -> ../../devices/platform/spmi-0/2-00/ldo16/regulator/regulator.7
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.8 -> ../../devices/platform/spmi-0/2-00/ldo17/regulator/regulator.8
> lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.9 -> ../../devices/platform/spmi-0/2-00/ldo33/regulator/regulator.9
>
> When playing with DT, the "regulator.[\d+]" namespace may change,
> making harder to debug stuff. So, basically, enabling just the logs
> at the regulator driver were requiring to double-check the sysfs
> entries in order to see what "regulator.3" were actually meaning
> with a particular DT setting.
>
> After adding the macro, the output is a lot easier to be understood:
>
> [ 2.069301] platform ldo3: LDO doesn't support economy mode.
> [ 2.074969] platform ldo3: voltage selector settings: reg: 0x51, mask: 0xf
> [ 2.094593] ldo3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV
> [ 2.105530] ldo3: hi6421_spmi_regulator_enable: off_on_delay=20000 us, enable_reg=0x16, enable_mask=0x1
> [ 2.153484] ldo3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV
> [ 2.163409] ldo3: 1500 <--> 2000 mV at 1800 mV normal
> [ 2.181305] ldo3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV
> [ 2.191289] ldo3: hi6421_spmi_regulator_probe_ldo: valid_modes_mask: 0x2, valid_ops_mask: 0x9
>
> As the name which got printed is the one that actually makes sense
> for someone working with the driver, as it matches the DT entries
> and the hardware power supply lines.
Fair enough. Guess this is one for Mark to ultimately decide.
>
> > > +static int hi6421_spmi_regulator_set_voltage_sel(struct regulator_dev *rdev,
> > > + unsigned int selector)
> > > +{
> > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > > + u32 reg_val;
> > > +
> > > + /* unlikely to happen. sanity test done by regulator core */
> >
> > Unlikely or can't?
> >
> > > + if (unlikely(selector >= rdev->desc->n_voltages))
> > > + return -EINVAL;
>
> Good question. I almost removed this check, but I didn't check the
> regulator code with enough care to be 100% sure. So, I opted to keep it
> here.
I'd drop the comment then :) If someone else wants to figure it out
in future then they are welcome to.
>
> > > +
> > > + reg_val = selector << (ffs(rdev->desc->vsel_mask) - 1);
> > > +
> > > + /* set voltage selector */
> > > + rdev_dbg(rdev,
> > > + "vsel_reg=0x%x, mask=0x%x, value=0x%x, voltage=%d mV\n",
> > > + rdev->desc->vsel_reg, rdev->desc->vsel_mask, reg_val,
> > > + rdev->desc->ops->list_voltage(rdev, selector) / 1000);
> > > +
> > > + hi6421_spmi_pmic_rmw(pmic, rdev->desc->vsel_reg,
> > > + rdev->desc->vsel_mask, reg_val);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static unsigned int hi6421_spmi_regulator_get_mode(struct regulator_dev *rdev)
> > > +{
> > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > > + u32 reg_val;
> > > + unsigned int mode;
> > > +
> > > + reg_val = hi6421_spmi_pmic_read(pmic, rdev->desc->enable_reg);
> > > +
> > > + if (reg_val & sreg->eco_mode_mask)
> > > + mode = REGULATOR_MODE_IDLE;
> > > + else
> > > + mode = REGULATOR_MODE_NORMAL;
> > > +
> > > + rdev_dbg(rdev,
> > > + "enable_reg=0x%x, eco_mode_mask=0x%x, reg_val=0x%x, %s mode\n",
> > > + rdev->desc->enable_reg, sreg->eco_mode_mask, reg_val,
> > > + mode == REGULATOR_MODE_IDLE ? "idle" : "normal");
> > > +
> > > + return mode;
> > > +}
> > > +
> > > +static int hi6421_spmi_regulator_set_mode(struct regulator_dev *rdev,
> > > + unsigned int mode)
> > > +{
> > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > > + u32 val;
> > > +
> > > + switch (mode) {
> > > + case REGULATOR_MODE_NORMAL:
> > > + val = 0;
> > > + break;
> > > + case REGULATOR_MODE_IDLE:
> > > + val = sreg->eco_mode_mask << (ffs(sreg->eco_mode_mask) - 1);
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* set mode */
> > > + rdev_dbg(rdev, "enable_reg=0x%x, eco_mode_mask=0x%x, value=0x%x\n",
> > > + rdev->desc->enable_reg, sreg->eco_mode_mask, val);
> > > +
> > > + hi6421_spmi_pmic_rmw(pmic, rdev->desc->enable_reg,
> > > + sreg->eco_mode_mask, val);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static unsigned int
> > > +hi6421_spmi_regulator_get_optimum_mode(struct regulator_dev *rdev,
> > > + int input_uV, int output_uV,
> > > + int load_uA)
> > > +{
> > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > > +
> > > + if (load_uA || ((unsigned int)load_uA > sreg->eco_uA)) {
> > > + rdev_dbg(rdev, "normal mode");
> >
> > Debug seems unnecessary to me, but maybe keep it if you want.
>
> I actually used this debug. There are some LDO lines which don't
> support eco mode. The original driver was hard to understand that.
> So, I ended by re-writing the part of the code which sets/uses it[1]:
>
> + /* hisi regulator supports two modes */
> + constraint = &initdata->constraints;
> +
> + constraint->valid_modes_mask = REGULATOR_MODE_NORMAL;
> + if (sreg->eco_mode_mask) {
> + constraint->valid_modes_mask |= REGULATOR_MODE_IDLE;
> + constraint->valid_ops_mask |= REGULATOR_CHANGE_MODE;
> + }
> +
>
> [1] https://lore.kernel.org/lkml/176043f329dfa9889f014feec04e7e1553077873.1597160086.git.mchehab+huawei@kernel.org/T/#m337e09adf04e4b8ce56af93ba37e3720b2a3002b
>
> Those debug messages are useful to double-check if something bad is
> not happening with the modes/ops masks.
That's fine, but is it useful to have it upstream now you have debugged
those issues? I'm not completely convinced it is and debug prints have
a habit of rotting just like comments.
>
>
> > > + /* register regulator */
> > > + rdev = regulator_register(rdesc, &config);
> > > + if (IS_ERR(rdev)) {
> > > + dev_err(dev, "failed to register %s\n",
> > > + rdesc->name);
> > > + ret = PTR_ERR(rdev);
> > > + goto probe_end;
> > > + }
> > > +
> > > + rdev_dbg(rdev, "valid_modes_mask: 0x%x, valid_ops_mask: 0x%x\n",
> > > + constraint->valid_modes_mask, constraint->valid_ops_mask);
> > > +
> > > + dev_set_drvdata(dev, rdev);
> > I'd do separate error path.
> >
> > return 0;
> >
> > > +probe_end:
> > > + if (ret)
> >
> > ret is set if separate error path.
> > I haven't figured out lifetime but can we not use devm for sreg?
> >
> > > + kfree(sreg);
>
> I guess we can. I'll check and change if ok.
>
> > > +static int hi6421_spmi_regulator_remove(struct platform_device *pdev)
> > > +{
> > > + struct regulator_dev *rdev = dev_get_drvdata(&pdev->dev);
> > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > > +
> > > + regulator_unregister(rdev);
> > > +
> > > + /* TODO: should i worry about that? devm_kzalloc */
> >
> > Answer that TODO. No unless something odd going on.
>
> Yeah, agreed. I'll cleanup useless comments on this driver.
>
> >
> > > + if (rdev->desc->volt_table)
> > > + devm_kfree(&pdev->dev, (unsigned int *)rdev->desc->volt_table);
> > > +
> > > + kfree(sreg);
> >
> > This is a worrying mix of devm and not. Never a good sign.
> > Lifetime of sreg seems strange.
>
> I guess we can just get rid of both, converting sreg to use devm alloc.
>
> Btw, at least on Hikey 970 (which happens to be the only board using
> it so far), in practice this driver should never be removed, as, among
> other things, it controls the power supply for storage (both SD and
> MMC).
Who needs storage? :)
...
>
> > > + ret = spmi_controller_add(ctrl);
> > > + if (ret)
> > > + goto err_add_controller;
> > > +
> > > + dev_info(&pdev->dev, "spmi_add_controller initialized\n");
> >
> > Too noisy.
>
> That's helpful to check if drivers initialized at the right order.
>
> I'll change it to dev_dbg().
More reasonable.
thanks,
Jonathan
Powered by blists - more mailing lists