diff mbox series

[v2,2/6] hwmon: (max6697) Drop platform data support

Message ID 20240723154447.2669995-3-linux@roeck-us.net (mailing list archive)
State Accepted
Headers show
Series hwmon: (max6697) Cleanup, use regmap and with_info API | expand

Commit Message

Guenter Roeck July 23, 2024, 3:44 p.m. UTC 
Platform data is not used anywhere in the upstram kernel. Drop support
for it to simplify code maintenance.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Drop unnecessary data->chip dereference
    Fix resistance-cancellation devicetree configuration

 drivers/hwmon/max6697.c               | 173 ++++++++++++--------------
 include/linux/platform_data/max6697.h |  33 -----
 2 files changed, 80 insertions(+), 126 deletions(-)
 delete mode 100644 include/linux/platform_data/max6697.h

Comments

Tzung-Bi Shih July 24, 2024, 1:03 p.m. UTC | #1 
On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
> +	if (data->type != max6581) {
> +		if (of_property_read_bool(node, "resistance-cancellation") &&
> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
> +			confreg |= MAX6697_CONF_RESISTANCE;
> +			factor = 1;
> +		}
> +	} else {
> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
> +			if (of_property_read_bool(node, "resistance-cancellation"))
> +				vals[0] = 0xfe;
> +			else
> +				vals[0] = 0;
> +		}
> +
> +		factor = hweight8(vals[0] & 0xfe);

It doesn't AND with 0xfe originally.
Guenter Roeck July 24, 2024, 2:25 p.m. UTC | #2 
On 7/24/24 06:03, Tzung-Bi Shih wrote:
> On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
>> +	if (data->type != max6581) {
>> +		if (of_property_read_bool(node, "resistance-cancellation") &&
>> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
>> +			confreg |= MAX6697_CONF_RESISTANCE;
>> +			factor = 1;
>> +		}
>> +	} else {
>> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
>> +			if (of_property_read_bool(node, "resistance-cancellation"))
>> +				vals[0] = 0xfe;
>> +			else
>> +				vals[0] = 0;
>> +		}
>> +
>> +		factor = hweight8(vals[0] & 0xfe);
> 
> It doesn't AND with 0xfe originally.
> 

Yes, but the original code uses the value in
	factor += hweight8(pdata->resistance_cancellation >> 1);
	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
                                         pdata->resistance_cancellation >> 1);
which is effectively the same. I can't just use
	factor = hweight8(vals[0] >> 1);
because, unlike resistance_cancellation, val[] is an u32 array which would
not auto-mask the upper bits.

Thanks,
Guenter
Tzung-Bi Shih July 24, 2024, 3:41 p.m. UTC | #3 
On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
> On 7/24/24 06:03, Tzung-Bi Shih wrote:
> > On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
> > > +	if (data->type != max6581) {
> > > +		if (of_property_read_bool(node, "resistance-cancellation") &&
> > > +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
> > > +			confreg |= MAX6697_CONF_RESISTANCE;
> > > +			factor = 1;
> > > +		}
> > > +	} else {
> > > +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
> > > +			if (of_property_read_bool(node, "resistance-cancellation"))
> > > +				vals[0] = 0xfe;
> > > +			else
> > > +				vals[0] = 0;
> > > +		}
> > > +
> > > +		factor = hweight8(vals[0] & 0xfe);
> > 
> > It doesn't AND with 0xfe originally.
> > 
> 
> Yes, but the original code uses the value in
> 	factor += hweight8(pdata->resistance_cancellation >> 1);
> 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
>                                         pdata->resistance_cancellation >> 1);
> which is effectively the same. I can't just use
> 	factor = hweight8(vals[0] >> 1);
> because, unlike resistance_cancellation, val[] is an u32 array which would
> not auto-mask the upper bits.

If you are worrying about:
* MSB, it should be fine as it should only prepend 0s for right shift on
  unsigned.
