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@xxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxxxxxxxxxx
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/
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
