Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
From:   Daniel Matyas <daniel.matyas@analog.com>
CC:     Daniel Matyas <daniel.matyas@analog.com>,
        Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        <linux-hwmon@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>
Subject: [PATCH 1/3] hwmon: max31827: Add PEC support
Date:   Thu, 14 Dec 2023 16:36:45 +0200
Message-ID: <20231214143648.175336-1-daniel.matyas@analog.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7BIT
Content-Type:   text/plain; charset=US-ASCII
To:     unlisted-recipients:; (no To-header on input)
Precedence: bulk

Removed regmap and used my functions to read, write and update bits. In
these functions i2c_smbus_ helper functions are used. These check if
there were any PEC errors during read. In the write function, if PEC is
enabled, I check for PEC Error bit, to see if there were any errors.

Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
 Documentation/hwmon/max31827.rst |  14 +-
 drivers/hwmon/max31827.c         | 219 +++++++++++++++++++++++--------
 2 files changed, 171 insertions(+), 62 deletions(-)

diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
index 44ab9dc064cb..ecbc1ddba6a7 100644
--- a/Documentation/hwmon/max31827.rst
+++ b/Documentation/hwmon/max31827.rst
@@ -131,7 +131,13 @@ The Fault Queue bits select how many consecutive temperature faults must occur
 before overtemperature or undertemperature faults are indicated in the
 corresponding status bits.
 
-Notes
------
-
-PEC is not implemented.
+PEC (packet error checking) can be enabled from the "pec" device attribute.
+If PEC is enabled, a PEC byte is appended to the end of each message transfer.
+This is a CRC-8 byte that is calculated on all of the message bytes (including
+the address/read/write byte). The last device to transmit a data byte also
+transmits the PEC byte. The master transmits the PEC byte after a write
+transaction, and the MAX31827 transmits the PEC byte after a read transaction.
+
+The read PEC error is handled inside the i2c_smbus_read_word_swapped() function.
+To check if the write had any PEC error a read is performed on the configuration
+register, to check the PEC Error bit.
\ No newline at end of file
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index 71ad3934dfb6..db93492193bd 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -11,8 +11,8 @@
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
 #include <linux/mutex.h>
-#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/of_device.h>
 
 #define MAX31827_T_REG			0x0
@@ -24,11 +24,13 @@
 
 #define MAX31827_CONFIGURATION_1SHOT_MASK	BIT(0)
 #define MAX31827_CONFIGURATION_CNV_RATE_MASK	GENMASK(3, 1)
+#define MAX31827_CONFIGURATION_PEC_EN_MASK	BIT(4)
 #define MAX31827_CONFIGURATION_TIMEOUT_MASK	BIT(5)
 #define MAX31827_CONFIGURATION_RESOLUTION_MASK	GENMASK(7, 6)
 #define MAX31827_CONFIGURATION_ALRM_POL_MASK	BIT(8)
 #define MAX31827_CONFIGURATION_COMP_INT_MASK	BIT(9)
 #define MAX31827_CONFIGURATION_FLT_Q_MASK	GENMASK(11, 10)
+#define MAX31827_CONFIGURATION_PEC_ERR_MASK	BIT(13)
 #define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK	BIT(14)
 #define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK	BIT(15)
 
@@ -94,23 +96,67 @@ struct max31827_state {
 	 * Prevent simultaneous access to the i2c client.
 	 */
 	struct mutex lock;
-	struct regmap *regmap;
 	bool enable;
 	unsigned int resolution;
 	unsigned int update_interval;
+	struct i2c_client *client;
 };
 
