hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver

[PATCH v2 3/5] hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver

Posted by Lakshmi Yadlapati 2 years, 10 months ago

Add the driver to support Acbel CRPS power supply.

Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com>
---
Changes since V1
- Removed debugfs stuff.
- Removed acbel_crps_read_word_data and acbel_crps_read_byte_data.
- Removed PMBUS_MFR_IIN_MAX.
- Added validation for the supported power supply.
- Fix the formatting.

 drivers/hwmon/pmbus/Kconfig      |  10 +++
 drivers/hwmon/pmbus/Makefile     |   1 +
 drivers/hwmon/pmbus/acbel-crps.c | 102 +++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/acbel-crps.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 59d9a7430499..0215709c3dd2 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -27,6 +27,16 @@ config SENSORS_PMBUS
 	  This driver can also be built as a module. If so, the module will
 	  be called pmbus.
 
+config SENSORS_ACBEL_CRPS
+	tristate "ACBEL CRPS Power Supply"
+	help
+	  If you say yes here you get hardware monitoring support for the ACBEL
+	  Common Redundant Power Supply.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called acbel-crps.
+	  Supported models: FSG032-00xG
+
 config SENSORS_ADM1266
 	tristate "Analog Devices ADM1266 Sequencer"
 	select CRC8
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 3ae019916267..39aef0cb9934 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -5,6 +5,7 @@
 
 obj-$(CONFIG_PMBUS)		+= pmbus_core.o
 obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
+obj-$(CONFIG_SENSORS_ACBEL_CRPS) += acbel-crps.o
 obj-$(CONFIG_SENSORS_ADM1266)	+= adm1266.o
 obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
 obj-$(CONFIG_SENSORS_BEL_PFE)	+= bel-pfe.o
diff --git a/drivers/hwmon/pmbus/acbel-crps.c b/drivers/hwmon/pmbus/acbel-crps.c
new file mode 100644
index 000000000000..ac281699709f
--- /dev/null
+++ b/drivers/hwmon/pmbus/acbel-crps.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2023 IBM Corp.
+ */
+
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include <linux/hwmon-sysfs.h>
+#include "pmbus.h"
+
+struct acbel_crps {
+	struct i2c_client *client;
+};
+
+static const struct i2c_device_id acbel_crps_id[] = {
+	{ "acbel_crps" },
+	{}
+};
+#define to_psu(x, y) container_of((x), struct acbel_crps, debugfs_entries[(y)])
+
+static const struct file_operations acbel_crps_fops = {
+	.llseek = noop_llseek,
+	.open = simple_open,
+};
+
+static struct pmbus_driver_info acbel_crps_info = {
+	.pages = 1,
+	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
+		   PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT | PMBUS_HAVE_POUT |
+		   PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+		   PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_VOUT |
+		   PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_TEMP |
+		   PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_FAN12,
+};
+
+static int acbel_crps_probe(struct i2c_client *client)
+{
+	struct acbel_crps *psu;
+	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
+	struct device *dev = &client->dev;
+	int rc;
+
+	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
+	if (rc < 0) {
+		dev_err(dev, "Failed to read PMBUS_MFR_ID\n");
+		return rc;
+	}
+	if (strncmp(buf, "ACBEL", 5)) {
+		buf[rc] = '\0';
+		dev_err(dev, "Manufacturer '%s' not supported\n", buf);
+		return -ENODEV;
+	}
+
+	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+	if (rc < 0) {
+		dev_err(dev, "Failed to read PMBUS_MFR_MODEL\n");
+		return rc;
+	}
+
+	if (strncmp(buf, "FSG032", 6)) {
+		buf[rc] = '\0';
+		dev_err(dev, "Model '%s' not supported\n", buf);
+		return -ENODEV;
+	}
+
+	rc = pmbus_do_probe(client, &acbel_crps_info);
+	if (rc)
+		return rc;
+	/*
+         * Don't fail the probe if there isn't enough memory for debugfs.
+         */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
+	if (!psu)
+		return 0;
+
+	return 0;
+}
+
+static const struct of_device_id acbel_crps_of_match[] = {
+	{ .compatible = "acbel,crps" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, acbel_crps_of_match);
+
+static struct i2c_driver acbel_crps_driver = {
+	.driver = {
+		.name = "acbel-crps",
+		.of_match_table = acbel_crps_of_match,
+	},
+	.probe_new = acbel_crps_probe,
+	.id_table = acbel_crps_id,
+};
+
+module_i2c_driver(acbel_crps_driver);
+
+MODULE_AUTHOR("Lakshmi Yadlapati");
+MODULE_DESCRIPTION("PMBus driver for AcBel Power System power supplies");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);
-- 
2.37.2

