[lm-sensors] [RFC][PATCH] hwmon/max6650.c: Optionally retrieve voltage and prescaler from devicetree

Discussion:

Mike Looijmans

2016-08-09 07:30:27 UTC

Parse devicetree parameters for voltage and prescaler setting. This allows
using multiple max6550 devices with varying settings, and also makes it
possible to instantiate and configure the device using devicetree.

Signed-off-by: Mike Looijmans <***@topic.nl>
---

This patch just does the minimal to get the device working properly with devicetree config.
What is the better path here, move to devicetree completely and remove the module parameters,
or do we need to keep backward compatibility with the moduel parameter (allowing only one
config for all chips)?
(in case of DT-only, the "clock" value could be obtained using the clock framework)

arch/arm/boot/dts/topic-miamiplus.dtsi | 1 +
drivers/hwmon/max6650.c | 18 ++++++++++++++----
2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/topic-miamiplus.dtsi b/arch/arm/boot/dts/topic-miamiplus.dtsi
index d433101..e927ff9 100644
--- a/arch/arm/boot/dts/topic-miamiplus.dtsi
+++ b/arch/arm/boot/dts/topic-miamiplus.dtsi
@@ -56,6 +56,7 @@
reg = <0x1b>; /* ADD pins high-Z, hence address 0011011b */
compatible = "max6650";
voltage = <12>;
+ prescaler = <8>;
};
};

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 162a520..c190954 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -566,6 +566,15 @@ static int max6650_init_client(struct max6650_data *data,
struct device *dev = &client->dev;
int config;
int err = -EIO;
+ u32 local_fan_voltage = (u32)fan_voltage;
+ u32 local_prescaler = (u32)prescaler;
+
+#ifdef CONFIG_OF
+ of_property_read_u32(client->dev.of_node,
+ "voltage", &local_fan_voltage);
+ of_property_read_u32(client->dev.of_node,
+ "prescaler", &local_prescaler);
+#endif

config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);

@@ -574,7 +583,7 @@ static int max6650_init_client(struct max6650_data *data,
return err;
}

