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

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

[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.

Signed-off-by: Mike Looijmans <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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Mike Looijmans]

[-- Attachment #1.1: Type: text/plain, Size: 5033 bytes --]

   On 09-08-16 09:30, Mike Looijmans wrote:
   > 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 <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>;
   >   };
   > };
   >
   Sorry, this part shouldn't have been in the patch!
   > 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
   >
   > Kind regards,
   >
   > Mike Looijmans
   >
   > System Expert
   >
   >
   >
   > *TOPIC Products*
   >
   >
   >
   >
   >
   > Materiaalweg 4
   >
   >
   >
   >
   >
   > 5681 RJ Best
   >
   >
   >
   > T:
   >
   >
   >
   > +31 (0) 499 33 69 69
   >
   > Postbus 440
   >
   >
   >
   > E:
   >
   >
   >
   > mike.looijmans@topicproducts.com
   >
   > 5680 AK Best
   >
   >
   >
   > W:
   >
   >
   >
   > 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>
   > 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:image7b51d0.PNG@75fc36a7.42abb6a5]

   TOPIC Products



   Materiaalweg 4



   5681 RJ Best

   T:

   +31 (0) 499 33 69 69

   Postbus 440

   E:

   mike.looijmans@topicproducts.com

   5680 AK Best

   W:

   [1]www.topicproducts.com
   The Netherlands

   [2][cid:image5918f1.JPG@f0d6511e.41b1bb4a]
   [3][cid:imagee19573.JPG@4027c0fd.4f82d4c9]
   [4][cid:image85634d.JPG@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/

[-- Attachment #1.2: image7b51d0.PNG --]
[-- Type: image/png, Size: 9075 bytes --]

[-- Attachment #1.3: image5918f1.JPG --]
[-- Type: image/jpeg, Size: 1088 bytes --]

[-- Attachment #1.4: imagee19573.JPG --]
[-- Type: image/jpeg, Size: 1087 bytes --]

[-- Attachment #1.5: image85634d.JPG --]
[-- Type: image/jpeg, Size: 1060 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Guenter Roeck]

On Tue, Aug 09, 2016 at 09:30:27AM +0200, Mike Looijmans wrote:
> 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 <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)?

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.

> (in case of DT-only, the "clock" value could be obtained using the clock framework)
> 
Yes, that should be done

>  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.

> @@ -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.

> +		prescaler = <8>;

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

>  	};
>  };
>  
> 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.

> +
> +#ifdef CONFIG_OF

ifdef not needed. 

> +	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).

> +	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
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] [PATCH v2] hwmon/max6650.c: Add devicetree support

[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.

Signed-off-by: Mike Looijmans <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: 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\x12V 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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH v2] hwmon/max6650.c: Add devicetree support

[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.

Signed-off-by: Mike Looijmans <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: 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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] [PATCH v3] hwmon/max6650.c: Add devicetree support

[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.

Signed-off-by: Mike Looijmans <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: 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\x12V 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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH v3] hwmon/max6650.c: Add devicetree support

[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.

Signed-off-by: Mike Looijmans <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: 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

[PATCH v3] hwmon/max6650.c: Add devicetree support

[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.

Signed-off-by: Mike Looijmans <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: 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


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v3] hwmon/max6650.c: Add devicetree support

[Guenter Roeck]

On 08/10/2016 07:19 AM, Mike Looijmans wrote:
> 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.
>

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

> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> v3: Resubmit because mailing lists bounced

It bounced because you sent to the old hwmon mailing list which is at best
unreliable. Please send future versions to linux-hwmon@vger.kernel.org.

>     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.

>  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.
> +
> +
One empty line is sufficient.

> +Example:
> +	fan-max6650: 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)

That doesn't add any value.

