_DESCRIPTION("Gigabyte WMI temperature Driver");
^
It's a minor thing, but I think a lowercase 'd' would be better.
> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
> --
> 2.31.1
Regards,
Barnabás Pőcze
Hi
2021. április 5., hétfő 22:48 keltezéssel, Thomas Weißschuh írta:
> Changes since v1:
> * Incorporate feedback from Barnabás Pőcze
> * Use a WMI driver instead of a platform driver
> * Let the kernel manage the driver lifecycle
> * Fix errno/ACPI error confusion
>
atus ret;
> +
> + ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, ,
> );
> + if (ret == 0)
> + *res = (s8) temp;
That cast will be done no matter if it's explicitly specified,
so you might want to drop it.
> + return ret;
> +}
> +
> +static int __init gigabyte_wmi_init(void)
> +{
> + struct platform_device *pdev;
> + int err;
> +
> + err = platform_driver_register(_wmi_driver);
> + if (err)
> + return err;
> + pdev = platform_device_alloc(DRVNAME, -1);
Prefer `PLATFORM_DEVID_NONE` instead of -1.
> + if (!pdev) {
> + platform_driver_unregister(_wmi_driver);
> + return -ENOMEM;
> + }
> + return platform_device_add(pdev);
The driver is not unregistered if this fails.
> +}
> +module_init(gigabyte_wmi_init);
> +
> +static void __exit gigabyte_wmi_exit(void)
> +{
> + platform_device_unregister(gigabyte_wmi_pdev);
> + platform_driver_unregister(_wmi_driver);
> +}
> +module_exit(gigabyte_wmi_exit);
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
> --
> 2.31.1
>
Regards,
Barnabás Pőcze
ysfs interface with string added to be more clearly reading
> v4 -> v5:
> * addressed feedback from Randy Dunlap
> * addressed feedback from Pierre-Louis Bossart
> * rebase to latest 5.12 rc4 upstream kernel
> * fix some space alignment problem
> v3 -> v4:
> * fix f
device)
> if (hp_wmi_rfkill_setup(device))
> hp_wmi_rfkill2_setup(device);
>
> - thermal_profile_setup(device);
> + thermal_profile_setup();
>
> return 0;
> }
> @@ -1030,5 +1120,8 @@ static void __exit hp_wmi_exit(void)
> platform_device_unregister(hp_wmi_platform_dev);
> platform_driver_unregister(_wmi_driver);
> }
> +
> + if (platform_profile_support)
> + platform_profile_remove();
I personally don't like the asymmetry that hp_wmi_bios_setup() registers (even
if only indirectly), but hp_wmi_exit() removes. I'd put this in
hp_wmi_bios_remove().
> }
> module_exit(hp_wmi_exit);
> --
> 2.29.2
Regards,
Barnabás Pőcze
2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta:
> On Mon, Jan 18, 2021 at 11:12:34AM +0000, Barnabás Pőcze wrote:
> > 2021. január 18., hétfő 1:34 keltezéssel, Daniel Scally írta:
>
> > Have you considered putting the source (and header) files into a dedicated
&
ed putting the source (and header) files into a dedicated
folder? I think it'd help manageability in the long run, and it'd be immediately
obvious that these source files form a single "unit".
Regards,
Barnabás Pőcze
gt; Yes, the wmi driver is the entry for whole privacy driver.
>
> if wmi guid query failed, the privacy driver will not be registered.
>
> dell-laptop driver will get "-ENOVDE" from dell_privacy_valid().
>
> it will register its micmute led trigger driver from dell laptop driver.
>
My point is that if dell-privacy is a dependency of dell-laptop, and
dell-privacy
fails to load, then dell-laptop cannot be loaded. Effectively, the lack of
the DELL_PRIVACY_GUID WMI interface will cause the dell-laptop module not be
able to be loaded.
> [...]
> > > +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> > > + err = dell_privacy_valid();
> > > + if (err == 0) {
> > > + dell_privacy_process_event(buffer_entry[1],
> > > + buffer_entry[3],
> > > buffer_entry[4]);
> >
> > What if `len < 5`?
>
> when CONFIG_DELL_PRIVACY is enabled, and dell_privacy_valid return zero which
> means privacy driver loaded as expected.
>
> for example ,the micmute report wmi event data len is "5", it will not be
> less than 5 words.
I'm wondering if there are such guarantees, why is the length checked just a
couple lines below?
>
> Process buffer (04 00 12 00 0e 00 01 00 03 00)
>
>
> #if IS_ENABLED(CONFIG_DELL_PRIVACY)
One thing I might have forgotten to point out initially, is that there is no
need
for this #if as dell-privacy-wmi.h provides stub definitions for
`dell_privacy_valid()` and `dell_privacy_process_event()`.
> err = dell_privacy_valid();
> if (err == 0) {
> dell_privacy_process_event(buffer_entry[1],
> buffer_entry[3],
> buffer_entry[4]);
> } else {
> if (len > 2)
> dell_wmi_process_key(wdev,
> buffer_entry[1],
> buffer_entry[2]);
> /* Extended data is currently ignored */
> }
> [...]
Another thing I may have forgotten to say: the name `dell_privacy_valid()` is
misleading in my opinion, as it gives the impression of being a predicate, even
though it is not. `dell_privacy_state()` or something like that would be better,
I think.
Regards,
Barnabás Pőcze
p.s. please send text emails.
ew comment and including the second patch of the
> series.
> [...]
I think first something needs to be figured out regarding
the integration with the rest of the Dell modules. I feel
that list is not a desirable way to do it.
Regards,
Barnabás Pőcze
2021. január 2., szombat 3:36 keltezéssel, Jiaxun Yang írta:
> 在 2021/1/2 上午1:09, Barnabás Pőcze 写道:
> > Hi
> >
> >
> > 2021. január 1., péntek 17:08 keltezéssel, Jiaxun Yang írta:
> >
> >> [...]
> >>>> @@ -1006,6 +1018,10 @@ static
this does not have any effect (at least not the desired one)? And the part of
the
code I made my comment about only runs on machines on which the touchpad
supposedly
cannot be controlled by the EC. What am I missing?
And there is the other problem: on some machines, this patch removes working
functionality.
Regards,
Barnabás Pőcze
t; if (!priv->has_hw_rfkill_switch)
> write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1);
>
> + /* The same for Touchpad */
> + if (!priv->has_touchpad_switch)
> + write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1);
> +
Shouldn't it be the other way around: `if (priv->has_touchpad_switch)`?
> for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
> if (test_bit(ideapad_rfk_data[i].cfgbit, >cfg))
> ideapad_register_rfkill(priv, i);
> --
> 2.30.0
Regards,
Barnabás Pőcze
PRIVACY */
> +
> +int dell_privacy_acpi_init(void);
> +void dell_privacy_acpi_exit(void);
> +
> +/* DELL Privacy Type */
> +enum {
> + DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
> + DELL_PRIVACY_TYPE_AUDIO,
> + DELL_PRIVACY_TYPE_CAMERA,
> +};
> +#endif
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index bbdb3e860892..4b22bd21fc42 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -27,6 +27,7 @@
> #include
> #include "dell-smbios.h"
> #include "dell-wmi-descriptor.h"
> +#include "dell-privacy-wmi.h"
>
> MODULE_AUTHOR("Matthew Garrett ");
> MODULE_AUTHOR("Pali Rohár ");
> @@ -381,6 +382,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> u16 *buffer_entry, *buffer_end;
> acpi_size buffer_size;
> int len, i;
> + int err;
>
> if (obj->type != ACPI_TYPE_BUFFER) {
> pr_warn("bad response type %x\n", obj->type);
> @@ -427,18 +429,30 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>
> switch (buffer_entry[1]) {
> case 0x: /* One key pressed or event occurred */
> - case 0x0012: /* Event with extended data occurred */
> - if (len > 2)
> - dell_wmi_process_key(wdev, buffer_entry[1],
> - buffer_entry[2]);
> - /* Extended data is currently ignored */
> - break;
> case 0x0010: /* Sequence of keys pressed */
> case 0x0011: /* Sequence of events occurred */
> for (i = 2; i < len; ++i)
> dell_wmi_process_key(wdev, buffer_entry[1],
>buffer_entry[i]);
> break;
> + case 0x0012:
The comment "Event with extended data occurred" has been deleted.
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> + err = dell_privacy_valid();
> + if (err == 0) {
> + dell_privacy_process_event(buffer_entry[1],
> + buffer_entry[3],
> buffer_entry[4]);
What if `len < 5`?
> + } else {
> + if (len > 2)
> + dell_wmi_process_key(wdev,
> buffer_entry[1],
> + buffer_entry[2]);
> + }
> +#else
> + /* Extended data is currently ignored */
> + if (len > 2)
> + dell_wmi_process_key(wdev, buffer_entry[1],
> + buffer_entry[2]);
> +#endif
> + break;
> default: /* Unknown event */
> pr_info("Unknown WMI event type 0x%x\n",
> (int)buffer_entry[1]);
> --
> 2.25.1
Regards,
Barnabás Pőcze
2020. december 9., szerda 8:00 keltezéssel, Greg KH írta:
> On Tue, Dec 08, 2020 at 09:59:20PM +0000, Barnabás Pőcze wrote:
>
> > 2020. november 25., szerda 16:07 keltezéssel, Greg KH írta:
> >
> > > [...]
> > >
> > > > +static u8 polling_mode
do you think something like what the usbcore has would be better?
A module parameter like
"quirks=::[,::]*"?
Regards,
Barnabás Pőcze
2020. december 4., péntek 0:45 keltezéssel, Barnabás Pőcze írta:
> Hi
>
> [...]
Oh well, I replied to the wrong email, apologies. :-(
orrectly */
> + if (dmi_check_system(dmi_vgbs_allow_list)) {
> + dev_info(>dev, "platform supports switches\n");
> + err = intel_hid_switches_setup(device);
> + if (err)
> + pr_err("Failed to setup Intel HID switches\n");
> + else
> + detect_tablet_mode(device);
> + }
> +
> status = acpi_install_notify_handler(handle,
>ACPI_DEVICE_NOTIFY,
>notify_handler,
> --
> 2.28.0
Regards,
Barnabás Pőcze
2020. november 25., szerda 11:57 keltezéssel, Coiby Xu írta:
> On Mon, Nov 23, 2020 at 04:32:40PM +0000, Barnabás Pőcze wrote:
> >> [...]
> >> >> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
> >> >
and store() methods must return `ssize_t`.
What I'm arguing here, is that there is no reason to use `ssize_t` in this case.
Because `get_gpio_pin_state()` returns `int`. So when you do
```
ssize_t status = get_gpio_pin_state(...);
```
then the return value of `get_gpio_pin_state()` (which is an `int`), will be
converted to an `ssize_t`, and saved into `status`. I'm arguing that that is
unnecessary and a plain `int` would work perfectly well in this case. Anyways,
both work fine, I just found the unnecessary use of `ssize_t` here odd.
Regards,
Barnabás Pőcze
> >> +
> >> + if (status < 0) {
> >> + dev_warn(>dev,
> >> + "Failed to get GPIO Interrupt line status for %s",
> >> + client->name);
> >
> >I think it's possible that the kernel message buffer is flooded with these
> >messages, which is not optimal in my opinion.
> >
> Thank you! Replaced with dev_dbg in v4.
> [...]
Have you looked at `dev_{warn,dbg,...}_ratelimited()`?
Regards,
Barnabás Pőcze
= 0x02, // battery/power subsystem
> >> + SSAM_SSH_TC_TMP = 0x03, // thermal subsystem
> >> + SSAM_SSH_TC_PMC = 0x04,
> >> + SSAM_SSH_TC_FAN = 0x05,
> >> + SSAM_SSH_TC_PoM = 0x06,
> >> + SSAM_SSH_TC_DBG = 0x07,
> >> + SSAM_SSH_TC_KBD = 0x08, // legacy keyboard (Laptop 1/2)
> >> + SSAM_SSH_TC_FWU = 0x09,
> >> + SSAM_SSH_TC_UNI = 0x0a,
> >> + SSAM_SSH_TC_LPC = 0x0b,
> >> + SSAM_SSH_TC_TCL = 0x0c,
> >> + SSAM_SSH_TC_SFL = 0x0d,
> >> + SSAM_SSH_TC_KIP = 0x0e,
> >> + SSAM_SSH_TC_EXT = 0x0f,
> >> + SSAM_SSH_TC_BLD = 0x10,
> >> + SSAM_SSH_TC_BAS = 0x11, // detachment system (Surface Book 2/3)
> >> + SSAM_SSH_TC_SEN = 0x12,
> >> + SSAM_SSH_TC_SRQ = 0x13,
> >> + SSAM_SSH_TC_MCU = 0x14,
> >> + SSAM_SSH_TC_HID = 0x15, // generic HID input subsystem
> >> + SSAM_SSH_TC_TCH = 0x16,
> >> + SSAM_SSH_TC_BKL = 0x17,
> >> + SSAM_SSH_TC_TAM = 0x18,
> >> + SSAM_SSH_TC_ACC = 0x19,
> >> + SSAM_SSH_TC_UFI = 0x1a,
> >> + SSAM_SSH_TC_USC = 0x1b,
> >> + SSAM_SSH_TC_PEN = 0x1c,
> >> + SSAM_SSH_TC_VID = 0x1d,
> >> + SSAM_SSH_TC_AUD = 0x1e,
> >> + SSAM_SSH_TC_SMC = 0x1f,
> >> + SSAM_SSH_TC_KPD = 0x20,
> >> + SSAM_SSH_TC_REG = 0x21,
> >> +};
> >
> > Is it known what these abbreviations stand for? Maybe I missed them?
>
> The comments state all we really know about these (through observation
> and experimentation). The table itself has been extracted from the
> Windows driver, but the abbreviations and values are all we're getting
> from it.
I see, thanks for the clarification. For some reason, I believed the
"Known SSH/EC target categories" comment means that those are known, as in,
it is known what they are for, etc., not just the abbreviation.
> [...]
Regards,
Barnabás Pőcze
= 0x21,
> +};
Is it known what these abbreviations stand for? Maybe I missed them?
> [...]
> +/**
> + * struct ssh_request_ops - Callback operations for a SSH request.
> + * @release: Function called when the request's reference count reaches
> zero.
> + *This callback must be relied upon to ensure that the request
> has
> + *left the transport systems (both, packet an request systems).
> + * @complete: Function called when the request is completed, either with
> + *success or failure. The command data for the request response
> + *is provided via the ssh_command parameter (``cmd``),
> + *the command payload of the request response via the
> + *ssh_span parameter (``data``).
> + *
> + *If the request does not have any response or has not been
> + *completed with success, both ``cmd`` and ``data`` parameters
> will
> + *be NULL. If the request response does not have any command
> + *payload, the ``data`` span will be an empty (zero-length) span.
> + *
> + *In case of failure, the reason for the failure is indicated by
> + *the value of the provided status code argument (``status``).
> This
> + *value will be zero in case of success.
I believe it should be noted if the `status` argument is a regular errno,
or something different.
> + *
> + *Note that a call to this callback does not guarantee that the
> + *request is not in use by the transport systems any more.
> + */
> +struct ssh_request_ops {
> + void (*release)(struct ssh_request *rqst);
> + void (*complete)(struct ssh_request *rqst,
> + const struct ssh_command *cmd,
> + const struct ssam_span *data, int status);
> +};
> [...]
I have to agree, this is quite a sizable patch, although I think it's
well-commented,
which helps reading the code by a lot, however, in some places I feel like it's
a bit
over-engineered (or maybe I just cannot fully appreciate the subject at hand at
the moment),
nonetheless, I applaud your efforts, I can only imagine the hours that must
have gone into it.
Regards,
Barnabás Pőcze
buffer_entry[i]);
> - break;
> - default: /* Unknown event */
> - pr_info("Unknown WMI event type 0x%x\n",
> - (int)buffer_entry[1]);
> - break;
> - }
> -
> - buffer_entry += len;
> -
> - }
> +while (buffer_entry < buffer_end) {
> +
> +len = buffer_entry[0];
> +if (len == 0)
> +break;
> +
> +len++;
> +
> +if (buffer_entry + len > buffer_end) {
> +pr_warn("Invalid length of WMI event\n");
> +break;
> +}
> +
> +pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> +
> +switch (buffer_entry[1]) {
> +case 0x: /* One key pressed or event occurred */
> +if (len > 2)
> +dell_wmi_process_key(wdev, buffer_entry[1],
> +buffer_entry[2]);
> +break;
> +case 0x0010: /* Sequence of keys pressed */
> +case 0x0011: /* Sequence of events occurred */
> +for (i = 2; i < len; ++i)
> +dell_wmi_process_key(wdev, buffer_entry[1],
> +buffer_entry[i]);
> +break;
> +case 0x0012:
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +if (dell_privacy_valid()) {
> +dell_privacy_process_event(buffer_entry[1],
> buffer_entry[3],
> +buffer_entry[4]);
> +} else {
> +if (len > 2)
> +dell_wmi_process_key(wdev, buffer_entry[1],
> buffer_entry[2]);
> +}
> +#else
> +/* Extended data is currently ignored */
> +if (len > 2)
> +dell_wmi_process_key(wdev, buffer_entry[1],
> buffer_entry[2]);
> +#endif
Wouldn't it be better if the header file provided a static inline definitions
for `dell_privacy_valid()` and `dell_privacy_process_event()` - if
CONFIG_DELL_PRIVACY
is not enabled - that return false and do nothing, respectively? The same way
it's done in dell-smbios.h.
> +break;
> +default: /* Unknown event */
> +pr_info("Unknown WMI event type 0x%x\n",
> +(int)buffer_entry[1]);
> +break;
> +}
> +
> +buffer_entry += len;
> +
> +}
>
> }
> [...]
Regards,
Barnabás Pőcze
> https://lore.kernel.org/linux-input/20201026144512.621479-1-markpear...@lenovo.com/
>
> Is that merged? If not, it's fine as-is until then, and someone has to
> pick to go first :)
It is not, to my knowledge. Nonetheless I figured the information may be
relevant.
Regards,
Barnabás Pőcze
with another one:
https://lore.kernel.org/linux-input/20201026144512.621479-1-markpear...@lenovo.com/
Regards,
Barnabás Pőcze
L;
> + }
> +
> + ihid->polling_thread = kthread_create(i2c_hid_polling_thread, ihid,
> + "I2C HID polling thread");
> +
> + if (!IS_ERR(ihid->polling_thread)) {
> + pr_info("I2C HID polling thread created");
> + wake_up_process(ihid->polling_thread);
> + return 0;
> + }
> +
> + return PTR_ERR(ihid->polling_thread);
I would personally rewrite this parts as
```
if (IS_ERR(...)) {
dev_err(...);
return PTR_ERR(...);
}
return 0;
```
> +}
> [...]
Regards,
Barnabás Pőcze
rmat
> of patch. Or do you think a reviewed-by tag from you after you think
> the next version is of production quality is a better way to credit
> you?
Thanks, but there is no need for any tag from me. I am no maintainer/reviewer
of/for
the affected subsystem.
> [...]
Regards,
Barnabás Pőcze
d this problem, only allow a certain number of
> > > iterations
> > > for the inner loop, to guarantee that the kthread can stop in any case.
> >
> > I mean if "data is supplied at a high enough rate" does happen, this is
> > an abnormal case and indicates a bug. So we shouldn't cover it up. We
> > expect the user to report it to us.
> >
> > >
>
> I agree in principle, but if this abnormal case ever occurs, that'll prevent
> this module from being unloaded since `kthread_stop()` will hang because the
> thread is "stuck" in the inner loop, never checking `kthread_should_stop()`.
> That's why I think it makes sense to only allow a certain number of operations
> for the inner loop, and maybe show a warning if that's exceeded:
>
> for (i = 0; i < max_iter && interrupt_line_active(...); i++) {
>
> }
>
> WARN_ON[CE](i == max_iter[, "data is coming in at an unreasonably high
> rate"]);
>
I now realize that WARN_ON[CE] is probably not the best fit here, `hid_warn()`
is possibly better.
> or something like this, where `max_iter` could possibly be some value
> dependent on
> `polling_interval_active_us`, or even just a constant.
> [...]
Regards,
Barnabás Pőcze
polling_interval_active_us + 100);
> >> >> + }
> >> >> +
> >> >> + usleep_range(polling_interval_idle,
> >> >> +polling_interval_idle + 1000);
> >> >> + }
> >> >> +
> >> >> + do_exit(0);
> >> >> + return 0;
> >> >> +}
> [...]
> Thank you for offering your understandings on this patch. When I'm going
> to submit next version, I will add a "Signed-off-by" tag with your name
> and email, does it look good to you?
> [...]
I'm not sure if that follows proper procedures.
"The sign-off is a simple line at the end of the explanation for the patch,
which
certifies that you wrote it or otherwise have the right to pass it on as an
open-source patch."[1]
I'm not the author, nor co-author, nor am I going to pass this patch on, so I
don't
think that's appropriate.
Furthermore, please note that
"[...] you may optionally add a Cc: tag to the patch. **This is the only tag
which
might be added without an explicit action by the person it names** - but it
should
indicate that this person was copied on the patch."[2]
(emphasis mine)
Regards,
Barnabás Pőcze
[1]:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
[2]:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
;> + return 0;
> >> +}
> [...]
> >Excuse my ignorance, but I do not understand why the following two changes
> >are not enough:
> >
> >in `i2c_hid_suspend()`:
> > if (polling_mode == I2C_POLLING_DISABLED)
> > disable_irq(client->irq);
> >
> >in `i2c_hid_resume()`:
> > if (polling_mode == I2C_POLLING_DISABLED)
> > enable_irq(client->irq);
> >
> I think we shouldn't call enable/disable_irq_wake in polling mode
> where we don't set up irq.
I think I now understand what you mean. I'm not sure, but it seems logical to me
that you can enable/disable irq wake regardless whether any irq handlers are
registered or not. Therefore, I figure it makes sense to take the safe path,
and don't touch irq wake when polling, just as you did.
> [...]
Regards,
Barnabás Pőcze
lies);
> if (ret)
> @@ -1225,7 +1343,8 @@ static int i2c_hid_resume(struct device *dev)
> wake_status);
> }
>
> - enable_irq(client->irq);
> + if (polling_mode == I2C_POLLING_DISABLED)
> + enable_irq(client->irq);
> [...]
Before this patch, if a device cannot wake up, then the regulators are disabled
when suspending, after this patch, regulators are only disabled if polling is
used. But they are enabled if the device cannot wake up *or* polling is used.
Excuse my ignorance, but I do not understand why the following two changes are
not enough:
in `i2c_hid_suspend()`:
if (polling_mode == I2C_POLLING_DISABLED)
disable_irq(client->irq);
in `i2c_hid_resume()`:
if (polling_mode == I2C_POLLING_DISABLED)
enable_irq(client->irq);
Regards,
Barnabás Pőcze
)
> wake_status);
> }
>
> - enable_irq(client->irq);
> + if (polling_mode != I2C_POLLING_DISABLED)
> + enable_irq(client->irq);
>
The IRQ is enabled when resuming if polling is *on*. It should be enabled if
polling is *off*
in my opinion.
> [...]
Regards,
Barnabás Pőcze
Hi,
I had some difficulty finding who should receive this patch, and I am not
sure I got it right. Could someone please confirm that any of you
can take this patch, or should I resend it? (In that case, to whom?)
Thank you,
Barnabás Pőcze
> Commit af3b854492f351d1ff3b4744a83bf5ff7eed4
lag a bit, so give it 10 minutes and
> then it should be there.
> [...]
I see, thanks!
Regards,
Barnabás Pőcze
]
I don't know if the problem is on my end, but opening that url
results in an "Invalid branch: review-hans" error.
Thanks,
Barnabás Pőcze
devices? Why is it not possible to write a driver that implements this
interface
and directly modifies device registers? Am I missing something obvious here?
> [...]
Thanks,
Barnabás Pőcze
the HP
> > Pavilion 11 x360"
>
> Oohoo, what a wonderful world :)
>
Splendid world, indeed. I'm wondering, however, why the incorrect state
is reported? Is it similar to the linked issue on the Manjaro forum, where a
different bit is seemingly used to report the tablet mode state, or something
else?
I'm also wondering why it was chosen that a *set* bit means that the tablet
mode is *off*. All these problems could've been easily avoided... (given that
I'm not missing anything obvious).
> [...]
Regards,
Barnabás Pőcze
UE in both functions appropriately by
* returning "TRUE" in error_type_string(),
* adjusting the return value to true (1) in adjust_error_retval().
Furthermore, simplify the logic of handling EI_ETYPE_NULL
in adjust_error_retval().
Signed-off-by: Barnabás Pőcze
---
kernel/fail_function.c |
ouldn't find any
> documentation on this, I assumed it didn't behave the same and would
> emit an error message.
>
> The reason I don't want to emit an error message here is that the module
> can be loaded for devices that it's not intended (and that's not
> something we can fix with a better MODULE_ALIAS as Microsoft cleverly
> named their 5th generation Surface Pro "Surface Pro", without any
> version number). Mainly, I don't want users to get a random error
> message that doesn't indicate an actual error.
>
I wasn't sure, so I tested it. It prints the "no device" message, but that's it,
no more indication of the -ENODEV error in the kernel log during automatic
module loading at boot.
You could print "no compatible Microsoft Surface device found, exitig" (or
something
similar). I think this provides enough information for any user to decide if
they should be concerned or not.
> >> + }
> [...]
Regards,
Barnabás Pőcze
er(_gpe_driver);
> + return status;
> + }
> +
It may be a matter of preference, but I think the 'if (err) goto X' pattern
would
be better in this function (at least for the last 3 or so error paths).
> + surface_gpe_device = pdev;
> + return 0;
> +}
> +module_init(surface_gpe_init);
> +
> +static void __exit surface_gpe_exit(void)
> +{
> + if (!surface_gpe_device)
> + return;
If you returned -ENODEV in init when no DMI match is found,
then this check would be redundant.
> [...]
Regards,
Barnabás Pőcze
40 matches
Mail list logo
