Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jul 14, 2020 at 12:38:01PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 13, 2020 at 04:02:01PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 13, 2020 at 10:44:36PM +0300, Andy Shevchenko wrote:
> > > Switch the platform code to use x86_id_table and accompanying API
> > > instead of custom comparison against x86 CPU model.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  arch/x86/pci/intel_mid_pci.c | 17 +++++++++++++++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
> > > index 00c62115f39c..d8af4787e616 100644
> > > --- a/arch/x86/pci/intel_mid_pci.c
> > > +++ b/arch/x86/pci/intel_mid_pci.c
> > > @@ -28,10 +28,12 @@
> > >  #include <linux/io.h>
> > >  #include <linux/smp.h>
> > >  
> > > +#include <asm/cpu_device_id.h>
> > >  #include <asm/segment.h>
> > >  #include <asm/pci_x86.h>
> > >  #include <asm/hw_irq.h>
> > >  #include <asm/io_apic.h>
> > > +#include <asm/intel-family.h>
> > >  #include <asm/intel-mid.h>
> > >  
> > >  #define PCIE_CAP_OFFSET	0x100
> > > @@ -211,9 +213,16 @@ static int pci_write(struct pci_bus *bus, unsigned int devfn, int where,
> > >  			       where, size, value);
> > >  }
> > >  
> > > +static const struct x86_cpu_id intel_mid_cpu_ids[] = {
> > > +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
> > > +	{}
> > > +};
> > > +
> > >  static int intel_mid_pci_irq_enable(struct pci_dev *dev)
> > >  {
> > > +	const struct x86_cpu_id *id;
> > >  	struct irq_alloc_info info;
> > > +	u16 model = 0;
> > >  	int polarity;
> > >  	int ret;
> > >  	u8 gsi;
> > > @@ -227,8 +236,12 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
> > >  		return ret;
> > >  	}
> > >  
> > > -	switch (intel_mid_identify_cpu()) {
> > > -	case INTEL_MID_CPU_CHIP_TANGIER:
> > > +	id = x86_match_cpu(intel_mid_cpu_ids);
> > > +	if (id)
> > > +		model = id->model;
> > > +
> > > +	switch (model) {
> > > +	case INTEL_FAM6_ATOM_SILVERMONT_MID:
> > 
> > Is there a magic decoder ring somewhere that connects
> > INTEL_MID_CPU_CHIP_TANGIER and INTEL_FAM6_ATOM_SILVERMONT_MID?
> 
> Yes. And the idea is to get rid of it.

OK.  You don't want to even include a mention of it in the commit log
to help people connect the dots and verify that this change is
correct?

> > I don't know how to verify that the new code is equivalent to the old.
> > 
> > Or maybe the new code is *better* than the old, in which case the
> > subject/commit log should mention that it's fixing or improving
> > something.
> > 
> > Also, there are a number of other places that check for
> > "intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER":
> > 
> >   mrfld_pinctrl_init
> >   register_mrfld_power_btn
> >   mrfld_legacy_rtc_init
> >   mrfld_sd_init
> >   spidev_platform_data
> >   register_mid_wdt
> >   sfi_parse_devs
> 
> >   atomisp_css_input_set_mode
> 
> This has been pending in Mauro's tree.
> 
> > Maybe they should all be changed together?  Or maybe this needs an
> > explanation about why some places need intel_mid_identify_cpu() and
> > others need x86_match_cpu()?
> 
> No. The rest is subject to huge clean up (complete removal) in the future.
> I don't want to waste time on something which I will remove for sure.

OK, I'll leave this to the x86 guys.  If I were merging it I would
want a little more explanation, but this isn't PCI-related at all and
I'm sure they understand this better than I do.

Bjorn