Hi Arnd, Thanks for your email On Wed, 23 Nov 2022 at 12:58, Arnd Bergmann <arnd@xxxxxxxx> wrote: > > On Tue, Nov 22, 2022, at 21:12, Tomer Maimon wrote: > > Add Nuvoton BMC NPCM LPC BIOS post code (BPC) driver. > > > > The NPCM BPC monitoring two configurable I/O address written by the host > > on the Low Pin Count (LPC) bus. > > > > Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx> > > --- > > drivers/soc/Kconfig | 1 + > > drivers/soc/Makefile | 1 + > > drivers/soc/nuvoton/Kconfig | 24 ++ > > drivers/soc/nuvoton/Makefile | 3 + > > drivers/soc/nuvoton/npcm-lpc-bpc.c | 396 +++++++++++++++++++++++++++++ > > In general, I try to keep drivers/soc/ for drivers that are > used purely inside of the kernel and don't provide their > own user space ABI, those should normally be part of > some subsystem grouped by functionality. > > It appears that we have similar drivers for aspeed already, > so there is some precedent, but I would still like to ask > you and Joel to try to make sure the two are compatible, > or ideally share the code for the user-facing part of the > LPC driver. Nuvoton and Aspeed use the same user-facing code to manage the host snooping. https://github.com/openbmc/phosphor-host-postd > > > +config NPCM_PCI_MBOX > > + tristate "NPCM PCI Mailbox Controller" > > + depends on (ARCH_NPCM || COMPILE_TEST) && REGMAP && MFD_SYSCON > > + help > > + Expose the NPCM BMC PCI MBOX registers found on Nuvoton SOCs > > + to userspace. > > This looks unrelated to the LPC driver, so this should > probably be a separate patch. The same comment about user > space presumably applies here, but I have not seen the driver. You are right, it was added by mistake. will drop off in the next patch > > The implementation of npcm-lpc-bpc looks fine otherwise, I only > noticed one minor detail that I would change: > > > + np = pdev->dev.parent->of_node; > > + if (!of_device_is_compatible(np, "nuvoton,npcm750-lpc") && > > + !of_device_is_compatible(np, "nuvoton,npcm845-lpc")) { > > + dev_err(dev, "unsupported LPC device binding\n"); > > + return -ENODEV; > > + } > > This check doesn't seem to make sense here, since those are > the only two types you support. About the LPC, I like to double check with our architectures on it because the BPC should working on eSPI as well. Maybe I should remove the LPC part. > > Arnd Best regards, Tomer
