Message-ID: <CAJZ5v0i4in=oyhXKOQ1MeoRP5fAhHdEFgZZp98N0pF8hB6BtbQ@mail.gmail.com>
Date:   Mon, 9 Oct 2023 14:27:43 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     "Wilczynski, Michal" <michal.wilczynski@...el.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        nvdimm@...ts.linux.dev, rafael.j.wysocki@...el.com,
        lenb@...nel.org, dan.j.williams@...el.com,
        vishal.l.verma@...el.com, ira.weiny@...el.com
Subject: Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver

On Mon, Oct 9, 2023 at 10:40 AM Wilczynski, Michal
<michal.wilczynski@...el.com> wrote:
>
>
> Hi !
>
> Thanks a lot for a review, to both of you ! :-)
>
> On 10/7/2023 12:43 PM, Rafael J. Wysocki wrote:
> > On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
> >> <andriy.shevchenko@...el.com> wrote:
> >>> On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
> >>>> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
> >>>> <michal.wilczynski@...el.com> wrote:
> >>> ...
> >>>
> >>>>>  struct acpi_ac {
> >>>>>         struct power_supply *charger;
> >>>>>         struct power_supply_desc charger_desc;
> >>>>> -       struct acpi_device *device;
> >>>>> +       struct device *dev;
> >>>> I'm not convinced about this change.
> >>>>
> >>>> If I'm not mistaken, you only use the dev pointer above to get the
> >>>> ACPI_COMPANION() of it, but the latter is already found in _probe(),
> >>>> so it can be stored in struct acpi_ac for later use and then the dev
> >>>> pointer in there will not be necessary any more.
> >>>>
> >>>> That will save you a bunch of ACPI_HANDLE() evaluations and there's
> >>>> nothing wrong with using ac->device->handle.  The patch will then
> >>>> become almost trivial AFAICS and if you really need to get from ac to
> >>>> the underlying platform device, a pointer to it can be added to struct
> >>>> acpi_ac without removing the ACPI device pointer from it.
>
> Yeah we could add platform device without removing acpi device, and
> yes that would introduce data duplication, like Andy noticed.

But he hasn't answered my question regarding what data duplication he
meant, exactly.

So what data duplication do you mean?

> And yes, maybe for this particular driver there is little impact (as struct device is not
> really used), but in my opinion we should adhere to some common coding
> pattern among all acpi drivers in order for the code to be easier to maintain
> and improve readability, also for any newcomer.

Well, maybe.

But then please minimize the number of code lines changed in this
particular patch and please avoid changes that are not directly
related to the purpose of the patch.

> >>> The idea behind is to eliminate data duplication.
> >> What data duplication exactly do you mean?
> >>
> >> struct acpi_device *device is replaced with struct device *dev which
> >> is the same size.  The latter is then used to obtain a struct
> >> acpi_device pointer.  Why is it better to do this than to store the
> >> struct acpi_device itself?
> > This should be "store the struct acpi_device pointer itself", sorry.
>
> Hi,
> So let me explain the reasoning here:
>
> 1) I've had a pleasure to work with different drivers in ACPI directory. In my
>     opinion the best ones I've seen, were build around the concept of struct
>     device (e.g NFIT). It's a struct that's well understood in the kernel, and
>     it's easier to understand without having to read any ACPI specific code.
>     If I see something like ACPI_HANDLE(dev), I think 'ah-ha -  having a struct
>     device I can easily get struct acpi_device - they're connected'. And I think
>     using a standardized macro, instead of manually dereferencing pointers is
>     also much easier for ACPI newbs reading the code, as it hides a bit complexity
>     of getting acpi device from struct device. And to be honest I don't think there would
>     be any measurable performance change, as those code paths are far from
>     being considered 'hot paths'. So if we can get the code easier to understand
>     from a newcomer perspective, why not do it.

I have a differing opinion on a couple of points above, and let's make
one change at a time.

>
> 2) I think it would be good to stick to the convention, and introduce some coding
>      pattern, for now some drivers store the struct device pointer, and other store
>      acpi device pointer . As I'm doing this change acpi device pointer become less
>      relevant, as we're using platform device. So to reflect that loss of relevance
>      we can choose to adhere to a pattern where we use it less and less, and the
>      winning approach would be to use 'struct device' by default everywhere we can
>      so maybe eventually we would be able to lose acpi_device altogether at some point,
>      as most of the usage is to retrieve acpi handle and execute some AML method.
>      So in my understanding acpi device is already obsolete at this point, if we can
>      manage to use it less and less, and eventually wipe it out then why not ;-)

No, ACPI device is not obsolete, it will still be needed for various
things, like power management and hotplug.

Also, I'd rather store a struct acpi_device than acpi_handle, because
the latter is much better from the compile-time type correctness
checks and similar.

I can send my version of the $subject patch just fine if you strongly
disagree with me.

Powered by blists - more mailing lists