On Tue, Apr 12, 2016 at 04:16:41PM +0100, Peter Griffin wrote:
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 61bfbb9..10be29b 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -108,6 +108,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
> obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
> obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
> obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
> -
> +obj-$(CONFIG_REGULATOR_ST_FLASHSS) += st-flashss.o
Please keep Kconfig and Makefile sorted. :/
> +static int st_set_voltage_sel(struct regulator_dev *dev, unsigned int selector)
This and the get_voltage_sel() look like you can just use a MMIO regmap.
> + voltage = st_type_voltages[selector];
No bounds checking on the array, though since you're working with
selectors it doesn't seem like there's any need to do this.
> + value |= vsense->en_psw_mask;
Enable should be a separate operation.
> +static void st_get_satinize_powerup_voltage(struct st_vsense *vsense)
> +{
> + void __iomem *ioaddr = vsense->ioaddr;
> + u32 value = readl_relaxed(ioaddr);
> +
> + dev_dbg(vsense->dev, "Initial start-up value: (0x%08x)\n", value);
> +
> + /* Sanitize voltage values forcing what is provided from start-up */
> + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC)
> + value |= TOP_VSENSE_CONFIG_REG_PSW_EMMC;
> + else
> + value &= ~TOP_VSENSE_CONFIG_REG_PSW_EMMC;
> +
> + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_NAND)
> + value |= TOP_VSENSE_CONFIG_REG_PSW_NAND;
> + else
> + value &= ~TOP_VSENSE_CONFIG_REG_PSW_NAND;
> +
> + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_SPI)
> + value |= TOP_VSENSE_CONFIG_REG_PSW_SPI;
> + else
> + value &= ~TOP_VSENSE_CONFIG_REG_PSW_SPI;
This looks like a very complicated way of writing
value &= TOP_VSENSE_CONFIG_LATCHED_PSW_SPI |
TOP_VSENSE_CONFIG_LATCHED_PSW_NAND |
TOP_VSENSE_CONFIG_REG_PSW_EMMC
or am I missing something? Why do we need to do this anyway, it's very
surprsing?
> + if (device->data) {
> + const struct st_vsense *data = device->data;
> +
> + vsense->en_psw_mask = data->en_psw_mask;
> + vsense->psw_mask = data->psw_mask;
> + vsense->latched_mask = data->latched_mask;
> + } else
> + return -ENODEV;
Coding style, { } on both sides.
> +
> + if (of_property_read_string(np, "regulator-name",
> + (const char **)&vsense->name))
> + return -EINVAL;
Don't make this mandatory, that's not what it's for. Use the compatible
if you really need something, or just make the framework cope with a
missing name (eg, by using dev_name() or something).
> + config.init_data = of_get_regulator_init_data(&pdev->dev, np, rdesc);
> + if (!config.init_data) {
> + dev_err(dev, "Failed to parse regulator init data\n");
> + return -ENOMEM;
> + }
Don't error out, carry on and let the driver work in read only mode for
diagnostics.
> + /* register regulator */
> + rdev = regulator_register(rdesc, &config);
devm_regulator_register().
> + dev_info(dev, "%s vsense voltage regulator registered\n", rdesc->name);
Remove this, it's just making the boot noisy.
> +static int __init st_vsense_init(void)
> +{
> + return platform_driver_register(&st_vsense_driver);
> +}
> +
> +subsys_initcall(st_vsense_init);
module_platform_driver().
Attachment:
signature.asc
Description: PGP signature
