Message-ID: <19caaf5c-dbdd-43a4-989f-35a810dbe91a@t-8ch.de>
Date: Wed, 8 Jan 2025 23:07:49 +0100
From: Thomas Weißschuh <thomas@...ch.de>
To: Joshua Grisham <josh@...huagrisham.com>
Cc: 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!

On 2025-01-08 22:37:01+0100, Joshua Grisham wrote:
> 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)?

If it works, then leave it as is.
To exact warning would have been nice though :-)

Did you try :ref:`userspace-api/sysfs-platform_profile`?

> > > +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... :)

I'm not sure if there is a preference.

But why can't you modify the buffer if it is shared between input and
output? The caller already has to accept that its buffer will be
overwritten.
If it is overwritten once or twice should not matter.

But maybe I'm misunderstanding.

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

Then leave it as is.

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

I would remove the conditionals for has_kbd_backlight and
has_camera_lens_cover. And unconditionally do:

INIT_WORK(&galaxybook->kbd_backlight_hotkey_work,
          galaxybook_kbd_backlight_hotkey_work);
INIT_WORK(&galaxybook->camera_lens_cover_hotkey_work,
          galaxybook_camera_lens_cover_hotkey_work);

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

debug would be even better, indeed.

> > [...]
> 
> 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.

Looking forward to v5!

Powered by blists - more mailing lists