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;
MIME-Version: 1.0
References: <20220809033048.1634583-1-luke@ljones.dev> <20220809033048.1634583-2-luke@ljones.dev>
In-Reply-To: <20220809033048.1634583-2-luke@ljones.dev>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 9 Aug 2022 10:37:05 +0200
Message-ID: <CAHp75VdAMTUpWd9d+vq1_4CqyPYyKJy4dYRyDrgVCb5cJtPF5Q@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum
To:     "Luke D. Jones" <luke@ljones.dev>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 9, 2022 at 5:30 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> Due to multiple types of tablet/lidflip, the existing code for
> handling these events is refactored to use an enum for each type.

...

>  static int asus_wmi_input_init(struct asus_wmi *asus)
>  {
> +       struct device *dev;
>         int err, result;
>
> +       dev = &asus->platform_device->dev;
> +

While the discussed pattern of splitting assignments is a good
practice, for some cases we don't do it when we rely on the guarantee
by the callers that some of the stuff won't be problematic. Here the
device is part of the platform device and can't be NULL, there is no
point to split definition and assignment (and you may find plenty
examples in the kernel), so

  struct device *dev = &asus->platform_device->dev;

is better to have as it's guaranteed not to be NULL and since that we
don't check it in the code anyway.


...

>                         input_report_switch(asus->inputdev, SW_TABLET_MODE,
> -                                           !result);
> +                                               !result);

Irrelevant change.

...

It also seems you switched to dev_err() here for the pr_err() that
aren't related to the change, either split that to a separate patch,
or don't do it right now. I.o.w. use dev_err() only in the lines your
change touches, otherwise it's irrelevant.

-- 
With Best Regards,
Andy Shevchenko