Re: [PATCH v2 3/5] hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver

Posted by kernel test robot 2 years, 10 months ago

Hi Lakshmi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.3-rc3 next-20230321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lakshmi-Yadlapati/dt-bindings-vendor-prefixes-Add-prefix-for-acbel/20230320-235222
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230320154019.1943770-4-lakshmiy%40us.ibm.com
patch subject: [PATCH v2 3/5] hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver
config: riscv-randconfig-c006-20230319 (https://download.01.org/0day-ci/archive/20230321/202303211303.U8Ip4xDC-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/d873cbdc9f171b066c3f6f6c2a39736e168ad19f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lakshmi-Yadlapati/dt-bindings-vendor-prefixes-Add-prefix-for-acbel/20230320-235222
        git checkout d873cbdc9f171b066c3f6f6c2a39736e168ad19f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/hwmon/pmbus/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303211303.U8Ip4xDC-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwmon/pmbus/acbel-crps.c:24:37: warning: unused variable 'acbel_crps_fops' [-Wunused-const-variable]
   static const struct file_operations acbel_crps_fops = {
                                       ^
   1 warning generated.


vim +/acbel_crps_fops +24 drivers/hwmon/pmbus/acbel-crps.c

    23	
  > 24	static const struct file_operations acbel_crps_fops = {
    25		.llseek = noop_llseek,
    26		.open = simple_open,
    27	};
    28	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 3/5] hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver

Posted by Guenter Roeck 2 years, 10 months ago

On Mon, Mar 20, 2023 at 10:40:17AM -0500, Lakshmi Yadlapati wrote:
> Add the driver to support Acbel CRPS power supply.
> 
> Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com>
> ---
> Changes since V1
> - Removed debugfs stuff.
> - Removed acbel_crps_read_word_data and acbel_crps_read_byte_data.
> - Removed PMBUS_MFR_IIN_MAX.
> - Added validation for the supported power supply.
> - Fix the formatting.
> 
>  drivers/hwmon/pmbus/Kconfig      |  10 +++
>  drivers/hwmon/pmbus/Makefile     |   1 +
>  drivers/hwmon/pmbus/acbel-crps.c | 102 +++++++++++++++++++++++++++++++
>  3 files changed, 113 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/acbel-crps.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 59d9a7430499..0215709c3dd2 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -27,6 +27,16 @@ config SENSORS_PMBUS
>  	  This driver can also be built as a module. If so, the module will
>  	  be called pmbus.
>  
> +config SENSORS_ACBEL_CRPS
> +	tristate "ACBEL CRPS Power Supply"
> +	help
> +	  If you say yes here you get hardware monitoring support for the ACBEL
> +	  Common Redundant Power Supply.
> +

This sounds like there is only one, but ...

> +	  This driver can also be built as a module. If so, the module will
> +	  be called acbel-crps.
> +	  Supported models: FSG032-00xG
> +
... here it says that only one model is (currently) supported.

This should just say "Support for Acbel FSG032-00xG CRPS Power Supply"
and not claim that it supports any others.

I am also not convinced that the Kconfig option driver name should simply
be "crps" There is no guarantee that all crps power supplies from this
vendor will always be supported (supportable) by this driver.


>  config SENSORS_ADM1266
>  	tristate "Analog Devices ADM1266 Sequencer"
>  	select CRC8
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 3ae019916267..39aef0cb9934 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -5,6 +5,7 @@
>  
>  obj-$(CONFIG_PMBUS)		+= pmbus_core.o
>  obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
> +obj-$(CONFIG_SENSORS_ACBEL_CRPS) += acbel-crps.o
>  obj-$(CONFIG_SENSORS_ADM1266)	+= adm1266.o
>  obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
>  obj-$(CONFIG_SENSORS_BEL_PFE)	+= bel-pfe.o
> diff --git a/drivers/hwmon/pmbus/acbel-crps.c b/drivers/hwmon/pmbus/acbel-crps.c
> new file mode 100644
> index 000000000000..ac281699709f
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/acbel-crps.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2023 IBM Corp.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include <linux/hwmon-sysfs.h>

Unused include

> +#include "pmbus.h"
> +
> +struct acbel_crps {
> +	struct i2c_client *client;
> +};
> +
> +static const struct i2c_device_id acbel_crps_id[] = {
> +	{ "acbel_crps" },
> +	{}
> +};
> +#define to_psu(x, y) container_of((x), struct acbel_crps, debugfs_entries[(y)])
> +
> +static const struct file_operations acbel_crps_fops = {
> +	.llseek = noop_llseek,
> +	.open = simple_open,
> +};

The above code is unused.

> +
> +static struct pmbus_driver_info acbel_crps_info = {
> +	.pages = 1,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
> +		   PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT | PMBUS_HAVE_POUT |
> +		   PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +		   PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_VOUT |
> +		   PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_TEMP |
> +		   PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_FAN12,
> +};
> +
> +static int acbel_crps_probe(struct i2c_client *client)
> +{
> +	struct acbel_crps *psu;
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> +	struct device *dev = &client->dev;
> +	int rc;
> +
> +	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
> +	if (rc < 0) {
> +		dev_err(dev, "Failed to read PMBUS_MFR_ID\n");
> +		return rc;
> +	}
> +	if (strncmp(buf, "ACBEL", 5)) {

this also needs to check for rc
	if (rc < 5 || ...)

> +		buf[rc] = '\0';
> +		dev_err(dev, "Manufacturer '%s' not supported\n", buf);
> +		return -ENODEV;
> +	}
> +
> +	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> +	if (rc < 0) {
> +		dev_err(dev, "Failed to read PMBUS_MFR_MODEL\n");
> +		return rc;
> +	}
> +
> +	if (strncmp(buf, "FSG032", 6)) {

	if (rc < 6 || ...)

> +		buf[rc] = '\0';
> +		dev_err(dev, "Model '%s' not supported\n", buf);
> +		return -ENODEV;
> +	}
> +
> +	rc = pmbus_do_probe(client, &acbel_crps_info);
> +	if (rc)
> +		return rc;
> +	/*
> +         * Don't fail the probe if there isn't enough memory for debugfs.
> +         */

Formatting

> +	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
> +	if (!psu)
> +		return 0;

This code doesn't make any sense.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id acbel_crps_of_match[] = {
> +	{ .compatible = "acbel,crps" },

This is way too generic. What if there is some other Acbel power supply
which needs some other options or supports other attributes ?
This needs to be something like "acbel,fsg032" or similar.

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, acbel_crps_of_match);
> +
> +static struct i2c_driver acbel_crps_driver = {
> +	.driver = {
> +		.name = "acbel-crps",
> +		.of_match_table = acbel_crps_of_match,
> +	},
> +	.probe_new = acbel_crps_probe,

I think probe_new may be gone.

> +	.id_table = acbel_crps_id,
> +};
> +
> +module_i2c_driver(acbel_crps_driver);
> +
> +MODULE_AUTHOR("Lakshmi Yadlapati");
> +MODULE_DESCRIPTION("PMBus driver for AcBel Power System power supplies");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);
> -- 
> 2.37.2
>

