Re: [PATCH 4/6] msi-laptop: Add MSI Wind U90/U100 support

[joeyli <jlee@suse.com>]

於 三，2012-11-28 於 19:41 +0200，Maxim Mikityanskiy 提到：
> 2012/11/28 joeyli <jlee@suse.com>:
> > 於 日，2012-11-25 於 00:28 +0200，Maxim Mikityanskiy 提到：
> >> Add MSI Wind U90/U100 to separate DMI table, add U90/U100 specific
> >> workarounds and add some missing EC features support such as basic fan
> >> control, turbo and ECO modes and touchpad state.
> >>
> >> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> >> ---
> >>  drivers/platform/x86/msi-laptop.c | 199 ++++++++++++++++++++++++++++++++------
> >>  1 file changed, 171 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
> >> index 3b6f494..16e9863 100644
> >> --- a/drivers/platform/x86/msi-laptop.c
> >> +++ b/drivers/platform/x86/msi-laptop.c
> >> @@ -82,8 +82,19 @@
> >>  #define MSI_STANDARD_EC_SCM_LOAD_ADDRESS     0x2d
> >>  #define MSI_STANDARD_EC_SCM_LOAD_MASK                (1 << 0)
> >>
> >> -#define MSI_STANDARD_EC_TOUCHPAD_ADDRESS     0xe4
> >> +#define MSI_STANDARD_EC_POWER_ADDRESS                0xe4
> >
> > If 0xE4 register is not just for touchpad and power, then I suggest use
> > a more general name like: MSI_STANDARD_EC_FUNCTIONS_ADDRESS or other.
> 
> OK, I will rename it.

thanks

> >> +static ssize_t show_touchpad(struct device *dev,
> >> +     struct device_attribute *attr, char *buf)
> >> +{
> >> +
> >> +     u8 rdata;
> >> +     int result;
> >> +
> >> +     result = ec_read(MSI_STANDARD_EC_POWER_ADDRESS, &rdata);
> >> +     if (result < 0)
> >> +             return result;
> >> +
> >> +     return sprintf(buf, "%i\n", !!(rdata & MSI_STANDARD_EC_TOUCHPAD_MASK));
> >> +}
> >
> > I don't think we need create a touchpad attribute interface because
> > there already have key code raise the change to user space.
> 
> Key codes only indicate touchpad state change. We cannot determine
> initial touchpad state on boot using only key codes. I think, it might
> be useful to have a file in sysfs that always keeps actual touchpad
> state, so that we can get initial touchpad state on boot. Do you
> disagree with that?
> 

I agree this reason for add a sysfs interface for raise touchpad status
to userland.
But this interface will be a special api and I am not sure there already
have standard way in input subsystem for raise touchpad status. If there
have no standard api to raise it, I think add it is make sense.

> >> +
> >> +static ssize_t show_turbo(struct device *dev,
> >> +     struct device_attribute *attr, char *buf)
> >> +{
...
> >> -     if (!old_ec_model)
> >> +     if (!old_ec_model) {
> >>               get_threeg_exists();
> >> +             if (dmi_check_system(msi_load_scm_models_dmi_table))
> >> +                     load_scm_model = 1;
> >> +             else if (dmi_check_system(msi_load_scm_ro_models_dmi_table)) {
> >> +                     load_scm_model = 1;
> >> +                     ec_read_only = 1;
> >> +             }
> >> +     }
> >>
> >>       if (!old_ec_model && dmi_check_system(msi_load_scm_models_dmi_table))
> >>               load_scm_model = 1;
> >
> > hmm... the load_scm_model dmi table check 2 times? I think you can just
> > remove the duplicate code.
> 
> Oops, I forgot to remove the last two lines when I were preparing patches.
> 
> Do I need to resend all patches after doing all needed fixes?

Yes, please modify your patches for all changed and I suggest re-send
them by add v2 to subject, like:
	[PATCH 0/6 v2] msi-laptop: Add MSI Wind U90/U100 support
	...
	[PATCH 4/6 v2] msi-laptop: Add MSI Wind U90/U100 support

Please write down what's the v2 change for each patch on description of
patch.


Thanks a lot!
Joey Lee