-static const struct regmap_config max31827_regmap = {
-	.reg_bits = 8,
-	.val_bits = 16,
-	.max_register = 0xA,
-};
+static int max31827_reg_read(struct i2c_client *client, u8 reg, u16 *val)
+{
+	u16 tmp = i2c_smbus_read_word_swapped(client, reg);
+
+	if (tmp < 0)
+		return tmp;
+
+	*val = tmp;
+	return 0;
+}
+
+static int max31827_reg_write(struct i2c_client *client, u8 reg, u16 val)
+{
+	u16 cfg;
+	int ret;
+
+	ret = i2c_smbus_write_word_swapped(client, reg, val);
+	if (ret)
+		return ret;
+
+	// If PEC is not enabled, return with success
+	if (!(client->flags & I2C_CLIENT_PEC))
+		return 0;
+
+	ret = max31827_reg_read(client, MAX31827_CONFIGURATION_REG, &cfg);
+	if (ret)
+		return ret;
+
+	if (cfg & MAX31827_CONFIGURATION_PEC_ERR_MASK)
+		return -EBADMSG;
+
+	return 0;
+}
+
+static int max31827_update_bits(struct i2c_client *client, u8 reg,
+				u16 mask, u16 val)
+{
+	u16 tmp;
+	int ret;
+
+	ret = max31827_reg_read(client, reg, &tmp);
+	if (ret)
+		return ret;
+
+	tmp = (tmp & ~mask) | (val & mask);
+	ret = max31827_reg_write(client, reg, tmp);
+
+	return ret;
+}
 
 static int shutdown_write(struct max31827_state *st, unsigned int reg,
 			  unsigned int mask, unsigned int val)
 {
-	unsigned int cfg;
-	unsigned int cnv_rate;
+	u16 cfg;
+	u16 cnv_rate;
 	int ret;
 
 	/*
@@ -125,34 +171,34 @@ static int shutdown_write(struct max31827_state *st, unsigned int reg,
 
 	if (!st->enable) {
 		if (!mask)
-			ret = regmap_write(st->regmap, reg, val);
+			ret = max31827_reg_write(st->client, reg, val);
 		else
-			ret = regmap_update_bits(st->regmap, reg, mask, val);
+			ret = max31827_update_bits(st->client, reg, mask, val);
 		goto unlock;
 	}
 
-	ret = regmap_read(st->regmap, MAX31827_CONFIGURATION_REG, &cfg);
+	ret = max31827_reg_read(st->client, MAX31827_CONFIGURATION_REG, &cfg);
 	if (ret)
 		goto unlock;
 
 	cnv_rate = MAX31827_CONFIGURATION_CNV_RATE_MASK & cfg;
 	cfg = cfg & ~(MAX31827_CONFIGURATION_1SHOT_MASK |
 		      MAX31827_CONFIGURATION_CNV_RATE_MASK);
-	ret = regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, cfg);
+	ret = max31827_reg_write(st->client, MAX31827_CONFIGURATION_REG, cfg);
 	if (ret)
 		goto unlock;
 
 	if (!mask)
-		ret = regmap_write(st->regmap, reg, val);
+		ret = max31827_reg_write(st->client, reg, val);
 	else
-		ret = regmap_update_bits(st->regmap, reg, mask, val);
+		ret = max31827_update_bits(st->client, reg, mask, val);
 
 	if (ret)
 		goto unlock;
 
-	ret = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
-				 MAX31827_CONFIGURATION_CNV_RATE_MASK,
-				 cnv_rate);
+	ret = max31827_update_bits(st->client, MAX31827_CONFIGURATION_REG,
+				   MAX31827_CONFIGURATION_CNV_RATE_MASK,
+				   cnv_rate);
 
 unlock:
 	mutex_unlock(&st->lock);
@@ -198,15 +244,16 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 			 u32 attr, int channel, long *val)
 {
 	struct max31827_state *st = dev_get_drvdata(dev);
-	unsigned int uval;
+	u16 uval;
 	int ret = 0;
 
 	switch (type) {
 	case hwmon_temp:
 		switch (attr) {
 		case hwmon_temp_enable:
-			ret = regmap_read(st->regmap,
-					  MAX31827_CONFIGURATION_REG, &uval);
+			ret = max31827_reg_read(st->client,
+						MAX31827_CONFIGURATION_REG,
+						&uval);
 			if (ret)
 				break;
 
@@ -226,10 +273,10 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 				 * be changed during the conversion process.
 				 */
 
-				ret = regmap_update_bits(st->regmap,
-							 MAX31827_CONFIGURATION_REG,
-							 MAX31827_CONFIGURATION_1SHOT_MASK,
-							 1);
+				ret = max31827_update_bits(st->client,
+							   MAX31827_CONFIGURATION_REG,
+							   MAX31827_CONFIGURATION_1SHOT_MASK,
+							   1);
 				if (ret) {
 					mutex_unlock(&st->lock);
 					return ret;
@@ -246,7 +293,8 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 			    st->update_interval == 125)
 				usleep_range(15000, 20000);
 
-			ret = regmap_read(st->regmap, MAX31827_T_REG, &uval);
+			ret = max31827_reg_read(st->client, MAX31827_T_REG,
+						&uval);
 
 			mutex_unlock(&st->lock);
 
@@ -257,23 +305,26 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 
 			break;
 		case hwmon_temp_max:
-			ret = regmap_read(st->regmap, MAX31827_TH_REG, &uval);
+			ret = max31827_reg_read(st->client, MAX31827_TH_REG,
+						&uval);
 			if (ret)
 				break;
 
 			*val = MAX31827_16_BIT_TO_M_DGR(uval);
 			break;
 		case hwmon_temp_max_hyst:
-			ret = regmap_read(st->regmap, MAX31827_TH_HYST_REG,
-					  &uval);
+			ret = max31827_reg_read(st->client,
+						MAX31827_TH_HYST_REG,
+						&uval);
 			if (ret)
 				break;
 
 			*val = MAX31827_16_BIT_TO_M_DGR(uval);
 			break;
 		case hwmon_temp_max_alarm:
-			ret = regmap_read(st->regmap,
-					  MAX31827_CONFIGURATION_REG, &uval);
+			ret = max31827_reg_read(st->client,
+						MAX31827_CONFIGURATION_REG,
+						&uval);
 			if (ret)
 				break;
 
@@ -281,23 +332,25 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 					 uval);
 			break;
 		case hwmon_temp_min:
