On Fri, Dec 24, 2010 at 5:34 AM, Guenter Roeck
<guenter.roeck@xxxxxxxxxxxx> wrote:
>
(snip)
>> +config SENSORS_NTC_THERMISTOR
>
> Should be in alphabetic order, ie before SENSORS_PC87360.
>
(snip)
> Â Â Â Â Â Â Â Â Â Â ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Text does not provide value. Please remove.
>
(snip)
>
> Please add text showing module name if built as module.
>
(sinp)
> Please put in alphabetic order (before PC87360).
>
(snip)
>
> Still not returning an error.
>
(snip)
>
> Still not returning an error.
>
Those will be done at the next patch. Thanks.
>> + Â Â Â return INT_MIN;
>> +}
>> +
>> +int ntc_thermistor_read(struct device *dev)
>> +{
>> + Â Â Â return _ntc_thermistor_read(dev_get_drvdata(dev));
>> +}
>
> Function should be removed since it is no longer exported.
>
I have been considering to use this function at the board file in the
kernel space.
However, if it is removed, the driver may have a weird callback (a) or
the board file
may need to access sysfs filesystem and depends on sysfs (b).
(a): add "int (*read_thermistor)(struct platform_device *pdev)" at
struct ntc_thermistor_platform_data (in include/linux/ntc.h) and let
ntc_thermistor_probe provide callback thru pdata back to the caller of
platform_device_register().
(b): read "/sysfs/devices/platform/blahblah/temp1_input"
and keep /sysfs mounted always.
Are these approaches fine?
(snip)
>> +
>> + Â Â Â /*
>> + Â Â Â Â* The compensation table is sorted by ohm value. Although it is
>> + Â Â Â Â* normally assumed that the temperature value monotonically
>> + Â Â Â Â* increases or decreases with the ohm value, but there could be
>> + Â Â Â Â* different thermistors. However, if we can assume so, we only need
>> + Â Â Â Â* to look up the first and the last values.
>
> The last sentence isn't really valuable. Either only look up first and last values
> only, and explain why you do it and if and when the code might have to change,
> or just explain why you loop through the table even if you don't need to right now.
Got it.
>
>> + Â Â Â Â*/
>> + Â Â Â for (i = 0; i < data->n_comp; i++) {
>> + Â Â Â Â Â Â Â if (data->comp[i].temp_C > data->maxtemp)
>> + Â Â Â Â Â Â Â Â Â Â Â data->maxtemp = data->comp[i].temp_C;
>> + Â Â Â Â Â Â Â if (data->comp[i].temp_C < data->mintemp)
>> + Â Â Â Â Â Â Â Â Â Â Â data->mintemp = data->comp[i].temp_C;
>> + Â Â Â }
>> +
>> + Â Â Â if (data->mintemp != INT_MAX)
>> + Â Â Â Â Â Â Â data->mintemp *= 1000;
>> + Â Â Â if (data->maxtemp != INT_MIN)
>> + Â Â Â Â Â Â Â data->maxtemp *= 1000;
>> +
> Doesn't this indicate a logical error in the compensation tables ? If so, you might
> want to do something better than just report INT_MAX for the minimum temperature
> and INT_MIN for the maximum temperature (which doesn't really make sense).
> Maybe have the probe function return an error and fail to install.
Ok, I'll let it report an error for these instances as well.
>
(snip)
>
Thank you so much.
Happy Holidays!
- MyungJoo
--
MyungJoo Ham (íëì), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