* BIT(8), other `val[0] >> 1` should also share the same concern.
Guenter Roeck July 24, 2024, 5:40 p.m. UTC | #4 
On 7/24/24 08:41, Tzung-Bi Shih wrote:
> On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
>> On 7/24/24 06:03, Tzung-Bi Shih wrote:
>>> On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
>>>> +	if (data->type != max6581) {
>>>> +		if (of_property_read_bool(node, "resistance-cancellation") &&
>>>> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
>>>> +			confreg |= MAX6697_CONF_RESISTANCE;
>>>> +			factor = 1;
>>>> +		}
>>>> +	} else {
>>>> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
>>>> +			if (of_property_read_bool(node, "resistance-cancellation"))
>>>> +				vals[0] = 0xfe;
>>>> +			else
>>>> +				vals[0] = 0;
>>>> +		}
>>>> +
>>>> +		factor = hweight8(vals[0] & 0xfe);
>>>
>>> It doesn't AND with 0xfe originally.
>>>
>>
>> Yes, but the original code uses the value in
>> 	factor += hweight8(pdata->resistance_cancellation >> 1);
>> 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
>>                                          pdata->resistance_cancellation >> 1);
>> which is effectively the same. I can't just use
>> 	factor = hweight8(vals[0] >> 1);
>> because, unlike resistance_cancellation, val[] is an u32 array which would
>> not auto-mask the upper bits.
> 
> If you are worrying about:
> * MSB, it should be fine as it should only prepend 0s for right shift on
>    unsigned.
> * BIT(8), other `val[0] >> 1` should also share the same concern.

BIT(8) is the concern. Yes, you are correct, I'll change the code to

	val[0] &= 0xfe;
	factor = hweight8(vals[0]);
	...

In practice it doesn't matter since bit 7 isn't used by the chip,
but that isn't a reason for the bad code.

Thanks a lot for noticing!

Guenter
Tzung-Bi Shih July 25, 2024, 12:54 a.m. UTC | #5 
On Wed, Jul 24, 2024 at 10:40:20AM -0700, Guenter Roeck wrote:
> On 7/24/24 08:41, Tzung-Bi Shih wrote:
> > On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
> > > On 7/24/24 06:03, Tzung-Bi Shih wrote:
> > > > On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
> > > > > +	if (data->type != max6581) {
> > > > > +		if (of_property_read_bool(node, "resistance-cancellation") &&
> > > > > +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
> > > > > +			confreg |= MAX6697_CONF_RESISTANCE;
> > > > > +			factor = 1;
> > > > > +		}
> > > > > +	} else {
> > > > > +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
> > > > > +			if (of_property_read_bool(node, "resistance-cancellation"))
> > > > > +				vals[0] = 0xfe;
> > > > > +			else
> > > > > +				vals[0] = 0;
> > > > > +		}
> > > > > +
> > > > > +		factor = hweight8(vals[0] & 0xfe);
> > > > 
> > > > It doesn't AND with 0xfe originally.
> > > > 
> > > 
> > > Yes, but the original code uses the value in
> > > 	factor += hweight8(pdata->resistance_cancellation >> 1);
> > > 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
> > >                                          pdata->resistance_cancellation >> 1);
> > > which is effectively the same. I can't just use
> > > 	factor = hweight8(vals[0] >> 1);
> > > because, unlike resistance_cancellation, val[] is an u32 array which would
> > > not auto-mask the upper bits.
> > 
> > If you are worrying about:
> > * MSB, it should be fine as it should only prepend 0s for right shift on
> >    unsigned.
> > * BIT(8), other `val[0] >> 1` should also share the same concern.
> 
> BIT(8) is the concern. Yes, you are correct, I'll change the code to
> 
> 	val[0] &= 0xfe;
> 	factor = hweight8(vals[0]);
> 	...

With that,
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

