lkml.org 
[lkml]   [2015]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH resend 2/6] toshiba_acpi: Add fan entry to sysfs
From
Hi Darren,

2015-02-09 21:02 GMT-07:00 Darren Hart <dvhart@infradead.org>:
> On Mon, Feb 09, 2015 at 08:34:50PM -0700, Azael Avalos wrote:
>> This patch adds a fan entry to sysfs, enabling the user to get and
>> set the fan status.
>>
>
> Hi Azael,
>
> I was finally getting around to these when you resent them. Apologies for the
> delay. Travel and still fighting a cold/flu/bug. Sigh. Anyway... on to patch
> review :-)

Sorry for the rushing of the patches, hope you're better now, in case you're
still in recovery, take my motto "la cerveza cura todo" into account XD

>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>> ---
>> drivers/platform/x86/toshiba_acpi.c | 51 ++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 334b65e..413af60 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -1516,6 +1516,11 @@ static const struct backlight_ops toshiba_backlight_data = {
>> */
>> static ssize_t toshiba_version_show(struct device *dev,
>> struct device_attribute *attr, char *buf);
>> +static ssize_t toshiba_fan_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count);
>> +static ssize_t toshiba_fan_show(struct device *dev,
>> + struct device_attribute *attr, char *buf);
>> static ssize_t toshiba_kbd_bl_mode_store(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf, size_t count);
>> @@ -1569,6 +1574,8 @@ static ssize_t toshiba_usb_sleep_music_store(struct device *dev,
>> const char *buf, size_t count);
>>
>> static DEVICE_ATTR(version, S_IRUGO, toshiba_version_show, NULL);
>> +static DEVICE_ATTR(fan, S_IRUGO | S_IWUSR,
>> + toshiba_fan_show, toshiba_fan_store);
>> static DEVICE_ATTR(kbd_backlight_mode, S_IRUGO | S_IWUSR,
>> toshiba_kbd_bl_mode_show, toshiba_kbd_bl_mode_store);
>> static DEVICE_ATTR(kbd_type, S_IRUGO, toshiba_kbd_type_show, NULL);
>
> At some point, before we add too much more, it would be nice to convert these
> over to DEVICE_ATTR_RW and DEVICE_ATTR_RO. Any reason not to do this sooner
> rather than later?

I have patches ready for all the functions in sysfs regarding this change,
I just wanted the patches to land in you tree (or next) to send them for 3.21,
I'll be in janitor mode after these patches, cleaning the driver according to
coding style, add files to Documentation/ABI, etc..

>
>> @@ -1594,6 +1601,7 @@ static DEVICE_ATTR(usb_sleep_music, S_IRUGO | S_IWUSR,
>>
>> static struct attribute *toshiba_attributes[] = {
>> &dev_attr_version.attr,
>> + &dev_attr_fan.attr,
>> &dev_attr_kbd_backlight_mode.attr,
>> &dev_attr_kbd_type.attr,
>> &dev_attr_available_kbd_modes.attr,
>> @@ -1621,6 +1629,45 @@ static ssize_t toshiba_version_show(struct device *dev,
>> return sprintf(buf, "%s\n", TOSHIBA_ACPI_VERSION);
>> }
>>
>> +static ssize_t toshiba_fan_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> + u32 result;
>> + int state;
>> + int ret;
>> +
>> + ret = kstrtoint(buf, 0, &state);
>> + if (ret)
>> + return ret;
>> +
>> + if (state != 0 && state != 1)
>> + return -EINVAL;
>> +
>> + result = hci_write1(toshiba, HCI_FAN, state);
>> + if (result == TOS_FAILURE)
>> + return -EIO;
>> + else if (result == TOS_NOT_SUPPORTED)
>> + return -ENODEV;
>> +
>
> A quick scan of hci_write1 makes me wonder if there are more than two possible
> failures. Should we also have an "else if (result)" or "else if (result !=
> WHATEVER_SUCCESS_IS)" before we assume success and return count?

Can't really tell you, I just ported the code "as-is" from the procfs files,
and to be honest, I haven't seen this function in any recent Toshiba
laptop out there,
I just wanted to port this in case some old laptop/app/script/etc. is
still using this.

>
> --
> Darren Hart
> Intel Open Source Technology Center


Cheers
Azael


--
-- El mundo apesta y vosotros apestais tambien --


\
 
 \ /
  Last update: 2015-02-10 05:41    [W:0.059 / U:3.720 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site