>   */
>
>  /* 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);

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

>  	}
>
> -	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));
>
>  	/*
>

Re: [lm-sensors] [PATCH v3] hwmon/max6650.c: Add devicetree support

[Guenter Roeck]

On 08/10/2016 07:19 AM, Mike Looijmans wrote:
> 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.
>

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

> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> v3: Resubmit because mailing lists bounced

It bounced because you sent to the old hwmon mailing list which is at best
unreliable. Please send future versions to linux-hwmon@vger.kernel.org.

>     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.

>  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.
> +
> +
One empty line is sufficient.

> +Example:
> +	fan-max6650: 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)

That doesn't add any value.

>   */
>
>  /* fan_voltage: 5=5V fan, 12\x12V 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);

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

>  	}
>
> -	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));
>
>  	/*
>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v3] hwmon/max6650.c: Add devicetree support

[Rob Herring]

On Wed, Aug 10, 2016 at 04:19:47PM +0200, Mike Looijmans wrote:
> 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 <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"

Needs a vendor prefix.

> +- reg        : I2C address
> +
> +Optional properties:
> +- 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.

> +                   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.

> + 
> +
> +Example:
> +	fan-max6650: max6650@1b {
> +		reg = <0x1b>;
> +		compatible = "max6650";
> +		fan-voltage = <12>;
> +		fan-prescale = <4>;
> +	};

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH v3] hwmon/max6650.c: Add devicetree support

[Rob Herring]

On Wed, Aug 10, 2016 at 04:19:47PM +0200, Mike Looijmans wrote:
> 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 <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"

Needs a vendor prefix.

> +- reg        : I2C address
> +
> +Optional properties:
> +- 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.

> +                   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.

> + 
> +
> +Example:
> +	fan-max6650: max6650@1b {
> +		reg = <0x1b>;
> +		compatible = "max6650";
> +		fan-voltage = <12>;
> +		fan-prescale = <4>;
> +	};

Re: [PATCH v3] hwmon/max6650.c: Add devicetree support

[Rob Herring]

On Wed, Aug 10, 2016 at 04:19:47PM +0200, Mike Looijmans wrote:
> 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 <mike.looijmans-Oq418RWZeHk@public.gmane.org>
> ---
> 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"

Needs a vendor prefix.

> +- reg        : I2C address
> +
> +Optional properties:
> +- 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.

> +                   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.

> + 
> +
> +Example:
> +	fan-max6650: max6650@1b {
> +		reg = <0x1b>;
> +		compatible = "max6650";
> +		fan-voltage = <12>;
> +		fan-prescale = <4>;
> +	};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mike Looijmans]

[-- Attachment #1.1: Type: text/plain, Size: 6311 bytes --]

   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.
   On 09-08-16 18:37, Guenter Roeck wrote:
   > On Tue, Aug 09, 2016 at 09:30:27AM +0200, Mike Looijmans wrote:
   >> 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 <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)?
   >
   > 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.
   > Properties will neeed to be described in
   > Documentation/devicetree/bindings/hwmon/max6650.
   >
   >> (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.
   >
   >>   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.
   My bad, I hadn't created a proper git branch yet for this patch, and it
   sorta
   sneaked in.
   >
   >> @@ -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.
   >
   >> +  prescaler = <8>;
   >
   > Probably better something like fan-prescale, unless there is a
   generic
   > 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.
   >>    };
   >>   };
   >>
   >> 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.
   >
   >> +
   >> +#ifdef CONFIG_OF
   >
   > ifdef not needed.
   >
   >> + 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).
   >
   >> + 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
   >>


   Kind regards,


   Mike Looijmans

   System Expert


   [cid:image90c2c8.PNG@63af9f37.46913c4e]

   TOPIC Products



   Materiaalweg 4



   5681 RJ Best

   T:

   +31 (0) 499 33 69 69

   Postbus 440

   E:

   mike.looijmans@topicproducts.com

   5680 AK Best

   W:

   [1]www.topicproducts.com
   The Netherlands

   [2][cid:imagead4153.JPG@422883f5.4eb0ba00]
   [3][cid:image5f2f6c.JPG@fdf16ab0.4aafe70f]
   [4][cid:image4621dd.JPG@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/

[-- Attachment #1.2: image90c2c8.PNG --]
[-- Type: image/png, Size: 9075 bytes --]

[-- Attachment #1.3: imagead4153.JPG --]
[-- Type: image/jpeg, Size: 1088 bytes --]

[-- Attachment #1.4: image5f2f6c.JPG --]
[-- Type: image/jpeg, Size: 1087 bytes --]

[-- Attachment #1.5: image4621dd.JPG --]
[-- Type: image/jpeg, Size: 1060 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors