Re: [PATCH v1 0/1] hwmon: (pmbus) Add Infineon IR38164, Renesas RAA228006, and TI SN1701022 driver
From: Guenter Roeck
Date: Thu Dec 26 2019 - 12:10:56 EST
On 12/26/19 12:47 AM, Andrew Peng wrote:
Add the pmbus driver for Infineon IR38164 Voltage Regulator driver.
Add the pmbus driver for Renesas RAA228006 Digital PWM Controller.
Add the pmbus driver for TI SN1701022 driver.
Signed-off-by: Andrew Peng <pengms1@xxxxxxxxxx>
Signed-off-by: Derek Lin <dlin23@xxxxxxxxxx>
Signed-off-by: Yonghui Liu <liuyh21@xxxxxxxxxx>
Please don't add three drivers with one patch. One patch per driver, please.
Also, don't label the patch "0/1" if there is just one patch.
More comments inline.
---
v1: initial version
drivers/hwmon/pmbus/Kconfig | 25 ++++++++
drivers/hwmon/pmbus/Makefile | 3 +
drivers/hwmon/pmbus/ir38164.c | 72 +++++++++++++++++++++++
drivers/hwmon/pmbus/raa228006.c | 127 ++++++++++++++++++++++++++++++++++++++++
drivers/hwmon/pmbus/sn1701022.c | 80 +++++++++++++++++++++++++
Documentation missing for each of those.
5 files changed, 307 insertions(+)
create mode 100644 drivers/hwmon/pmbus/ir38164.c
create mode 100644 drivers/hwmon/pmbus/raa228006.c
create mode 100644 drivers/hwmon/pmbus/sn1701022.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b658848..d51e67d 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -64,6 +64,15 @@ config SENSORS_IR38064
This driver can also be built as a module. If so, the module will
be called ir38064.
+config SENSORS_IR38164
+ tristate "Infineon IR38164"
+ help
+ If you say yes here you get hardware monitoring support for Infineon
+ IR38164.
+
Please more information.
+ This driver can also be built as a module. If so, the module will
+ be called ir38164.
+
config SENSORS_IRPS5401
tristate "Infineon IRPS5401"
help
@@ -221,4 +230,20 @@ config SENSORS_ZL6100
This driver can also be built as a module. If so, the module will
be called zl6100.
+config SENSORS_RAA228006
+ tristate "RAA228006 and compatibles"
+ help
+ If you say yes here you get hardware monitoring support for RAA228006.
+
Please more information. What is that ?
+ This driver can also be built as a module. If so, the module will
+ be called RAA228006.
+
+config SENSORS_SN1701022
+ tristate "SN1701022"
+ help
+ If you say yes here you get hardware monitoring support for SN1701022.
+
Please more information.
+ This driver can also be built as a module. If so, the module will
+ be called sn1701022.
+
endif # PMBUS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index c950ea9..21d259f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
+obj-$(CONFIG_SENSORS_IR38164) += ir38164.o
obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
obj-$(CONFIG_SENSORS_ISL68137) += isl68137.o
obj-$(CONFIG_SENSORS_LM25066) += lm25066.o
@@ -25,3 +26,5 @@ obj-$(CONFIG_SENSORS_TPS53679) += tps53679.o
obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o
obj-$(CONFIG_SENSORS_UCD9200) += ucd9200.o
obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o
+obj-$(CONFIG_SENSORS_RAA228006) += raa228006.o
+obj-$(CONFIG_SENSORS_SN1701022) += sn1701022.o
diff --git a/drivers/hwmon/pmbus/ir38164.c b/drivers/hwmon/pmbus/ir38164.c
new file mode 100644
index 0000000..db89473
--- /dev/null
+++ b/drivers/hwmon/pmbus/ir38164.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for Infineon ir38164
+ *
No one but you knows what that is. Please provide a little more information.
Same for other drivers.
+ * Copyright (C) 2019-present Lenovo
+ *
Is "-present" legal ? I see you and Facebook are using it, but for me
it doesn't really make sense. If you want to use it, please provide
a link describing the legal rationale and implications.
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
Please no such boilerplate. This is what SPDX is for.
Same comments for the other drivers.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+static struct pmbus_driver_info ir38164_info = {
+ .pages = 1,
+ .format[PSC_VOLTAGE_IN] = linear,
+ .format[PSC_VOLTAGE_OUT] = linear,
+ .format[PSC_CURRENT_OUT] = linear,
+ .format[PSC_POWER] = linear,
+ .format[PSC_TEMPERATURE] = linear,
+
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
+ | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
+ | PMBUS_HAVE_POUT,
+};
+
+static int ir38164_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ return pmbus_do_probe(client, id, &ir38164_info);
+}
+
+static const struct i2c_device_id ir38164_id[] = {
+ {"ir38164", 0},
+ {}
+};
+
I don't immediately see why this driver is necessary. It should
be sufficient to add ir38164 to pmbus.c. If that doesn't work,
please explain.
+MODULE_DEVICE_TABLE(i2c, ir38164_id);
+
+/* This is the driver that will be inserted */
This is a pretty pointless comment. I would suggest to remove it.
Yes, I know, you'll find it throughout the kernel. That doesn't make it better.
+static struct i2c_driver ir38164_driver = {
+ .driver = {
+ .name = "ir38164",
+ },
+ .probe = ir38164_probe,
+ .remove = pmbus_do_remove,
+ .id_table = ir38164_id,
+};
+
+module_i2c_driver(ir38164_driver);
+
+MODULE_AUTHOR("Yonghui Liu <liuyh21@xxxxxxxxxx>");
+MODULE_DESCRIPTION("PMBus driver for Infineon ir38164");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/pmbus/raa228006.c b/drivers/hwmon/pmbus/raa228006.c
new file mode 100644
index 0000000..9aed481
--- /dev/null
+++ b/drivers/hwmon/pmbus/raa228006.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for raa228006
+ *
Here the missing information is really bad. There is almost nothing
on the web describing what "raa228006" actually is. I am not inclined
to accept this patch without further information.
+ * Copyright (C) 2019-present Lenovo
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+static int raa228006_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct pmbus_driver_info *info;
+ u8 buf[I2C_SMBUS_BLOCK_MAX];
+ int ret;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_BYTE_DATA
+ | I2C_FUNC_SMBUS_READ_WORD_DATA
+ | I2C_FUNC_SMBUS_READ_BLOCK_DATA))
+ return -ENODEV;
+
+ /* By default this device doesn't boot to page 0, so set page 0
+ * to access all pmbus registers.
+ */
Is that true or is it cut-and-paste ?
+
+ i2c_smbus_write_byte_data(client, 0, 0);
+
+ /* Read Manufacturer id */
+ ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to read PMBUS_MFR_ID\n");
+ return ret;
+ }
+
What is the point of reading the manufacturer ID and not doing anything
with it ?
+ info = devm_kzalloc(&client->dev, sizeof(struct pmbus_driver_info),
+ GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ info->format[PSC_VOLTAGE_IN] = direct;
+ info->format[PSC_VOLTAGE_OUT] = direct;
+ info->format[PSC_CURRENT_IN] = direct;
+ info->format[PSC_CURRENT_OUT] = direct;
+ info->format[PSC_POWER] = direct;
+ info->format[PSC_TEMPERATURE] = direct;
+ info->vrm_version = vr13;
+
+ info->m[PSC_VOLTAGE_IN] = 1,
+ info->b[PSC_VOLTAGE_IN] = 0,
+ info->R[PSC_VOLTAGE_IN] = 3,
+
+ info->m[PSC_VOLTAGE_OUT] = 1,
+ info->b[PSC_VOLTAGE_OUT] = 0,
+ info->R[PSC_VOLTAGE_OUT] = 0,
+
+ info->m[PSC_CURRENT_IN] = 1,
+ info->b[PSC_CURRENT_IN] = 0,
+ info->R[PSC_CURRENT_IN] = 1,
+
+ info->m[PSC_CURRENT_OUT] = 1,
+ info->b[PSC_CURRENT_OUT] = 0,
+ info->R[PSC_CURRENT_OUT] = 2,
+
+ info->m[PSC_POWER] = 1,
+ info->b[PSC_POWER] = 0,
+ info->R[PSC_POWER] = 3,
+
+ info->m[PSC_TEMPERATURE] = 1,
+ info->b[PSC_TEMPERATURE] = 0,
+ info->R[PSC_TEMPERATURE] = 3,
+
+ info->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;
+
+ info->pages = id->driver_data;
+
There is nothing else but a single page. Why that complexity and not
just use astatic allocation ? More cut-and-paste ?
+ return pmbus_do_probe(client, id, info);
+}
+
+static const struct i2c_device_id raa228006_id[] = {
+ {"raa228006", 1},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, raa228006_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver raa228006_driver = {
+ .driver = {
+ .name = "raa228006",
+ },
+ .probe = raa228006_probe,
+ .remove = pmbus_do_remove,
+ .id_table = raa228006_id,
+};
+
+module_i2c_driver(raa228006_driver);
+
+MODULE_AUTHOR("Yonghui Liu <liuyh21@xxxxxxxxxx>");
+MODULE_DESCRIPTION("PMBus driver for raa228006");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/pmbus/sn1701022.c b/drivers/hwmon/pmbus/sn1701022.c
new file mode 100644
index 0000000..0ba45ac
--- /dev/null
+++ b/drivers/hwmon/pmbus/sn1701022.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for sn1701022
+ *
Same here.
+ * Copyright (C) 2019-present Lenovo
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+static struct pmbus_driver_info sn1701022_info = {
+ .pages = 2,
+ .format[PSC_VOLTAGE_IN] = linear,
+ .format[PSC_VOLTAGE_OUT] = vid,
+ .vrm_version = vr12,
+ .format[PSC_CURRENT_OUT] = linear,
+ .format[PSC_CURRENT_IN] = linear,
+ .format[PSC_POWER] = linear,
+ .format[PSC_TEMPERATURE] = linear,
+
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
+ | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
+ | PMBUS_HAVE_POUT,
+
+ .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
+ | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
+ | PMBUS_HAVE_POUT,
+};
+
+static int sn1701022_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ return pmbus_do_probe(client, id, &sn1701022_info);
+}
+
+static const struct i2c_device_id sn1701022_id[] = {
+ {"sn1701022", 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, sn1701022_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver sn1701022_driver = {
+ .driver = {
+ .name = "sn1701022",
+ },
+ .probe = sn1701022_probe,
+ .remove = pmbus_do_remove,
+ .id_table = sn1701022_id,
+};
+
+module_i2c_driver(sn1701022_driver);
+
+MODULE_AUTHOR("Yonghui Liu <liuyh21@xxxxxxxxxx>");
+MODULE_DESCRIPTION("PMBus driver for sn1701022");
+MODULE_LICENSE("GPL");