Message ID | 20230426090356.745979-1-Naresh.Solanki@9elements.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp105326vqo; Wed, 26 Apr 2023 02:13:15 -0700 (PDT) X-Google-Smtp-Source: AKy350YghMkWBgxwIUFPtElBaMtb36DO+SsdmO8Ru3E/XP88Nr9NkPqpW7i1NQVcCOiBvk16/SKp X-Received: by 2002:a05:6a20:7d9a:b0:f2:4f56:be57 with SMTP id v26-20020a056a207d9a00b000f24f56be57mr24478817pzj.2.1682500394945; Wed, 26 Apr 2023 02:13:14 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682500394; cv=none; d=google.com; s=arc-20160816; b=L07AWLmGbf+cv3XsKKgQpKOPew25mdo8WcA0FTFD0ETx9yg9dkt5E9Igguj1PR/Ig5 kgu4jLKE/wqqT7lS4bhRoadFmfH7KVsHlXHr07D3W9yj8dmaAcxw74QB2OROeT8b1cmX +tf5aRECvQc716j1WKMWDHUeOxxmwUVMZ7dZ2VfP1IaK7k+ViaUfTcOVQ9hQkK0ojBC1 X7b2ZAtIqryYgcMQZHbqf3BMXfJYwMzoUVg901zXj2Yj6RR03hEfMWT1qeNesjaHeMHF 9MTtkc3hS4UEJiTHB2REvVajFl2t+K8Pdn0eMCy4qffzLckSsaacVtK7Rt7mPphG5B7m u8mg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=gTe9Erp8A3FszZ2u9J7n6BQSH7RUrGdSBEbUVVQT+CU=; b=AWyBjEjC4qtKmtO1fZxmLZUy1otlP3iSNNeAFD1Tfo4nGrcNYLKqdfRPRSksCcJGqq hLh9/jp1CTcmmvjnC4xD4kq26rchHhzuRX57tJ6/g8TYfs39eB4l6omrQOBn8xigPX/j m2xmq2XVmMpRroEZ66qwZcyXzj5HY23nBpBiiYkT3muYhTkJEWzslHndZqshKOtd8XwA wGdz+pWN+uiMVLnFh1SZHSlFEeEKdOoHSFjeh/W9eMls+IF7mtzUH9UCtO+zzsgmJxTi 2FPBTciSvDYAUBNDI5SiWdOr/Yyxr2I5qc0OOvUHIsYZCnykho2EIRvqyvREePi38YB3 k1Gw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=DvWydOSz; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u3-20020a654c03000000b0051f6974b6f9si15839217pgq.789.2023.04.26.02.13.02; Wed, 26 Apr 2023 02:13:14 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=DvWydOSz; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239869AbjDZJEM (ORCPT <rfc822;zxc52fgh@gmail.com> + 99 others); Wed, 26 Apr 2023 05:04:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60340 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239757AbjDZJEK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 26 Apr 2023 05:04:10 -0400 Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BC43640DE for <linux-kernel@vger.kernel.org>; Wed, 26 Apr 2023 02:04:04 -0700 (PDT) Received: by mail-wm1-x336.google.com with SMTP id 5b1f17b1804b1-3f173af665fso44813935e9.3 for <linux-kernel@vger.kernel.org>; Wed, 26 Apr 2023 02:04:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9elements.com; s=google; t=1682499843; x=1685091843; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=gTe9Erp8A3FszZ2u9J7n6BQSH7RUrGdSBEbUVVQT+CU=; b=DvWydOSzPpE2ZNg3OBCevSDs4MTY/t+5ZWETDzwzyN/I8tIzRI7Rx/OzTu3xjeWW73 zbgJfjtIT4VgZfH5XGr8bu7Gy6imXfp/UJeYV2m+KhUrHru4hkVUma6NyJUbgISv7RhE aF04cjZI+7kgTyyaHrPHyl7rTdNFaQqXavFx2M/q2WJqSFnnwiLDsKG9SGIyuEYm5qsQ +Nb59obfPcbFYgUuAcJjNEibJOIq0JiQ8uWPHcfFjZRVFDPscJa32HZgp5smEGNFNKxw X5RpuPt0EtCYDTTO4dzc1ZbZGY2+c/I9JdDnBx+34f3aXZM+oPYWuBW/qpMTlYor3Awn ceWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682499843; x=1685091843; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=gTe9Erp8A3FszZ2u9J7n6BQSH7RUrGdSBEbUVVQT+CU=; b=CojVVqnwY3szC83h2fIGc38atJ292l6pIoSuNGZifNQT59mSizCpv2kxayeIJcJKCk Sog2YgmVUaNlV5YwE0N7L8Pu3+zShL0IDj4tKuVRezu/kBeRMb6G4rodKT0CmtxcerEm 0z53TMEv8Cf9Kef1bvbm2nBsg0z5NuJbbVmRkofYXSPwqADpQI8LM8yqgyFSjT1UGWr1 HZbFsaWRE0fsSMAGtYSrMhctb9iqn+vLDIJuSLvQtCeqkK107sRbCcJLWf4am91cHn36 O6yYQ2NE5lTd4mjlIbhIb5MoXC9Cdj5rCd2LaCm95fD5OTmWYZS+lLdgFFCCMLqc7gQd JQzQ== X-Gm-Message-State: AAQBX9ei4d16y3ZEHHrEXfJhrDea4MQUFQJx7699SpxxadSCJ3pYtGrn ha2gHZvT/zLOgPh5Rf30BudZBoIaJJwUTv0LbrqekA== X-Received: by 2002:a05:600c:3657:b0:3f0:4734:bef8 with SMTP id y23-20020a05600c365700b003f04734bef8mr12041627wmq.39.1682499843148; Wed, 26 Apr 2023 02:04:03 -0700 (PDT) Received: from stroh80.sec.9e.network (ip-078-094-000-051.um19.pools.vodafone-ip.de. [78.94.0.51]) by smtp.gmail.com with ESMTPSA id k18-20020a05600c0b5200b003edf2dc7ca3sm17243673wmr.34.2023.04.26.02.04.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Apr 2023 02:04:02 -0700 (PDT) From: Naresh Solanki <naresh.solanki@9elements.com> X-Google-Original-From: Naresh Solanki <Naresh.Solanki@9elements.com> To: Jean Delvare <jdelvare@suse.com>, Guenter Roeck <linux@roeck-us.net> Cc: Patrick Rudolph <patrick.rudolph@9elements.com>, Naresh Solanki <Naresh.Solanki@9elements.com>, linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org Subject: [PATCH] hwmon: (max597x) Add Maxim Max597x Date: Wed, 26 Apr 2023 11:03:55 +0200 Message-Id: <20230426090356.745979-1-Naresh.Solanki@9elements.com> X-Mailer: git-send-email 2.39.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764229533981615987?= X-GMAIL-MSGID: =?utf-8?q?1764229533981615987?= |
Series |
hwmon: (max597x) Add Maxim Max597x
|
|
Commit Message
Naresh Solanki
April 26, 2023, 9:03 a.m. UTC
From: Patrick Rudolph <patrick.rudolph@9elements.com> Add support for the Maxim Max59x power switch with current/voltage monitor. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> --- drivers/hwmon/Kconfig | 9 ++ drivers/hwmon/Makefile | 1 + drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 222 insertions(+) create mode 100644 drivers/hwmon/max597x.c base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
Comments
Le 26/04/2023 à 11:03, Naresh Solanki a écrit : > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Add support for the Maxim Max59x power switch with current/voltage > monitor. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> Hi, a few nit below, should there be a v2. CJ > --- > drivers/hwmon/Kconfig | 9 ++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 222 insertions(+) > create mode 100644 drivers/hwmon/max597x.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 5b3b76477b0e..164d980d9de2 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760 > This driver can also be built as a module. If so, the module > will be called max31760. > > +config SENSORS_MAX597X > + tristate "Maxim 597x power switch and monitor" > + depends on I2C > + depends on OF > + select MFD_MAX597X > + help > + This driver exposes Maxim 5970/5978 voltage/current monitoring > + interface. > + > config SENSORS_MAX6620 > tristate "Maxim MAX6620 fan controller" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 88712b5031c8..720eb7d5fe46 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o > obj-$(CONFIG_SENSORS_MAX31722) += max31722.o > obj-$(CONFIG_SENSORS_MAX31730) += max31730.o > obj-$(CONFIG_SENSORS_MAX31760) += max31760.o > +obj-$(CONFIG_SENSORS_MAX597X) += max597x.o > obj-$(CONFIG_SENSORS_MAX6620) += max6620.o > obj-$(CONFIG_SENSORS_MAX6621) += max6621.o > obj-$(CONFIG_SENSORS_MAX6639) += max6639.o > diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c > new file mode 100644 > index 000000000000..d4d8c2faf55c > --- /dev/null > +++ b/drivers/hwmon/max597x.c > @@ -0,0 +1,212 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Device driver for regulators in MAX5970 and MAX5978 IC > + * > + * Copyright (c) 2022 9elements GmbH > + * > + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> > + */ > + > +#include <linux/hwmon.h> > +#include <linux/i2c.h> > +#include <linux/mfd/max597x.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +struct max597x_hwmon { > + int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES]; Having 1 item per line is much more usual. > + struct regmap *regmap; > +}; > + > +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val) > +{ > + u8 reg_data[2]; > + int ret; > + > + ret = regmap_bulk_read(ddata->regmap, reg, ®_data[0], 2); > + if (ret < 0) > + return ret; > + > + *val = (reg_data[0] << 2) | (reg_data[1] & 3); > + *val = *val * range; > + /* > + * From datasheet, the range is fractionally less. > + * To compensate that, divide with 1033 number. > + */ > + *val = *val / 1033; > + > + return 0; > +} > + > +static int max597x_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct max597x_hwmon *ddata = dev_get_drvdata(dev); > + int ret; > + > + switch (type) { > + case hwmon_curr: > + switch (attr) { > + case hwmon_curr_input: > + ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel), > + ddata->irng[channel], val); > + if (ret < 0) > + return ret; > + > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + > + case hwmon_in: > + switch (attr) { > + case hwmon_in_input: > + ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel), > + ddata->mon_rng[channel], val); > + if (ret < 0) > + return ret; > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + } > + return -EOPNOTSUPP; > +} > + > +static umode_t max597x_is_visible(const void *data, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + struct max597x_hwmon *ddata = (struct max597x_hwmon *)data; > + > + if (channel >= ddata->num_switches) > + return 0; > + > + switch (type) { > + case hwmon_in: > + switch (attr) { > + case hwmon_in_input: > + return 0444; > + } > + break; > + case hwmon_curr: > + switch (attr) { > + case hwmon_curr_input: > + return 0444; > + } > + break; > + default: > + break; > + } > + return 0; > +} > + > +static const struct hwmon_ops max597x_hwmon_ops = { > + .is_visible = max597x_is_visible, > + .read = max597x_read, > +}; > + > +#define HWMON_CURRENT HWMON_C_INPUT > +#define HWMON_VOLTAGE HWMON_I_INPUT > + > +static const struct hwmon_channel_info *max597x_info[] = { > + HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE), > + HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT), > + NULL > +}; > + > +static const struct hwmon_chip_info max597x_chip_info = { > + .ops = &max597x_hwmon_ops, > + .info = max597x_info, > +}; > + > +static int max597x_adc_range(struct regmap *regmap, const int ch, > + u32 *irng, u32 *mon_rng) > +{ > + unsigned int reg; > + int ret; > + > + /* Decode current ADC range */ > + ret = regmap_read(regmap, MAX5970_REG_STATUS2, ®); > + if (ret) > + return ret; > + switch (MAX5970_IRNG(reg, ch)) { > + case 0: > + *irng = 100000; /* 100 mV */ > + break; > + case 1: > + *irng = 50000; /* 50 mV */ > + break; > + case 2: > + *irng = 25000; /* 25 mV */ > + break; > + default: > + return -EINVAL; > + } > + > + /* Decode current voltage monitor range */ > + ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, ®); > + if (ret) > + return ret; > + > + *mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch); > + *mon_rng /= 1000; /* uV to mV */ > + > + return 0; > +} > + > +static int max597x_sensor_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent); > + struct max597x_hwmon *ddata; > + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); > + struct device *hwmon_dev; > + int err; > + > + if (!regmap) > + return -EPROBE_DEFER; > + > + ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL); > + if (!ddata) > + return -ENOMEM; > + > + if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978")) > + ddata->num_switches = MAX597x_TYPE_MAX5978; > + else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970")) > + ddata->num_switches = MAX597x_TYPE_MAX5970; > + else > + return -ENODEV; > + > + ddata->regmap = regmap; > + > + for (int i = 0; i < ddata->num_switches; i++) { This is unusual to define 'i' within a 'for' loop. > + err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]); > + if (err < 0) > + return err; > + } > + > + hwmon_dev = devm_hwmon_device_register_with_info(dev, > + "max597x_hwmon", ddata, > + &max597x_chip_info, NULL); > + Unneeded empty line. > + if (IS_ERR(hwmon_dev)) { > + err = PTR_ERR(hwmon_dev); > + dev_err(dev, "Unable to register hwmon device, returned %d\n", err); > + return err; return dev_err_probe()? CJ > + } > + > + return 0; > +} > + > +static struct platform_driver max597x_sensor_driver = { > + .probe = max597x_sensor_probe, > + .driver = { > + .name = "max597x-hwmon", > + }, > +}; > +module_platform_driver(max597x_sensor_driver); > + > +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); > +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); > +MODULE_LICENSE("GPL v2"); > > base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b
Hi Naresh, kernel test robot noticed the following build warnings: [auto build test WARNING on b4c288cfd2f84c44994330c408e14645d45dee5b] url: https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556 base: b4c288cfd2f84c44994330c408e14645d45dee5b patch link: https://lore.kernel.org/r/20230426090356.745979-1-Naresh.Solanki%409elements.com patch subject: [PATCH] hwmon: (max597x) Add Maxim Max597x config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230426/202304262021.nGJgsK76-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 12.1.0 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 # https://github.com/intel-lab-lkp/linux/commit/3d6f729d86a79adf0603717c79bc389a5dcc4444 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556 git checkout 3d6f729d86a79adf0603717c79bc389a5dcc4444 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/ 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/202304262021.nGJgsK76-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/hwmon/max597x.c: In function 'max597x_read': >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_chip' not handled in switch [-Wswitch] 47 | switch (type) { | ^~~~~~ >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_temp' not handled in switch [-Wswitch] >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_power' not handled in switch [-Wswitch] >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_energy' not handled in switch [-Wswitch] >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_humidity' not handled in switch [-Wswitch] >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_fan' not handled in switch [-Wswitch] >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_pwm' not handled in switch [-Wswitch] >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_intrusion' not handled in switch [-Wswitch] >> drivers/hwmon/max597x.c:47:9: warning: enumeration value 'hwmon_max' not handled in switch [-Wswitch] vim +/hwmon_chip +47 drivers/hwmon/max597x.c 40 41 static int max597x_read(struct device *dev, enum hwmon_sensor_types type, 42 u32 attr, int channel, long *val) 43 { 44 struct max597x_hwmon *ddata = dev_get_drvdata(dev); 45 int ret; 46 > 47 switch (type) { 48 case hwmon_curr: 49 switch (attr) { 50 case hwmon_curr_input: 51 ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel), 52 ddata->irng[channel], val); 53 if (ret < 0) 54 return ret; 55 56 return 0; 57 default: 58 return -EOPNOTSUPP; 59 } 60 61 case hwmon_in: 62 switch (attr) { 63 case hwmon_in_input: 64 ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel), 65 ddata->mon_rng[channel], val); 66 if (ret < 0) 67 return ret; 68 return 0; 69 default: 70 return -EOPNOTSUPP; 71 } 72 } 73 return -EOPNOTSUPP; 74 } 75
On Wed, Apr 26, 2023 at 11:03:55AM +0200, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Add support for the Maxim Max59x power switch with current/voltage > monitor. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> Please no wildcards in file names or descriptions for hwmon drivers, much less dual-digit wildcards. This patch does not add support for MAX597{0..9} (and much less for MAX59{00..99}), only for MAX5970 and MAX5978. For example, it does not and never will support MAX5977 because that chip does not have an I2C interface, or MAX5974 which happens to be a PWM controller. Pick one of {5970,5978} for the file name, and mention both in descriptions. Yes, I understand, this was accepted with wildcard in mfd and regulator subsystems, but that doesn't make it acceptable here. Either case, what does this driver provide that isn't already available through drivers/regulator/max597x-regulator.c ? As written, the driver doesn't support any of the limit or alarm registers, and there seems to be quite some overlap with the regulator driver in terms of functionality. Please explore if that is acceptable for the regulator subsystem. If it is not, come back and we can continue discussing feasibility as separate hwmon driver. > --- > drivers/hwmon/Kconfig | 9 ++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++ Documentation missing. > 3 files changed, 222 insertions(+) > create mode 100644 drivers/hwmon/max597x.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 5b3b76477b0e..164d980d9de2 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760 > This driver can also be built as a module. If so, the module > will be called max31760. > > +config SENSORS_MAX597X > + tristate "Maxim 597x power switch and monitor" > + depends on I2C > + depends on OF > + select MFD_MAX597X That should be "depends on". > + help > + This driver exposes Maxim 5970/5978 voltage/current monitoring > + interface. > + > config SENSORS_MAX6620 > tristate "Maxim MAX6620 fan controller" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 88712b5031c8..720eb7d5fe46 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o > obj-$(CONFIG_SENSORS_MAX31722) += max31722.o > obj-$(CONFIG_SENSORS_MAX31730) += max31730.o > obj-$(CONFIG_SENSORS_MAX31760) += max31760.o > +obj-$(CONFIG_SENSORS_MAX597X) += max597x.o > obj-$(CONFIG_SENSORS_MAX6620) += max6620.o > obj-$(CONFIG_SENSORS_MAX6621) += max6621.o > obj-$(CONFIG_SENSORS_MAX6639) += max6639.o > diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c > new file mode 100644 > index 000000000000..d4d8c2faf55c > --- /dev/null > +++ b/drivers/hwmon/max597x.c > @@ -0,0 +1,212 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Device driver for regulators in MAX5970 and MAX5978 IC > + * > + * Copyright (c) 2022 9elements GmbH > + * > + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> > + */ > + > +#include <linux/hwmon.h> > +#include <linux/i2c.h> > +#include <linux/mfd/max597x.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +struct max597x_hwmon { > + int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES]; > + struct regmap *regmap; > +}; > + > +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val) > +{ > + u8 reg_data[2]; > + int ret; > + > + ret = regmap_bulk_read(ddata->regmap, reg, ®_data[0], 2); > + if (ret < 0) > + return ret; > + > + *val = (reg_data[0] << 2) | (reg_data[1] & 3); > + *val = *val * range; > + /* > + * From datasheet, the range is fractionally less. > + * To compensate that, divide with 1033 number. > + */ > + *val = *val / 1033; Where in the datasheet, and why is this conversion not needed in the regulator driver ? > + > + return 0; > +} > + > +static int max597x_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct max597x_hwmon *ddata = dev_get_drvdata(dev); > + int ret; > + > + switch (type) { > + case hwmon_curr: > + switch (attr) { > + case hwmon_curr_input: > + ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel), > + ddata->irng[channel], val); > + if (ret < 0) > + return ret; > + > + return 0; max597x_read_reg() returns a negative value or 0. Why not just return it ? The regulator driver takes shunt resistor values into account. Again, I think it would make much more sense to add hwmon support to the regulator driver than having a separate driver since there is lots of overlap. For example, the regulator driver already sets and monitors limits. > + default: > + return -EOPNOTSUPP; > + } > + > + case hwmon_in: > + switch (attr) { > + case hwmon_in_input: > + ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel), > + ddata->mon_rng[channel], val); > + if (ret < 0) > + return ret; > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + } Please add missing default: cases as reported by 0-day. > + return -EOPNOTSUPP; > +} > + > +static umode_t max597x_is_visible(const void *data, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + struct max597x_hwmon *ddata = (struct max597x_hwmon *)data; > + > + if (channel >= ddata->num_switches) > + return 0; > + > + switch (type) { > + case hwmon_in: > + switch (attr) { > + case hwmon_in_input: > + return 0444; > + } > + break; > + case hwmon_curr: > + switch (attr) { > + case hwmon_curr_input: > + return 0444; > + } > + break; > + default: > + break; > + } > + return 0; > +} > + > +static const struct hwmon_ops max597x_hwmon_ops = { > + .is_visible = max597x_is_visible, > + .read = max597x_read, > +}; > + > +#define HWMON_CURRENT HWMON_C_INPUT > +#define HWMON_VOLTAGE HWMON_I_INPUT Please drop and use HWMON_C_INPUT as well as HWMON_I_INPUT directly. > + > +static const struct hwmon_channel_info *max597x_info[] = { > + HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE), > + HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT), > + NULL > +}; > + > +static const struct hwmon_chip_info max597x_chip_info = { > + .ops = &max597x_hwmon_ops, > + .info = max597x_info, > +}; > + > +static int max597x_adc_range(struct regmap *regmap, const int ch, > + u32 *irng, u32 *mon_rng) > +{ That function also exists on the regulator driver. > + unsigned int reg; > + int ret; > + > + /* Decode current ADC range */ > + ret = regmap_read(regmap, MAX5970_REG_STATUS2, ®); > + if (ret) > + return ret; > + switch (MAX5970_IRNG(reg, ch)) { > + case 0: > + *irng = 100000; /* 100 mV */ > + break; > + case 1: > + *irng = 50000; /* 50 mV */ > + break; > + case 2: > + *irng = 25000; /* 25 mV */ > + break; > + default: > + return -EINVAL; > + } > + > + /* Decode current voltage monitor range */ > + ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, ®); > + if (ret) > + return ret; > + > + *mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch); > + *mon_rng /= 1000; /* uV to mV */ > + > + return 0; > +} > + > +static int max597x_sensor_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent); > + struct max597x_hwmon *ddata; > + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); > + struct device *hwmon_dev; > + int err; > + > + if (!regmap) > + return -EPROBE_DEFER; Since the parent driver is a mfd driver, I'd assume that this driver should be instantiated from there, and I don't see why probe would ever have to be deferred. Please explain how that can happen. > + > + ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL); > + if (!ddata) > + return -ENOMEM; > + > + if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978")) > + ddata->num_switches = MAX597x_TYPE_MAX5978; > + else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970")) > + ddata->num_switches = MAX597x_TYPE_MAX5970; Uuh, no. That is just accidentally correct, based on enum max597x_chip_type { MAX597x_TYPE_MAX5978 = 1, MAX597x_TYPE_MAX5970, }; The assumption that the numeric chip type matches the number of channels it supports is as wrong as it can be. There are defines for the number of channels. I am pretty much completely at loss why they are not used. Tha same applies for the regulator driver, of course. > + else > + return -ENODEV; > + > + ddata->regmap = regmap; > + > + for (int i = 0; i < ddata->num_switches; i++) { > + err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]); > + if (err < 0) > + return err; Why no error message here but one below ? I am not in favor of error messages in the probe function, but if you use them please be consistent. > + } > + > + hwmon_dev = devm_hwmon_device_register_with_info(dev, > + "max597x_hwmon", ddata, > + &max597x_chip_info, NULL); > + > + if (IS_ERR(hwmon_dev)) { > + err = PTR_ERR(hwmon_dev); > + dev_err(dev, "Unable to register hwmon device, returned %d\n", err); Use dev_err_probe() > + return err; > + } > + > + return 0; > +} > + > +static struct platform_driver max597x_sensor_driver = { > + .probe = max597x_sensor_probe, > + .driver = { > + .name = "max597x-hwmon", > + }, > +}; > +module_platform_driver(max597x_sensor_driver); > + > +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); > +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); > +MODULE_LICENSE("GPL v2"); > > base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b > -- > 2.39.1 >
Hi Guenter, On 26-04-2023 08:05 pm, Guenter Roeck wrote: > On Wed, Apr 26, 2023 at 11:03:55AM +0200, Naresh Solanki wrote: >> From: Patrick Rudolph <patrick.rudolph@9elements.com> >> >> Add support for the Maxim Max59x power switch with current/voltage >> monitor. >> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > Please no wildcards in file names or descriptions for hwmon drivers, > much less dual-digit wildcards. > > This patch does not add support for MAX597{0..9} (and much less for > MAX59{00..99}), only for MAX5970 and MAX5978. For example, it does not > and never will support MAX5977 because that chip does not have an I2C > interface, or MAX5974 which happens to be a PWM controller. > > Pick one of {5970,5978} for the file name, and mention both in > descriptions. Yes, I understand, this was accepted with wildcard > in mfd and regulator subsystems, but that doesn't make it acceptable > here. > > Either case, what does this driver provide that isn't already > available through drivers/regulator/max597x-regulator.c ? > As written, the driver doesn't support any of the limit or alarm > registers, and there seems to be quite some overlap with the regulator > driver in terms of functionality. Please explore if that is acceptable > for the regulator subsystem. If it is not, come back and we can continue > discussing feasibility as separate hwmon driver.Thanks for your feedback. I agree that file name wildcard doesn't makes sense here & there are other chips that aren't related to this driver. Hence will work on it to get it fixed for sure. Will check with regulator subsystem maintainers & based on feedback will proceed. Thanks :) > >> --- >> drivers/hwmon/Kconfig | 9 ++ >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++ > > Documentation missing. > >> 3 files changed, 222 insertions(+) >> create mode 100644 drivers/hwmon/max597x.c >> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 5b3b76477b0e..164d980d9de2 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760 >> This driver can also be built as a module. If so, the module >> will be called max31760. >> >> +config SENSORS_MAX597X >> + tristate "Maxim 597x power switch and monitor" >> + depends on I2C >> + depends on OF >> + select MFD_MAX597X > > That should be "depends on". Ack > >> + help >> + This driver exposes Maxim 5970/5978 voltage/current monitoring >> + interface. >> + >> config SENSORS_MAX6620 >> tristate "Maxim MAX6620 fan controller" >> depends on I2C >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index 88712b5031c8..720eb7d5fe46 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o >> obj-$(CONFIG_SENSORS_MAX31722) += max31722.o >> obj-$(CONFIG_SENSORS_MAX31730) += max31730.o >> obj-$(CONFIG_SENSORS_MAX31760) += max31760.o >> +obj-$(CONFIG_SENSORS_MAX597X) += max597x.o >> obj-$(CONFIG_SENSORS_MAX6620) += max6620.o >> obj-$(CONFIG_SENSORS_MAX6621) += max6621.o >> obj-$(CONFIG_SENSORS_MAX6639) += max6639.o >> diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c >> new file mode 100644 >> index 000000000000..d4d8c2faf55c >> --- /dev/null >> +++ b/drivers/hwmon/max597x.c >> @@ -0,0 +1,212 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Device driver for regulators in MAX5970 and MAX5978 IC >> + * >> + * Copyright (c) 2022 9elements GmbH >> + * >> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> >> + */ >> + >> +#include <linux/hwmon.h> >> +#include <linux/i2c.h> >> +#include <linux/mfd/max597x.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +struct max597x_hwmon { >> + int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES]; >> + struct regmap *regmap; >> +}; >> + >> +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val) >> +{ >> + u8 reg_data[2]; >> + int ret; >> + >> + ret = regmap_bulk_read(ddata->regmap, reg, ®_data[0], 2); >> + if (ret < 0) >> + return ret; >> + >> + *val = (reg_data[0] << 2) | (reg_data[1] & 3); >> + *val = *val * range; >> + /* >> + * From datasheet, the range is fractionally less. >> + * To compensate that, divide with 1033 number. >> + */ >> + *val = *val / 1033; > > Where in the datasheet, and why is this conversion not needed in the > regulator driver ? This is taken from the typical value mentioned in table containing MON_ LSB Voltage in datasheet. i.e., divide typical value with corresponding range. This is applicable for use in hwmon driver to accurate report readings from device. > >> + >> + return 0; >> +} >> + >> +static int max597x_read(struct device *dev, enum hwmon_sensor_types type, >> + u32 attr, int channel, long *val) >> +{ >> + struct max597x_hwmon *ddata = dev_get_drvdata(dev); >> + int ret; >> + >> + switch (type) { >> + case hwmon_curr: >> + switch (attr) { >> + case hwmon_curr_input: >> + ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel), >> + ddata->irng[channel], val); >> + if (ret < 0) >> + return ret; >> + >> + return 0; > > max597x_read_reg() returns a negative value or 0. Why not just return it ? Ack. > > The regulator driver takes shunt resistor values into account. Agree. The shunt resistor value should be accounted for correct current measurement. This driver lacks that. This needs to be fixed. Will post new patch revision to address that. > > Again, I think it would make much more sense to add hwmon support > to the regulator driver than having a separate driver since there > is lots of overlap. For example, the regulator driver already > sets and monitors limits. I agree. But the chip also has led functionality. I got review feedback to make it MFD. So when rewriting as MFD driver made separate driver based on subsystem. I'm not sure if it is ok to use hwmon subsytem in regulator driver. Will once check with maintainer on this. > >> + default: >> + return -EOPNOTSUPP; >> + } >> + >> + case hwmon_in: >> + switch (attr) { >> + case hwmon_in_input: >> + ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel), >> + ddata->mon_rng[channel], val); >> + if (ret < 0) >> + return ret; >> + return 0; >> + default: >> + return -EOPNOTSUPP; >> + } >> + } > > Please add missing default: cases as reported by 0-day. Ack > >> + return -EOPNOTSUPP; >> +} >> + >> +static umode_t max597x_is_visible(const void *data, >> + enum hwmon_sensor_types type, >> + u32 attr, int channel) >> +{ >> + struct max597x_hwmon *ddata = (struct max597x_hwmon *)data; >> + >> + if (channel >= ddata->num_switches) >> + return 0; >> + >> + switch (type) { >> + case hwmon_in: >> + switch (attr) { >> + case hwmon_in_input: >> + return 0444; >> + } >> + break; >> + case hwmon_curr: >> + switch (attr) { >> + case hwmon_curr_input: >> + return 0444; >> + } >> + break; >> + default: >> + break; >> + } >> + return 0; >> +} >> + >> +static const struct hwmon_ops max597x_hwmon_ops = { >> + .is_visible = max597x_is_visible, >> + .read = max597x_read, >> +}; >> + >> +#define HWMON_CURRENT HWMON_C_INPUT >> +#define HWMON_VOLTAGE HWMON_I_INPUT > > Please drop and use HWMON_C_INPUT as well as HWMON_I_INPUT > directly. Ack. > >> + >> +static const struct hwmon_channel_info *max597x_info[] = { >> + HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE), >> + HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT), >> + NULL >> +}; >> + >> +static const struct hwmon_chip_info max597x_chip_info = { >> + .ops = &max597x_hwmon_ops, >> + .info = max597x_info, >> +}; >> + >> +static int max597x_adc_range(struct regmap *regmap, const int ch, >> + u32 *irng, u32 *mon_rng) >> +{ > > That function also exists on the regulator driver. Yes. This is to read setting from chip. Added this to avoid dependency & race condition with regulator driver. > >> + unsigned int reg; >> + int ret; >> + >> + /* Decode current ADC range */ >> + ret = regmap_read(regmap, MAX5970_REG_STATUS2, ®); >> + if (ret) >> + return ret; >> + switch (MAX5970_IRNG(reg, ch)) { >> + case 0: >> + *irng = 100000; /* 100 mV */ >> + break; >> + case 1: >> + *irng = 50000; /* 50 mV */ >> + break; >> + case 2: >> + *irng = 25000; /* 25 mV */ >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + /* Decode current voltage monitor range */ >> + ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, ®); >> + if (ret) >> + return ret; >> + >> + *mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch); >> + *mon_rng /= 1000; /* uV to mV */ >> + >> + return 0; >> +} >> + >> +static int max597x_sensor_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent); >> + struct max597x_hwmon *ddata; >> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); >> + struct device *hwmon_dev; >> + int err; >> + >> + if (!regmap) >> + return -EPROBE_DEFER; > > Since the parent driver is a mfd driver, I'd assume that this driver > should be instantiated from there, and I don't see why probe would ever > have to be deferred. Please explain how that can happen. I agree with you but this implementation is referenced from other MFD leaf driver. Example: "drivers/hwmon/sy7636a-hwmon.c" > >> + >> + ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL); >> + if (!ddata) >> + return -ENOMEM; >> + >> + if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978")) >> + ddata->num_switches = MAX597x_TYPE_MAX5978; >> + else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970")) >> + ddata->num_switches = MAX597x_TYPE_MAX5970; > > Uuh, no. That is just accidentally correct, based on > > enum max597x_chip_type { > MAX597x_TYPE_MAX5978 = 1, > MAX597x_TYPE_MAX5970, > }; > > The assumption that the numeric chip type matches the number of channels > it supports is as wrong as it can be. There are defines for the number > of channels. I am pretty much completely at loss why they are not used. > Tha same applies for the regulator driver, of course. Ack. Will use defines. > >> + else >> + return -ENODEV; >> + >> + ddata->regmap = regmap; >> + >> + for (int i = 0; i < ddata->num_switches; i++) { >> + err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]); >> + if (err < 0) >> + return err; > > Why no error message here but one below ? I am not in favor of error > messages in the probe function, but if you use them please be consistent. Ack. > >> + } >> + >> + hwmon_dev = devm_hwmon_device_register_with_info(dev, >> + "max597x_hwmon", ddata, >> + &max597x_chip_info, NULL); >> + >> + if (IS_ERR(hwmon_dev)) { >> + err = PTR_ERR(hwmon_dev); >> + dev_err(dev, "Unable to register hwmon device, returned %d\n", err); > > Use dev_err_probe() Ack > >> + return err; >> + } >> + >> + return 0; >> +} >> + >> +static struct platform_driver max597x_sensor_driver = { >> + .probe = max597x_sensor_probe, >> + .driver = { >> + .name = "max597x-hwmon", >> + }, >> +}; >> +module_platform_driver(max597x_sensor_driver); >> + >> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); >> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); >> +MODULE_LICENSE("GPL v2"); >> >> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b >> -- >> 2.39.1 >> Regards, Naresh
On 4/27/23 02:50, Naresh Solanki wrote: > Hi Guenter, > > On 26-04-2023 08:05 pm, Guenter Roeck wrote: >> On Wed, Apr 26, 2023 at 11:03:55AM +0200, Naresh Solanki wrote: >>> From: Patrick Rudolph <patrick.rudolph@9elements.com> >>> >>> Add support for the Maxim Max59x power switch with current/voltage >>> monitor. >>> >>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> >> >> Please no wildcards in file names or descriptions for hwmon drivers, >> much less dual-digit wildcards. >> >> This patch does not add support for MAX597{0..9} (and much less for >> MAX59{00..99}), only for MAX5970 and MAX5978. For example, it does not >> and never will support MAX5977 because that chip does not have an I2C >> interface, or MAX5974 which happens to be a PWM controller. >> >> Pick one of {5970,5978} for the file name, and mention both in >> descriptions. Yes, I understand, this was accepted with wildcard >> in mfd and regulator subsystems, but that doesn't make it acceptable >> here. >> >> Either case, what does this driver provide that isn't already >> available through drivers/regulator/max597x-regulator.c ? >> As written, the driver doesn't support any of the limit or alarm >> registers, and there seems to be quite some overlap with the regulator >> driver in terms of functionality. Please explore if that is acceptable >> for the regulator subsystem. If it is not, come back and we can continue >> discussing feasibility as separate hwmon driver.Thanks for your feedback. > I agree that file name wildcard doesn't makes sense here & there are other chips that aren't related to this driver. Hence will work on it to get it fixed for sure. > Will check with regulator subsystem maintainers & based on feedback will proceed. > Thanks :) >> >>> --- >>> drivers/hwmon/Kconfig | 9 ++ >>> drivers/hwmon/Makefile | 1 + >>> drivers/hwmon/max597x.c | 212 ++++++++++++++++++++++++++++++++++++++++ >> >> Documentation missing. >> >>> 3 files changed, 222 insertions(+) >>> create mode 100644 drivers/hwmon/max597x.c >>> >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >>> index 5b3b76477b0e..164d980d9de2 100644 >>> --- a/drivers/hwmon/Kconfig >>> +++ b/drivers/hwmon/Kconfig >>> @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760 >>> This driver can also be built as a module. If so, the module >>> will be called max31760. >>> +config SENSORS_MAX597X >>> + tristate "Maxim 597x power switch and monitor" >>> + depends on I2C >>> + depends on OF >>> + select MFD_MAX597X >> >> That should be "depends on". > Ack >> >>> + help >>> + This driver exposes Maxim 5970/5978 voltage/current monitoring >>> + interface. >>> + >>> config SENSORS_MAX6620 >>> tristate "Maxim MAX6620 fan controller" >>> depends on I2C >>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >>> index 88712b5031c8..720eb7d5fe46 100644 >>> --- a/drivers/hwmon/Makefile >>> +++ b/drivers/hwmon/Makefile >>> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o >>> obj-$(CONFIG_SENSORS_MAX31722) += max31722.o >>> obj-$(CONFIG_SENSORS_MAX31730) += max31730.o >>> obj-$(CONFIG_SENSORS_MAX31760) += max31760.o >>> +obj-$(CONFIG_SENSORS_MAX597X) += max597x.o >>> obj-$(CONFIG_SENSORS_MAX6620) += max6620.o >>> obj-$(CONFIG_SENSORS_MAX6621) += max6621.o >>> obj-$(CONFIG_SENSORS_MAX6639) += max6639.o >>> diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c >>> new file mode 100644 >>> index 000000000000..d4d8c2faf55c >>> --- /dev/null >>> +++ b/drivers/hwmon/max597x.c >>> @@ -0,0 +1,212 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Device driver for regulators in MAX5970 and MAX5978 IC >>> + * >>> + * Copyright (c) 2022 9elements GmbH >>> + * >>> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> >>> + */ >>> + >>> +#include <linux/hwmon.h> >>> +#include <linux/i2c.h> >>> +#include <linux/mfd/max597x.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> + >>> +struct max597x_hwmon { >>> + int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES]; >>> + struct regmap *regmap; >>> +}; >>> + >>> +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val) >>> +{ >>> + u8 reg_data[2]; >>> + int ret; >>> + >>> + ret = regmap_bulk_read(ddata->regmap, reg, ®_data[0], 2); >>> + if (ret < 0) >>> + return ret; >>> + >>> + *val = (reg_data[0] << 2) | (reg_data[1] & 3); >>> + *val = *val * range; >>> + /* >>> + * From datasheet, the range is fractionally less. >>> + * To compensate that, divide with 1033 number. >>> + */ >>> + *val = *val / 1033; >> >> Where in the datasheet, and why is this conversion not needed in the >> regulator driver ? > This is taken from the typical value mentioned in table containing MON_ LSB Voltage in datasheet. i.e., divide typical value with corresponding range. > > This is applicable for use in hwmon driver to accurate report readings from device. Please add respective comment. >> >>> + >>> + return 0; >>> +} >>> + >>> +static int max597x_read(struct device *dev, enum hwmon_sensor_types type, >>> + u32 attr, int channel, long *val) >>> +{ >>> + struct max597x_hwmon *ddata = dev_get_drvdata(dev); >>> + int ret; >>> + >>> + switch (type) { >>> + case hwmon_curr: >>> + switch (attr) { >>> + case hwmon_curr_input: >>> + ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel), >>> + ddata->irng[channel], val); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return 0; >> >> max597x_read_reg() returns a negative value or 0. Why not just return it ? > Ack. >> >> The regulator driver takes shunt resistor values into account. > Agree. The shunt resistor value should be accounted for correct current measurement. This driver lacks that. This needs to be fixed. Will post new patch revision to address that. >> >> Again, I think it would make much more sense to add hwmon support >> to the regulator driver than having a separate driver since there >> is lots of overlap. For example, the regulator driver already >> sets and monitors limits. > I agree. But the chip also has led functionality. I got review feedback to make it MFD. So when rewriting as MFD driver made separate driver based on subsystem. I'm not sure if it is ok to use hwmon subsytem in regulator driver. Will once check with maintainer on this. >> >>> + default: >>> + return -EOPNOTSUPP; >>> + } >>> + >>> + case hwmon_in: >>> + switch (attr) { >>> + case hwmon_in_input: >>> + ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel), >>> + ddata->mon_rng[channel], val); >>> + if (ret < 0) >>> + return ret; >>> + return 0; >>> + default: >>> + return -EOPNOTSUPP; >>> + } >>> + } >> >> Please add missing default: cases as reported by 0-day. > Ack >> >>> + return -EOPNOTSUPP; >>> +} >>> + >>> +static umode_t max597x_is_visible(const void *data, >>> + enum hwmon_sensor_types type, >>> + u32 attr, int channel) >>> +{ >>> + struct max597x_hwmon *ddata = (struct max597x_hwmon *)data; >>> + >>> + if (channel >= ddata->num_switches) >>> + return 0; >>> + >>> + switch (type) { >>> + case hwmon_in: >>> + switch (attr) { >>> + case hwmon_in_input: >>> + return 0444; >>> + } >>> + break; >>> + case hwmon_curr: >>> + switch (attr) { >>> + case hwmon_curr_input: >>> + return 0444; >>> + } >>> + break; >>> + default: >>> + break; >>> + } >>> + return 0; >>> +} >>> + >>> +static const struct hwmon_ops max597x_hwmon_ops = { >>> + .is_visible = max597x_is_visible, >>> + .read = max597x_read, >>> +}; >>> + >>> +#define HWMON_CURRENT HWMON_C_INPUT >>> +#define HWMON_VOLTAGE HWMON_I_INPUT >> >> Please drop and use HWMON_C_INPUT as well as HWMON_I_INPUT >> directly. > Ack. >> >>> + >>> +static const struct hwmon_channel_info *max597x_info[] = { >>> + HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE), >>> + HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT), >>> + NULL >>> +}; >>> + >>> +static const struct hwmon_chip_info max597x_chip_info = { >>> + .ops = &max597x_hwmon_ops, >>> + .info = max597x_info, >>> +}; >>> + >>> +static int max597x_adc_range(struct regmap *regmap, const int ch, >>> + u32 *irng, u32 *mon_rng) >>> +{ >> >> That function also exists on the regulator driver. > Yes. This is to read setting from chip. Added this to avoid dependency & race condition with regulator driver. >> >>> + unsigned int reg; >>> + int ret; >>> + >>> + /* Decode current ADC range */ >>> + ret = regmap_read(regmap, MAX5970_REG_STATUS2, ®); >>> + if (ret) >>> + return ret; >>> + switch (MAX5970_IRNG(reg, ch)) { >>> + case 0: >>> + *irng = 100000; /* 100 mV */ >>> + break; >>> + case 1: >>> + *irng = 50000; /* 50 mV */ >>> + break; >>> + case 2: >>> + *irng = 25000; /* 25 mV */ >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + >>> + /* Decode current voltage monitor range */ >>> + ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, ®); >>> + if (ret) >>> + return ret; >>> + >>> + *mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch); >>> + *mon_rng /= 1000; /* uV to mV */ >>> + >>> + return 0; >>> +} >>> + >>> +static int max597x_sensor_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent); >>> + struct max597x_hwmon *ddata; >>> + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); >>> + struct device *hwmon_dev; >>> + int err; >>> + >>> + if (!regmap) >>> + return -EPROBE_DEFER; >> >> Since the parent driver is a mfd driver, I'd assume that this driver >> should be instantiated from there, and I don't see why probe would ever >> have to be deferred. Please explain how that can happen. > I agree with you but this implementation is referenced from other MFD leaf driver. > Example: "drivers/hwmon/sy7636a-hwmon.c" That doesn't make it better, unless driving above the speed limit is ok because everyone is doing it. >> >>> + >>> + ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL); >>> + if (!ddata) >>> + return -ENOMEM; >>> + >>> + if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978")) >>> + ddata->num_switches = MAX597x_TYPE_MAX5978; >>> + else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970")) >>> + ddata->num_switches = MAX597x_TYPE_MAX5970; >> >> Uuh, no. That is just accidentally correct, based on >> >> enum max597x_chip_type { >> MAX597x_TYPE_MAX5978 = 1, >> MAX597x_TYPE_MAX5970, >> }; >> >> The assumption that the numeric chip type matches the number of channels >> it supports is as wrong as it can be. There are defines for the number >> of channels. I am pretty much completely at loss why they are not used. >> Tha same applies for the regulator driver, of course. > Ack. Will use defines. >> >>> + else >>> + return -ENODEV; >>> + >>> + ddata->regmap = regmap; >>> + >>> + for (int i = 0; i < ddata->num_switches; i++) { >>> + err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]); >>> + if (err < 0) >>> + return err; >> >> Why no error message here but one below ? I am not in favor of error >> messages in the probe function, but if you use them please be consistent. > Ack. >> >>> + } >>> + >>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, >>> + "max597x_hwmon", ddata, >>> + &max597x_chip_info, NULL); >>> + >>> + if (IS_ERR(hwmon_dev)) { >>> + err = PTR_ERR(hwmon_dev); >>> + dev_err(dev, "Unable to register hwmon device, returned %d\n", err); >> >> Use dev_err_probe() > Ack >> >>> + return err; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static struct platform_driver max597x_sensor_driver = { >>> + .probe = max597x_sensor_probe, >>> + .driver = { >>> + .name = "max597x-hwmon", >>> + }, >>> +}; >>> +module_platform_driver(max597x_sensor_driver); >>> + >>> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); >>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); >>> +MODULE_LICENSE("GPL v2"); >>> >>> base-commit: b4c288cfd2f84c44994330c408e14645d45dee5b >>> -- >>> 2.39.1 >>> > Regards, > Naresh
On 4/27/23 02:50, Naresh Solanki wrote: > Hi Guenter, > [ ... ] >> Again, I think it would make much more sense to add hwmon support >> to the regulator driver than having a separate driver since there >> is lots of overlap. For example, the regulator driver already >> sets and monitors limits. > I agree. But the chip also has led functionality. I got review feedback to make it MFD. So when rewriting as MFD driver made separate driver based on subsystem. I'm not sure if it is ok to use hwmon subsytem in regulator driver. Will once check with maintainer on this. LED has completely different functionality. Overlap between regulator and hwmon is substantial because the regulator code handles limits and alarms. Sure, you kind of bypassed (ignored) that by not implementing limit and alarm support in the hwmon driver, but that is really just avoiding the problem, not solving it. Guenter
Hi Naresh, kernel test robot noticed the following build warnings: [auto build test WARNING on b4c288cfd2f84c44994330c408e14645d45dee5b] url: https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556 base: b4c288cfd2f84c44994330c408e14645d45dee5b patch link: https://lore.kernel.org/r/20230426090356.745979-1-Naresh.Solanki%409elements.com patch subject: [PATCH] hwmon: (max597x) Add Maxim Max597x config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20230503/202305031640.SL6o9vFH-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) 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 # https://github.com/intel-lab-lkp/linux/commit/3d6f729d86a79adf0603717c79bc389a5dcc4444 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Naresh-Solanki/hwmon-max597x-Add-Maxim-Max597x/20230426-170556 git checkout 3d6f729d86a79adf0603717c79bc389a5dcc4444 # 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=i386 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/ 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/202305031640.SL6o9vFH-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/max597x.c:47:10: warning: 9 enumeration values not handled in switch: 'hwmon_chip', 'hwmon_temp', 'hwmon_power'... [-Wswitch] switch (type) { ^~~~ 1 warning generated. vim +47 drivers/hwmon/max597x.c 40 41 static int max597x_read(struct device *dev, enum hwmon_sensor_types type, 42 u32 attr, int channel, long *val) 43 { 44 struct max597x_hwmon *ddata = dev_get_drvdata(dev); 45 int ret; 46 > 47 switch (type) { 48 case hwmon_curr: 49 switch (attr) { 50 case hwmon_curr_input: 51 ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel), 52 ddata->irng[channel], val); 53 if (ret < 0) 54 return ret; 55 56 return 0; 57 default: 58 return -EOPNOTSUPP; 59 } 60 61 case hwmon_in: 62 switch (attr) { 63 case hwmon_in_input: 64 ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel), 65 ddata->mon_rng[channel], val); 66 if (ret < 0) 67 return ret; 68 return 0; 69 default: 70 return -EOPNOTSUPP; 71 } 72 } 73 return -EOPNOTSUPP; 74 } 75
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 5b3b76477b0e..164d980d9de2 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1097,6 +1097,15 @@ config SENSORS_MAX31760 This driver can also be built as a module. If so, the module will be called max31760. +config SENSORS_MAX597X + tristate "Maxim 597x power switch and monitor" + depends on I2C + depends on OF + select MFD_MAX597X + help + This driver exposes Maxim 5970/5978 voltage/current monitoring + interface. + config SENSORS_MAX6620 tristate "Maxim MAX6620 fan controller" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 88712b5031c8..720eb7d5fe46 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o obj-$(CONFIG_SENSORS_MAX31722) += max31722.o obj-$(CONFIG_SENSORS_MAX31730) += max31730.o obj-$(CONFIG_SENSORS_MAX31760) += max31760.o +obj-$(CONFIG_SENSORS_MAX597X) += max597x.o obj-$(CONFIG_SENSORS_MAX6620) += max6620.o obj-$(CONFIG_SENSORS_MAX6621) += max6621.o obj-$(CONFIG_SENSORS_MAX6639) += max6639.o diff --git a/drivers/hwmon/max597x.c b/drivers/hwmon/max597x.c new file mode 100644 index 000000000000..d4d8c2faf55c --- /dev/null +++ b/drivers/hwmon/max597x.c @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Device driver for regulators in MAX5970 and MAX5978 IC + * + * Copyright (c) 2022 9elements GmbH + * + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> + */ + +#include <linux/hwmon.h> +#include <linux/i2c.h> +#include <linux/mfd/max597x.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +struct max597x_hwmon { + int num_switches, irng[MAX5970_NUM_SWITCHES], mon_rng[MAX5970_NUM_SWITCHES]; + struct regmap *regmap; +}; + +static int max597x_read_reg(struct max597x_hwmon *ddata, int reg, int range, long *val) +{ + u8 reg_data[2]; + int ret; + + ret = regmap_bulk_read(ddata->regmap, reg, ®_data[0], 2); + if (ret < 0) + return ret; + + *val = (reg_data[0] << 2) | (reg_data[1] & 3); + *val = *val * range; + /* + * From datasheet, the range is fractionally less. + * To compensate that, divide with 1033 number. + */ + *val = *val / 1033; + + return 0; +} + +static int max597x_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct max597x_hwmon *ddata = dev_get_drvdata(dev); + int ret; + + switch (type) { + case hwmon_curr: + switch (attr) { + case hwmon_curr_input: + ret = max597x_read_reg(ddata, MAX5970_REG_CURRENT_H(channel), + ddata->irng[channel], val); + if (ret < 0) + return ret; + + return 0; + default: + return -EOPNOTSUPP; + } + + case hwmon_in: + switch (attr) { + case hwmon_in_input: + ret = max597x_read_reg(ddata, MAX5970_REG_VOLTAGE_H(channel), + ddata->mon_rng[channel], val); + if (ret < 0) + return ret; + return 0; + default: + return -EOPNOTSUPP; + } + } + return -EOPNOTSUPP; +} + +static umode_t max597x_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + struct max597x_hwmon *ddata = (struct max597x_hwmon *)data; + + if (channel >= ddata->num_switches) + return 0; + + switch (type) { + case hwmon_in: + switch (attr) { + case hwmon_in_input: + return 0444; + } + break; + case hwmon_curr: + switch (attr) { + case hwmon_curr_input: + return 0444; + } + break; + default: + break; + } + return 0; +} + +static const struct hwmon_ops max597x_hwmon_ops = { + .is_visible = max597x_is_visible, + .read = max597x_read, +}; + +#define HWMON_CURRENT HWMON_C_INPUT +#define HWMON_VOLTAGE HWMON_I_INPUT + +static const struct hwmon_channel_info *max597x_info[] = { + HWMON_CHANNEL_INFO(in, HWMON_VOLTAGE, HWMON_VOLTAGE), + HWMON_CHANNEL_INFO(curr, HWMON_CURRENT, HWMON_CURRENT), + NULL +}; + +static const struct hwmon_chip_info max597x_chip_info = { + .ops = &max597x_hwmon_ops, + .info = max597x_info, +}; + +static int max597x_adc_range(struct regmap *regmap, const int ch, + u32 *irng, u32 *mon_rng) +{ + unsigned int reg; + int ret; + + /* Decode current ADC range */ + ret = regmap_read(regmap, MAX5970_REG_STATUS2, ®); + if (ret) + return ret; + switch (MAX5970_IRNG(reg, ch)) { + case 0: + *irng = 100000; /* 100 mV */ + break; + case 1: + *irng = 50000; /* 50 mV */ + break; + case 2: + *irng = 25000; /* 25 mV */ + break; + default: + return -EINVAL; + } + + /* Decode current voltage monitor range */ + ret = regmap_read(regmap, MAX5970_REG_MON_RANGE, ®); + if (ret) + return ret; + + *mon_rng = MAX5970_MON_MAX_RANGE_UV >> MAX5970_MON(reg, ch); + *mon_rng /= 1000; /* uV to mV */ + + return 0; +} + +static int max597x_sensor_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct i2c_client *i2c = to_i2c_client(pdev->dev.parent); + struct max597x_hwmon *ddata; + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL); + struct device *hwmon_dev; + int err; + + if (!regmap) + return -EPROBE_DEFER; + + ddata = devm_kzalloc(dev, sizeof(struct max597x_hwmon), GFP_KERNEL); + if (!ddata) + return -ENOMEM; + + if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5978")) + ddata->num_switches = MAX597x_TYPE_MAX5978; + else if (of_device_is_compatible(i2c->dev.of_node, "maxim,max5970")) + ddata->num_switches = MAX597x_TYPE_MAX5970; + else + return -ENODEV; + + ddata->regmap = regmap; + + for (int i = 0; i < ddata->num_switches; i++) { + err = max597x_adc_range(regmap, i, &ddata->irng[i], &ddata->mon_rng[i]); + if (err < 0) + return err; + } + + hwmon_dev = devm_hwmon_device_register_with_info(dev, + "max597x_hwmon", ddata, + &max597x_chip_info, NULL); + + if (IS_ERR(hwmon_dev)) { + err = PTR_ERR(hwmon_dev); + dev_err(dev, "Unable to register hwmon device, returned %d\n", err); + return err; + } + + return 0; +} + +static struct platform_driver max597x_sensor_driver = { + .probe = max597x_sensor_probe, + .driver = { + .name = "max597x-hwmon", + }, +}; +module_platform_driver(max597x_sensor_driver); + +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver"); +MODULE_LICENSE("GPL v2");