Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH arm/aspeed/ast2500 v2] eSPI: add ASPEED AST2500 eSPI
 driver to boot a host with PCH runs on eSPI
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        joel@jms.id.au, andrew@aj.id.au, arnd@arndb.de,
        gregkh@linuxfoundation.org, openbmc@lists.ozlabs.org,
        linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org
References: <1519369459-12468-1-git-send-email-haiyue.wang@linux.intel.com>
 <1519381535.10722.99.camel@linux.intel.com>
From:   "Wang, Haiyue" <haiyue.wang@linux.intel.com>
Message-ID: <ae72a9ca-e506-5279-c1b3-114e4eec2be3@linux.intel.com>
Date:   Fri, 23 Feb 2018 21:19:16 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <1519381535.10722.99.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
Precedence: bulk

On 2018-02-23 18:25, Andy Shevchenko wrote:
> On Fri, 2018-02-23 at 15:04 +0800, Haiyue Wang wrote:
>> When PCH works under eSPI mode, the PMC (Power Management Controller)
>> in
>> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
>> dead loop if no SUS_ACK assert. This is the basic requirement for the
>> BMC
>> works as eSPI slave.
> So, do we have an agreement that the driver should go in this shape w/o
> interacting with SPI subsystem?
Not sure, I've added the specification of eSPI, hope people don't feel 
confused with SPI. :-)
> Also few comments below.
>
>> +config ASPEED_ESPI_SLAVE
>> +	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP_MMIO
> I would rather split this to two
> 	depends on REGMAP_MMIO
> 	depends on ARCH_ASPEED || COMPILE_TEST
OK, it looks clean. I referred to:
config ASPEED_LPC_CTRL
         depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON

>> +	tristate "Aspeed ast2500 eSPI slave device driver"
>> +	---help---
>> +	  Control Aspeed ast2500 eSPI slave controller to handle
>> event
>> +	  which needs the firmware's processing.
>> +#include <linux/of.h>
> What exactly requires this header?
Oh, I ctrl+c / ctrl+v from other device tree usage module. :-(
Remove it now. Thanks for making the code more clean.
>> +static int aspeed_espi_slave_probe(struct platform_device *pdev)
>> +{
>> +	struct aspeed_espi_slave_data *priv;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	void __iomem *regs;
>> +	int rc;
>> +
>> +	dev_set_name(dev, DEVICE_NAME);
> Do this after checks and memory allocations.
Fixed!
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->map = devm_regmap_init_mmio(dev, regs,
>> &espi_slave_regmap_cfg);
>> +	if (IS_ERR(priv->map))
>> +		return PTR_ERR(priv->map);
>> +
>> +static const struct of_device_id of_espi_slave_match_table[] = {
>> +	{ .compatible = "aspeed,ast2500-espi-slave" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_espi_slave_match_table);
> This one should be closer to the struct of_device_id.
Fixed.