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

[Mike Looijmans <mike.looijmans@topic.nl>]

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