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 19:22:54 +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: <667CGR.TI5EUKB68MWN2@ljones.dev>
In-Reply-To: <CAHp75Vda5KX5pVrNeueQEODoEy405eTb9SYJtts-Lm9jMNocHQ@mail.gmail.com>
References: <20220808031132.10075-1-luke@ljones.dev>
        <20220808031132.10075-3-luke@ljones.dev>
        <CAHp75VcjTG=fFBjjHRYT2+ARfE-VHWKJ9aV8sF4JtRVo7Ke2vQ@mail.gmail.com>
        <F8WBGR.2I3OK4ERR79N3@ljones.dev>
        <CAHp75Vda5KX5pVrNeueQEODoEy405eTb9SYJtts-Lm9jMNocHQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk


On Tue, Aug 9 2022 at 09:12:37 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 9, 2022 at 5:26 AM Luke Jones <luke@ljones.dev> wrote:
> 
> ...
> 
>>  >>  +                       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?
> 
> Yes, it's more specific when the user sees it. The pr_err() is global
> and anonymous (you can only point to the driver, and not the instance
> of the device bound to it), while dev_err() is device specific and the
> user will immediately see which device instance is failing. Yet it's
> not a problem for this particular driver, because I don't believe one
> may have two, but it's a good coding practice in general.
> 
> (Note the last sentence: "good coding practice")
> 
> ...
> 
>>  >>  +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).
> 
> See above, "good coding practice". Why?
> 
> Imagine your code to be in hypothetical v5.10:
> 
>   int x = foo(param1, param2, ...);
> 
>   if (x)
>     return Y;
> 
> 
> Now, at v5.12 somebody adds a new feature which touches your code:
> 
>   int x = foo(param1, param2, ...);
>   struct bar *baz;
> 
>   if (we_have_such_feature_disabled)
>     return Z;
> 
>   if (x)
>     return Y;
> 
>   baz = ...
> 
> And then somebody else in v5.13 does another feature:
> 
>   int x = foo(param1, param2, ...);
>   struct bar *baz;
> 
>   if (we_have_such_feature_disabled)
>     return Z;
> 
>   /* parameter 1 can be NULL, check it */
>   if (!param1)
>     return -EINVAL;
> 
>   if (x)
>     return Y;
> 
>   baz = ...
> 
> Do you see now an issue? If you emulate this as a sequence of Git
> changes the last one is easily missing subtle detail. That's why "good
> coding practice".
> 
> --
> With Best Regards,
> Andy Shevchenko

That's a great example! Thanks mate, really appreciate it.