On Wed, 24 Sep 2014, Guenter Roeck wrote:
Hi Guenter,
> On Wed, Sep 24, 2014 at 12:57:56PM -0500, atull@xxxxxxxxxxxxxxxxxxxxx wrote:
> > From: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
> >
> > Add simple on/off regulator support for ltc2978 and
> > other pmbus parts supported by ltc2978.c
> >
> > Signed-off-by: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
> >
> > 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/Kconfig | 7 ++++++
> > drivers/hwmon/pmbus/ltc2978.c | 51 +++++++++++++++++++++++++++++++++++++++++
>
> This will also require devicetree documentation describing the device nodes.
Yes, I'll add that as a separate patch to v4. It will be a new file since
there currently isn't any pmbus or ltc2978 bindings documentation that I
could find.
>
> > 2 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 6e1e493..79117b7 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -56,6 +56,13 @@ config SENSORS_LTC2978
> > This driver can also be built as a module. If so, the module will
> > be called ltc2978.
> >
> > +config SENSORS_LTC2978_REGULATOR
> > + boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> > + depends on SENSORS_LTC2978 && REGULATOR
> > + help
> > + If you say yes here you get regulator support for Linear
> > + Technology LTC2974, LTC2978, LTC3880, and LTC3883.
> > +
> > config SENSORS_MAX16064
> > tristate "Maxim MAX16064"
> > default n
> > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > index e24ed52..7d4dcd7 100644
> > --- a/drivers/hwmon/pmbus/ltc2978.c
> > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > @@ -22,6 +22,8 @@
> > #include <linux/err.h>
> > #include <linux/slab.h>
> > #include <linux/i2c.h>
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/of_regulator.h>
> > #include "pmbus.h"
> >
> > enum chips { ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676 };
> > @@ -374,6 +376,30 @@ static const struct i2c_device_id ltc2978_id[] = {
> > };
> > MODULE_DEVICE_TABLE(i2c, ltc2978_id);
> >
> > +#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> > +static const struct regulator_desc ltc2978_reg_desc[] = {
> > + PMBUS_REGULATOR("vout_en", 0),
> > + PMBUS_REGULATOR("vout_en", 1),
> > + PMBUS_REGULATOR("vout_en", 2),
> > + PMBUS_REGULATOR("vout_en", 3),
> > + PMBUS_REGULATOR("vout_en", 4),
> > + PMBUS_REGULATOR("vout_en", 5),
> > + PMBUS_REGULATOR("vout_en", 6),
> > + PMBUS_REGULATOR("vout_en", 7),
>
> How about just vout[0-7] ? I don't see a value in "_en".
That's cool. I'll do it.
>
> > +};
> > +
> > +static struct of_regulator_match ltc2978_reg_matches[] = {
> > + { .name = "vout_en0" },
> > + { .name = "vout_en1" },
> > + { .name = "vout_en2" },
> > + { .name = "vout_en3" },
> > + { .name = "vout_en4" },
> > + { .name = "vout_en5" },
> > + { .name = "vout_en6" },
> > + { .name = "vout_en7" },
>
> If there are multiple LTC chips in the system, this will result in duplicate
> regulator names. Does that matter ? Any ideas how other regulators handle this ?
>
> Example on my test system:
>
> root@localhost:/sys/class/regulator# grep vout_en0 */name
> regulator.15/name:vout_en0
> regulator.2/name:vout_en0
> regulator.23/name:vout_en0
> regulator.31/name:vout_en0
> regulator.39/name:vout_en0
> regulator.47/name:vout_en0
These are just default names, but I think I could make the name better.
How about <part #>-<i2c address>-vout<#> such as "ltc2978-5c-vout0"
If the board has regulator_init_data, then these default names get overwritten.
In my case, I'm just using 3 supplies so those 3 get overwritten:
root@socfpga_cyclone5:/sys/class/regulator# cat */name
regulator-dummy
FPGA-2.5V
vout_en1
FPGA-1.5V
vout_en3
FPGA-1.1V
vout_en5
vout_en6
vout_en7
>
> > +};
> > +#endif /* CONFIG_REGULATOR */
>
> Nitpick, but
>
> CONFIG_SENSORS_LTC2978_REGULATOR
I'll change it.
> > +
> > static int ltc2978_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -487,6 +513,31 @@ static int ltc2978_probe(struct i2c_client *client,
> > default:
> > return -ENODEV;
> > }
> > +
> > +#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> > + info->reg_desc = ltc2978_reg_desc;
> > + info->reg_matches = ltc2978_reg_matches;
> > +
> > + switch (data->id) {
> > + case ltc2974:
> > + info->num_regulators = LTC2974_NUM_PAGES;
> > + break;
> > + case ltc2977:
> > + case ltc2978:
> > + info->num_regulators = LTC2978_NUM_PAGES;
> > + break;
> > + case ltc3880:
> > + case ltm4676:
> > + info->num_regulators = LTC3880_NUM_PAGES;
> > + break;
> > + case ltc3883:
> > + info->num_regulators = LTC3883_NUM_PAGES;
> > + break;
> > + default:
> > + return -ENODEV;
> > + }
> > + BUG_ON(info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc));
>
> How about an error message and reducing info->num_regulators to
> ARRAY_SIZE(ltc2978_reg_desc) if that happens ? I am not really a friend
> of BUG_ON() as it seems a bit drastic. Sure, one can argue that the programmer
> doesn't deserve better, but the idea behind BUG_ON is that the kernel can not
> continue to operate, and that is not really the case here.
That sounds right to me. I'll do that.
>
> Also, please drop the ifdef here, and merge the initialization into
> the first switch statement. The few saved bytes of code are not really
> worth it. You can use defines for ltc2978_reg_desc and ltc2978_reg_matches
> and initialize with NULL if CONFIG_SENSORS_LTC2978_REGULATOR is not defined.
OK, that will be cleaner.
>
> Thanks,
> Guenter
>
Thanks for the feedback,
Alan
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
