From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751737AbeAZFd1 (ORCPT ); Fri, 26 Jan 2018 00:33:27 -0500 Received: from mga11.intel.com ([192.55.52.93]:30008 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751712AbeAZFd0 (ORCPT ); Fri, 26 Jan 2018 00:33:26 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,415,1511856000"; d="scan'208";a="12730595" Subject: Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver To: Andy Shevchenko , minyard@acm.org, joel@jms.id.au, avifishman70@gmail.com, openbmc@lists.ozlabs.org, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org References: <1516810009-16353-1-git-send-email-haiyue.wang@linux.intel.com> <1516813514.7000.1235.camel@linux.intel.com> From: "Wang, Haiyue" Message-ID: <793dbcc0-4d28-cddb-c5eb-bf491dff4d92@linux.intel.com> Date: Fri, 26 Jan 2018 13:33:22 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <1516813514.7000.1235.camel@linux.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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!