On Tue, Nov 13, 2018 at 11:29:41AM +0000, Andrei.Stefanescu@xxxxxxxxxxxxx wrote:
> index 0000000..29c72d3
> --- /dev/null
> +++ b/drivers/regulator/mcp16502.c
> @@ -0,0 +1,524 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MCP16502 PMIC driver
Please make the entire comment a C++ comment so it looks more
intentional.
> +/*
> + * This macro is useful for iterating over all regulators and accessing their
> + * registers in a generic way or accessing a regulator device by its id.
> + */
> +#define BASE(i) (((i) + 1) << 4)
This macro name is likely to run into collisions at some point, a prefix
would be better.
> +static int mcp16502_update_regulator(struct regulator_dev *rdev,
> + bool hibernate,
> + unsigned int mask, unsigned int val)
> +{
> + struct mcp16502 *mcp = rdev_get_drvdata(rdev);
> + unsigned int reg = BASE(rdev_get_id(rdev));
> +
> + reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;
Please write this as a normal if statement to improve legibility.
> +static int mcp16502_read(struct regulator_dev *rdev, bool hibernate,
> + unsigned int mask)
> +{
> + struct mcp16502 *mcp = rdev_get_drvdata(rdev);
> + unsigned int reg = BASE(rdev_get_id(rdev));
> + int ret, val;
> +
> + reg += (hibernate) ? OFFSET_MODE_HIB : OFFSET_MODE_A;
> +
> + ret = regmap_read(mcp->rmap, reg, &val);
> + if (ret)
> + return ret;
> +
> + return val & mask;
> +}
I'm not convinced that these custom read/write functions are adding much
compared to just using the standard regmap helpers for things - they're
adding a bunch of code instead of data. If you need extra helpers for
suspend mode just add those, I'm sure they'd be useful for other
drivers.
> +static unsigned int mcp16502_get_mode(struct regulator_dev *rdev)
> +{
> + int val;
> +
> + val = mcp16502_read(rdev, false, MCP16502_MODE_MASK);
> + if (val < 0)
> + return val;
> +
> + if (val == MCP16502_MODE_FPWM)
> + return REGULATOR_MODE_NORMAL;
> + else if (val == MCP16502_MODE_AUTO_PFM)
> + return REGULATOR_MODE_IDLE;
> +
> + return REGULATOR_MODE_INVALID;
> +}
Please just write a switch statement to improve legibility.
> +/*
> + * mcp16502_is_enabled - regulator_ops is_enabled
> + */
> +static int mcp16502_is_enabled(struct regulator_dev *rdev)
> +{
> + int val;
> +
> + val = mcp16502_read(rdev, false, MCP16502_EN_MASK);
> + if (val < 0)
> + return val;
> +
> + return !!val;
> +}
This can use regulator_is_enabled_regmap().
> + if (mode != REGULATOR_MODE_NORMAL && mode != REGULATOR_MODE_IDLE)
> + return -EINVAL;
> +
> + val = (mode == REGULATOR_MODE_NORMAL) ? MCP16502_MODE_FPWM :
> + MCP16502_MODE_AUTO_PFM;
> +
> + return mcp16502_update_regulator(rdev, hibernate, MCP16502_MODE_MASK,
> + val);
Again a switch statement would be clearer.
> +static int mcp16502_get_status(struct regulator_dev *rdev)
> +{
> + int mode = mcp16502_get_mode(rdev);
> +
> + if (!mcp16502_is_enabled(rdev))
> + return REGULATOR_STATUS_OFF;
> + else if (mode == REGULATOR_MODE_NORMAL)
> + return REGULATOR_STATUS_NORMAL;
> + else if (mode == REGULATOR_MODE_IDLE)
> + return REGULATOR_STATUS_IDLE;
> +
> + return REGULATOR_STATUS_UNDEFINED;
> +}
The _status() function should only be implemented if there's hardware
support for reading back the actual status from the device, we already
know what we set through software.
> +#ifdef CONFIG_PM_SLEEP
> +static int mcp16502_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct mcp16502 *mcp = i2c_get_clientdata(client);
> +
> + mcp16502_gpio_set_mode(mcp, MCP16502_MODE_HIB);
> +
> + return 0;
> +}
This will put the regulators into hibernate mode before the system has
finished suspending which is likely to cause breakage - hibernate mode
in the PMIC is expected to be triggered by the SoC as it shuts down.
> +static int __init mcp16502_init(void)
> +{
> + return i2c_add_driver(&mcp16502_drv);
> +}
module_i2c_driver.
Attachment:
signature.asc
Description: PGP signature