-			ret = regmap_read(st->regmap, MAX31827_TL_REG, &uval);
+			ret = max31827_reg_read(st->client, MAX31827_TL_REG,
+						&uval);
 			if (ret)
 				break;
 
 			*val = MAX31827_16_BIT_TO_M_DGR(uval);
 			break;
 		case hwmon_temp_min_hyst:
-			ret = regmap_read(st->regmap, MAX31827_TL_HYST_REG,
-					  &uval);
+			ret = max31827_reg_read(st->client, MAX31827_TL_HYST_REG,
+						&uval);
 			if (ret)
 				break;
 
 			*val = MAX31827_16_BIT_TO_M_DGR(uval);
 			break;
 		case hwmon_temp_min_alarm:
-			ret = regmap_read(st->regmap,
-					  MAX31827_CONFIGURATION_REG, &uval);
+			ret = max31827_reg_read(st->client,
+						MAX31827_CONFIGURATION_REG,
+						&uval);
 			if (ret)
 				break;
 
@@ -313,8 +366,9 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 
 	case hwmon_chip:
 		if (attr == hwmon_chip_update_interval) {
-			ret = regmap_read(st->regmap,
-					  MAX31827_CONFIGURATION_REG, &uval);
+			ret = max31827_reg_read(st->client,
+						MAX31827_CONFIGURATION_REG,
+						&uval);
 			if (ret)
 				break;
 
@@ -355,11 +409,11 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
 
 			st->enable = val;
 
-			ret = regmap_update_bits(st->regmap,
-						 MAX31827_CONFIGURATION_REG,
-						 MAX31827_CONFIGURATION_1SHOT_MASK |
-						 MAX31827_CONFIGURATION_CNV_RATE_MASK,
-						 MAX31827_DEVICE_ENABLE(val));
+			ret = max31827_update_bits(st->client,
+						   MAX31827_CONFIGURATION_REG,
+						   MAX31827_CONFIGURATION_1SHOT_MASK |
+						   MAX31827_CONFIGURATION_CNV_RATE_MASK,
+						   MAX31827_DEVICE_ENABLE(val));
 
 			mutex_unlock(&st->lock);
 
@@ -402,10 +456,10 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
 			res = FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
 					 res);
 
