lkml.org 
[lkml]   [2022]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode


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.



\
 
 \ /
  Last update: 2022-08-09 09:24    [W:0.053 / U:0.812 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site