Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

On Thu, 2019-03-28 at 22:29 -0700, Rajat Jain wrote:
> Hi Srinivas,
> 
> 

[...]

> So if everyone here thinks we should completely switch to using
> > > the
> > > ACPI HID "INT33A1" for attaching to the device, sure, we can do
> > > that.
> > > Yes, for Chromeos, we can put in a work around internally that
> > > ensures
> > > that shipping chromebooks (Kabylake etc) can work fine without
> > > that
> > > ACPI ID. What I do not know is if that will cause any regressions
> > > outside of Chromeos. Can you discuss with Rafael, Andy, Srinivas
> > > internally and let me know on how they'd like to proceed on this.
> > > 
> > > The other option is to apply this patch as-is so we know that
> > > there
> > > is
> > > no "functional change" and thus no possible regression (so the
> > > driver
> > > continues to attach to those and only those systems that s it
> > > used
> > > to,
> > > before this patch). And then introduce the ACPI ID Change as a
> > > new
> > > independent patch.
> > 
> > Use INT33A1 to enumerate, if this id doesn't exist then fallback to
> > the
> > cpuid style. The aim should be that we don't have to add any more
> > CPU
> > model to enumerate. But most probably register map will differ so
> > we
> > still may end up adding some CPU model relationship.
> 
> Thanks for the guidance. Just to reconfirm my understanding of your
> suggestion:
> 
> Here is the suggestive code Rajneesh provided, and I intend to do it
> similarly:
> 
> +static const struct acpi_device_id pmc_acpi_ids[] = {
> +             {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID
> PNP0D80*/
> +             { }
> +};
> 
> +static struct platform_driver pmc_plat_driver = {
> +             .remove = pmc_plat_remove,
> +             .probe = pmc_plat_probe,
> +             .driver = {
> +                             .name = "pmc_core_driver",
> +                             .acpi_match_table =
> ACPI_PTR(pmc_acpi_ids),
> +             },
> +};
> 
> My understanding is that with this, the kernel would use
> acpi_match_table to automatically create the platform_device on a
> platform where ACPI tables contain the INT33A1 HID, and thus we don't
> need to create the platform_device in module_init time on such
> platforms.
Yes. There will be /sys/bus/platform/devices/INT33A1:00.


>  So are you saying that during init, I should check if the
> ACPI HID INT33A1 is not present on the platform, then use the cpu_id
> table to create the platform_device? Thus newer platforms will not
> need an entry in the table.
Yes. Preferably in a different file as Andy would like. So the the
current driver only implements platform driver for INT33A1. The other driver which will enumerate on CPUID and create INT33A1 platform device  if there is no ACPI match via acpi_match_device() or similar API, for INT33A1. When you create a platform device the pmc driver will be probed.

Thanks,
Srinivas