Note: there is another `vals[0] >> 1` for MAX6581_REG_IDEALITY_SELECT.
Guenter Roeck July 25, 2024, 5:52 a.m. UTC | #6 
On 7/24/24 17:54, Tzung-Bi Shih wrote:
> On Wed, Jul 24, 2024 at 10:40:20AM -0700, Guenter Roeck wrote:
>> On 7/24/24 08:41, Tzung-Bi Shih wrote:
>>> On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
>>>> On 7/24/24 06:03, Tzung-Bi Shih wrote:
>>>>> On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
>>>>>> +	if (data->type != max6581) {
>>>>>> +		if (of_property_read_bool(node, "resistance-cancellation") &&
>>>>>> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
>>>>>> +			confreg |= MAX6697_CONF_RESISTANCE;
>>>>>> +			factor = 1;
>>>>>> +		}
>>>>>> +	} else {
>>>>>> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
>>>>>> +			if (of_property_read_bool(node, "resistance-cancellation"))
>>>>>> +				vals[0] = 0xfe;
>>>>>> +			else
>>>>>> +				vals[0] = 0;
>>>>>> +		}
>>>>>> +
>>>>>> +		factor = hweight8(vals[0] & 0xfe);
>>>>>
>>>>> It doesn't AND with 0xfe originally.
>>>>>
>>>>
>>>> Yes, but the original code uses the value in
>>>> 	factor += hweight8(pdata->resistance_cancellation >> 1);
>>>> 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
>>>>                                           pdata->resistance_cancellation >> 1);
>>>> which is effectively the same. I can't just use
>>>> 	factor = hweight8(vals[0] >> 1);
>>>> because, unlike resistance_cancellation, val[] is an u32 array which would
>>>> not auto-mask the upper bits.
>>>
>>> If you are worrying about:
>>> * MSB, it should be fine as it should only prepend 0s for right shift on
>>>     unsigned.
>>> * BIT(8), other `val[0] >> 1` should also share the same concern.
>>
>> BIT(8) is the concern. Yes, you are correct, I'll change the code to
>>
>> 	val[0] &= 0xfe;
>> 	factor = hweight8(vals[0]);
>> 	...
> 
> With that,
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> 
> Note: there is another `vals[0] >> 1` for MAX6581_REG_IDEALITY_SELECT.

Good point. I masked that as well.

Thanks a lot for the reviews,
Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
index 9a2a21230c7d..62b03c79c039 100644
--- a/drivers/hwmon/max6697.c
+++ b/drivers/hwmon/max6697.c
@@ -17,8 +17,6 @@ 
 #include <linux/of.h>
 #include <linux/slab.h>
 
-#include <linux/platform_data/max6697.h>
-
 enum chips { max6581, max6602, max6622, max6636, max6689, max6693, max6694,
 	     max6697, max6698, max6699 };
 
@@ -558,54 +556,96 @@  static const struct attribute_group max6697_group = {
 };
 __ATTRIBUTE_GROUPS(max6697);
 
