Message-ID: <CAMF+KebS6eEGEVzrO3Bm3CfL7OYP7-XxUp7hLiDiwUrjWOEJYQ@mail.gmail.com>
Date: Wed, 8 Jan 2025 22:37:01 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: Thomas Weißschuh <thomas@...ch.de>
Cc: Joshua Grisham <josh@...huagrisham.com>, ilpo.jarvinen@...ux.intel.com, 
	hdegoede@...hat.com, W_Armin@....de, platform-driver-x86@...r.kernel.org, 
	corbet@....net, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

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@...ch.de>:
>

> > +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

Powered by blists - more mailing lists