Hi Thomas! I was prepping my v5 patch to send in and trying to figure
out everything I changed for the change list comments, but I stumbled
on a few comments here that I wanted to ask you about as I realized I
did not fully address them.
Den fre 3 jan. 2025 kl 20:37 skrev Thomas Weißschuh <thomas@xxxxxxxx>:
>
> > +This driver implements the
> > +Documentation/userspace-api/sysfs-platform_profile.rst interface for working
>
> You can make this real reST link which will be converted into a
> hyperlink.
>
Here I actually tried this a few different ways (linking to the entire
page instead of a specific section within the page) but would always
get a warning and then no link when I built the docs. However, from
finding other examples then I found just giving the path like this is
actually giving me a link in both the htmldocs and pdfdocs with the
title of the target page exactly as I wanted... with that in mind,
does it seem ok to leave as-is or is there a syntax that you would
recommend instead to link directly to a page (and not a section within
a page)?
> > +static int galaxybook_acpi_method(struct samsung_galaxybook *galaxybook, acpi_string method,
> > + struct sawb *in_buf, size_t len, struct sawb *out_buf)
>
> in_buf and out_buf are always the same.
>
> > +{
> > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> > + union acpi_object in_obj, *out_obj;
> > + struct acpi_object_list input;
> > + acpi_status status;
> > + int err;
> > +
> > + in_obj.type = ACPI_TYPE_BUFFER;
> > + in_obj.buffer.length = len;
> > + in_obj.buffer.pointer = (u8 *)in_buf;
> > +
> > + input.count = 1;
> > + input.pointer = &in_obj;
> > +
> > + status = acpi_evaluate_object_typed(galaxybook->acpi->handle, method, &input, &output,
> > + ACPI_TYPE_BUFFER);
> > +
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(&galaxybook->acpi->dev, "failed to execute method %s; got %s\n",
> > + method, acpi_format_exception(status));
> > + return -EIO;
> > + }
> > +
> > + out_obj = output.pointer;
> > +
> > + if (out_obj->buffer.length != len || out_obj->buffer.length < SAWB_GUNM_POS + 1) {
> > + dev_err(&galaxybook->acpi->dev, "failed to execute method %s; "
> > + "response length mismatch\n", method);
> > + err = -EPROTO;
> > + goto out_free;
> > + }
> > + if (out_obj->buffer.pointer[SAWB_RFLG_POS] != RFLG_SUCCESS) {
> > + dev_err(&galaxybook->acpi->dev, "failed to execute method %s; "
> > + "device did not respond with success code 0x%x\n",
> > + method, RFLG_SUCCESS);
> > + err = -ENXIO;
> > + goto out_free;
> > + }
> > + if (out_obj->buffer.pointer[SAWB_GUNM_POS] == GUNM_FAIL) {
> > + dev_err(&galaxybook->acpi->dev,
> > + "failed to execute method %s; device responded with failure code 0x%x\n",
> > + method, GUNM_FAIL);
> > + err = -ENXIO;
> > + goto out_free;
> > + }
> > +
> > + memcpy(out_buf, out_obj->buffer.pointer, len);
>
> Nit: This memcpy() could be avoided by having the ACPI core write directly
> into out_buf. It would also remove the allocation.
>
Now I have replaced in_buf and out_buf with just one parameter, buf.
Now it feels like I cannot write directly to it (since I am reusing
the same buf as the outgoing value) so have left the memcpy in place.
I guess I would need to choose to have 2 buffers or use one and do a
memcpy at the end like this (which is how I have it now in my v5
draft) .. am I thinking wrong here and/or is there a preference
between the two alternatives? I can just for now say that "usage" of
this function in all of the other functions feels easier to just have
one buffer... :)
> > +static int power_on_lid_open_acpi_set(struct samsung_galaxybook *galaxybook, const bool value)
> > +{
> > + struct sawb buf = { 0 };
> > +
> > + buf.safn = SAFN;
> > + buf.sasb = SASB_POWER_MANAGEMENT;
> > + buf.gunm = GUNM_POWER_MANAGEMENT;
> > + buf.guds[0] = GUDS_POWER_ON_LID_OPEN;
> > + buf.guds[1] = GUDS_POWER_ON_LID_OPEN_SET;
> > + buf.guds[2] = value ? 1 : 0;
>
> No need for the ternary.
>
I did not have this before but it was requested to be added by Ilpo
IIRC. I am ok with either way but would just need to know which is
preferred between the two :)
> > +static void galaxybook_i8042_filter_remove(void *data)
> > +{
> > + struct samsung_galaxybook *galaxybook = data;
> > +
> > + i8042_remove_filter(galaxybook_i8042_filter);
> > + if (galaxybook->has_kbd_backlight)
> > + cancel_work_sync(&galaxybook->kbd_backlight_hotkey_work);
> > + if (galaxybook->has_camera_lens_cover)
> > + cancel_work_sync(&galaxybook->camera_lens_cover_hotkey_work);
> > +}
> > +
> > +static int galaxybook_i8042_filter_install(struct samsung_galaxybook *galaxybook)
> > +{
> > + int err;
> > +
> > + if (!galaxybook->has_kbd_backlight && !galaxybook->has_camera_lens_cover)
> > + return 0;
> > +
> > + if (galaxybook->has_kbd_backlight)
> > + INIT_WORK(&galaxybook->kbd_backlight_hotkey_work,
> > + galaxybook_kbd_backlight_hotkey_work);
> > +
> > + if (galaxybook->has_camera_lens_cover)
> > + INIT_WORK(&galaxybook->camera_lens_cover_hotkey_work,
> > + galaxybook_camera_lens_cover_hotkey_work);
>
> I would just always initialize and cancel the work_structs.
> This is no hot path and it makes the code simpler.
>
I apologize but I don't think I am 100% following what you mean here.
Is there an example or more information that can be provided so I can
know what should be changed here?
> > + err = galaxybook_enable_acpi_notify(galaxybook);
> > + if (err)
> > + dev_warn(&galaxybook->platform->dev, "failed to enable ACPI notifications; "
> > + "some hotkeys will not be supported\n");
>
> Will this dev_warn() trigger always for certain devices? If so a
> dev_info() would be more appropriate IMO.
>
Yes good point here; for the devices which have this condition, they
will get this message every single time, so I will change it to info.
I can also change it to debug if that makes even more sense.
> [...]
Other than these I think (hope) I have tried to address everything
else from all other comments. I will hold off on sending this v5 in
case you reply soon-ish but otherwise will go ahead and send it as-is
in the next day or two just to keep the feedback cycle going.
Thank you again!
Best regards,
Joshua