-static void max6697_get_config_of(struct device_node *node,
-				  struct max6697_platform_data *pdata)
+static int max6697_config_of(struct max6697_data *data, struct i2c_client *client)
 {
-	int len;
-	const __be32 *prop;
+	const struct max6697_chip_data *chip = data->chip;
+	struct device_node *node = client->dev.of_node;
+	int ret, confreg;
+	int factor = 0;
+	u32 vals[2];
 
-	pdata->smbus_timeout_disable =
-		of_property_read_bool(node, "smbus-timeout-disable");
-	pdata->extended_range_enable =
-		of_property_read_bool(node, "extended-range-enable");
-	pdata->beta_compensation =
-		of_property_read_bool(node, "beta-compensation-enable");
+	confreg = 0;
+	if (of_property_read_bool(node, "smbus-timeout-disable") &&
+	    (chip->valid_conf & MAX6697_CONF_TIMEOUT)) {
+		confreg |= MAX6697_CONF_TIMEOUT;
+	}
+	if (of_property_read_bool(node, "extended-range-enable") &&
+	    (chip->valid_conf & MAX6581_CONF_EXTENDED)) {
+		confreg |= MAX6581_CONF_EXTENDED;
+		data->temp_offset = 64;
+	}
+	if (of_property_read_bool(node, "beta-compensation-enable") &&
+	    (chip->valid_conf & MAX6693_CONF_BETA)) {
+		confreg |= MAX6693_CONF_BETA;
+	}
 
-	prop = of_get_property(node, "alert-mask", &len);
-	if (prop && len == sizeof(u32))
-		pdata->alert_mask = be32_to_cpu(prop[0]);
-	prop = of_get_property(node, "over-temperature-mask", &len);
-	if (prop && len == sizeof(u32))
-		pdata->over_temperature_mask = be32_to_cpu(prop[0]);
-	prop = of_get_property(node, "resistance-cancellation", &len);
-	if (prop) {
-		if (len == sizeof(u32))
-			pdata->resistance_cancellation = be32_to_cpu(prop[0]);
-		else
-			pdata->resistance_cancellation = 0xfe;
-	}
-	prop = of_get_property(node, "transistor-ideality", &len);
-	if (prop && len == 2 * sizeof(u32)) {
-			pdata->ideality_mask = be32_to_cpu(prop[0]);
-			pdata->ideality_value = be32_to_cpu(prop[1]);
+	if (of_property_read_u32(node, "alert-mask", vals))
+		vals[0] = 0;
+	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_ALERT_MASK,
+					MAX6697_ALERT_MAP_BITS(vals[0]));
+	if (ret)
+		return ret;
+
+	if (of_property_read_u32(node, "over-temperature-mask", vals))
+		vals[0] = 0;
+	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_OVERT_MASK,
+					MAX6697_OVERT_MAP_BITS(vals[0]));
+	if (ret)
+		return ret;
+
+	if (data->type != max6581) {
+		if (of_property_read_bool(node, "resistance-cancellation") &&
+		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
+			confreg |= MAX6697_CONF_RESISTANCE;
+			factor = 1;
+		}
+	} else {
+		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
+			if (of_property_read_bool(node, "resistance-cancellation"))
+				vals[0] = 0xfe;
+			else
+				vals[0] = 0;
+		}
+
+		factor = hweight8(vals[0] & 0xfe);
+		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
+						vals[0] >> 1);
+		if (ret < 0)
+			return ret;
+
+		if (of_property_read_u32_array(node, "transistor-ideality", vals, 2)) {
+			vals[0] = 0;
+			vals[1] = 0;
+		}
+
+		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_IDEALITY,
+						vals[1]);
+		if (ret < 0)
+			return ret;
+		ret = i2c_smbus_write_byte_data(client,
+						MAX6581_REG_IDEALITY_SELECT,
+						vals[0] >> 1);
+		if (ret < 0)
+			return ret;
 	}