-			ret = regmap_update_bits(st->regmap,
-						 MAX31827_CONFIGURATION_REG,
-						 MAX31827_CONFIGURATION_CNV_RATE_MASK,
-						 res);
+			ret = max31827_update_bits(st->client,
+						   MAX31827_CONFIGURATION_REG,
+						   MAX31827_CONFIGURATION_CNV_RATE_MASK,
+						   res);
 			if (ret)
 				return ret;
 
@@ -425,10 +479,10 @@ static ssize_t temp1_resolution_show(struct device *dev,
 				     char *buf)
 {
 	struct max31827_state *st = dev_get_drvdata(dev);
-	unsigned int val;
+	u16 val;
 	int ret;
 
-	ret = regmap_read(st->regmap, MAX31827_CONFIGURATION_REG, &val);
+	ret = max31827_reg_read(st->client, MAX31827_CONFIGURATION_REG, &val);
 	if (ret)
 		return ret;
 
@@ -473,10 +527,63 @@ static ssize_t temp1_resolution_store(struct device *dev,
 	return ret ? ret : count;
 }
 
+static ssize_t pec_show(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct max31827_state *st = dev_get_drvdata(dev);
+	struct i2c_client *client = st->client;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", !!(client->flags & I2C_CLIENT_PEC));
+}
+
+static ssize_t pec_store(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct max31827_state *st = dev_get_drvdata(dev);
+	struct i2c_client *client = st->client;
+	unsigned int val;
+	u16 val2;
+	int err;
+
+	err = kstrtouint(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	val2 = FIELD_PREP(MAX31827_CONFIGURATION_PEC_EN_MASK, val);
+
+	if (err)
+		return err;
+
+	switch (val) {
+	case 0:
+		client->flags &= ~I2C_CLIENT_PEC;
+		err = max31827_update_bits(client, MAX31827_CONFIGURATION_REG,
+					   MAX31827_CONFIGURATION_PEC_EN_MASK,
+					   val2);
+		if (err)
+			return err;
+		break;
+	case 1:
+		err = max31827_update_bits(client, MAX31827_CONFIGURATION_REG,
+					   MAX31827_CONFIGURATION_PEC_EN_MASK,
+					   val2);
+		if (err)
+			return err;
+		client->flags |= I2C_CLIENT_PEC;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return count;
+}
+
 static DEVICE_ATTR_RW(temp1_resolution);
+static DEVICE_ATTR_RW(pec);
 
 static struct attribute *max31827_attrs[] = {
 	&dev_attr_temp1_resolution.attr,
+	&dev_attr_pec.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(max31827);
@@ -489,9 +596,9 @@ static const struct i2c_device_id max31827_i2c_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
 
-static int max31827_init_client(struct max31827_state *st,
-				struct device *dev)
+static int max31827_init_client(struct max31827_state *st)
 {
+	struct device *dev = &st->client->dev;
 	struct fwnode_handle *fwnode;
 	unsigned int res = 0;
 	u32 data, lsb_idx;
@@ -575,7 +682,7 @@ static int max31827_init_client(struct max31827_state *st,
 		}
 	}
 
-	return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
+	return max31827_reg_write(st->client, MAX31827_CONFIGURATION_REG, res);
 }
 
 static const struct hwmon_channel_info *max31827_info[] = {
@@ -613,17 +720,13 @@ static int max31827_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	mutex_init(&st->lock);
-
-	st->regmap = devm_regmap_init_i2c(client, &max31827_regmap);
-	if (IS_ERR(st->regmap))
-		return dev_err_probe(dev, PTR_ERR(st->regmap),
-				     "Failed to allocate regmap.\n");
+	st->client = client;
 
 	err = devm_regulator_get_enable(dev, "vref");
 	if (err)
 		return dev_err_probe(dev, err, "failed to enable regulator\n");
 
-	err = max31827_init_client(st, dev);
+	err = max31827_init_client(st);
 	if (err)
 		return err;
 
-- 
2.34.1