Re: [PATCH RESEND v6] platform: x86: Add ChromeOS ACPI device driver

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Muhammad Usama Anjum" <usama.anjum@collabora.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Len Brown" <lenb@kernel.org>,
	"Mark Gross" <markgross@kernel.org>,
	"Collabora Kernel ML" <kernel@collabora.com>,
	"Guenter Roeck" <groeck@chromium.org>,
	"Benson Leung" <bleung@chromium.org>,
	"Dmitry Torokhov" <dtor@chromium.org>,
	"Gwendal Grignou" <gwendal@chromium.org>,
	vbendeb@chromium.org, "Andy Shevchenko" <andy@infradead.org>,
	"Ayman Bagabas" <ayman.bagabas@gmail.com>,
	"Benjamin Tissoires" <benjamin.tissoires@redhat.com>,
	"Blaž Hrastnik" <blaz@mxxn.io>,
	"Darren Hart" <dvhart@infradead.org>,
	"Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Jeremy Soller" <jeremy@system76.com>,
	"Mattias Jacobsson" <2pi@mok.nu>,
	"Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>,
	"Rajat Jain" <rajatja@google.com>,
	"Srinivas Pandruvada" <srinivas.pandruvada@linux.intel.com>,
	"Platform Driver" <platform-driver-x86@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"ACPI Devel Maling List" <linux-acpi@vger.kernel.org>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	"Enric Balletbo i Serra" <eballetbo@gmail.com>
Subject: Re: [PATCH RESEND v6] platform: x86: Add ChromeOS ACPI device driver
Date: Mon, 11 Apr 2022 15:37:32 +0200	[thread overview]
Message-ID: <CAJZ5v0g2UDOR3mYsdqnPcpYgmecY706YQcTKTWMRtezkK0sfaQ@mail.gmail.com> (raw)
In-Reply-To: <708fb1ec-4e57-7a1d-b0a0-a3a10b3cacf3@redhat.com>

On Mon, Apr 11, 2022 at 3:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 4/7/22 14:35, Muhammad Usama Anjum wrote:
> > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >
> > The x86 Chromebooks have ChromeOS ACPI device. This driver attaches to
> > the ChromeOS ACPI device and exports the values reported by ACPI in a
> > sysfs directory. This data isn't present in ACPI tables when read
> > through ACPI tools, hence a driver is needed to do it. The driver gets
> > data from firmware using ACPI component of the kernel. The ACPI values
> > are presented in string form (numbers as decimal values) or binary
> > blobs, and can be accessed as the contents of the appropriate read only
> > files in the standard ACPI device's sysfs directory tree. This data is
> > consumed by the ChromeOS user space.
> >
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>
>
> Thanks overall this looks pretty good to me.  The only remark which
> I have is that I would like to see the Kconfig symbol changed
> from CONFIG_ACPI_CHROMEOS to CONFIG_CHROMEOS_ACPI to match the
> filename.
>
> CONFIG_ACPI_CHROMEOS to me suggests that this is an ACPI subsystem
> Kconfig option which, with the driver living under
> drivers/platform/x86 it is not.
>
> There is no need to send a new version for this, if you agree
> with the change let me know and I can change this while merging
> the driver.
>
> Rafael, before I merge this do you have any (more) remarks
> about this driver?

I'm not sure why it has to be an acpi_driver.

I think that the generic enumeration code creates a platform device
for this ACPI device object, so why can't it bind to that platform
device?

Generally speaking, IMV we should avoid adding drivers binding
directly to ACPI device objects, because that is confusing (it is kind
of like binding directly to an of_node) and it should be entirely
avoidable.