Re: [PATCH 1/4] toshiba_acpi: Add support for USB Sleep and Charge function

Darren Hart
2015-01-18T18:46:51Z

On Wed, Jan 14, 2015 at 02:40:18PM -0700, Azael Avalos wrote:
> Newer Toshiba models now come with a feature called Sleep and Charge,
> where the computer USB ports remain powered when the computer is
> asleep or turned off.
>
> This patch adds support to such feature, creating a sysfs entry
> called "usb_sleep_charge" to set the desired charging mode or to
> disable it.
>
> The sysfs entry accepts three parameters, 0x0, 0x9 and 0x21, beign
> disabled, alternate and auto respectively.
>
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> ---
> drivers/platform/x86/toshiba_acpi.c | 112 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 112 insertions(+)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 71ac7c12..b03129d 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -122,6 +122,7 @@ MODULE_LICENSE("GPL");
> #define HCI_ECO_MODE 0x0097
> #define HCI_ACCELEROMETER2 0x00a6
> #define SCI_ILLUMINATION 0x014e
> +#define SCI_USB_SLEEP_CHARGE 0x0150
> #define SCI_KBD_ILLUM_STATUS 0x015c
> #define SCI_TOUCHPAD 0x050e
>
> @@ -146,6 +147,10 @@ MODULE_LICENSE("GPL");
> #define SCI_KBD_MODE_ON 0x8
> #define SCI_KBD_MODE_OFF 0x10
> #define SCI_KBD_TIME_MAX 0x3c001a
> +#define SCI_USB_CHARGE_MODE_MASK 0xff
> +#define SCI_USB_CHARGE_DISABLED 0x30000
> +#define SCI_USB_CHARGE_ALTERNATE 0x30009
> +#define SCI_USB_CHARGE_AUTO 0x30021
>
> struct toshiba_acpi_dev {
> struct acpi_device *acpi_dev;
> @@ -177,6 +182,7 @@ struct toshiba_acpi_dev {
> unsigned int touchpad_supported:1;
> unsigned int eco_supported:1;
> unsigned int accelerometer_supported:1;
> + unsigned int usb_sleep_charge_supported:1;
> unsigned int sysfs_created:1;
>
> struct mutex mutex;
> @@ -761,6 +767,53 @@ static int toshiba_accelerometer_get(struct toshiba_acpi_dev *dev,
> return 0;
> }
>
> +/* Sleep (Charge and Music) utilities support */
> +static int toshiba_usb_sleep_charge_get(struct toshiba_acpi_dev *dev,
> + u32 *mode)
> +{
> + u32 result;
> +
> + if (!sci_open(dev))
> + return -EIO;
> +
> + result = sci_read(dev, SCI_USB_SLEEP_CHARGE, mode);
> + sci_close(dev);
> + if (result == TOS_FAILURE) {
> + pr_err("ACPI call to set USB S&C mode failed\n");
> + return -EIO;
> + } else if (result == TOS_NOT_SUPPORTED) {
> + pr_info("USB Sleep and Charge not supported\n");
> + return -ENODEV;
> + } else if (result == TOS_INPUT_DATA_ERROR) {
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int toshiba_usb_sleep_charge_set(struct toshiba_acpi_dev *dev,
> + u32 mode)
> +{
> + u32 result;
> +
> + if (!sci_open(dev))
> + return -EIO;
> +
> + result = sci_write(dev, SCI_USB_SLEEP_CHARGE, mode);
> + sci_close(dev);
> + if (result == TOS_FAILURE) {
> + pr_err("ACPI call to set USB S&C mode failed\n");
> + return -EIO;
> + } else if (result == TOS_NOT_SUPPORTED) {
> + pr_info("USB Sleep and Charge not supported\n");
> + return -ENODEV;
> + } else if (result == TOS_INPUT_DATA_ERROR) {
> + return -EIO;

Personally I would feel more comfortable relying on our own data input
validation than that of the AML.

We can also present the user with an abstracted interface, we don't need to have
them send in register values that match the underlying hardware. In fact, should
the firmware in future machines change, you will be faced with having to remap
values should they mean different things. I would suggest defining a user
visible namespace for these, consider:

0: Disabled
1: Auto
2: Alternate (what does this mean anyway?)

Also, per Documentation/sysfs-rules.txt, which I believe I added based on
previous review with you?, -EIO is typically returned by sysfs itself from some
sort of general failure, like a NULL read or store pointer.

When the read or store operation fails as above, -ENXIO is the preferred error
code.