Messages in this thread | | | Subject | Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver | From | "Wang, Haiyue" <> | Date | Fri, 26 Jan 2018 13:33:22 +0800 |
| |
On 2018-01-25 01:05, Andy Shevchenko wrote: > On Thu, 2018-01-25 at 00:06 +0800, Haiyue Wang wrote: >> The KCS (Keyboard Controller Style) interface is used to perform in- >> band >> IPMI communication between a server host and its BMC (BaseBoard >> Management >> Controllers). >> >> >> +config ASPEED_KCS_IPMI_BMC >> + depends on ARCH_ASPEED || COMPILE_TEST >> + depends on IPMI_KCS_BMC >> + select REGMAP_MMIO >> + tristate "Aspeed KCS IPMI BMC driver" >> + help >> + Provides a driver for the KCS (Keyboard Controller Style) >> IPMI >> + interface found on Aspeed SOCs (AST2400 and AST2500). >> + >> + The driver implements the BMC side of the KCS contorller, >> it >> + provides the access of KCS IO space for BMC side. >> +static inline u8 read_data(struct kcs_bmc *kcs_bmc) >> +{ >> + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr); >> +} >> + >> +static inline void write_data(struct kcs_bmc *kcs_bmc, u8 data) >> +{ >> + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data); >> +} >> + >> +static inline u8 read_status(struct kcs_bmc *kcs_bmc) >> +{ >> + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str); >> +} >> + >> +static inline void write_status(struct kcs_bmc *kcs_bmc, u8 data) >> +{ >> + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data); >> +} >> + >> +static void update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8 >> val) >> +{ >> + u8 tmp; >> + >> + tmp = read_status(kcs_bmc); >> + >> + tmp &= ~mask; >> + tmp |= val & mask; >> + >> + write_status(kcs_bmc, tmp); >> +} > Shouldn't be above some kind of regmap API? It is KCS spec defined IO access for hidden the low level, if the low level supports regmap, such as in kcs_bmc_aspeed.c, aspeed_kcs_inb & aspeed_kcs_outb. > >> +/* Different phases of the KCS BMC module */ >> +enum kcs_phases { >> + /* BMC should not be expecting nor sending any data. */ >> + KCS_PHASE_IDLE, > Perhaps kernel-doc? Code + inline comments should be better than kernel-doc ? Or move it out like :
/* The interface for checksum offload between the stack and networking drivers * is as follows... * * A. IP checksum related features * * Drivers advertise checksum offload capabilities in the features of a device. * From the stack's point of view these are capabilities offered by the driver, * a driver typically only advertises features that it is capable of offloading * to its device. * * The checksum related features are: * * NETIF_F_HW_CSUM - The driver (or its device) is able to compute one * IP (one's complement) checksum for any combination * of protocols or protocol layering. The checksum is * computed and set in a packet per the CHECKSUM_PARTIAL * interface (see below). * * NETIF_F_IP_CSUM - Driver (device) is only able to checksum plain * TCP or UDP packets over IPv4. These are specifically * unencapsulated packets of the form IPv4|TCP or * IPv4|UDP where the Protocol field in the IPv4 header * is TCP or UDP. The IPv4 header may contain IP options * This feature cannot be set in features for a device * with NETIF_F_HW_CSUM also set. This feature is being * DEPRECATED (see below). >> +}; > >> +/* IPMI 2.0 - 9.5, KCS Interface Registers */ >> +struct kcs_ioreg { >> + u32 idr; /* Input Data Register */ >> + u32 odr; /* Output Data Register */ >> + u32 str; /* Status Register */ > kernel-doc >> +}; >> + >> +static inline void *kcs_bmc_priv(const struct kcs_bmc *kcs_bmc) >> +{ >> + return kcs_bmc->priv; >> +} >> + >> +extern int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc); >> +extern struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int >> sizeof_priv, >> + u32 channel); > Drop extern. After dropping extern, it truly passed compilation, have any special reason to drop 'extern' ? I saw in kernel still use extern like : extern void printk_nmi_init(void); >> +#endif > Next one could be reviewed when you split this patch to two. Got it!
| |