Re: [PATCH] toshiba_acpi: Adapt kbd_bl_timeout_store to the new kbd type

[Darren Hart]

On Thu, Oct 02, 2014 at 02:02:10PM -0600, Azael Avalos wrote:
> Hi there,
> 
> 2014-10-02 12:32 GMT-06:00 Darren Hart <dvhart@xxxxxxxxxxxxx>:
> > On Mon, Sep 29, 2014 at 08:57:04PM -0600, Azael Avalos wrote:
> >> With the introduccion of the new keyboard backlight
> >> implementation, the *_timeout_store function is
> >> broken, as it only supports the first kbd_type.
> >>
> >> This patch adapt such function for the new kbd_type,
> >> as well as convert from using sscanf to kstrtoint.
> >>
> >> Signed-off-by: Azael Avalos <coproscefalo@xxxxxxxxx>
> >> ---
> >>  drivers/platform/x86/toshiba_acpi.c | 33 +++++++++++++++++++++++++--------
> >>  1 file changed, 25 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> >> index 5d509ea..13ee56b 100644
> >> --- a/drivers/platform/x86/toshiba_acpi.c
> >> +++ b/drivers/platform/x86/toshiba_acpi.c
> >> @@ -1453,18 +1453,35 @@ static ssize_t toshiba_kbd_bl_timeout_store(struct device *dev,
> >>                                           const char *buf, size_t count)
> >>  {
> >>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> >> -     int time = -1;
> >> +     int time;
> >> +     int ret;
> >> +
> >> +     ret = kstrtoint(buf, 0, &time);
> >> +     if (ret)
> >> +             return ret;
> >>
> >> -     if (sscanf(buf, "%i", &time) != 1 && (time < 0 || time > 60))
> >> +     if (time < 1 || time > 60)
> >>               return -EINVAL;
> >
> > If I'm parsing this correctly, previously a time==0 was valid, and now it will
> > return -EINVAL. Is that intentional?
> 
> Yes, see below.
> 
> >
> >>
> >> -     /* Set the Keyboard Backlight Timeout: 0-60 seconds */
> >> -     if (time != -1 && toshiba->kbd_time != time) {
> >> +     /* Set the Keyboard Backlight Timeout: 1-60 seconds */
> >
> > So the time range change appears intentional. Why is that?
> 
> The previous implementation (type 1) accepted values 0-60,
> but the new one (type 2) just accepts 1-60, so I basically
> just changed both to the new range.

I see how setting it to 0 is not particularly useful :-) The concern, again, is
with the userspace interface. If anyone complains, this will get reverted. Can
we not work around the 0 value for type 2?

> 
> >
> >> +
> >> +     /* Only make a change if the actual timeout has changed */
> >> +     if (toshiba->kbd_time != time) {
> >> +             /* Shift the time to "base time" (0x3c0000 == 60 seconds)*/
> >>               time = time << HCI_MISC_SHIFT;
> >> -             time = (toshiba->kbd_mode == SCI_KBD_MODE_AUTO) ?
> >> -                                                     time + 1 : time + 2;
> >> -             if (toshiba_kbd_illum_status_set(toshiba, time) < 0)
> >> -                     return -EIO;
> >> +             /* OR the "base time" to the actual method format */
> >> +             if (toshiba->kbd_type == 1) {
> >> +                     /* Type 1 requires the oposite mode */
> >
> > opposite
> 
> Typo there, sorry.
> 
> >
> > Is it "opposite" or "current"?
> >
> 
> Opposite (again, welcome to Toshiba's KBD BL implementation).
> 
> For type 1, to change modes you set (OR?) the value to
> the current mode, if FNZ, you set it to AUTO, and viceversa.
> Now, to change the time (in case we are in AUTO), you set
> the timeout to the opposite mode, if AUTO, you set it to FNZ,
> and viceversa (of course this will never happen as the sysfs
> entry is hidden).
> 
> For type 2, to change modes and time you set the value to the desired
> mode (ON, OFF or AUTO), again, the time can only be changed when
> in AUTO mode (entry is hidden).
> 
> Perhaps something like this?
> 
> /* Type 1 requires FNZ mode, if set to AUTO, the time
>  * will change, but the mode will be changed as well
>  */


> 
> >> +                     time |= SCI_KBD_MODE_FNZ;
> >> +             } else if (toshiba->kbd_type == 2) {
> >> +                     /* Type 2 requires the actual mode */
> >
> > actual... as in the mode you are changing to or the mode you are changing from?
> 
> We're not changing modes here, just time. Perhaps that comment
> is misleading, I can probaly change it to:
> 
> /* Type 2 requires SCI_KBD_MODE_AUTO */> 
> Or leave it blank, or perhaps be more verbose:
> 
> /* Type 2 requires SCI_KBD_MODE_AUTO, if set to another
>  * mode, the time will change but the mode will change as well
>  */

Just leave it blank for both. As you say, toshiba interface specific stuff. Not
much to say other than "this is required" which is about as helpful as reading
the assignment :-)

> 
> >
> > From the previous keyboard backlight type patch:
> >
> > toshiba_acpi: Support new keyboard backlight type
> >
> > There are several keyboard modes, why do we have only 2 of them here?
> 
> Because the timeout entry only takes place (and appears)
> whenever the keyboard mode is set to AUTO, on any other
> mode is hidden.

Got it.

> 
> > Is it because by setting the timeout we are always changing to _AUTO?
> 
> Yes and no.
> If the timeout entry exists, that means we are in AUTO, however,
> the timeout can be changed even if we are on another mode (without
> changing modes).
> 
> > Even if that's
> > the case, shouldn't one of these be OR'ing in the current mode - whatever it is,
> > instead of a fixed one?
> 
> Yes, you can do that, and it will change the time successfuly,
> but again, this entry only appears whenever in AUTO mode,
> so we only have one mode to deal here with.
> 

nevermind, got it.

> >
> >> +                     time |= SCI_KBD_MODE_AUTO;
> >> +             }
> >> +
> >> +             ret = toshiba_kbd_illum_status_set(toshiba, time);
> >> +             if (ret)
> >> +                     return ret;
> >> +
> >
> > So here you are changing the sysfs API as you can now return -ENODEV in addition
> > to -EIO. We *can* do this, but it is a risk, and if a regression is reported, I
> > will be forced to revert this patch.
> 
> I was just propagating the error code from toshiba_kbd_illum_status_set,
> as it was already done on toshiba_kbd_bl_mode_store, but I can leave
> this part intact if that's a concern, even tho' the kbd_mode file was recently
> introduced.
> 

Nope, you're OK then I think. Please consider the input value change and update
the comments and resubmit.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center
--
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/