Message-ID: <c3e7e78b-a38c-dcc2-092d-e1809ae02a4c@linux.intel.com>
Date: Thu, 9 Jan 2025 10:34:40 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Thomas Weißschuh <thomas@...ch.de>
cc: Joshua Grisham <josh@...huagrisham.com>, 
    Hans de Goede <hdegoede@...hat.com>, W_Armin@....de, 
    platform-driver-x86@...r.kernel.org, corbet@....net, 
    linux-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] platform/x86: samsung-galaxybook: Add samsung-galaxybook
 driver

On Wed, 8 Jan 2025, Thomas Weißschuh wrote:
> 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.

Yes, in the bool -> uXX conversions, I prefer explicit values even if they 
in many cases happen to match to what C implicit conversion does (if the 
value wouldn't match to 1, you'd need to use that operator anyway).

"true" and "BIT(0)" are conceptially distinct things even if they map to 
the same representation.

Having the explicit conversion confirms the submitter (hopefully :-)) 
spend a second to confirm that true => 1 holds. Without the explicit 
conversion, it would be hard to see what went on inside the submitter 
head. I can ask for it today, but my perspective is long-term, say 5-10 
years from now, the person might no longer be around and somebody ends up 
staring a commit which has such a problem. Explicit conversion avoids that 
ambiguity. (Obviously such problems are quire rare but could happen.)

-- 
 i.

Powered by blists - more mailing lists