| Message ID | 20221201044643.1150870-4-saravanan@linumiz.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for mpq7932 PMIC IC | expand |
On 01/12/2022 05:46, Saravanan Sekar wrote: > The MPQ7932 is a power management IC designed to operate from 5V buses to > power a variety of Advanced driver-assistance system SOCs. Six integrated > buck converters with hardware monitoring capability powers a variety of > target rails configurable over PMBus interface. > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> > --- > drivers/hwmon/pmbus/Kconfig | 10 +++ > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++ > 3 files changed, 155 insertions(+) > create mode 100644 drivers/hwmon/pmbus/mpq7932.c This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. Best regards, Krzysztof
On 01/12/22 11:26, Krzysztof Kozlowski wrote: > On 01/12/2022 05:46, Saravanan Sekar wrote: >> The MPQ7932 is a power management IC designed to operate from 5V buses to >> power a variety of Advanced driver-assistance system SOCs. Six integrated >> buck converters with hardware monitoring capability powers a variety of >> target rails configurable over PMBus interface. >> >> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >> --- >> drivers/hwmon/pmbus/Kconfig | 10 +++ >> drivers/hwmon/pmbus/Makefile | 1 + >> drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 155 insertions(+) >> create mode 100644 drivers/hwmon/pmbus/mpq7932.c > > This is a friendly reminder during the review process. > > It seems my previous comments were not fully addressed. Maybe my > feedback got lost between the quotes, maybe you just forgot to apply it. > Please go back to the previous discussion and either implement all > requested changes or keep discussing them. > Thank you again for your time for review. I saw two comments from you on V1 which I believe addressed on V2 1. Missing maybe_unused, so drop of_match_ptr. ".of_match_table = of_match_ptr(mpq7932_of_match)" dropped of_match_ptr. 2. It's a regulator, not hwmon. "config SENSORS_MPQ7932_REGULATOR tristate "MPS MPQ7932 buck regulator" " It is PMIC chip with hwmon support access over PMBUS. Please help if anything I missed > Thank you. > > Best regards, > Krzysztof >
On 11/30/22 20:46, Saravanan Sekar wrote: > The MPQ7932 is a power management IC designed to operate from 5V buses to > power a variety of Advanced driver-assistance system SOCs. Six integrated > buck converters with hardware monitoring capability powers a variety of > target rails configurable over PMBus interface. > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> > --- > drivers/hwmon/pmbus/Kconfig | 10 +++ > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++ > 3 files changed, 155 insertions(+) > create mode 100644 drivers/hwmon/pmbus/mpq7932.c > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index 89668af67206..4a1538949a73 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -317,6 +317,16 @@ config SENSORS_MP5023 > This driver can also be built as a module. If so, the module will > be called mp5023. > > +config SENSORS_MPQ7932 > + tristate "MPS MPQ7932" As written, a dependency on REGULATOR is missing. However, we want the driver enabled even if CONFIG_REGULATOR is not enabled. I would suggest to follow the approach used by other drivers: add a second configuration option SENSORS_MPQ7932_REGULATOR which depends on SENSORS_MPQ7932 and REGULATOR and enables regulator functionality, and use that in the driver to make regulator support optional. > + help > + If you say yes here you get six integrated buck converter regulator > + with hardware monitoring functionality support for power management > + IC MPS MPQ7932. That description is more appropriate for the second configuration option. Primarily one gets hardware monitoring support for the chip. > + > + This driver can also be built as a module. If so, the module will > + be called mpq7932. > + > config SENSORS_PIM4328 > tristate "Flex PIM4328 and compatibles" > help > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > index 0002dbe22d52..28a534629cc3 100644 > --- a/drivers/hwmon/pmbus/Makefile > +++ b/drivers/hwmon/pmbus/Makefile > @@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_MAX8688) += max8688.o > obj-$(CONFIG_SENSORS_MP2888) += mp2888.o > obj-$(CONFIG_SENSORS_MP2975) += mp2975.o > obj-$(CONFIG_SENSORS_MP5023) += mp5023.o > +obj-$(CONFIG_SENSORS_MPQ7932_REGULATOR) += mpq7932.o > obj-$(CONFIG_SENSORS_PLI1209BC) += pli1209bc.o > obj-$(CONFIG_SENSORS_PM6764TR) += pm6764tr.o > obj-$(CONFIG_SENSORS_PXE1610) += pxe1610.o > diff --git a/drivers/hwmon/pmbus/mpq7932.c b/drivers/hwmon/pmbus/mpq7932.c > new file mode 100644 > index 000000000000..3747d7862afd > --- /dev/null > +++ b/drivers/hwmon/pmbus/mpq7932.c > @@ -0,0 +1,144 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0+ The SPDX license must be in the first line and be a C++ style comment. Please run checkpatch --strict and fix what it reports (including the various continuation line misalignments and unnecessary empty lines). > + * > + * mpq7932.c - regulator driver for mps mpq7932 > + * Copyright 2022 Monolithic Power Systems, Inc This is a hwmon driver with optional regulator functionality. > + * > + * Author: Saravanan Sekar <saravanan@linumiz.com> > + */ > + > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/pmbus.h> > +#include "pmbus.h" > + > +#define MPQ7932_BUCK_UV_MIN 206250 > +#define MPQ7932_UV_STEP 6250 > +#define MPQ7932_N_VOLTAGES 0xFF > +#define MPQ7932_NUM_PAGES 6 > + > +#define MPQ7932_TON_DELAY 0x60 > +#define MPQ7932_VOUT_STARTUP_SLEW 0xA3 > +#define MPQ7932_VOUT_SHUTDOWN_SLEW 0xA5 > +#define MPQ7932_VOUT_SLEW_MASK GENMASK(1, 0) > +#define MPQ7932_TON_DELAY_MASK GENMASK(4, 0) Please include the appropriate include file defining GENMASK. > + > +struct mpq7932_data { > + struct pmbus_driver_info info; > + struct pmbus_platform_data pdata; > +}; > + > +static struct regulator_desc mpq7932_regulators_desc[] = { > + PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES, > + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), > + PMBUS_REGULATOR_STEP("buck", 1, MPQ7932_N_VOLTAGES, > + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), > + PMBUS_REGULATOR_STEP("buck", 2, MPQ7932_N_VOLTAGES, > + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), > + PMBUS_REGULATOR_STEP("buck", 3, MPQ7932_N_VOLTAGES, > + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), > + PMBUS_REGULATOR_STEP("buck", 4, MPQ7932_N_VOLTAGES, > + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), > + PMBUS_REGULATOR_STEP("buck", 5, MPQ7932_N_VOLTAGES, > + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), > +}; > + > +static int mpq7932_write_word_data(struct i2c_client *client, int page, int reg, > + u16 word) > +{ > + > + switch (reg) { > + case PMBUS_VOUT_COMMAND: This needs a comment explaining why it is needed. > + return pmbus_write_byte_data(client, page, reg, (u8)word); word should be clamped to [0, 255], not cut off. > + > + default: > + return -ENODATA; > + } > +} > + > +static int mpq7932_read_word_data(struct i2c_client *client, int page, > + int phase, int reg) > +{ > + > + switch (reg) { > + case PMBUS_MFR_VOUT_MIN: > + return 0; > + > + case PMBUS_MFR_VOUT_MAX: > + return MPQ7932_N_VOLTAGES; The above need comments. Also, MPQ7932_N_VOLTAGES is inappropriate. This is not the number of voltages, it is the maximum voltage. Even if the values happen to be the same, the content is different. Also, with PMBUS_MFR_VOUT_MIN=0 and PMBUS_MFR_VOUT_MAX=0xff, the number of voltages would actually be 256, not 255. > + > + case PMBUS_READ_VOUT: > + return pmbus_read_byte_data(client, page, PMBUS_VOUT_COMMAND); > + Needs same comment as above. > + default: > + return -ENODATA; > + } > +} > + > +static int mpq7932_probe(struct i2c_client *client) > +{ > + struct mpq7932_data *data; > + struct pmbus_driver_info *info; > + struct device *dev = &client->dev; > + int i; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_READ_WORD_DATA)) Unnecessary check. This code doesn't use it, and pmbus_do_probe() does its own check. > + return -ENODEV; > + > + data = devm_kzalloc(&client->dev, sizeof(struct mpq7932_data), Use dev. > + GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + info = &data->info; > + info->pages = MPQ7932_NUM_PAGES; > + info->num_regulators = ARRAY_SIZE(mpq7932_regulators_desc); > + info->reg_desc = mpq7932_regulators_desc; > + info->format[PSC_VOLTAGE_OUT] = direct; > + info->m[PSC_VOLTAGE_OUT] = 160; > + info->b[PSC_VOLTAGE_OUT] = -33; > + for (i = 0; i < info->pages; i++) { > + info->func[i] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT > + | PMBUS_HAVE_STATUS_TEMP; I think I already asked: Is this really all telemetry supported by the chip ? I keep asking because that would be highly unusual. > + } > + > + info->read_word_data = mpq7932_read_word_data; > + info->write_word_data = mpq7932_write_word_data; > + > + data->pdata.flags = PMBUS_NO_CAPABILITY; > + dev->platform_data = &data->pdata; > + > + return pmbus_do_probe(client, info); > +} > + > +static const struct of_device_id mpq7932_of_match[] = { > + { .compatible = "mps,mpq7932"}, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, mpq7932_of_match); > + > +static const struct i2c_device_id mpq7932_id[] = { > + { "mpq7932", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, mpq7932_id); > + > +static struct i2c_driver mpq7932_regulator_driver = { > + .driver = { > + .name = "mpq7932", > + .of_match_table = mpq7932_of_match, > + }, > + .probe_new = mpq7932_probe, > + .id_table = mpq7932_id, > +}; > +module_i2c_driver(mpq7932_regulator_driver); > + > +MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>"); > +MODULE_DESCRIPTION("MPQ7932 PMIC regulator driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(PMBUS);
On 01/12/22 16:34, Guenter Roeck wrote: > On 11/30/22 20:46, Saravanan Sekar wrote: >> The MPQ7932 is a power management IC designed to operate from 5V buses to >> power a variety of Advanced driver-assistance system SOCs. Six integrated >> buck converters with hardware monitoring capability powers a variety of >> target rails configurable over PMBus interface. >> >> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >> --- >> drivers/hwmon/pmbus/Kconfig | 10 +++ >> drivers/hwmon/pmbus/Makefile | 1 + >> drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 155 insertions(+) >> create mode 100644 drivers/hwmon/pmbus/mpq7932.c >> >> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >> index 89668af67206..4a1538949a73 100644 >> --- a/drivers/hwmon/pmbus/Kconfig >> +++ b/drivers/hwmon/pmbus/Kconfig >> @@ -317,6 +317,16 @@ config SENSORS_MP5023 >> This driver can also be built as a module. If so, the module will >> be called mp5023. >> +config SENSORS_MPQ7932 >> + tristate "MPS MPQ7932" > > As written, a dependency on REGULATOR is missing. However, we want the > driver > enabled even if CONFIG_REGULATOR is not enabled. I would suggest to > follow the > approach used by other drivers: add a second configuration option > SENSORS_MPQ7932_REGULATOR which depends on SENSORS_MPQ7932 and REGULATOR > and enables regulator functionality, and use that in the driver to > make regulator support optional. > Hello Guenter, I need clarification or confirmation from you before V3. Here is my view, communication to MPQ7932 PMIC chip is based on pmbus specification. Now the conflict is that we have pmbus directory under hwmon subsystem, if pmbus spec implementation would have been separate like i2c-smbus then we can implement chip driver in regulator subsystem which access pmbus. pmbus_core does supports regulator framework PMBUS_REGUALTOR and I believe it is valid MPQ7932 driver implantation under pmbus directory. Kindly suggest to remove pmbus dependency on hwmon and proceed further. >> + help >> + If you say yes here you get six integrated buck converter >> regulator >> + with hardware monitoring functionality support for power >> management >> + IC MPS MPQ7932. > > That description is more appropriate for the second configuration option. > Primarily one gets hardware monitoring support for the chip. > >> + >> + This driver can also be built as a module. If so, the module will >> + be called mpq7932. >> + >> config SENSORS_PIM4328 >> tristate "Flex PIM4328 and compatibles" >> help >> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >> index 0002dbe22d52..28a534629cc3 100644 >> --- a/drivers/hwmon/pmbus/Makefile >> +++ b/drivers/hwmon/pmbus/Makefile >> @@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_MAX8688) += max8688.o >> obj-$(CONFIG_SENSORS_MP2888) += mp2888.o >> obj-$(CONFIG_SENSORS_MP2975) += mp2975.o >> obj-$(CONFIG_SENSORS_MP5023) += mp5023.o >> +obj-$(CONFIG_SENSORS_MPQ7932_REGULATOR) += mpq7932.o >> obj-$(CONFIG_SENSORS_PLI1209BC) += pli1209bc.o >> obj-$(CONFIG_SENSORS_PM6764TR) += pm6764tr.o >> obj-$(CONFIG_SENSORS_PXE1610) += pxe1610.o >> diff --git a/drivers/hwmon/pmbus/mpq7932.c >> b/drivers/hwmon/pmbus/mpq7932.c >> new file mode 100644 >> index 000000000000..3747d7862afd >> --- /dev/null >> +++ b/drivers/hwmon/pmbus/mpq7932.c >> @@ -0,0 +1,144 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0+ > > The SPDX license must be in the first line and be a C++ style comment. > Please run checkpatch --strict and fix what it reports (including the > various continuation line misalignments and unnecessary empty lines). > >> + * >> + * mpq7932.c - regulator driver for mps mpq7932 >> + * Copyright 2022 Monolithic Power Systems, Inc > > This is a hwmon driver with optional regulator functionality. > >> + * >> + * Author: Saravanan Sekar <saravanan@linumiz.com> >> + */ >> + >> +#include <linux/err.h> >> +#include <linux/i2c.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/pmbus.h> >> +#include "pmbus.h" >> + >> +#define MPQ7932_BUCK_UV_MIN 206250 >> +#define MPQ7932_UV_STEP 6250 >> +#define MPQ7932_N_VOLTAGES 0xFF >> +#define MPQ7932_NUM_PAGES 6 >> + >> +#define MPQ7932_TON_DELAY 0x60 >> +#define MPQ7932_VOUT_STARTUP_SLEW 0xA3 >> +#define MPQ7932_VOUT_SHUTDOWN_SLEW 0xA5 >> +#define MPQ7932_VOUT_SLEW_MASK GENMASK(1, 0) >> +#define MPQ7932_TON_DELAY_MASK GENMASK(4, 0) > > Please include the appropriate include file defining GENMASK. > >> + >> +struct mpq7932_data { >> + struct pmbus_driver_info info; >> + struct pmbus_platform_data pdata; >> +}; >> + >> +static struct regulator_desc mpq7932_regulators_desc[] = { >> + PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES, >> + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), >> + PMBUS_REGULATOR_STEP("buck", 1, MPQ7932_N_VOLTAGES, >> + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), >> + PMBUS_REGULATOR_STEP("buck", 2, MPQ7932_N_VOLTAGES, >> + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), >> + PMBUS_REGULATOR_STEP("buck", 3, MPQ7932_N_VOLTAGES, >> + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), >> + PMBUS_REGULATOR_STEP("buck", 4, MPQ7932_N_VOLTAGES, >> + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), >> + PMBUS_REGULATOR_STEP("buck", 5, MPQ7932_N_VOLTAGES, >> + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), >> +}; >> + >> +static int mpq7932_write_word_data(struct i2c_client *client, int >> page, int reg, >> + u16 word) >> +{ >> + >> + switch (reg) { >> + case PMBUS_VOUT_COMMAND: > > This needs a comment explaining why it is needed. > >> + return pmbus_write_byte_data(client, page, reg, (u8)word); > > word should be clamped to [0, 255], not cut off. > >> + >> + default: >> + return -ENODATA; >> + } >> +} >> + >> +static int mpq7932_read_word_data(struct i2c_client *client, int page, >> + int phase, int reg) >> +{ >> + >> + switch (reg) { >> + case PMBUS_MFR_VOUT_MIN: >> + return 0; >> + >> + case PMBUS_MFR_VOUT_MAX: >> + return MPQ7932_N_VOLTAGES; > > The above need comments. Also, MPQ7932_N_VOLTAGES is inappropriate. This > is not the > number of voltages, it is the maximum voltage. Even if the values happen > to be the > same, the content is different. > > Also, with PMBUS_MFR_VOUT_MIN=0 and PMBUS_MFR_VOUT_MAX=0xff, the number > of voltages > would actually be 256, not 255. > >> + >> + case PMBUS_READ_VOUT: >> + return pmbus_read_byte_data(client, page, PMBUS_VOUT_COMMAND); >> + > Needs same comment as above. > >> + default: >> + return -ENODATA; >> + } >> +} >> + >> +static int mpq7932_probe(struct i2c_client *client) >> +{ >> + struct mpq7932_data *data; >> + struct pmbus_driver_info *info; >> + struct device *dev = &client->dev; >> + int i; >> + >> + if (!i2c_check_functionality(client->adapter, >> + I2C_FUNC_SMBUS_READ_WORD_DATA)) > > Unnecessary check. This code doesn't use it, and pmbus_do_probe() > does its own check. > >> + return -ENODEV; >> + >> + data = devm_kzalloc(&client->dev, sizeof(struct mpq7932_data), > > Use dev. > >> + GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + info = &data->info; >> + info->pages = MPQ7932_NUM_PAGES; >> + info->num_regulators = ARRAY_SIZE(mpq7932_regulators_desc); >> + info->reg_desc = mpq7932_regulators_desc; >> + info->format[PSC_VOLTAGE_OUT] = direct; >> + info->m[PSC_VOLTAGE_OUT] = 160; >> + info->b[PSC_VOLTAGE_OUT] = -33; >> + for (i = 0; i < info->pages; i++) { >> + info->func[i] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT >> + | PMBUS_HAVE_STATUS_TEMP; > > I think I already asked: Is this really all telemetry supported by the > chip ? > I keep asking because that would be highly unusual. > Yes, only the above. Thanks, Saravanan >> + } >> + >> + info->read_word_data = mpq7932_read_word_data; >> + info->write_word_data = mpq7932_write_word_data; >> + >> + data->pdata.flags = PMBUS_NO_CAPABILITY; >> + dev->platform_data = &data->pdata; >> + >> + return pmbus_do_probe(client, info); >> +} >> + >> +static const struct of_device_id mpq7932_of_match[] = { >> + { .compatible = "mps,mpq7932"}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, mpq7932_of_match); >> + >> +static const struct i2c_device_id mpq7932_id[] = { >> + { "mpq7932", }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(i2c, mpq7932_id); >> + >> +static struct i2c_driver mpq7932_regulator_driver = { >> + .driver = { >> + .name = "mpq7932", >> + .of_match_table = mpq7932_of_match, >> + }, >> + .probe_new = mpq7932_probe, >> + .id_table = mpq7932_id, >> +}; >> +module_i2c_driver(mpq7932_regulator_driver); >> + >> +MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>"); >> +MODULE_DESCRIPTION("MPQ7932 PMIC regulator driver"); >> +MODULE_LICENSE("GPL"); >> +MODULE_IMPORT_NS(PMBUS); >
On 12/6/22 19:51, Saravanan Sekar wrote: > On 01/12/22 16:34, Guenter Roeck wrote: >> On 11/30/22 20:46, Saravanan Sekar wrote: >>> The MPQ7932 is a power management IC designed to operate from 5V buses to >>> power a variety of Advanced driver-assistance system SOCs. Six integrated >>> buck converters with hardware monitoring capability powers a variety of >>> target rails configurable over PMBus interface. >>> >>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >>> --- >>> drivers/hwmon/pmbus/Kconfig | 10 +++ >>> drivers/hwmon/pmbus/Makefile | 1 + >>> drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 155 insertions(+) >>> create mode 100644 drivers/hwmon/pmbus/mpq7932.c >>> >>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>> index 89668af67206..4a1538949a73 100644 >>> --- a/drivers/hwmon/pmbus/Kconfig >>> +++ b/drivers/hwmon/pmbus/Kconfig >>> @@ -317,6 +317,16 @@ config SENSORS_MP5023 >>> This driver can also be built as a module. If so, the module will >>> be called mp5023. >>> +config SENSORS_MPQ7932 >>> + tristate "MPS MPQ7932" >> >> As written, a dependency on REGULATOR is missing. However, we want the driver >> enabled even if CONFIG_REGULATOR is not enabled. I would suggest to follow the >> approach used by other drivers: add a second configuration option >> SENSORS_MPQ7932_REGULATOR which depends on SENSORS_MPQ7932 and REGULATOR >> and enables regulator functionality, and use that in the driver to >> make regulator support optional. >> > > Hello Guenter, > > I need clarification or confirmation from you before V3. > > Here is my view, communication to MPQ7932 PMIC chip is based on pmbus specification. > > Now the conflict is that we have pmbus directory under hwmon subsystem, if pmbus spec implementation would have been separate like i2c-smbus then we can implement chip driver in regulator subsystem which access pmbus. > > pmbus_core does supports regulator framework PMBUS_REGUALTOR and I believe it is valid MPQ7932 driver implantation under pmbus directory. > > Kindly suggest to remove pmbus dependency on hwmon and proceed further. > Every chip supporting PMBus has hwmon functionality. The PMBus core as implemented primarily supports hardware monitoring. Some can act as regulators; that is why regulator support was added to the PMBus core. Trying to extract it would make no sense. If you want to implement a driver without hardware monitoring support for this chip, you won't need the PMBus core. I would not agree with such an approach, but there is nothing that prevents you from implementing a regulator-only driver for this chip in the regulator subsystem. Guenter
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 89668af67206..4a1538949a73 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -317,6 +317,16 @@ config SENSORS_MP5023 This driver can also be built as a module. If so, the module will be called mp5023. +config SENSORS_MPQ7932 + tristate "MPS MPQ7932" + help + If you say yes here you get six integrated buck converter regulator + with hardware monitoring functionality support for power management + IC MPS MPQ7932. + + This driver can also be built as a module. If so, the module will + be called mpq7932. + config SENSORS_PIM4328 tristate "Flex PIM4328 and compatibles" help diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 0002dbe22d52..28a534629cc3 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_MAX8688) += max8688.o obj-$(CONFIG_SENSORS_MP2888) += mp2888.o obj-$(CONFIG_SENSORS_MP2975) += mp2975.o obj-$(CONFIG_SENSORS_MP5023) += mp5023.o +obj-$(CONFIG_SENSORS_MPQ7932_REGULATOR) += mpq7932.o obj-$(CONFIG_SENSORS_PLI1209BC) += pli1209bc.o obj-$(CONFIG_SENSORS_PM6764TR) += pm6764tr.o obj-$(CONFIG_SENSORS_PXE1610) += pxe1610.o diff --git a/drivers/hwmon/pmbus/mpq7932.c b/drivers/hwmon/pmbus/mpq7932.c new file mode 100644 index 000000000000..3747d7862afd --- /dev/null +++ b/drivers/hwmon/pmbus/mpq7932.c @@ -0,0 +1,144 @@ +/* + * SPDX-License-Identifier: GPL-2.0+ + * + * mpq7932.c - regulator driver for mps mpq7932 + * Copyright 2022 Monolithic Power Systems, Inc + * + * Author: Saravanan Sekar <saravanan@linumiz.com> + */ + +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pmbus.h> +#include "pmbus.h" + +#define MPQ7932_BUCK_UV_MIN 206250 +#define MPQ7932_UV_STEP 6250 +#define MPQ7932_N_VOLTAGES 0xFF +#define MPQ7932_NUM_PAGES 6 + +#define MPQ7932_TON_DELAY 0x60 +#define MPQ7932_VOUT_STARTUP_SLEW 0xA3 +#define MPQ7932_VOUT_SHUTDOWN_SLEW 0xA5 +#define MPQ7932_VOUT_SLEW_MASK GENMASK(1, 0) +#define MPQ7932_TON_DELAY_MASK GENMASK(4, 0) + +struct mpq7932_data { + struct pmbus_driver_info info; + struct pmbus_platform_data pdata; +}; + +static struct regulator_desc mpq7932_regulators_desc[] = { + PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES, + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), + PMBUS_REGULATOR_STEP("buck", 1, MPQ7932_N_VOLTAGES, + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), + PMBUS_REGULATOR_STEP("buck", 2, MPQ7932_N_VOLTAGES, + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), + PMBUS_REGULATOR_STEP("buck", 3, MPQ7932_N_VOLTAGES, + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), + PMBUS_REGULATOR_STEP("buck", 4, MPQ7932_N_VOLTAGES, + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), + PMBUS_REGULATOR_STEP("buck", 5, MPQ7932_N_VOLTAGES, + MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN), +}; + +static int mpq7932_write_word_data(struct i2c_client *client, int page, int reg, + u16 word) +{ + + switch (reg) { + case PMBUS_VOUT_COMMAND: + return pmbus_write_byte_data(client, page, reg, (u8)word); + + default: + return -ENODATA; + } +} + +static int mpq7932_read_word_data(struct i2c_client *client, int page, + int phase, int reg) +{ + + switch (reg) { + case PMBUS_MFR_VOUT_MIN: + return 0; + + case PMBUS_MFR_VOUT_MAX: + return MPQ7932_N_VOLTAGES; + + case PMBUS_READ_VOUT: + return pmbus_read_byte_data(client, page, PMBUS_VOUT_COMMAND); + + default: + return -ENODATA; + } +} + +static int mpq7932_probe(struct i2c_client *client) +{ + struct mpq7932_data *data; + struct pmbus_driver_info *info; + struct device *dev = &client->dev; + int i; + + if (!i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_READ_WORD_DATA)) + return -ENODEV; + + data = devm_kzalloc(&client->dev, sizeof(struct mpq7932_data), + GFP_KERNEL); + if (!data) + return -ENOMEM; + + info = &data->info; + info->pages = MPQ7932_NUM_PAGES; + info->num_regulators = ARRAY_SIZE(mpq7932_regulators_desc); + info->reg_desc = mpq7932_regulators_desc; + info->format[PSC_VOLTAGE_OUT] = direct; + info->m[PSC_VOLTAGE_OUT] = 160; + info->b[PSC_VOLTAGE_OUT] = -33; + for (i = 0; i < info->pages; i++) { + info->func[i] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT + | PMBUS_HAVE_STATUS_TEMP; + } + + info->read_word_data = mpq7932_read_word_data; + info->write_word_data = mpq7932_write_word_data; + + data->pdata.flags = PMBUS_NO_CAPABILITY; + dev->platform_data = &data->pdata; + + return pmbus_do_probe(client, info); +} + +static const struct of_device_id mpq7932_of_match[] = { + { .compatible = "mps,mpq7932"}, + {}, +}; +MODULE_DEVICE_TABLE(of, mpq7932_of_match); + +static const struct i2c_device_id mpq7932_id[] = { + { "mpq7932", }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, mpq7932_id); + +static struct i2c_driver mpq7932_regulator_driver = { + .driver = { + .name = "mpq7932", + .of_match_table = mpq7932_of_match, + }, + .probe_new = mpq7932_probe, + .id_table = mpq7932_id, +}; +module_i2c_driver(mpq7932_regulator_driver); + +MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>"); +MODULE_DESCRIPTION("MPQ7932 PMIC regulator driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(PMBUS);
The MPQ7932 is a power management IC designed to operate from 5V buses to power a variety of Advanced driver-assistance system SOCs. Six integrated buck converters with hardware monitoring capability powers a variety of target rails configurable over PMBus interface. Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> --- drivers/hwmon/pmbus/Kconfig | 10 +++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/mpq7932.c | 144 ++++++++++++++++++++++++++++++++++ 3 files changed, 155 insertions(+) create mode 100644 drivers/hwmon/pmbus/mpq7932.c