Hi,
I cut the message just to the comments, here we go:
On Wed, 1 Dec 2010 00:22:03 +0100
Corentin Chary <corentin.chary@xxxxxxxxx> wrote:
> On Tue, Nov 30, 2010 at 6:27 PM, Herton Ronaldo Krzesinski
> <herton@xxxxxxxxxxxxxxx> wrote:
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/brightness_up
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â This is a write only option (accepts any single value, eg.
> > + Â Â Â Â Â Â Â "echo 1 > brightness_up") that is equivalent of pressing
> > + Â Â Â Â Â Â Â fn+<brightness up> function on notebooks. This option exists
> > + Â Â Â Â Â Â Â because of shuttle machines that are notebooks in desktop form
> > + Â Â Â Â Â Â Â factor, and which don't have the notebook keyboard, thus no
> > + Â Â Â Â Â Â Â way to use fn+<brightness up>.
>
> Why such files ? Can't we do the same using the backlight device ?
I use the backlight device, but also added this sysfs attribute. The reason
for it was testing, and also because the acpi video interface on Shuttle
DA18IE has a bug, if you try to set the brightness it writes the value but
brightness isn't updated on the screen. That's why also I had to made the
following workaround on backlight support without acpi video on the driver:
   /* change brightness by steps, this is a quirk for shuttle
   Â* machines which don't accept direct write to ec for this */
   if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER0, &val))
       return -EIO;
   val &= 0x7;
   steps = bd->props.brightness - val;
   while (steps > 0) {
       wmi_ec_cmd(CMD_SCMD, 0, 0, 0x0c, NULL);
       steps--;
   }
   while (steps < 0) {
       wmi_ec_cmd(CMD_SCMD, 0, 0, 0x0b, NULL);
       steps++;
   }
