Re: [RFC PATCH 0/2] HWMON: add in-kernel interfaces to read sensor values.

[MyungJoo Ham]

On Sun, Nov 6, 2011 at 2:53 PM, Guenter Roeck
<guenter.roeck@xxxxxxxxxxxx> wrote:
> On Fri, Nov 04, 2011 at 02:50:31AM -0400, MyungJoo Ham wrote:
>> We have been reading hwmon values (TMU, the SoC-core temperature sensor,
>> and NTC, the ambient or battery surface temperature sensor) for
>> Charger-Manager. However, because hwmon does not have in-kernel interface,
>> we have been using undesired method, including "../../../fs/*.h".
>>
>> This patch is to provide in-kernel interface for hwmon:
>> hwmon_get_value and hwmon_set_value. In order to use these two functions,
>> the hwmon driver should provide its sysfs attributes to hwmon framework
>> as well. If the hwmon driver does not provide (by providing NULL), the users
>> of the hwmon won't be able to use hwmon_get/set_value();
>> The sysfs attribute (struct attribuyte_group *) is provided with
>> hwmon_device_register; adding the second parameter to hwmon_device_register().
>> The 2/2 patch shows the changes in device drivers due to this.
>>
>> Among the HWMON device, all but ntc_thermistor.c does not setup attribute_group
>> for HWMON; thus they do not support hwmon_get/set_value with this patch.
>>
>> As soon as the server synchronizes git repositories, you can see how
>> hwmon_get_value() is used at:
>> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/charger-manager
>>
> Hi,
>
> I agree that we'll need a solution for the callback problem at some point.
> However, I have a number of concerns with the approach used in your patch set.
>
> The first patch breaks compilation. It changes the API w/o making the same changes
> in the affected drivers. This is unacceptable. Each patch of a patch series must
> result in working code.

Hello,

I simply thought having a single patch with API change and device
driver change altogether too ugly.
Anyway, by adding another API function to add get/set properties to
HWMON drivers, the need for this issue will disappear.

>
> Merging functionality into one function and then breaking out again, as you do with
> the set/get functions, results in unnecessarily complexity.
>        {set, get} -> access(flag) --> {set, get}
> Better write separate functions and keep the common code in a function called by the
> {set, get} functions.

Ok.

>
> We should not have to change the API for every caller, especially if many of those will
> never benefit from the new API. It would be better to define a new API function and map
> the old API function to the new one. Saves a lot of calls with unnecessary "NULL" parameter,
> and coincidentially reduces the patch size and avoids the breakage mentioned above.
>

Good. I'd like to add a new API function for this functionality, too.
I guess that it'd be easier to address your following comments by
adding another API function.

> The get and set functions are extremely expensive, having to loop through the list
> of attributes and performing a string comparison on each attribute to find the sensor.
>
> The proposed change only works for a small subset of drivers, those with a well defined
> single attribute groups. If and when we implement a solution for the internal-API
> problem, it should work for all hwmon drivers, not only for a small subset.
>
> Thanks,
> Guenter
>
>

Thank you.


Cheers!
MyungJoo

-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/