Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver

[Wang, Haiyue]

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!