Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i5ec1447f:Fastmail
Date:   Tue, 09 Aug 2022 15:26:39 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Message-Id: <F8WBGR.2I3OK4ERR79N3@ljones.dev>
In-Reply-To: <CAHp75VcjTG=fFBjjHRYT2+ARfE-VHWKJ9aV8sF4JtRVo7Ke2vQ@mail.gmail.com>
References: <20220808031132.10075-1-luke@ljones.dev>
        <20220808031132.10075-3-luke@ljones.dev>
        <CAHp75VcjTG=fFBjjHRYT2+ARfE-VHWKJ9aV8sF4JtRVo7Ke2vQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk

Hi Andy,
> 
>>  -       { KE_KEY, 0xFA, { KEY_PROG2 } },           /* Lid flip 
>> action */
>>  +       { KE_KEY, 0xFA, { KEY_PROG2 } }, /* Lid flip action */
> 
> Have maintainers asked you about this? Otherwise it is irrelevant 
> change.
> 

Fixed

> ...
> 
>>  +                       pr_err("This device has lid-flip-rog quirk 
>> but got ENODEV checking it. This is a bug.");
> 
> dev_err() ?

Okay, changed here and in previous patch to match it.

So that I'm clearer on dev_err(), this doesn't do something like exit 
the module does it? It's just a more detailed error print?

> 
>>  +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi 
>> *asus)
>>  +{
>>  +       int result = asus_wmi_get_devstate_simple(asus, 
>> ASUS_WMI_DEVID_LID_FLIP_ROG);
>>  +
>>  +       if (result >= 0) {
> 
> First of all, it's better to decouple assignment and definition, and
> move assignment closer to its user. This is usual pattern.
> 

I don't fully understand why you would want the separation given how 
short these two blocks are (I'll change in this and previous patch of 
course, I just don't personally understand it).

Cheers,
Luke.
>