Re: [PATCH v3 2/3] pmbus: add regulator support

From: Guenter Roeck
Date: Wed Sep 24 2014 - 15:59:02 EST

Next message: Rafael J. Wysocki: "Re: [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper"
Previous message: Russell King - ARM Linux: "Re: [PATCH v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option"
In reply to: atull: "Re: [PATCH v3 2/3] pmbus: add regulator support"
Next in thread: atull: "Re: [PATCH v3 2/3] pmbus: add regulator support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Sep 24, 2014 at 12:57:55PM -0500, atull@xxxxxxxxxxxxxxxxxxxxx wrote:
> From: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
>
> Add support for simple on/off control of each channel.
>
> To add regulator support, the pmbus part driver needs to add
> regulator_desc information, of_regulator_match information,
> and number of regulators to its pmbus_driver_info struct.
>
> regulator_desc can be declared using default macro for a
> regulator (PMBUS_REGULATOR) that is in pmbus.h
>
> The regulator_init_data can be intialized from either
> platform data or the device tree.
>
> Signed-off-by: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
>

Hi Alan,

Overall looks pretty good. Couple of comments inline.

> v2: Remove '#include <linux/regulator/machine.h>'
> Only one regulator per pmbus device
> Get regulator_init_data from pdata or device tree
>
> v3: Support multiple regulators for each chip
> Move most code to pmbus_core.c
> fixed values for on/off
> ---
> drivers/hwmon/pmbus/pmbus.h | 27 ++++++++
> drivers/hwmon/pmbus/pmbus_core.c | 133 ++++++++++++++++++++++++++++++++++++++
> include/linux/i2c/pmbus.h | 4 ++
> 3 files changed, 164 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index fa9beb3..74aa382 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -19,6 +19,9 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
> +
> #ifndef PMBUS_H
> #define PMBUS_H
>
> @@ -186,6 +189,12 @@
> #define PMBUS_VIRT_STATUS_VMON (PMBUS_VIRT_BASE + 35)
>
> /*
> + * OPERATION
> + */
> +#define PB_OPERATION_CONTROL_ON (1<<7)
> +#define PB_OPERATION_CONTROL_SEQ_OFF (1<<6)

Can those defines be more consistent ? Does it really need SEQ_OFF or can it
just be OFF ?

> +
> +/*
> * CAPABILITY
> */
> #define PB_CAPABILITY_SMBALERT (1<<4)
> @@ -365,8 +374,26 @@ struct pmbus_driver_info {
> */
> int (*identify)(struct i2c_client *client,
> struct pmbus_driver_info *info);
> +
> + /* Regulator functionality, if supported by this chip driver. */
> + int num_regulators;
> + const struct regulator_desc *reg_desc;
> + struct of_regulator_match *reg_matches;
> };
>
> +/* Regulator ops */
> +
> +extern struct regulator_ops pmbus_regulator_regulator_ops;
> +
How about just pmbus_regulator_ops ? I don't see a double regulator_
variable name anywhere else in the code, and I don't really see the
benefit of it.

