Hi Arnd, Thanks for your comments On Fri, 25 Nov 2022 at 12:25, Arnd Bergmann <arnd@xxxxxxxx> wrote: > > On Wed, Nov 23, 2022, at 19:01, Tomer Maimon wrote: > > 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 > > Ok, great! > > >> 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. > > The version you posted only has LPC support, not eSPI, so that > wouldn't work. I'm not sure how eSPI is normally represented > in device drivers, does that show up the same way as an LPC > device, or do you need to register a separate spi_driver? The eSPI is a successor to its Low Pin Count (LPC) and it will show up in the same way as the LPC. NPCM BPC can be connected to the CPU (host) through LPC or eSPI bus and the NPCM BPC driver not is handling the LPC or the eSPI bus therefore I should remove the LPC naming from the and only use BPC. > > If it's part of the same platform driver with different > OF compatible strings, the normal way to handle this would > be to use the .data field in the of_device_id to pass > model specific information to other parts of the driver. > > Arnd Best regards, Tomer
