lkml.org 
[lkml]   [2018]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH arm/aspeed/ast2500 v2] eSPI: add ASPEED AST2500 eSPI driver to boot a host with PCH runs on eSPI
    From
    Date
    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.

    \
     
     \ /
      Last update: 2018-02-23 14:19    [W:3.123 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site