> +/* Macro for filling in array of struct regulator_desc */
> +#define PMBUS_REGULATOR(_name, _id) \
> + [_id] = { \
> + .name = (_name # _id), \
> + .id = (_id), \
> + .ops = &pmbus_regulator_regulator_ops, \
> + .owner = THIS_MODULE, \
> + }
> +

Any idea how/if we can get rid of the resulting checkpatch error ?

> /* Function declarations */
>
> void pmbus_clear_cache(struct i2c_client *client);
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index d6c3701..9ab8bd4 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -29,6 +29,9 @@
> #include <linux/hwmon-sysfs.h>
> #include <linux/jiffies.h>
> #include <linux/i2c/pmbus.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> #include "pmbus.h"
>
> /*
> @@ -1758,6 +1761,125 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_REGULATOR)
> +static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + struct device *dev = rdev_get_dev(rdev);
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + u8 page = rdev_get_id(rdev);
> + int ret;
> +
> + ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> + if (ret < 0)
> + return ret;
> +
> + return !!(ret & PB_OPERATION_CONTROL_ON);
> +}
> +
> +static int _pmbus_regulator_enable(struct regulator_dev *rdev, bool enable)
> +{

Can you find a better name for this function ? After all,
it doesn't just enable the regulator, it also disables it.

> + struct device *dev = rdev_get_dev(rdev);
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + u8 val, page = rdev_get_id(rdev);
> +
> + if (enable)
> + val = PB_OPERATION_CONTROL_ON;
> + else
> + val = 0;
> +
> + return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> + PB_OPERATION_CONTROL_ON, val);

enable ? PB_OPERATION_CONTROL_ON : 0

would be much simpler here.

> +}
> +
> +static int pmbus_regulator_enable(struct regulator_dev *rdev)
> +{
> + return _pmbus_regulator_enable(rdev, 1);
> +}
> +
> +static int pmbus_regulator_disable(struct regulator_dev *rdev)
> +{
> + return _pmbus_regulator_enable(rdev, 0);
> +}
> +
> +struct regulator_ops pmbus_regulator_regulator_ops = {
> + .enable = pmbus_regulator_enable,
> + .disable = pmbus_regulator_disable,
> + .is_enabled = pmbus_regulator_is_enabled,

No get_voltage support ?

[ Guess it isn't mandatory. We can add it later to get this going. ]

> +};
> +EXPORT_SYMBOL_GPL(pmbus_regulator_regulator_ops);
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static int pmbus_regulator_parse_dt(struct device *dev,
> + const struct pmbus_driver_info *info)
> +{
> + struct device_node *np_regulators;
> + int ret;
> +
> + if (!info->num_regulators)
> + return 0;
> +
> + if (!info->reg_matches || !info->reg_desc)
> + return -EINVAL;
> +
> + np_regulators = of_get_child_by_name(dev->of_node, "regulators");
> + if (!np_regulators)
> + return 0;
> +
> + ret = of_regulator_match(dev, np_regulators, info->reg_matches,
> + info->num_regulators);
> + of_node_put(np_regulators);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +#else
> +static int pmbus_regulator_parse_dt(struct device *dev,
> + const struct pmbus_driver_info *info)
> +{
> + return 0;
> +}
> +#endif
> +
> +static int pmbus_regulator_register(struct pmbus_data *data)
> +{
> + struct device *dev = data->dev;
> + const struct pmbus_driver_info *info = data->info;
> + const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
> + struct regulator_dev *rdev;
> + int i;
> +
> + for (i = 0; i < info->num_regulators; i++) {
> + struct regulator_config config = { };
> +
> + config.dev = dev;
> + config.driver_data = data;
> +
> + if (pdata && pdata->reg_init_data) {
> + config.init_data = &pdata->reg_init_data[i];
> + } else {
> + config.init_data = info->reg_matches[i].init_data;
> + config.of_node = info->reg_matches[i].of_node;
> + }
> +
> + rdev = devm_regulator_register(dev, &info->reg_desc[i],
> + &config);
> + if (IS_ERR(rdev)) {
> + dev_err(dev, "Failed to register %s regulator\n",
> + info->reg_desc[i].name);
> + return PTR_ERR(rdev);
> + }
> + }
> +
> + return 0;
> +}
> +#else
> +static int pmbus_regulator_register(struct pmbus_data *data)
> +{
> + return 0;
> +}
> +#endif
> +
> int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> struct pmbus_driver_info *info)
> {
> @@ -1769,6 +1891,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> if (!info)
> return -ENODEV;
>
> + ret = pmbus_regulator_parse_dt(dev, info);
> + if (ret)
> + return ret;
> +

You have the conditions wrong above.

If CONFIG_REGULATOR is not enabled, this will fail to build,
since pmbus_regulator_parse_dt is not declared at all in this case.

I can understand that you want to parse the dt early, but it would be
simpler to just parse it from pmbus_regulator_register(). It is only
relevant if regulators are configured anyway, and we don't really need
to optimize the code for the error case.

> if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE
> | I2C_FUNC_SMBUS_BYTE_DATA
> | I2C_FUNC_SMBUS_WORD_DATA))
> @@ -1812,8 +1938,15 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> dev_err(dev, "Failed to register hwmon device\n");
> goto out_kfree;
> }
> +
> + ret = pmbus_regulator_register(data);
> + if (ret)
> + goto out_unregister;
> +
> return 0;
>
> +out_unregister:
> + hwmon_device_unregister(data->hwmon_dev);
> out_kfree:
> kfree(data->group.attrs);
> return ret;
> diff --git a/include/linux/i2c/pmbus.h b/include/linux/i2c/pmbus.h
> index 69280db..ee3c2ab 100644
> --- a/include/linux/i2c/pmbus.h
> +++ b/include/linux/i2c/pmbus.h
> @@ -40,6 +40,10 @@
>
> struct pmbus_platform_data {
> u32 flags; /* Device specific flags */
> +
> + /* regulator support */
> + int num_regulators;
> + struct regulator_init_data *reg_init_data;
> };
>
> #endif /* _PMBUS_H_ */
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Rafael J. Wysocki: "Re: [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper"
Previous message: Russell King - ARM Linux: "Re: [PATCH v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option"
In reply to: atull: "Re: [PATCH v3 2/3] pmbus: add regulator support"
Next in thread: atull: "Re: [PATCH v3 2/3] pmbus: add regulator support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]