| Message ID | 20200228212349.GA1929@raspberrypi (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase | expand |
On Fri, Feb 28, 2020 at 03:23:49PM -0600, Grant Peltier wrote: > Add a driver to support 2nd generation Renesas digital multiphase power > regulators. The driver is meant to support a large family of part > numbers spanning isl682xx, isl692xx, and some raa228/9 part designations. > > Signed-off-by: Grant Peltier <grantpeltier93@gmail.com> > --- > drivers/hwmon/pmbus/Kconfig | 9 + > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/isl692xx.c | 352 +++++++++++++++++++++++++++++++++ One of the supported chips should be selected as driver name. There is no guarantee that there will never be any ISL692XX chips with different functionality. Besides, the name is misleading anyway since the driver already supports other chips besides those named isl692xx. Please provide Documentation/hwmon/isl692xx (or whatever the final name is). This needs to briefly explain what each of those chips actually is. There is no public information available for several of the chips listed in the driver. Without datasheets I can only accept support for chips which are by some means confirmed to actually exist. In this context, I have to admit that I _really_ dislike the secrecy around those chips. I have seen several instances where I accepted a driver which turned out to be buggy, simply because I did not have access to a datasheet. In some cases, even if I know that there may be a problem or missing support for something I can't talk about it it because, say, I have seen an internal driver which does things a bit differently and my employer didn't get permission to publish the driver. Yes, I understand that this is becoming more and more common in the industry, but that doesn't make it better and ultimately just hurts everyone. More comments inline. Guenter > 3 files changed, 362 insertions(+) > create mode 100644 drivers/hwmon/pmbus/isl692xx.c > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index a9ea06204767..fbe7bbc8b37c 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -100,6 +100,15 @@ config SENSORS_ISL68137 > This driver can also be built as a module. If so, the module will > be called isl68137. > > +config SENSORS_ISL692XX > + tristate "Renesas 2nd Gen Digital Multiphase" ... Power Regulators > + help > + If you say yes here you get hardware monitoring support for Renesas > + 2nd Generation Digital Multiphase power regulators. > + > + This driver can also be built as a module. If so, the module will > + be called isl692xx. > + > config SENSORS_LM25066 > tristate "National Semiconductor LM25066 and compatibles" > help > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > index 5feb45806123..bf1ea99a6120 100644 > --- a/drivers/hwmon/pmbus/Makefile > +++ b/drivers/hwmon/pmbus/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o > obj-$(CONFIG_SENSORS_ISL68137) += isl68137.o > +obj-$(CONFIG_SENSORS_ISL692XX) += isl692xx.o > obj-$(CONFIG_SENSORS_LM25066) += lm25066.o > obj-$(CONFIG_SENSORS_LTC2978) += ltc2978.o > obj-$(CONFIG_SENSORS_LTC3815) += ltc3815.o > diff --git a/drivers/hwmon/pmbus/isl692xx.c b/drivers/hwmon/pmbus/isl692xx.c > new file mode 100644 > index 000000000000..26f3d90a7ddc > --- /dev/null > +++ b/drivers/hwmon/pmbus/isl692xx.c > @@ -0,0 +1,352 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Hardware monitoring driver for Renesas Gen 2 Digital Multiphase Devices > + * > + * Copyright (c) 2020 Renesas Electronics America > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > + Alphabetic order please > +#include "pmbus.h" > + > +#define ISL692XX_READ_VMON 0xc8 > + > +enum parts { > + isl68220, > + isl68221, > + isl68222, > + isl68223, > + isl68224, > + isl68225, > + isl68226, > + isl68227, > + isl68229, > + isl68233, > + isl68239, > + isl69222, > + isl69223, > + isl69224, > + isl69225, > + isl69227, > + isl69228, > + isl69234, > + isl69236, > + isl69239, > + isl69242, > + isl69243, > + isl69247, > + isl69248, > + isl69254, > + isl69255, > + isl69256, > + isl69259, > + isl69260, > + isl69268, > + isl69269, > + isl69298, > + raa228000, > + raa228004, > + raa228006, > + raa228228, > + raa229001, > + raa229004, > +}; > + > +enum rail_configs { high_voltage, one_rail, two_rail, three_rail }; > + > +static const struct i2c_device_id isl692xx_id[] = { > + { "isl68220", isl68220 }, > + { "isl68221", isl68221 }, > + { "isl68222", isl68222 }, > + { "isl68223", isl68223 }, > + { "isl68224", isl68224 }, > + { "isl68225", isl68225 }, > + { "isl68226", isl68226 }, > + { "isl68227", isl68227 }, > + { "isl68229", isl68229 }, > + { "isl68233", isl68233 }, > + { "isl68239", isl68239 }, > + { "isl69222", isl69222 }, > + { "isl69223", isl69223 }, > + { "isl69224", isl69224 }, > + { "isl69225", isl69225 }, > + { "isl69227", isl69227 }, > + { "isl69228", isl69228 }, > + { "isl69234", isl69234 }, > + { "isl69236", isl69236 }, > + { "isl69239", isl69239 }, > + { "isl69242", isl69242 }, > + { "isl69243", isl69243 }, > + { "isl69247", isl69247 }, > + { "isl69248", isl69248 }, > + { "isl69254", isl69254 }, > + { "isl69255", isl69255 }, > + { "isl69256", isl69256 }, > + { "isl69259", isl69259 }, > + { "isl69260", isl69260 }, > + { "isl69268", isl69268 }, > + { "isl69269", isl69269 }, > + { "isl69298", isl69298 }, > + { "raa228000", raa228000 }, > + { "raa228004", raa228004 }, > + { "raa228006", raa228006 }, > + { "raa228228", raa228228 }, > + { "raa229001", raa229001 }, > + { "raa229004", raa229004 }, It would be much simpler to just specify high_voltage, one_rail, two_rail, and three_rail as parameters. I don't see value in the code translating chip types to enum rail_configs (nor in enum parts). > + { }, > +}; > + > +MODULE_DEVICE_TABLE(i2c, isl692xx_id); > + > +static int isl692xx_read_word_data(struct i2c_client *client, int page, int reg) > +{ > + int ret; > + > + switch (reg) { > + case PMBUS_VIRT_READ_VMON: > + ret = pmbus_read_word_data(client, page, ISL692XX_READ_VMON); > + break; > + default: > + ret = -ENODATA; > + break; > + } > + > + return ret; > +} > + > +static struct pmbus_driver_info isl692xx_info[] = { > + [high_voltage] = { > + .pages = 1, > + .format[PSC_VOLTAGE_IN] = direct, > + .format[PSC_VOLTAGE_OUT] = direct, > + .format[PSC_CURRENT_IN] = direct, > + .format[PSC_CURRENT_OUT] = direct, > + .format[PSC_POWER] = direct, > + .format[PSC_TEMPERATURE] = direct, > + .m[PSC_VOLTAGE_IN] = 1, > + .b[PSC_VOLTAGE_IN] = 0, > + .R[PSC_VOLTAGE_IN] = 1, > + .m[PSC_VOLTAGE_OUT] = 2, > + .b[PSC_VOLTAGE_OUT] = 0, > + .R[PSC_VOLTAGE_OUT] = 2, > + .m[PSC_CURRENT_IN] = 2, > + .b[PSC_CURRENT_IN] = 0, > + .R[PSC_CURRENT_IN] = 2, > + .m[PSC_CURRENT_OUT] = 1, > + .b[PSC_CURRENT_OUT] = 0, > + .R[PSC_CURRENT_OUT] = 1, > + .m[PSC_POWER] = 2, > + .b[PSC_POWER] = 0, > + .R[PSC_POWER] = -1, > + .m[PSC_TEMPERATURE] = 1, > + .b[PSC_TEMPERATURE] = 0, > + .R[PSC_TEMPERATURE] = 0, > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | > + PMBUS_HAVE_VMON, > + .read_word_data = isl692xx_read_word_data, > + }, > + [one_rail] = { > + .pages = 1, > + .format[PSC_VOLTAGE_IN] = direct, > + .format[PSC_VOLTAGE_OUT] = direct, > + .format[PSC_CURRENT_IN] = direct, > + .format[PSC_CURRENT_OUT] = direct, > + .format[PSC_POWER] = direct, > + .format[PSC_TEMPERATURE] = direct, > + .m[PSC_VOLTAGE_IN] = 1, > + .b[PSC_VOLTAGE_IN] = 0, > + .R[PSC_VOLTAGE_IN] = 2, > + .m[PSC_VOLTAGE_OUT] = 1, > + .b[PSC_VOLTAGE_OUT] = 0, > + .R[PSC_VOLTAGE_OUT] = 3, > + .m[PSC_CURRENT_IN] = 1, > + .b[PSC_CURRENT_IN] = 0, > + .R[PSC_CURRENT_IN] = 2, > + .m[PSC_CURRENT_OUT] = 1, > + .b[PSC_CURRENT_OUT] = 0, > + .R[PSC_CURRENT_OUT] = 1, > + .m[PSC_POWER] = 1, > + .b[PSC_POWER] = 0, > + .R[PSC_POWER] = 0, > + .m[PSC_TEMPERATURE] = 1, > + .b[PSC_TEMPERATURE] = 0, > + .R[PSC_TEMPERATURE] = 0, > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | > + PMBUS_HAVE_VMON, > + .read_word_data = isl692xx_read_word_data, > + }, > + [two_rail] = { > + .pages = 2, > + .format[PSC_VOLTAGE_IN] = direct, > + .format[PSC_VOLTAGE_OUT] = direct, > + .format[PSC_CURRENT_IN] = direct, > + .format[PSC_CURRENT_OUT] = direct, > + .format[PSC_POWER] = direct, > + .format[PSC_TEMPERATURE] = direct, > + .m[PSC_VOLTAGE_IN] = 1, > + .b[PSC_VOLTAGE_IN] = 0, > + .R[PSC_VOLTAGE_IN] = 2, > + .m[PSC_VOLTAGE_OUT] = 1, > + .b[PSC_VOLTAGE_OUT] = 0, > + .R[PSC_VOLTAGE_OUT] = 3, > + .m[PSC_CURRENT_IN] = 1, > + .b[PSC_CURRENT_IN] = 0, > + .R[PSC_CURRENT_IN] = 2, > + .m[PSC_CURRENT_OUT] = 1, > + .b[PSC_CURRENT_OUT] = 0, > + .R[PSC_CURRENT_OUT] = 1, > + .m[PSC_POWER] = 1, > + .b[PSC_POWER] = 0, > + .R[PSC_POWER] = 0, > + .m[PSC_TEMPERATURE] = 1, > + .b[PSC_TEMPERATURE] = 0, > + .R[PSC_TEMPERATURE] = 0, > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | > + PMBUS_HAVE_VMON, > + .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | > + PMBUS_HAVE_VMON, > + .read_word_data = isl692xx_read_word_data, > + }, > + [three_rail] = { > + .pages = 3, > + .format[PSC_VOLTAGE_IN] = direct, > + .format[PSC_VOLTAGE_OUT] = direct, > + .format[PSC_CURRENT_IN] = direct, > + .format[PSC_CURRENT_OUT] = direct, > + .format[PSC_POWER] = direct, > + .format[PSC_TEMPERATURE] = direct, > + .m[PSC_VOLTAGE_IN] = 1, > + .b[PSC_VOLTAGE_IN] = 0, > + .R[PSC_VOLTAGE_IN] = 2, > + .m[PSC_VOLTAGE_OUT] = 1, > + .b[PSC_VOLTAGE_OUT] = 0, > + .R[PSC_VOLTAGE_OUT] = 3, > + .m[PSC_CURRENT_IN] = 1, > + .b[PSC_CURRENT_IN] = 0, > + .R[PSC_CURRENT_IN] = 2, > + .m[PSC_CURRENT_OUT] = 1, > + .b[PSC_CURRENT_OUT] = 0, > + .R[PSC_CURRENT_OUT] = 1, > + .m[PSC_POWER] = 1, > + .b[PSC_POWER] = 0, > + .R[PSC_POWER] = 0, > + .m[PSC_TEMPERATURE] = 1, > + .b[PSC_TEMPERATURE] = 0, > + .R[PSC_TEMPERATURE] = 0, > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | > + PMBUS_HAVE_VMON, > + .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | > + PMBUS_HAVE_VMON, > + .func[2] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | > + PMBUS_HAVE_VMON, > + .read_word_data = isl692xx_read_word_data, > + }, > +}; > + > +static int isl692xx_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int ret; > + > + switch (id->driver_data) { > + case raa228000: > + case raa228004: > + case raa228006: > + ret = pmbus_do_probe(client, id, &isl692xx_info[high_voltage]); > + break; > + case isl68227: > + case isl69243: > + ret = pmbus_do_probe(client, id, &isl692xx_info[one_rail]); > + break; > + case isl68220: > + case isl68222: > + case isl68223: > + case isl68225: > + case isl68233: > + case isl69222: > + case isl69224: > + case isl69225: > + case isl69234: > + case isl69236: > + case isl69242: > + case isl69247: > + case isl69248: > + case isl69254: > + case isl69255: > + case isl69256: > + case isl69259: > + case isl69260: > + case isl69268: > + case isl69298: > + case raa228228: > + case raa229001: > + case raa229004: > + ret = pmbus_do_probe(client, id, &isl692xx_info[two_rail]); > + break; > + case isl68221: > + case isl68224: > + case isl68226: > + case isl68229: > + case isl68239: > + case isl69223: > + case isl69227: > + case isl69228: > + case isl69239: > + case isl69269: > + ret = pmbus_do_probe(client, id, &isl692xx_info[three_rail]); > + break; > + default: > + ret = -ENODEV; > + } > + > + return ret; > +} > + > +static struct i2c_driver isl692xx_driver = { > + .driver = { > + .name = "isl692xx", > + }, > + .probe = isl692xx_probe, > + .remove = pmbus_do_remove, > + .id_table = isl692xx_id, > +}; > + > +module_i2c_driver(isl692xx_driver); > + > +MODULE_AUTHOR("Grant Peltier"); > +MODULE_DESCRIPTION("PMBus driver for 2nd Gen Renesas digital multiphase " > + "devices"); > +MODULE_LICENSE("GPL"); > + > -- > 2.20.1 >
Hi Guenter, Thank you for your expedient review. I will need to consult with my coworkers to determine a more appropriate driver name. In the meantime I will make the desired changes and I will also create a document for the driver, which I will submit as a linked but separate patch. With regard to the part numbers, this family of parts is currently in the process of being released and we have not yet published all of the corresponding datasheets. However, I have been assured that all of the parts listed are slated to have a datasheet published publicly in the near future. Thank you, Grant
On 2/28/20 3:52 PM, Grant Peltier wrote: > Hi Guenter, > > Thank you for your expedient review. I will need to consult with my > coworkers to determine a more appropriate driver name. In the meantime I > will make the desired changes and I will also create a document for the > driver, which I will submit as a linked but separate patch. > > With regard to the part numbers, this family of parts is currently in > the process of being released and we have not yet published all of the > corresponding datasheets. However, I have been assured that all of the > parts listed are slated to have a datasheet published publicly in the near > future. > That would be great. As for the driver name, I had a look into drivers/hwmon/pmbus/isl68137.c, and I don't immediately see why the new chips would warrant a new driver. The only differences seem to be that VMON is a new command, and of course only the ISL68137 supports AVL. But then there is, for example, ISL68127, which is again quite similar. The only other difference as far as I can see is input voltage scaling, but that doesn't warrant a separate driver (and, of course, I have no means to validate if input voltage scaling is indeed different for all the new chips). Overall I would suggest to extend the isl68137 driver. I would also suggest to not add separate tables for each of the rail configurations but use the three-phase entry as starting point, copy it, and adjust its values as needed. For the multi-phase chips, I question if reporting the input voltage for each phase make sense. Is it really a different voltage ? For IIN and PIN, the question is if the registers are indeed paged, since they are not paged in the older chips. Guenter
On Fri, Feb 28, 2020 at 05:55:44PM -0800, Guenter Roeck wrote: > On 2/28/20 3:52 PM, Grant Peltier wrote: > > Hi Guenter, > > > > Thank you for your expedient review. I will need to consult with my > > coworkers to determine a more appropriate driver name. In the meantime I > > will make the desired changes and I will also create a document for the > > driver, which I will submit as a linked but separate patch. > > > > With regard to the part numbers, this family of parts is currently in > > the process of being released and we have not yet published all of the > > corresponding datasheets. However, I have been assured that all of the > > parts listed are slated to have a datasheet published publicly in the near > > future. > > > That would be great. > > As for the driver name, I had a look into drivers/hwmon/pmbus/isl68137.c, > and I don't immediately see why the new chips would warrant a new driver. > The only differences seem to be that VMON is a new command, and of course > only the ISL68137 supports AVL. But then there is, for example, ISL68127, > which is again quite similar. The only other difference as far as I can > see is input voltage scaling, but that doesn't warrant a separate driver > (and, of course, I have no means to validate if input voltage scaling > is indeed different for all the new chips). > > Overall I would suggest to extend the isl68137 driver. I would also > suggest to not add separate tables for each of the rail configurations > but use the three-phase entry as starting point, copy it, and adjust its > values as needed. > > For the multi-phase chips, I question if reporting the input voltage > for each phase make sense. Is it really a different voltage ? For IIN > and PIN, the question is if the registers are indeed paged, since they > are not paged in the older chips. > > Guenter The ISL68137 is part of the first generation of our digital multiphase parts which are all exclusively 2-rail (2-page) devices. There are a couple of reasons that we are opting for a new driver for the new generation of devices: 1) Gen 2 has multiple rail configurations (1, 2, or 3) with different scaling parameters than Gen 1 2) We are planning to support some of the non-generic PMBus functions of the Gen 2 devices using the debugfs interface. I am currently working on point 2 and those features are not quite ready to be included in a patch set but we wanted to move forward with the hwmon functionality for now as that is useful on it's own. Fair point on the global vs paged commands. I will modify the page functions so that global commands are only read from page 0. Thank you, Grant
On 2/29/20 7:48 AM, Grant Peltier wrote: > On Fri, Feb 28, 2020 at 05:55:44PM -0800, Guenter Roeck wrote: >> On 2/28/20 3:52 PM, Grant Peltier wrote: >>> Hi Guenter, >>> >>> Thank you for your expedient review. I will need to consult with my >>> coworkers to determine a more appropriate driver name. In the meantime I >>> will make the desired changes and I will also create a document for the >>> driver, which I will submit as a linked but separate patch. >>> >>> With regard to the part numbers, this family of parts is currently in >>> the process of being released and we have not yet published all of the >>> corresponding datasheets. However, I have been assured that all of the >>> parts listed are slated to have a datasheet published publicly in the near >>> future. >>> >> That would be great. >> >> As for the driver name, I had a look into drivers/hwmon/pmbus/isl68137.c, >> and I don't immediately see why the new chips would warrant a new driver. >> The only differences seem to be that VMON is a new command, and of course >> only the ISL68137 supports AVL. But then there is, for example, ISL68127, >> which is again quite similar. The only other difference as far as I can >> see is input voltage scaling, but that doesn't warrant a separate driver >> (and, of course, I have no means to validate if input voltage scaling >> is indeed different for all the new chips). >> >> Overall I would suggest to extend the isl68137 driver. I would also >> suggest to not add separate tables for each of the rail configurations >> but use the three-phase entry as starting point, copy it, and adjust its >> values as needed. >> >> For the multi-phase chips, I question if reporting the input voltage >> for each phase make sense. Is it really a different voltage ? For IIN >> and PIN, the question is if the registers are indeed paged, since they >> are not paged in the older chips. >> >> Guenter > > The ISL68137 is part of the first generation of our digital multiphase > parts which are all exclusively 2-rail (2-page) devices. There are a > couple of reasons that we are opting for a new driver for the new > generation of devices: > > 1) Gen 2 has multiple rail configurations (1, 2, or 3) with different scaling > parameters than Gen 1 That would only mean a single additional entry for the Gen1 devices. This is not a valid argument for a separate driver, especially since the difference in scaling only affects a single parameter as far as I can see. > 2) We are planning to support some of the non-generic PMBus functions of > the Gen 2 devices using the debugfs interface. > This is not a valid argument either. We have, for example, a single driver for all Linear chips, even though they have quite some difference across individual chips. The above can simply be solved by not instantiating debugfs for Gen1 devices, and marking VMON as supported for Gen2 devices only. If the Gen1 devices are all pretty much the same, you'd only need a single additional table entry for those in the driver (if you insist using a table). For the debugfs functions, please keep in mind that those will have to be documented in a way that lets people without access to datasheets understand what they are for. We can't have cryptic debugfs functions which, if misused, blow up the chip. > I am currently working on point 2 and those features are not > quite ready to be included in a patch set but we wanted to move forward > with the hwmon functionality for now as that is useful on it's own. > > Fair point on the global vs paged commands. I will modify the page > functions so that global commands are only read from page 0. > Also a fair point showing that not having the datasheet hurts. I see the same problem with recent TI devices. In some cases I _know_ that a driver is buggy or much less than perfect, because I have access to a datasheet through my employer, but I can't talk about it because what I know is under NDA. Then I end up spending a lot of time trying to find leaks that let me comment. I am _not_ happy about this situation, not at all. Please understand that I won't accept a driver adding support for a chip if there is no public information available that the chip exists in the first place. Imagine a situation where you are requesting to add support for a chip, and that chip isn't mentioned anywhere, not even in a datasheet I (hypothetically) may have access to through my employer. How would _you_ handle such a situation if you were in my place ? You could of course send me all the datasheets under NDA to my work e-mail, but that wouldn't really be much better since I still would not be able to comment in public. On the other side, I'd have at least some confirmation that those magic chips do indeed exist, so you might possibly want to consider that. Thanks, Guenter
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index a9ea06204767..fbe7bbc8b37c 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -100,6 +100,15 @@ config SENSORS_ISL68137 This driver can also be built as a module. If so, the module will be called isl68137. +config SENSORS_ISL692XX + tristate "Renesas 2nd Gen Digital Multiphase" + help + If you say yes here you get hardware monitoring support for Renesas + 2nd Generation Digital Multiphase power regulators. + + This driver can also be built as a module. If so, the module will + be called isl692xx. + config SENSORS_LM25066 tristate "National Semiconductor LM25066 and compatibles" help diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 5feb45806123..bf1ea99a6120 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_IR35221) += ir35221.o obj-$(CONFIG_SENSORS_IR38064) += ir38064.o obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o obj-$(CONFIG_SENSORS_ISL68137) += isl68137.o +obj-$(CONFIG_SENSORS_ISL692XX) += isl692xx.o obj-$(CONFIG_SENSORS_LM25066) += lm25066.o obj-$(CONFIG_SENSORS_LTC2978) += ltc2978.o obj-$(CONFIG_SENSORS_LTC3815) += ltc3815.o diff --git a/drivers/hwmon/pmbus/isl692xx.c b/drivers/hwmon/pmbus/isl692xx.c new file mode 100644 index 000000000000..26f3d90a7ddc --- /dev/null +++ b/drivers/hwmon/pmbus/isl692xx.c @@ -0,0 +1,352 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Hardware monitoring driver for Renesas Gen 2 Digital Multiphase Devices + * + * Copyright (c) 2020 Renesas Electronics America + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/i2c.h> + +#include "pmbus.h" + +#define ISL692XX_READ_VMON 0xc8 + +enum parts { + isl68220, + isl68221, + isl68222, + isl68223, + isl68224, + isl68225, + isl68226, + isl68227, + isl68229, + isl68233, + isl68239, + isl69222, + isl69223, + isl69224, + isl69225, + isl69227, + isl69228, + isl69234, + isl69236, + isl69239, + isl69242, + isl69243, + isl69247, + isl69248, + isl69254, + isl69255, + isl69256, + isl69259, + isl69260, + isl69268, + isl69269, + isl69298, + raa228000, + raa228004, + raa228006, + raa228228, + raa229001, + raa229004, +}; + +enum rail_configs { high_voltage, one_rail, two_rail, three_rail }; + +static const struct i2c_device_id isl692xx_id[] = { + { "isl68220", isl68220 }, + { "isl68221", isl68221 }, + { "isl68222", isl68222 }, + { "isl68223", isl68223 }, + { "isl68224", isl68224 }, + { "isl68225", isl68225 }, + { "isl68226", isl68226 }, + { "isl68227", isl68227 }, + { "isl68229", isl68229 }, + { "isl68233", isl68233 }, + { "isl68239", isl68239 }, + { "isl69222", isl69222 }, + { "isl69223", isl69223 }, + { "isl69224", isl69224 }, + { "isl69225", isl69225 }, + { "isl69227", isl69227 }, + { "isl69228", isl69228 }, + { "isl69234", isl69234 }, + { "isl69236", isl69236 }, + { "isl69239", isl69239 }, + { "isl69242", isl69242 }, + { "isl69243", isl69243 }, + { "isl69247", isl69247 }, + { "isl69248", isl69248 }, + { "isl69254", isl69254 }, + { "isl69255", isl69255 }, + { "isl69256", isl69256 }, + { "isl69259", isl69259 }, + { "isl69260", isl69260 }, + { "isl69268", isl69268 }, + { "isl69269", isl69269 }, + { "isl69298", isl69298 }, + { "raa228000", raa228000 }, + { "raa228004", raa228004 }, + { "raa228006", raa228006 }, + { "raa228228", raa228228 }, + { "raa229001", raa229001 }, + { "raa229004", raa229004 }, + { }, +}; + +MODULE_DEVICE_TABLE(i2c, isl692xx_id); + +static int isl692xx_read_word_data(struct i2c_client *client, int page, int reg) +{ + int ret; + + switch (reg) { + case PMBUS_VIRT_READ_VMON: + ret = pmbus_read_word_data(client, page, ISL692XX_READ_VMON); + break; + default: + ret = -ENODATA; + break; + } + + return ret; +} + +static struct pmbus_driver_info isl692xx_info[] = { + [high_voltage] = { + .pages = 1, + .format[PSC_VOLTAGE_IN] = direct, + .format[PSC_VOLTAGE_OUT] = direct, + .format[PSC_CURRENT_IN] = direct, + .format[PSC_CURRENT_OUT] = direct, + .format[PSC_POWER] = direct, + .format[PSC_TEMPERATURE] = direct, + .m[PSC_VOLTAGE_IN] = 1, + .b[PSC_VOLTAGE_IN] = 0, + .R[PSC_VOLTAGE_IN] = 1, + .m[PSC_VOLTAGE_OUT] = 2, + .b[PSC_VOLTAGE_OUT] = 0, + .R[PSC_VOLTAGE_OUT] = 2, + .m[PSC_CURRENT_IN] = 2, + .b[PSC_CURRENT_IN] = 0, + .R[PSC_CURRENT_IN] = 2, + .m[PSC_CURRENT_OUT] = 1, + .b[PSC_CURRENT_OUT] = 0, + .R[PSC_CURRENT_OUT] = 1, + .m[PSC_POWER] = 2, + .b[PSC_POWER] = 0, + .R[PSC_POWER] = -1, + .m[PSC_TEMPERATURE] = 1, + .b[PSC_TEMPERATURE] = 0, + .R[PSC_TEMPERATURE] = 0, + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | + PMBUS_HAVE_VMON, + .read_word_data = isl692xx_read_word_data, + }, + [one_rail] = { + .pages = 1, + .format[PSC_VOLTAGE_IN] = direct, + .format[PSC_VOLTAGE_OUT] = direct, + .format[PSC_CURRENT_IN] = direct, + .format[PSC_CURRENT_OUT] = direct, + .format[PSC_POWER] = direct, + .format[PSC_TEMPERATURE] = direct, + .m[PSC_VOLTAGE_IN] = 1, + .b[PSC_VOLTAGE_IN] = 0, + .R[PSC_VOLTAGE_IN] = 2, + .m[PSC_VOLTAGE_OUT] = 1, + .b[PSC_VOLTAGE_OUT] = 0, + .R[PSC_VOLTAGE_OUT] = 3, + .m[PSC_CURRENT_IN] = 1, + .b[PSC_CURRENT_IN] = 0, + .R[PSC_CURRENT_IN] = 2, + .m[PSC_CURRENT_OUT] = 1, + .b[PSC_CURRENT_OUT] = 0, + .R[PSC_CURRENT_OUT] = 1, + .m[PSC_POWER] = 1, + .b[PSC_POWER] = 0, + .R[PSC_POWER] = 0, + .m[PSC_TEMPERATURE] = 1, + .b[PSC_TEMPERATURE] = 0, + .R[PSC_TEMPERATURE] = 0, + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | + PMBUS_HAVE_VMON, + .read_word_data = isl692xx_read_word_data, + }, + [two_rail] = { + .pages = 2, + .format[PSC_VOLTAGE_IN] = direct, + .format[PSC_VOLTAGE_OUT] = direct, + .format[PSC_CURRENT_IN] = direct, + .format[PSC_CURRENT_OUT] = direct, + .format[PSC_POWER] = direct, + .format[PSC_TEMPERATURE] = direct, + .m[PSC_VOLTAGE_IN] = 1, + .b[PSC_VOLTAGE_IN] = 0, + .R[PSC_VOLTAGE_IN] = 2, + .m[PSC_VOLTAGE_OUT] = 1, + .b[PSC_VOLTAGE_OUT] = 0, + .R[PSC_VOLTAGE_OUT] = 3, + .m[PSC_CURRENT_IN] = 1, + .b[PSC_CURRENT_IN] = 0, + .R[PSC_CURRENT_IN] = 2, + .m[PSC_CURRENT_OUT] = 1, + .b[PSC_CURRENT_OUT] = 0, + .R[PSC_CURRENT_OUT] = 1, + .m[PSC_POWER] = 1, + .b[PSC_POWER] = 0, + .R[PSC_POWER] = 0, + .m[PSC_TEMPERATURE] = 1, + .b[PSC_TEMPERATURE] = 0, + .R[PSC_TEMPERATURE] = 0, + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | + PMBUS_HAVE_VMON, + .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | + PMBUS_HAVE_VMON, + .read_word_data = isl692xx_read_word_data, + }, + [three_rail] = { + .pages = 3, + .format[PSC_VOLTAGE_IN] = direct, + .format[PSC_VOLTAGE_OUT] = direct, + .format[PSC_CURRENT_IN] = direct, + .format[PSC_CURRENT_OUT] = direct, + .format[PSC_POWER] = direct, + .format[PSC_TEMPERATURE] = direct, + .m[PSC_VOLTAGE_IN] = 1, + .b[PSC_VOLTAGE_IN] = 0, + .R[PSC_VOLTAGE_IN] = 2, + .m[PSC_VOLTAGE_OUT] = 1, + .b[PSC_VOLTAGE_OUT] = 0, + .R[PSC_VOLTAGE_OUT] = 3, + .m[PSC_CURRENT_IN] = 1, + .b[PSC_CURRENT_IN] = 0, + .R[PSC_CURRENT_IN] = 2, + .m[PSC_CURRENT_OUT] = 1, + .b[PSC_CURRENT_OUT] = 0, + .R[PSC_CURRENT_OUT] = 1, + .m[PSC_POWER] = 1, + .b[PSC_POWER] = 0, + .R[PSC_POWER] = 0, + .m[PSC_TEMPERATURE] = 1, + .b[PSC_TEMPERATURE] = 0, + .R[PSC_TEMPERATURE] = 0, + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | + PMBUS_HAVE_VMON, + .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | + PMBUS_HAVE_VMON, + .func[2] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN | + PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP | + PMBUS_HAVE_VMON, + .read_word_data = isl692xx_read_word_data, + }, +}; + +static int isl692xx_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int ret; + + switch (id->driver_data) { + case raa228000: + case raa228004: + case raa228006: + ret = pmbus_do_probe(client, id, &isl692xx_info[high_voltage]); + break; + case isl68227: + case isl69243: + ret = pmbus_do_probe(client, id, &isl692xx_info[one_rail]); + break; + case isl68220: + case isl68222: + case isl68223: + case isl68225: + case isl68233: + case isl69222: + case isl69224: + case isl69225: + case isl69234: + case isl69236: + case isl69242: + case isl69247: + case isl69248: + case isl69254: + case isl69255: + case isl69256: + case isl69259: + case isl69260: + case isl69268: + case isl69298: + case raa228228: + case raa229001: + case raa229004: + ret = pmbus_do_probe(client, id, &isl692xx_info[two_rail]); + break; + case isl68221: + case isl68224: + case isl68226: + case isl68229: + case isl68239: + case isl69223: + case isl69227: + case isl69228: + case isl69239: + case isl69269: + ret = pmbus_do_probe(client, id, &isl692xx_info[three_rail]); + break; + default: + ret = -ENODEV; + } + + return ret; +} + +static struct i2c_driver isl692xx_driver = { + .driver = { + .name = "isl692xx", + }, + .probe = isl692xx_probe, + .remove = pmbus_do_remove, + .id_table = isl692xx_id, +}; + +module_i2c_driver(isl692xx_driver); + +MODULE_AUTHOR("Grant Peltier"); +MODULE_DESCRIPTION("PMBus driver for 2nd Gen Renesas digital multiphase " + "devices"); +MODULE_LICENSE("GPL"); +
Add a driver to support 2nd generation Renesas digital multiphase power regulators. The driver is meant to support a large family of part numbers spanning isl682xx, isl692xx, and some raa228/9 part designations. Signed-off-by: Grant Peltier <grantpeltier93@gmail.com> --- drivers/hwmon/pmbus/Kconfig | 9 + drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/isl692xx.c | 352 +++++++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+) create mode 100644 drivers/hwmon/pmbus/isl692xx.c