Re: [RFC, PATCH v1] platform/x86: intel-vbtn: Convert to pure ACPI driver

[Rafael J. Wysocki]

On Tuesday, February 6, 2018 11:37:54 PM CET Andy Shevchenko wrote:
> Yeah, that's fixed in the branch.
> 
> P.S. Apologies for top-posting, wrote from phone in order to not waste time
> 
> On Tuesday, February 6, 2018, <Mario.Limonciello@xxxxxxxx> wrote:
> 
> > > -----Original Message-----
> > > From: platform-driver-x86-owner@xxxxxxxxxxxxxxx [mailto:
> > platform-driver-x86-
> > > owner@xxxxxxxxxxxxxxx] On Behalf Of Andy Shevchenko
> > > Sent: Monday, February 5, 2018 10:18 AM
> > > To: Darren Hart <dvhart@xxxxxxxxxxxxx>
> > > Cc: platform-driver-x86@xxxxxxxxxxxxxxx; AceLan Kao
> > > <acelan.kao@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > > pali.rohar@xxxxxxxxx; Limonciello, Mario <Mario_Limonciello@xxxxxxxx>;
> > Rafael
> > > Wysocki <rjw@xxxxxxxxxxxxx>
> > > Subject: Re: [RFC, PATCH v1] platform/x86: intel-vbtn: Convert to pure
> > ACPI driver
> > >
> > > On Wed, 2018-01-31 at 13:37 -0800, Darren Hart wrote:
> > > > On Wed, Jan 31, 2018 at 08:26:14PM +0200, Andy Shevchenko wrote:
> > > > > Remove code duplication by converting driver to pure ACPI one.
> > > >
> > >
> > > I have fixed couple issues, but...
> > >
> > > > +Rafael
> > > >
> > > > I don't see any reason *not* to do this, it seems the acpi driver
> > > > model provides
> > > > some of the boilerplate stuff we were doing manually before as a
> > > > platform
> > > > device. I'm scratching my head wondering why we did this as a
> > > > platform-driver in
> > > > the first place now...

It looks like I have overlooked this.

Generally speaking, binding drivers to ACPI device objects is a bad idea,
because it is sort of what binding drivers to struct device_node in OF would
be.  It was an unfortunate design choice for struct acpi_device to be based
on struct device in the first place, but undoing it is not worth the effort
and pain.

In some instances (PCI, PNP, I2C, SPI etc) there is a device object to bind a
driver to already and the corresponding struct acpi_device is the "companion"
of it.  In those cases binding a driver to the struct acpi_device itself
clearly doesn't make sense.

In the cases like the $subject one it is less clear, but for symmetry and
general confusion avoidance it is better to have a struct device representing
the "physical" thing *in* *addition* to the struct acpi_device and use the
latter as the "companion" of it.  That's why we have the platform device in
this particular case.

Please drop this patch, it doesn't go in the right direction IMO.

Thanks,
Rafael