+	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_CONFIG, confreg);
+	if (ret < 0)
+		return ret;
+	return factor;
 }
 
 static int max6697_init_chip(struct max6697_data *data,
 			     struct i2c_client *client)
 {
-	struct max6697_platform_data *pdata = dev_get_platdata(&client->dev);
-	struct max6697_platform_data p;
 	const struct max6697_chip_data *chip = data->chip;
 	int factor = chip->channels;
 	int ret, reg;
 
 	/*
-	 * Don't touch configuration if neither platform data nor OF
-	 * configuration was specified. If that is the case, use the
-	 * current chip configuration.
+	 * Don't touch configuration if there is no devicetree configuration.
+	 * If that is the case, use the current chip configuration.
 	 */
-	if (!pdata && !client->dev.of_node) {
+	if (!client->dev.of_node) {
 		reg = i2c_smbus_read_byte_data(client, MAX6697_REG_CONFIG);
 		if (reg < 0)
 			return reg;
@@ -621,67 +661,14 @@  static int max6697_init_chip(struct max6697_data *data,
 			if (reg & MAX6697_CONF_RESISTANCE)
 				factor++;
 		}
-		goto done;
-	}
-
-	if (client->dev.of_node) {
-		memset(&p, 0, sizeof(p));
-		max6697_get_config_of(client->dev.of_node, &p);
-		pdata = &p;
-	}
-
-	reg = 0;
-	if (pdata->smbus_timeout_disable &&
-	    (chip->valid_conf & MAX6697_CONF_TIMEOUT)) {
-		reg |= MAX6697_CONF_TIMEOUT;
-	}
-	if (pdata->extended_range_enable &&
-	    (chip->valid_conf & MAX6581_CONF_EXTENDED)) {
-		reg |= MAX6581_CONF_EXTENDED;
-		data->temp_offset = 64;
-	}
-	if (pdata->resistance_cancellation &&
-	    (chip->valid_conf & MAX6697_CONF_RESISTANCE)) {
-		reg |= MAX6697_CONF_RESISTANCE;
-		factor++;
-	}
-	if (pdata->beta_compensation &&
-	    (chip->valid_conf & MAX6693_CONF_BETA)) {
-		reg |= MAX6693_CONF_BETA;
-	}
-
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_CONFIG, reg);
-	if (ret < 0)
-		return ret;
-
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_ALERT_MASK,
-				MAX6697_ALERT_MAP_BITS(pdata->alert_mask));
-	if (ret < 0)
-		return ret;
-
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_OVERT_MASK,
-			MAX6697_OVERT_MAP_BITS(pdata->over_temperature_mask));
-	if (ret < 0)
-		return ret;
-
-	if (data->type == max6581) {
-		factor += hweight8(pdata->resistance_cancellation >> 1);
-		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
-					pdata->resistance_cancellation >> 1);
-		if (ret < 0)
-			return ret;
-		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_IDEALITY,
-						pdata->ideality_value);
-		if (ret < 0)
-			return ret;
-		ret = i2c_smbus_write_byte_data(client,
-						MAX6581_REG_IDEALITY_SELECT,
-						pdata->ideality_mask >> 1);
+		data->update_interval = factor * MAX6697_CONV_TIME;
+	} else {
+		ret = max6697_config_of(data, client);
 		if (ret < 0)
 			return ret;
+		data->update_interval = (factor + ret) * MAX6697_CONV_TIME;
 	}
-done:
-	data->update_interval = factor * MAX6697_CONV_TIME;
+
 	return 0;
 }
 
diff --git a/include/linux/platform_data/max6697.h b/include/linux/platform_data/max6697.h
deleted file mode 100644
index 6fbb70005541..000000000000
--- a/include/linux/platform_data/max6697.h
+++ /dev/null
@@ -1,33 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * max6697.h
- *     Copyright (c) 2012 Guenter Roeck <linux@roeck-us.net>
- */
-
-#ifndef MAX6697_H
-#define MAX6697_H
-
-#include <linux/types.h>
-
-/*
- * For all bit masks:
- * bit 0:    local temperature
- * bit 1..7: remote temperatures
- */
-struct max6697_platform_data {
-	bool smbus_timeout_disable;	/* set to disable SMBus timeouts */
-	bool extended_range_enable;	/* set to enable extended temp range */
-	bool beta_compensation;		/* set to enable beta compensation */
-	u8 alert_mask;			/* set bit to 1 to disable alert */
-	u8 over_temperature_mask;	/* set bit to 1 to disable */
-	u8 resistance_cancellation;	/* set bit to 0 to disable
-					 * bit mask for MAX6581,
-					 * boolean for other chips
-					 */
-	u8 ideality_mask;		/* set bit to 0 to disable */
-	u8 ideality_value;		/* transistor ideality as per
-					 * MAX6581 datasheet
-					 */
-};
-
-#endif /* MAX6697_H */