Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 9 Aug 2022 09:12:37 +0200 | Subject | Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode |
| |
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
| |