On Wed, 13 May 2020 at 08:08, Mika Westerberg <mika.westerb...@linux.intel.com> wrote: > I think this one should contain KernelVersion as well, see > Documentation/ABI/README.
Thanks, I'll fix that up. > I think you can always include this header without #ifs Thanks. > > static struct resource wdt_ich_res[] = { > > @@ -221,6 +236,16 @@ enum lpc_chipsets { > > LPC_APL, /* Apollo Lake SoC */ > > LPC_GLK, /* Gemini Lake SoC */ > > LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/ > > + LPC_SPT, /* Sunrise Point */ > > + LPC_KLK, /* Kaby Lake */ > KBL for Kaby Lake I can fix up all those, but out of interest how did you "know" the right three digit identifier to use? > This is not PCH, Cactus Ridge is Thunderbolt host controller AFAIK. This was suggested from someone testing the original spi_lpc.c code on a macbook, I can remove it for now and work out if it's incorrect later. > For example these PCI IDs are for the SPI-NOR controller (not LPC > controller) so this causes this driver to try to bind to a completely > different device which it cannot handle. I'm really wondering if drivers/mfd/lpc_ich.c is the right place for this kind of "just expose one byte of PCI config space" functionality. Certainly drivers/platform/x86/intel_spi_lpc.c is much simpler, and would also allow me to do some of the chipsec tests in the future -- for instance if BIOSWE is unset but BLE is set, try setting BIOSWE and check that SMM clears it back. > > + char tmp[2]; > > Wouldn't this need to account the '\0' as well? It's one char ('1' or '0') plus '`\0` -- no? > I think "spi" is bit too general name here. I would expect "spi" to > actually refer to something connected to spi bus and possibly coming > from drivers/spi/*. > Perhaps "bios_protections" or something like that. Sure, that's a good idea. I know BIOS is a badword now, so how about just "firmware"? so /sys/kernel/security/firmware/bioswe > > + securityfs_remove(priv->spi_dir); > > + return -1; > I don't know securityfs well enought but I think -1 is not correct here > and if you want that then maybe -EPERM instead. I will look, I don't think the actual value is terribly important. The only time I can trigger this is forgetting to remove the securityfs file in module unload, and then trying to re-insert the module -- which failed with -EEXIST from memory. > I wonder if you can simply call > securityfs_remove(priv->spi_dir); > and that removes the children automatically? I'm do not know securityfs > so it may not be the case. No, that doesn't work. > > struct intel_spi_boardinfo { > > enum intel_spi_type type; > > bool writeable; > > + bool ble; > > + bool smm_bwp; > > I don't think these belong here. They should be part of the lpc private > structure instead (lpc_ich_priv). Can fix, thanks. Richard.