That is, on shuttle DA18IE, the brightness will only be effectively set if
you send the command with same code as fn+<brightness>, it doesn't work
writing to address 0x79 directly in EC as the firmware and my backlight
code does for the other machines. So, if you load the module with acpi
video support on, you can't change the brightness without this. If there is
an way that for Shuttle DA18IE I could disable acpi video support and always
only use the backlight class, then I can remove the sysfs attribute.
Shuttle DA18IE is weird because it has an hack on hardware: the LCD of the
machine is connected on the same VGA output of the machine, that is, instead
of using the LVDS port, they connect the VGA port, and because this you can't
have proper display switch on it, it works like a VGA splitter, very ugly.
This hardware may change, so may be we will not have to care any more about
this and remove both sysfs attribute and quirk, and one reason I don't send
the driver yet as final version.
>
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/cut_lvds
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â This is a write only option. Writing any single non-zero value
> > + Â Â Â Â Â Â Â to it enables main screen output, 0 to disable.
>
> Same, could be handled by the backlight device.
I'll remove and see to move to backlight.
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/lbar_brightness_down
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â This is a write only option (accepts any single value, eg.
> > + Â Â Â Â Â Â Â "echo 1 > lbar_brightness_down"). Decreases one step of lightbar
> > + Â Â Â Â Â Â Â brightness.
> > +
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/lbar_brightness_up
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â This is a write only option (accepts any single value, eg.
> > + Â Â Â Â Â Â Â "echo 1 > lbar_brightness_up"). Increases one step of lightbar
> > + Â Â Â Â Â Â Â brightness.
>
> What is the lightbar exactly ? Some kind of led ? Can't you use the
> led class instead ?
None of shuttle machines I have here have this lightbar, but it should be a
cosmetic light in the front of the machine at bottom. I don't know if it's
useful treat is as led, check this pdf:
http://www.shuttle.eu/fileadmin/resources/download/docs/spec/complete_systems/X5020XA_e.pdf
At page 6, there is a picture with the light bar, it's the item 8.
I don't know if the light bar should be handled by any kernel subsystem, or
keep as is (just sysfs attributes).
> Also the driver seems to reference keyboard brightness, if available,
> this should be
> implemented as a led named shuttle::kbd_backlight.
I don't know what's the brightness of keyboard too, that is on shuttle
documentation without specific explanation, hardware I have doesn't have
this.
> > +
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/volume_down
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â This is a write only option (accepts any single value, eg.
> > + Â Â Â Â Â Â Â "echo 1 > volume_down") that is equivalent of pressing
> > + Â Â Â Â Â Â Â fn+<volume down> function on notebooks.
> > +
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/volume_up
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â This is a write only option (accepts any single value, eg.
> > + Â Â Â Â Â Â Â "echo 1 > volume_up") that is equivalent of pressing
> > + Â Â Â Â Â Â Â fn+<volume up> function on notebooks.
>
> Are volume_down and volume_up really needed ? can't alsamixer do the same ?
Yes, I just kept for testing, I will remove it.
>
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/webcam
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â Control webcam state. 1 means on, 0 means off.
> > +
> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/white_balance
> > +Date: Â Â Â Â ÂNovember 2010
> > +KernelVersion: 2.6.37
> > +Contact: Â Â Â "Herton Ronaldo Krzesinski" <herton@xxxxxxxxxxxxxxx>
> > +Description:
> > + Â Â Â Â Â Â Â This is a write only option (accepts any single value, eg.
> > + Â Â Â Â Â Â Â "echo 1 > white_balance"). Probably triggers an automatic
> > + Â Â Â Â Â Â Â white balance adjustment for lcd, function not explained in
> > + Â Â Â Â Â Â Â shuttle wmi documentation.
>
>
> Here, I don't really understand why you reference "fn+<keys>". We
> don't really care about the keys right ? What we want is make the feature
> available for the user.
I use fn+<code> because that's what the firmware/wmi interface understands.
The 0xEC000200000000<byte command number> command sent by wmi (CMD_SCMD) in
fact in most cases is the same as sending fn+<byte command number> using the
keyboard.
So if the notebook keyboard has webcam switch at fn+f9, the command we are
sending to the bios is:
0xEC00020000000009
to turn on or off the webcam. The state (on or off) is read using CMD_READEC
from some address, when it saves state somewhere.
If the sound mute is fn+f4, then the command is 0xEC00020000000004, and so
on...
The hardware is a notebook without notebook keyboard (it should have the same
keyboard bios as similar notebook models). There is a button on the side of
the machine, that sends a key code when pressed (in the driver I translate
it to KEY_PROG1). This button should load an OSD application on the screen,
with buttons similar to the function buttons on notebook keyboard. When you
click the button on application, it must write a value
to /sys/devices/platform/shuttle_wmi/<attribute>, which will call the driver
that send the command as if it was the keyboard. That's why that many sysfs
attributes, but some of them like volume aren't needed indeed, mixer can
be called directly for example, etc. so no problem in removing them.
>
> Some of the files likes volume_up/volume_down seems to be here for
> debug purpose. Maybe you could add a simple debugfs interface
> to send custom commands to the wmi device.
Yes, I'll see here and I'll move the testing attributes I used to debugfs,
or just a generic fn+number.
>
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index faec777..ef84a4d 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -639,4 +639,20 @@ config XO1_RFKILL
> > Â Â Â Â ÂSupport for enabling/disabling the WLAN interface on the OLPC XO-1
> > Â Â Â Â Âlaptop.
> >
> > +config SHUTTLE_WMI
> > + Â Â Â tristate "Shuttle WMI Extras"
> > + Â Â Â depends on ACPI
> > + Â Â Â depends on BACKLIGHT_CLASS_DEVICE
> > + Â Â Â depends on RFKILL
> > + Â Â Â depends on INPUT
> > + Â Â Â select ACPI_WMI
> > + Â Â Â select INPUT_SPARSEKMAP
> > + Â Â Â ---help---
> > + Â Â Â Â This is a driver for Shuttle machines (mainly for laptops in desktop
> > + Â Â Â Â form factor). It adds controls for wireless, bluetooth, and 3g control
> > + Â Â Â Â radios, webcam switch, backlight controls, among others.
> > +
> > + Â Â Â Â If you have an Shuttle machine with ACPI-WMI interface say Y or M
> > + Â Â Â Â here.
> > +
>
> Add some DA18IE/DA18IM/MA ref here ?
>
> > Âendif # X86_PLATFORM_DEVICES
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 9950ccc..6a8fa82 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_IPS) Â Â Â Â Â Â Â += intel_ips.o
> > Âobj-$(CONFIG_GPIO_INTEL_PMIC) Â+= intel_pmic_gpio.o
> > Âobj-$(CONFIG_XO1_RFKILL) Â Â Â += xo1-rfkill.o
> > Âobj-$(CONFIG_IBM_RTL) Â Â Â Â Â+= ibm_rtl.o
> > +obj-$(CONFIG_SHUTTLE_WMI) Â Â Â+= shuttle-wmi.o
> > diff --git a/drivers/platform/x86/shuttle-wmi.c b/drivers/platform/x86/shuttle-wmi.c
> > new file mode 100644
> > index 0000000..389a16d
> > --- /dev/null
> > +++ b/drivers/platform/x86/shuttle-wmi.c
> > @@ -0,0 +1,843 @@
> > +/*
> > + * ACPI-WMI driver for Shuttle DA18IE/DA18IM/MA
> > + *
> > + * Copyright (c) 2009 Herton Ronaldo Krzesinski <herton@xxxxxxxxxxxxxxx>
>
> 2010 ?
>
Ops, going to fix these.
> > +static acpi_status wmi_setget_mtd(struct shuttle_cmd *scmd, u32 **res)
> > +{
> > + Â Â Â acpi_status status;
> > + Â Â Â union acpi_object *obj;
> > + Â Â Â struct acpi_buffer input;
> > + Â Â Â static DEFINE_MUTEX(mtd_lock);
> > + Â Â Â struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> > +
> > + Â Â Â input.length = sizeof(struct shuttle_cmd);
> > + Â Â Â scmd->hdr = 0xec00;
> > + Â Â Â input.pointer = (u8 *) scmd;
> > +
> > + Â Â Â mutex_lock(&mtd_lock);
> > + Â Â Â status = wmi_evaluate_method(SHUTTLE_WMI_SETGET_GUID, 0, 2,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&input, &output);
> > + Â Â Â mutex_unlock(&mtd_lock);
>
> I'm not sure why you need a mutex here ?
I need mutex because of the way the vendor implemented things in its wmi
interface. If I don't serialize the access to it, when sending commands
in parallel some of them will fail, for example:
# echo 1 > webcam & rfkill unblock <idx> & echo 1 > touchpad
some of the echos or rfkill will fail. This happens because the vendor used a
common buffer/variable to store parameters in AML code, from the DSDT:
Name (AC00, Buffer (0x28)
...
CreateDWordField (AC00, Zero, SAC0)
CreateDWordField (AC00, 0x04, SAC1)
...
CreateByteField (AC00, Zero, SA00)
CreateByteField (AC00, One, SA01)
CreateByteField (AC00, 0x02, SA02)
CreateByteField (AC00, 0x03, SA03)
CreateByteField (AC00, 0x04, SA04)
...
Method (WMBC, 3, NotSerialized)
{
If (LEqual (Arg1, One))
{
Return (GETC (Arg0))
}
If (LEqual (Arg1, 0x02))
{
Return (SETC (Arg0, Arg2))
}
...
Method (SETC, 2, NotSerialized)
{
If (LEqual (Arg0, Zero))
{
Store (Arg1, AC00)
OEMF (AC00)
Return (SAC0)
}
...
Method (OEMF, 1, NotSerialized)
{
If (LEqual (SAC1, 0xEC000300))
{
Store (0x73, DBG8)
Store ("LS", SAC0)
Return (SAC0)
}
If (LEqual (SAC1, 0xEC000000))
{
WKBC (SA00, SA01, SA02, SA03)
Return (SAC0)
}
...
Note that it uses only buffer AC00 to store and read the
parameters, so parallel calls will overwrite the value in AC00
(referenced as SAC?? and SA?? in some places) and some commands
will fail or have unpredicted behaviour.
Placing the lock prevents the parallel echo/rfkill command I placed
above as example to fail, all writes/commands are sent successfuly.
I'm attaching here DSDT of both machines I have for you to see the
entire code.
>
> > + Â Â Â if (ACPI_FAILURE(status))
> > + Â Â Â Â Â Â Â return status;
> > +
> > + Â Â Â obj = output.pointer;
> > + Â Â Â if (obj) {
> > + Â Â Â Â Â Â Â if (obj->type == ACPI_TYPE_INTEGER) {
> > + Â Â Â Â Â Â Â Â Â Â Â if (*res)
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â **res = obj->integer.value;
> > + Â Â Â Â Â Â Â } else
> > + Â Â Â Â Â Â Â Â Â Â Â pr_err("Unsupported object returned (%s)", __func__);
> > + Â Â Â Â Â Â Â kfree(obj);
> > + Â Â Â } else {
> > + Â Â Â Â Â Â Â if (*res) {
> > + Â Â Â Â Â Â Â Â Â Â Â pr_warning("No result from WMI method (%s)", __func__);
> > + Â Â Â Â Â Â Â Â Â Â Â *res = NULL;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> > + Â Â Â return AE_OK;
> > +}
> >
>
> wmi_setget_mtd would probably be simpler like that:
>
> static int wmi_setget_mtd(struct shuttle_cmd *scmd, u32 *res).
> You don't really need to return the acpi_status, you just want to
> return -1, 0, 1 for wmi_ec_cmd.
Good point, will simplify it.
>
> > +static int wmi_ec_cmd(unsigned char cmd, unsigned char arg,
> > + Â Â Â Â Â Â Â Â Â Â unsigned short param1, unsigned short param2,
> > + Â Â Â Â Â Â Â Â Â Â u32 *res)
> > +{
> > + Â Â Â struct shuttle_cmd scmd = {
> > + Â Â Â Â Â Â Â .cmd = cmd,
> > + Â Â Â Â Â Â Â .arg = arg,
> > + Â Â Â Â Â Â Â .param1 = param1,
> > + Â Â Â Â Â Â Â .param2 = param2
> > + Â Â Â };
> > +
> > + Â Â Â if (ACPI_FAILURE(wmi_setget_mtd(&scmd, &res)))
> > + Â Â Â Â Â Â Â return -1;
> > + Â Â Â return (res) ? 0 : 1;
> > +}
> > +
> > +struct shuttle_rfkill {
> > + Â Â Â char *name;
> > + Â Â Â struct rfkill *rfk;
> > + Â Â Â enum rfkill_type type;
> > + Â Â Â unsigned short fn;
> > + Â Â Â u32 mask;
> > + Â Â Â u32 list_on[3];
> > + Â Â Â u32 list_off[3];
> > +};
> > +
> > +static struct shuttle_rfkill shuttle_rfk_list[] = {
> > + Â Â Â { "shuttle_wlan", NULL, RFKILL_TYPE_WLAN,
> > + Â Â Â Â 0x04, 0x80, { 0x08 }, { 0x09 } },
> > + Â Â Â { "shuttle_bluetooth", NULL, RFKILL_TYPE_BLUETOOTH,
> > + Â Â Â Â 0x0d, 0x20, { 0x0c, 0x29 }, { 0x0d, 0x2a } },
> > + Â Â Â { "shuttle_3g", NULL, RFKILL_TYPE_WWAN,
> > + Â Â Â Â 0x05, 0x40, { 0x10, 0x29 }, { 0x11, 0x2a } },
> > +};
>
> I would rather use simple if/else constructs instead of list_on/list_off,
> it would probably be more easy to read.
I don't know, I like the array approach as that it's easier to add new
rfkill types, less hand written code, and more flexible as number of
notification codes are variable, current we have:
fn+f4 = send command byte == 0x04 == wireless switch
wmi notification codes:
- wireless on = 0x08
- wireless off = 0x09
fn+f13? = send command byte == 0xd == bluetooth switch
wmi notification codes:
- bluetooth on = 0x0c
- bluetooth off = 0x0d
- 3g/bluetooth on = 0x29
- 3g/bluetooth off = 0x2a
fn+f5 = send command byte == 0x5 == 3g switch
wmi notification codes:
- 3g on = 0x10
- 3g off = 0x11
- 3g/bluetooth on = 0x29
- 3g/bluetooth off = 0x2a
>
> > +static int rfkill_common_set_block(void *data, bool blocked)
> > +{
> > + Â Â Â u32 val;
> > + Â Â Â bool sw;
> > + Â Â Â struct shuttle_rfkill *srfk = data;
> > +
> > + Â Â Â if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER1, &val))
> > + Â Â Â Â Â Â Â return -EIO;
> > + Â Â Â sw = val & srfk->mask;
> > +
> > + Â Â Â if ((blocked && sw) || (!blocked && !sw))
>
> Maybe
> sw = !!(val & srfk->mask);
> if (blocked == sw)
> ?
True, will change it.
>
> > + Â Â Â Â Â Â Â wmi_ec_cmd(CMD_SCMD, 0, 0, srfk->fn, NULL);
> > + Â Â Â else
> > + Â Â Â Â Â Â Â return 0;
> > +
> > + Â Â Â if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER1, &val))
> > + Â Â Â Â Â Â Â return -EIO;
> > + Â Â Â return ((val & srfk->mask) != blocked) ? 0 : -EIO;
> > +}
> > +
> > +static const struct rfkill_ops rfkill_common_ops = {
> > + Â Â Â .set_block = rfkill_common_set_block,
> > +};
> > +
> > +static int common_rfkill_init(struct shuttle_rfkill *srfk, struct device *dev,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â u32 init_val)
> > +{
> > + Â Â Â int rc;
> > +
> > + Â Â Â srfk->rfk = rfkill_alloc(srfk->name, dev, srfk->type,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&rfkill_common_ops, srfk);
> > + Â Â Â if (!srfk->rfk)
> > + Â Â Â Â Â Â Â return -ENOMEM;
> > +
> > + Â Â Â rfkill_init_sw_state(srfk->rfk, !(init_val & srfk->mask));
> > +
> > + Â Â Â rc = rfkill_register(srfk->rfk);
> > + Â Â Â if (rc) {
> > + Â Â Â Â Â Â Â rfkill_destroy(srfk->rfk);
> > + Â Â Â Â Â Â Â srfk->rfk = NULL;
> > + Â Â Â Â Â Â Â return rc;
> > + Â Â Â }
> > +
> > + Â Â Â return 0;
> > +}
> > +
> > +static int shuttle_rfkill_init(struct platform_device *pdev)
> > +{
> > + Â Â Â int rc, i;
> > + Â Â Â u32 val;
> > + Â Â Â struct shuttle_rfkill *srfk;
> > +
> > + Â Â Â if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER1, &val))
> > + Â Â Â Â Â Â Â return -EIO;
> > +
> > + Â Â Â for (i = 0; i < ARRAY_SIZE(shuttle_rfk_list); i++) {
> > + Â Â Â Â Â Â Â srfk = &shuttle_rfk_list[i];
> > +
> > + Â Â Â Â Â Â Â /* check that hardware is available (when missing we can't
> > + Â Â Â Â Â Â Â Â* unblock it), to avoid having rfkill switch when not needed;
> > + Â Â Â Â Â Â Â Â* after check, reset to initial setting */
> > + Â Â Â Â Â Â Â if (rfkill_common_set_block(srfk, false))
> > + Â Â Â Â Â Â Â Â Â Â Â continue;
> > + Â Â Â Â Â Â Â if (!(val & srfk->mask))
> > + Â Â Â Â Â Â Â Â Â Â Â rfkill_common_set_block(srfk, true);
>
> There is no other solution to guess if the hardware is present ? Because
> I don't think it's a good idea to toggle every devices on load (may take time,
> etc...)
Unfortunately there is no other way, there is nothing that I know currently
which can be used to see if hardware is available or not.
>
> > + Â Â Â Â Â Â Â rc = common_rfkill_init(srfk, &pdev->dev, val);
> > + Â Â Â Â Â Â Â if (rc)
> > + Â Â Â Â Â Â Â Â Â Â Â goto err_rfk_init;
> > + Â Â Â }
> > + Â Â Â return 0;
> > +
> > +err_rfk_init:
> > + Â Â Â for (i--; i >= 0; i--) {
> > + Â Â Â Â Â Â Â srfk = &shuttle_rfk_list[i];
> > + Â Â Â Â Â Â Â if (srfk->rfk) {
> > + Â Â Â Â Â Â Â Â Â Â Â rfkill_unregister(srfk->rfk);
> > + Â Â Â Â Â Â Â Â Â Â Â rfkill_destroy(srfk->rfk);
> > + Â Â Â Â Â Â Â Â Â Â Â srfk->rfk = NULL;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
>
> Just call shuttfle_rfkill_remove, the if(srfk->rfk) should
> make it work without any issue.
indeed.
>
> > + Â Â Â return rc;
> > +}
> > +
> > +static void shuttle_rfkill_remove(void)
> > +{
> > + Â Â Â int i;
> > + Â Â Â struct shuttle_rfkill *srfk;
> > +
> > + Â Â Â for (i = 0; i < ARRAY_SIZE(shuttle_rfk_list); i++) {
> > + Â Â Â Â Â Â Â srfk = &shuttle_rfk_list[i];
> > + Â Â Â Â Â Â Â if (srfk->rfk) {
> > + Â Â Â Â Â Â Â Â Â Â Â rfkill_unregister(srfk->rfk);
> > + Â Â Â Â Â Â Â Â Â Â Â rfkill_destroy(srfk->rfk);
> > + Â Â Â Â Â Â Â Â Â Â Â srfk->rfk = NULL;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> > +}
> > +
> > +static bool set_rfkill_sw(u32 *list, u32 code, struct rfkill *rfk, bool blocked)
> > +{
> > + Â Â Â while (*list) {
> > + Â Â Â Â Â Â Â if (*list == code) {
> > + Â Â Â Â Â Â Â Â Â Â Â rfkill_set_sw_state(rfk, blocked);
> > + Â Â Â Â Â Â Â Â Â Â Â return true;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â Â Â Â Â list++;
> > + Â Â Â }
> > + Â Â Â return false;
> > +}
> > +
> > +static bool notify_switch_rfkill(u32 code)
> > +{
> > + Â Â Â int i;
> > + Â Â Â struct rfkill *rfk;
> > + Â Â Â u32 *rfk_evnt;
> > + Â Â Â bool res = false;
> > +
> > + Â Â Â for (i = 0; i < ARRAY_SIZE(shuttle_rfk_list); i++) {
> > + Â Â Â Â Â Â Â rfk = shuttle_rfk_list[i].rfk;
> > + Â Â Â Â Â Â Â if (!rfk)
> > + Â Â Â Â Â Â Â Â Â Â Â continue;
> > +
> > + Â Â Â Â Â Â Â rfk_evnt = shuttle_rfk_list[i].list_on;
> > + Â Â Â Â Â Â Â if (set_rfkill_sw(rfk_evnt, code, rfk, false)) {
> > + Â Â Â Â Â Â Â Â Â Â Â res = true;
> > + Â Â Â Â Â Â Â Â Â Â Â continue;
> > + Â Â Â Â Â Â Â }
> > +
> > + Â Â Â Â Â Â Â rfk_evnt = shuttle_rfk_list[i].list_off;
> > + Â Â Â Â Â Â Â if (set_rfkill_sw(rfk_evnt, code, rfk, true))
> > + Â Â Â Â Â Â Â Â Â Â Â res = true;
> > + Â Â Â }
> > + Â Â Â return res;
> > +}
>
> See my previous comment, but I believe that the code would be
> more obvious with basic if/else (or at least a command explaining
> what is does).
I'll add some extra comments.
>
> > +static int __devexit shuttle_wmi_remove(struct platform_device *pdev)
> > +{
> > + Â Â Â struct shuttle_wmi_priv *priv = platform_get_drvdata(pdev);
> > +
> > + Â Â Â shuttle_wmi_backlight_exit(priv);
> > + Â Â Â wmi_remove_notify_handler(SHUTTLE_WMI_EVENT_GUID);
> > + Â Â Â shuttle_wmi_input_remove(priv);
> > + Â Â Â shuttle_rfkill_remove();
> > + Â Â Â kfree(priv);
> > + Â Â Â return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
>
> WMI depends on ACPI
> ACPI depends on PM
>
> So it's not really needed.
ok.
>
> > +static int shuttle_wmi_resume(struct device *dev)
> > +{
> > + Â Â Â u32 val;
> > + Â Â Â int i;
> > + Â Â Â struct shuttle_rfkill *srfk;
> > +
> > + Â Â Â if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER1, &val))
> > + Â Â Â Â Â Â Â return -EIO;
> > +
> > + Â Â Â for (i = 0; i < ARRAY_SIZE(shuttle_rfk_list); i++) {
> > + Â Â Â Â Â Â Â srfk = &shuttle_rfk_list[i];
> > + Â Â Â Â Â Â Â if (srfk->rfk)
> > + Â Â Â Â Â Â Â Â Â Â Â rfkill_set_sw_state(srfk->rfk, !(val & srfk->mask));
> > + Â Â Â }
> > + Â Â Â return 0;
> > +}
>
> You're code would probably be cleaner with a rfkill helper to get the current
> state of a given deivce, to avoid playing with masks everytime.
I'll check this.
>
> > +
> > +static const struct dev_pm_ops shuttle_wmi_pm_ops = {
> > + Â Â Â .restore = shuttle_wmi_resume,
> > + Â Â Â .resume = shuttle_wmi_resume,
> > +};
> > +#endif
> > +
> > +static struct platform_driver shuttle_wmi_driver = {
> > + Â Â Â .driver = {
> > + Â Â Â Â Â Â Â .name = KBUILD_MODNAME,
> > + Â Â Â Â Â Â Â .owner = THIS_MODULE,
> > +#ifdef CONFIG_PM
> > + Â Â Â Â Â Â Â .pm = &shuttle_wmi_pm_ops,
> > +#endif
> > + Â Â Â },
> > + Â Â Â .probe = shuttle_wmi_probe,
> > + Â Â Â .remove = __devexit_p(shuttle_wmi_remove),
> > +};
> > +
> > +static struct platform_device *shuttle_wmi_device;
> > +
> > +static ssize_t show_state_common(char *buf, unsigned short ecram, u32 mask)
> > +{
> > + Â Â Â u32 val;
> > +
> > + Â Â Â if (wmi_ec_cmd(CMD_READEC, 0, 0, ecram, &val))
> > + Â Â Â Â Â Â Â return -EIO;
> > + Â Â Â return sprintf(buf, "%d\n", (val & mask) ? 1 : 0);
> > +}
> > +
> > +static ssize_t store_state_common(const char *buf, size_t count,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned short ecram, u32 mask,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned short fn)
> > +{
> > + Â Â Â int enable;
> > + Â Â Â int rc;
> > + Â Â Â u32 val;
> > +
> > + Â Â Â if (sscanf(buf, "%i", &enable) != 1)
> > + Â Â Â Â Â Â Â return -EINVAL;
> > +
> > + Â Â Â if (wmi_ec_cmd(CMD_READEC, 0, 0, ecram, &val))
> > + Â Â Â Â Â Â Â return -EIO;
> > + Â Â Â val &= mask;
>
> Here again, having to use a mask makes the code harder to read.
> Can't you use !!(val & mask) instead of val & mask ? Maybe a wrapper
> around CMD_READEC to do that ? int wmi_ec_read_state(ecram, mask) ?
I'll see if I can make it better.
>
> > + Â Â Â if ((val && !enable) ||
> > + Â Â Â Â Â (!val && enable)) {
> > + Â Â Â Â Â Â Â wmi_ec_cmd(CMD_SCMD, 0, 0, fn, NULL);
> > + Â Â Â Â Â Â Â rc = wmi_ec_cmd(CMD_READEC, 0, 0, ecram, &val);
> > + Â Â Â Â Â Â Â val &= mask;
> > + Â Â Â Â Â Â Â if (rc || (!val && enable) ||
> > + Â Â Â Â Â Â Â Â Â (val && !enable))
> > + Â Â Â Â Â Â Â Â Â Â Â return -EIO;
> > + Â Â Â }
> > + Â Â Â return count;
> > +}
--
[]'s
Herton
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