Re: [PATCH v2 3/5] hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver

Posted by kernel test robot 2 years, 10 months ago

Hi Lakshmi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.3-rc3 next-20230320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lakshmi-Yadlapati/dt-bindings-vendor-prefixes-Add-prefix-for-acbel/20230320-235222
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230320154019.1943770-4-lakshmiy%40us.ibm.com
patch subject: [PATCH v2 3/5] hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230321/202303210322.ZZ2VlGIE-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d873cbdc9f171b066c3f6f6c2a39736e168ad19f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lakshmi-Yadlapati/dt-bindings-vendor-prefixes-Add-prefix-for-acbel/20230320-235222
        git checkout d873cbdc9f171b066c3f6f6c2a39736e168ad19f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/pmbus/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303210322.ZZ2VlGIE-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwmon/pmbus/acbel-crps.c:24:37: warning: 'acbel_crps_fops' defined but not used [-Wunused-const-variable=]
      24 | static const struct file_operations acbel_crps_fops = {
         |                                     ^~~~~~~~~~~~~~~


vim +/acbel_crps_fops +24 drivers/hwmon/pmbus/acbel-crps.c

    23	
  > 24	static const struct file_operations acbel_crps_fops = {
    25		.llseek = noop_llseek,
    26		.open = simple_open,
    27	};
    28	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH v2 1/5] dt-bindings: vendor-prefixes: Add prefix for acbel
[PATCH v2 2/5] dt-bindings: trivial-devices: Add acbel,crps
[PATCH v2 3/5] hwmon: (pmbus/acbel-crps) Add Acbel CRPS power supply driver
[PATCH v2 4/5] docs: hwmon: Add documenttaion for acbel-crps PSU
[PATCH v2 5/5] ARM: dts: aspeed: p10bmc: Change power supply info