- switch (fan_voltage) {
+ switch (local_fan_voltage) {
case 0:
break;
case 5:
@@ -585,13 +594,13 @@ static int max6650_init_client(struct max6650_data *data,
break;
default:
dev_err(dev, "illegal value for fan_voltage (%d)\n",
- fan_voltage);
+ local_fan_voltage);
}

dev_info(dev, "Fan voltage is set to %dV.\n",
(config & MAX6650_CFG_V12) ? 12 : 5);

- switch (prescaler) {
+ switch (local_prescaler) {
case 0:
break;
case 1:
@@ -614,7 +623,8 @@ static int max6650_init_client(struct max6650_data *data,
| MAX6650_CFG_PRESCALER_16;
break;
default:
- dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
+ dev_err(dev, "illegal value for prescaler (%d)\n",
+ local_prescaler);
}

dev_info(dev, "Prescaler is set to %d.\n",

--
1.9.1

Mike Looijmans

2016-08-09 07:52:48 UTC

Permalink

with

Post by Mike Looijmans
devicetree config.
What is the better path here, move to devicetree completely and

remove the

Post by Mike Looijmans
module parameters,
or do we need to keep backward compatibility with the moduel

parameter

Post by Mike Looijmans
(allowing only one
config for all chips)?
(in case of DT-only, the "clock" value could be obtained using the

clock

Post by Mike Looijmans
framework)
arch/arm/boot/dts/topic-miamiplus.dtsi | 1 +
drivers/hwmon/max6650.c | 18 ++++++++++++++----
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/dts/topic-miamiplus.dtsi
b/arch/arm/boot/dts/topic-miamiplus.dtsi
index d433101..e927ff9 100644
--- a/arch/arm/boot/dts/topic-miamiplus.dtsi
+++ b/arch/arm/boot/dts/topic-miamiplus.dtsi
@@ -56,6 +56,7 @@
reg = <0x1b>; /* ADD pins high-Z, hence address 0011011b */
compatible = "max6650";
voltage = <12>;
+ prescaler = <8>;
};
};

Sorry, this part shouldn't have been in the patch!

Post by Mike Looijmans
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 162a520..c190954 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -566,6 +566,15 @@ static int max6650_init_client(struct

max6650_data *data,

Post by Mike Looijmans
struct device *dev = &client->dev;
int config;
int err = -EIO;
+ u32 local_fan_voltage = (u32)fan_voltage;
+ u32 local_prescaler = (u32)prescaler;
+
+#ifdef CONFIG_OF
+ of_property_read_u32(client->dev.of_node,
+ "voltage", &local_fan_voltage);
+ of_property_read_u32(client->dev.of_node,
+ "prescaler", &local_prescaler);
+#endif
config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
@@ -574,7 +583,7 @@ static int max6650_init_client(struct

max6650_data *data,

Post by Mike Looijmans
return err;
}
- switch (fan_voltage) {
+ switch (local_fan_voltage) {
break;
@@ -585,13 +594,13 @@ static int max6650_init_client(struct

max6650_data *data,

Post by Mike Looijmans
break;
dev_err(dev, "illegal value for fan_voltage (%d)\n",
- fan_voltage);
+ local_fan_voltage);
}
dev_info(dev, "Fan voltage is set to %dV.\n",
(config & MAX6650_CFG_V12) ? 12 : 5);
- switch (prescaler) {
+ switch (local_prescaler) {
break;
@@ -614,7 +623,8 @@ static int max6650_init_client(struct

max6650_data *data,

Post by Mike Looijmans
| MAX6650_CFG_PRESCALER_16;
break;
- dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
+ dev_err(dev, "illegal value for prescaler (%d)\n",
+ local_prescaler);
}
dev_info(dev, "Prescaler is set to %d.\n",
--
1.9.1
Kind regards,
Mike Looijmans
System Expert
*TOPIC Products*
Materiaalweg 4
5681 RJ Best
+31 (0) 499 33 69 69
Postbus 440
5680 AK Best
www.topicproducts.com <http://www.topicproducts.com>
The Netherlands

<https://www.facebook.com/TopicProducts><https://twitter.com/TopicProdu
cts><https://www.linkedin.com/company/topic-embedded-products>

Post by Mike Looijmans
Please consider the environment before printing this e-mail
Topic zoekt gedreven (embedded) software specialisten!
<http://topic.nl/vacancy/topic-zoekt-technische-software-engineers/>

Kind regards,

Mike Looijmans

System Expert

[cid:***@75fc36a7.42abb6a5]

TOPIC Products

Materiaalweg 4

5681 RJ Best

T:

+31 (0) 499 33 69 69

Postbus 440

E:

***@topicproducts.com

5680 AK Best

W:

[1]www.topicproducts.com
The Netherlands

[2][cid:***@f0d6511e.41b1bb4a]
[3][cid:***@4027c0fd.4f82d4c9]
[4][cid:***@20a8dbe1.41be85a6]
Please consider the environment before printing this e-mail
[5]Topic zoekt gedreven (embedded) software specialisten!

References

1. http://www.topicproducts.com/
2. https://www.facebook.com/TopicProducts
3. https://twitter.com/TopicProducts
4. https://www.linkedin.com/company/topic-embedded-products
5. http://topic.nl/vacancy/topic-zoekt-technische-software-engineers/

Guenter Roeck

2016-08-09 16:37:27 UTC

Permalink

Post by Mike Looijmans
Parse devicetree parameters for voltage and prescaler setting. This allows
using multiple max6550 devices with varying settings, and also makes it
possible to instantiate and configure the device using devicetree.
---
This patch just does the minimal to get the device working properly with devicetree config.
What is the better path here, move to devicetree completely and remove the module parameters,
or do we need to keep backward compatibility with the moduel parameter (allowing only one
config for all chips)?

I am relatively sure that no one is using the module parameters, but we'll have
to keep them for compatibility.

Properties will neeed to be described in
Documentation/devicetree/bindings/hwmon/max6650.

Post by Mike Looijmans
(in case of DT-only, the "clock" value could be obtained using the clock framework)

Yes, that should be done

Post by Mike Looijmans
arch/arm/boot/dts/topic-miamiplus.dtsi | 1 +
drivers/hwmon/max6650.c | 18 ++++++++++++++----
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/dts/topic-miamiplus.dtsi b/arch/arm/boot/dts/topic-miamiplus.dtsi
index d433101..e927ff9 100644
--- a/arch/arm/boot/dts/topic-miamiplus.dtsi
+++ b/arch/arm/boot/dts/topic-miamiplus.dtsi

This file is neither upstream nor in -next.

Post by Mike Looijmans
@@ -56,6 +56,7 @@
reg = <0x1b>; /* ADD pins high-Z, hence address 0011011b */
compatible = "max6650";
voltage = <12>;

Should probably be something like fan-volts (or fan-volt), though that will
need to be confirmed by DT maintainers.

Post by Mike Looijmans
+ prescaler = <8>;

Probably better something like fan-prescale, unless there is a generic
clock property that can be used. Needs feedback from DT maintainers.

Post by Mike Looijmans
};
};
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 162a520..c190954 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -566,6 +566,15 @@ static int max6650_init_client(struct max6650_data *data,
struct device *dev = &client->dev;
int config;
int err = -EIO;
+ u32 local_fan_voltage = (u32)fan_voltage;
+ u32 local_prescaler = (u32)prescaler;

Please use shorter variable names. voltage and prescale, maybe.

Post by Mike Looijmans
+
+#ifdef CONFIG_OF

ifdef not needed.

Post by Mike Looijmans
+ of_property_read_u32(client->dev.of_node,
+ "voltage", &local_fan_voltage);

Better something like
if (of_property_read_u32(client->dev.of_node, "fan-volts",
&voltage))
voltage = fan_voltage;

(typecast not needed).

Post by Mike Looijmans
+ of_property_read_u32(client->dev.of_node,
+ "prescaler", &local_prescaler);
+#endif
config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
@@ -574,7 +583,7 @@ static int max6650_init_client(struct max6650_data *data,
return err;
}
- switch (fan_voltage) {
+ switch (local_fan_voltage) {
break;
@@ -585,13 +594,13 @@ static int max6650_init_client(struct max6650_data *data,
break;
dev_err(dev, "illegal value for fan_voltage (%d)\n",
- fan_voltage);
+ local_fan_voltage);
}
dev_info(dev, "Fan voltage is set to %dV.\n",
(config & MAX6650_CFG_V12) ? 12 : 5);
- switch (prescaler) {
+ switch (local_prescaler) {
break;
@@ -614,7 +623,8 @@ static int max6650_init_client(struct max6650_data *data,
| MAX6650_CFG_PRESCALER_16;
break;
- dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
+ dev_err(dev, "illegal value for prescaler (%d)\n",
+ local_prescaler);
}
dev_info(dev, "Prescaler is set to %d.\n",
--
1.9.1

Mike Looijmans

2016-08-10 07:06:15 UTC

Permalink

Thank you very much for your feedback, much appreciated.
Comments inline. No comment from my side means I totally agree and will
integrate it into a v2 patch.
Once I have the userspace software in place, I'll probably want to add
some
extra features like hooking up the alarm to an IRQ. Currently the
driver
doesn't use the gpio pins. That'll be a separate patch.

Post by Guenter Roeck

Post by Mike Looijmans
Parse devicetree parameters for voltage and prescaler setting. This allows
using multiple max6550 devices with varying settings, and also makes it
possible to instantiate and configure the device using devicetree.
---
This patch just does the minimal to get the device working properly with devicetree config.
What is the better path here, move to devicetree completely and

remove the module parameters,

Post by Guenter Roeck

Post by Mike Looijmans
or do we need to keep backward compatibility with the moduel

parameter (allowing only one

Post by Guenter Roeck

Post by Mike Looijmans
config for all chips)?

I am relatively sure that no one is using the module parameters, but we'll have
to keep them for compatibility.

I was afraid so yeah. I'll keep them in.

Post by Guenter Roeck
Properties will neeed to be described in
Documentation/devicetree/bindings/hwmon/max6650.

Post by Mike Looijmans
(in case of DT-only, the "clock" value could be obtained using the clock framework)

Yes, that should be done

I read the datasheet on that. The clock input/output of the chip is
intented
to synchonize multiple controllers together, where one will act as
clock
master, so you don't hear the annoying "whir" of fans running just out
of
sync. The 254kHz is just fixed in the chip.
Also this only applies to the max6651 variant which I don't have, so I
cannot
test my changes properly.
I guess for simplicity I'll just keep the current clock constant.

Post by Guenter Roeck

b/arch/arm/boot/dts/topic-miamiplus.dtsi

Post by Guenter Roeck

Post by Mike Looijmans
index d433101..e927ff9 100644
--- a/arch/arm/boot/dts/topic-miamiplus.dtsi
+++ b/arch/arm/boot/dts/topic-miamiplus.dtsi

This file is neither upstream nor in -next.

My bad, I hadn't created a proper git branch yet for this patch, and it
sorta
sneaked in.

Post by Guenter Roeck

Post by Mike Looijmans
@@ -56,6 +56,7 @@
reg = <0x1b>; /* ADD pins high-Z, hence address 0011011b */
compatible = "max6650";
voltage = <12>;

Should probably be something like fan-volts (or fan-volt), though that will
need to be confirmed by DT maintainers.

Post by Mike Looijmans
+ prescaler = <8>;

Probably better something like fan-prescale, unless there is a

generic

Post by Guenter Roeck
clock property that can be used. Needs feedback from DT maintainers.

Looking at other trees, I'll got with "fan-voltage" and "fan-prescale"
and
wait for feedback.

Post by Guenter Roeck

max6650_data *data,

Post by Guenter Roeck

Post by Mike Looijmans
struct device *dev = &client->dev;
int config;
int err = -EIO;
+ u32 local_fan_voltage = (u32)fan_voltage;
+ u32 local_prescaler = (u32)prescaler;

Please use shorter variable names. voltage and prescale, maybe.

Post by Mike Looijmans
+
+#ifdef CONFIG_OF

ifdef not needed.

Post by Mike Looijmans
+ of_property_read_u32(client->dev.of_node,
+ "voltage", &local_fan_voltage);

Better something like
if (of_property_read_u32(client->dev.of_node, "fan-volts",
&voltage))
voltage = fan_voltage;
(typecast not needed).

max6650_data *data,

Post by Guenter Roeck

Post by Mike Looijmans
return err;
}
- switch (fan_voltage) {
+ switch (local_fan_voltage) {
break;
@@ -585,13 +594,13 @@ static int max6650_init_client(struct

max6650_data *data,

Post by Guenter Roeck

max6650_data *data,

Post by Guenter Roeck

Kind regards,

Mike Looijmans

System Expert

[cid:***@63af9f37.46913c4e]

TOPIC Products

Materiaalweg 4

5681 RJ Best

T:

+31 (0) 499 33 69 69

Postbus 440

E:

***@topicproducts.com

5680 AK Best

W:

[1]www.topicproducts.com
The Netherlands

[2][cid:***@422883f5.4eb0ba00]
[3][cid:***@fdf16ab0.4aafe70f]
[4][cid:***@dcd06c37.40af81aa]
Please consider the environment before printing this e-mail
[5]Topic zoekt gedreven (embedded) software specialisten!

References

1. http://www.topicproducts.com/
2. https://www.facebook.com/TopicProducts
3. https://twitter.com/TopicProducts
4. https://www.linkedin.com/company/topic-embedded-products
5. http://topic.nl/vacancy/topic-zoekt-technische-software-engineers/

Mike Looijmans

2016-08-10 07:46:53 UTC

Permalink

Parse devicetree parameters for voltage and prescaler setting. This allows
using multiple max6550 devices with varying settings, and also makes it
possible to instantiate and configure the device using devicetree.

Signed-off-by: Mike Looijmans <***@topic.nl>
---
v2: Add devicetree binding documentation
Code changes as suggested by Guenter
Reduce log info, output only a single line

.../devicetree/bindings/hwmon/max6650.txt | 20 ++++++++++++++++
drivers/hwmon/max6650.c | 28 +++++++++++++---------
2 files changed, 37 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/max6650.txt

diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt b/Documentation/devicetree/bindings/hwmon/max6650.txt
new file mode 100644
index 0000000..89d87c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
@@ -0,0 +1,20 @@
+Bindings for MAX6651 and MAX6650 I2C fan controllers
+
+Required properties:
+- compatible : One of "max6650" or "max6651"
+- reg : I2C address
+
+Optional properties:
+- fan-voltage : The supply voltage of the fan. Valid values are 5 and 12.
+ Default is to use the chip's current setting.
+- fan-prescale : Pre-scaling value as per datasheet. Valid are 1, 2, 4, 8, 16.
+ Default is to use the chip's current setting.
+
+
+Example:
+ fan-max6650: ***@1b {
+ reg = <0x1b>;
+ compatible = "max6650";
+ fan-voltage = <12>;
+ fan-prescale = <4>;
+ };
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 162a520..858cd01 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -41,14 +41,14 @@
#include <linux/err.h>

/*
- * Insmod parameters
+ * Insmod parameters (for backward compatibility)
*/

/* fan_voltage: 5=5V fan, 12=12V fan, 0=don't change */
static int fan_voltage;
/* prescaler: Possible values are 1, 2, 4, 8, 16 or 0 for don't change */
static int prescaler;
-/* clock: The clock frequency of the chip the driver should assume */
+/* clock: The clock frequency of the chip (Fixed for 6550, 6651 can be clocked externally) */
static int clock = 254000;

module_param(fan_voltage, int, S_IRUGO);
@@ -566,6 +566,15 @@ static int max6650_init_client(struct max6650_data *data,
struct device *dev = &client->dev;
int config;
int err = -EIO;
+ u32 voltage;
+ u32 prescale;
+
+ if (of_property_read_u32(client->dev.of_node, "fan-voltage",
+ &voltage))
+ voltage = fan_voltage;
+ if (of_property_read_u32(client->dev.of_node, "fan-prescale",
+ &voltage))
+ prescale = prescaler;

config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);

@@ -574,7 +583,7 @@ static int max6650_init_client(struct max6650_data *data,
return err;
}

- switch (fan_voltage) {
+ switch (voltage) {
case 0:
break;
case 5:
@@ -584,14 +593,10 @@ static int max6650_init_client(struct max6650_data *data,
config |= MAX6650_CFG_V12;
break;
default:
- dev_err(dev, "illegal value for fan_voltage (%d)\n",
- fan_voltage);
+ dev_err(dev, "illegal value for fan_voltage (%d)\n", voltage);
}

- dev_info(dev, "Fan voltage is set to %dV.\n",
- (config & MAX6650_CFG_V12) ? 12 : 5);
-
- switch (prescaler) {
+ switch (prescale) {
case 0:
break;
case 1:
@@ -614,10 +619,11 @@ static int max6650_init_client(struct max6650_data *data,
| MAX6650_CFG_PRESCALER_16;
break;
default:
- dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
+ dev_err(dev, "illegal value for prescaler (%d)\n", prescale);
}

- dev_info(dev, "Prescaler is set to %d.\n",
+ dev_info(dev, "Fan voltage: %dV, prescaler: %d.\n",
+ (config & MAX6650_CFG_V12) ? 12 : 5,
1 << (config & MAX6650_CFG_PRESCALER_MASK));

/*

--
1.9.1

Mike Looijmans

2016-08-10 14:19:47 UTC

Permalink

Parse devicetree parameters for voltage and prescaler setting. This allows
using multiple max6550 devices with varying settings, and also makes it
possible to instantiate and configure the device using devicetree.

Signed-off-by: Mike Looijmans <***@topic.nl>
---
v3: Resubmit because mailing lists bounced
Fix style errors as reported by checkpatch.pl
Fix bug in DT parsing of fan-prescale
v2: Add devicetree binding documentation
Code changes as suggested by Guenter
Reduce log info, output only a single line

.../devicetree/bindings/hwmon/max6650.txt | 20 ++++++++++++++++
drivers/hwmon/max6650.c | 28 +++++++++++++---------
2 files changed, 37 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/max6650.txt

diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt b/Documentation/devicetree/bindings/hwmon/max6650.txt
new file mode 100644
index 0000000..89d87c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
@@ -0,0 +1,20 @@
+Bindings for MAX6651 and MAX6650 I2C fan controllers
+
+Required properties:
+- compatible : One of "max6650" or "max6651"
+- reg : I2C address
+
+Optional properties:
+- fan-voltage : The supply voltage of the fan. Valid values are 5 and 12.
+ Default is to use the chip's current setting.
+- fan-prescale : Pre-scaling value as per datasheet. Valid are 1, 2, 4, 8, 16.
+ Default is to use the chip's current setting.
+
+
+Example:
+ fan-max6650: ***@1b {
+ reg = <0x1b>;
+ compatible = "max6650";
+ fan-voltage = <12>;
+ fan-prescale = <4>;
+ };
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 162a520..56a6c87 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -41,14 +41,14 @@
#include <linux/err.h>

/*
- * Insmod parameters
+ * Insmod parameters (for backward compatibility)
*/

/* fan_voltage: 5=5V fan, 12=12V fan, 0=don't change */
static int fan_voltage;
/* prescaler: Possible values are 1, 2, 4, 8, 16 or 0 for don't change */
static int prescaler;
-/* clock: The clock frequency of the chip the driver should assume */
+/* clock: The clock frequency of the chip (max6651 can be clocked externally) */
static int clock = 254000;

module_param(fan_voltage, int, S_IRUGO);
@@ -566,6 +566,15 @@ static int max6650_init_client(struct max6650_data *data,
struct device *dev = &client->dev;
int config;
int err = -EIO;
+ u32 voltage;
+ u32 prescale;
+
+ if (of_property_read_u32(client->dev.of_node, "fan-voltage",
+ &voltage))
+ voltage = fan_voltage;
+ if (of_property_read_u32(client->dev.of_node, "fan-prescale",
+ &prescale))
+ prescale = prescaler;

config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);

@@ -574,7 +583,7 @@ static int max6650_init_client(struct max6650_data *data,
return err;
}

- switch (fan_voltage) {
+ switch (voltage) {
case 0:
break;
case 5:
@@ -584,14 +593,10 @@ static int max6650_init_client(struct max6650_data *data,
config |= MAX6650_CFG_V12;
break;
default:
- dev_err(dev, "illegal value for fan_voltage (%d)\n",
- fan_voltage);
+ dev_err(dev, "illegal value for fan_voltage (%d)\n", voltage);
}

- dev_info(dev, "Fan voltage is set to %dV.\n",
- (config & MAX6650_CFG_V12) ? 12 : 5);
-
- switch (prescaler) {
+ switch (prescale) {
case 0:
break;
case 1:
@@ -614,10 +619,11 @@ static int max6650_init_client(struct max6650_data *data,
| MAX6650_CFG_PRESCALER_16;
break;
default:
- dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
+ dev_err(dev, "illegal value for prescaler (%d)\n", prescale);
}

- dev_info(dev, "Prescaler is set to %d.\n",
+ dev_info(dev, "Fan voltage: %dV, prescaler: %d.\n",
+ (config & MAX6650_CFG_V12) ? 12 : 5,
1 << (config & MAX6650_CFG_PRESCALER_MASK));

/*

--
1.9.1

Guenter Roeck

2016-08-11 14:43:58 UTC

Permalink

For the subject: Please use "hwmon: (max6650) Add devicetree support"

Post by Mike Looijmans
---
v3: Resubmit because mailing lists bounced

It bounced because you sent to the old hwmon mailing list which is at best

Post by Mike Looijmans
Fix style errors as reported by checkpatch.pl
Fix bug in DT parsing of fan-prescale
v2: Add devicetree binding documentation
Code changes as suggested by Guenter
Reduce log info, output only a single line
.../devicetree/bindings/hwmon/max6650.txt | 20 ++++++++++++++++

Please submit the devicetree properties as separate patch.

Post by Mike Looijmans
drivers/hwmon/max6650.c | 28 +++++++++++++---------
2 files changed, 37 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/max6650.txt
diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt b/Documentation/devicetree/bindings/hwmon/max6650.txt
new file mode 100644
index 0000000..89d87c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
@@ -0,0 +1,20 @@
+Bindings for MAX6651 and MAX6650 I2C fan controllers
+
+- compatible : One of "max6650" or "max6651"
+- reg : I2C address
+
+- fan-voltage : The supply voltage of the fan. Valid values are 5 and 12.
+ Default is to use the chip's current setting.
+- fan-prescale : Pre-scaling value as per datasheet. Valid are 1, 2, 4, 8, 16.
+ Default is to use the chip's current setting.
+
+

One empty line is sufficient.

Post by Mike Looijmans
+ reg = <0x1b>;
+ compatible = "max6650";
+ fan-voltage = <12>;
+ fan-prescale = <4>;
+ };
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 162a520..56a6c87 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -41,14 +41,14 @@
#include <linux/err.h>
/*
- * Insmod parameters
+ * Insmod parameters (for backward compatibility)

That doesn't add any value.

Post by Mike Looijmans
*/
/* fan_voltage: 5=5V fan, 12=12V fan, 0=don't change */
static int fan_voltage;
/* prescaler: Possible values are 1, 2, 4, 8, 16 or 0 for don't change */
static int prescaler;
-/* clock: The clock frequency of the chip the driver should assume */
+/* clock: The clock frequency of the chip (max6651 can be clocked externally) */
static int clock = 254000;
module_param(fan_voltage, int, S_IRUGO);
@@ -566,6 +566,15 @@ static int max6650_init_client(struct max6650_data *data,
struct device *dev = &client->dev;
int config;
int err = -EIO;
+ u32 voltage;
+ u32 prescale;
+
+ if (of_property_read_u32(client->dev.of_node, "fan-voltage",
+ &voltage))
+ voltage = fan_voltage;
+ if (of_property_read_u32(client->dev.of_node, "fan-prescale",
+ &prescale))
+ prescale = prescaler;
config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
@@ -574,7 +583,7 @@ static int max6650_init_client(struct max6650_data *data,
return err;
}
- switch (fan_voltage) {
+ switch (voltage) {
break;
@@ -584,14 +593,10 @@ static int max6650_init_client(struct max6650_data *data,
config |= MAX6650_CFG_V12;
break;
- dev_err(dev, "illegal value for fan_voltage (%d)\n",
- fan_voltage);
+ dev_err(dev, "illegal value for fan_voltage (%d)\n", voltage);
}
- dev_info(dev, "Fan voltage is set to %dV.\n",
- (config & MAX6650_CFG_V12) ? 12 : 5);
-
- switch (prescaler) {
+ switch (prescale) {
break;
@@ -614,10 +619,11 @@ static int max6650_init_client(struct max6650_data *data,
| MAX6650_CFG_PRESCALER_16;
break;
- dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
+ dev_err(dev, "illegal value for prescaler (%d)\n", prescale);

Side note: Those errors should really result in an -EINVAL return.
Separate patch, though.

Post by Mike Looijmans
}
- dev_info(dev, "Prescaler is set to %d.\n",
+ dev_info(dev, "Fan voltage: %dV, prescaler: %d.\n",
+ (config & MAX6650_CFG_V12) ? 12 : 5,
1 << (config & MAX6650_CFG_PRESCALER_MASK));
/*

Rob Herring

2016-08-12 18:26:21 UTC

Permalink

Post by Mike Looijmans
Parse devicetree parameters for voltage and prescaler setting. This allows
using multiple max6550 devices with varying settings, and also makes it
possible to instantiate and configure the device using devicetree.
---
v3: Resubmit because mailing lists bounced
Fix style errors as reported by checkpatch.pl
Fix bug in DT parsing of fan-prescale
v2: Add devicetree binding documentation
Code changes as suggested by Guenter
Reduce log info, output only a single line
.../devicetree/bindings/hwmon/max6650.txt | 20 ++++++++++++++++
drivers/hwmon/max6650.c | 28 +++++++++++++---------
2 files changed, 37 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/max6650.txt
diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt b/Documentation/devicetree/bindings/hwmon/max6650.txt
new file mode 100644
index 0000000..89d87c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
@@ -0,0 +1,20 @@
+Bindings for MAX6651 and MAX6650 I2C fan controllers
+
+- compatible : One of "max6650" or "max6651"

Needs a vendor prefix.

Post by Mike Looijmans
+- reg : I2C address
+
+- fan-voltage : The supply voltage of the fan. Valid values are 5 and 12.

This is in volts? Please use microvolt and the standard unit suffix as
found in property-units.txt.

Post by Mike Looijmans
+ Default is to use the chip's current setting.
+- fan-prescale : Pre-scaling value as per datasheet. Valid are 1, 2, 4, 8, 16.
+ Default is to use the chip's current setting.

Add a vendor prefix here.

Post by Mike Looijmans
+
+
+ reg = <0x1b>;
+ compatible = "max6650";
+ fan-voltage = <12>;
+ fan-prescale = <4>;
+ };