Messages in this thread | | | Date | Tue, 09 Aug 2022 19:22:54 +1200 | From | Luke Jones <> | Subject | Re: [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.
| |