| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<TY2PPF5CB9A1BE6E6353CEB66FFD0E498C1F29EA@TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com>
Date: Thu, 29 Jan 2026 02:08:00 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Jeremy Kerr <jk@...econstruct.com.au>, BMC-SW <BMC-SW@...eedtech.com>,
"andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
"benh@...nel.crashing.org" <benh@...nel.crashing.org>, "joel@....id.au"
<joel@....id.au>, "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"naresh.solanki@...ements.com" <naresh.solanki@...ements.com>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v24 3/4] i2c: ast2600: Add controller driver for new
register layout
Hello Jeremy,
Thanks the review.
> linux-arm-kernel@...ts.infradead.org; linux-aspeed@...ts.ozlabs.org;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v24 3/4] i2c: ast2600: Add controller driver for new
> register layout
>
> Hi Ryan,
>
> > Add i2c-ast2600 new register mode driver to support AST2600 i2c new
> > register mode. This i2c-ast2600 new driver and the legacy i2c-aspeed
> > driver both match the same compatible string "aspeed,ast2600-i2c-bus"
> > because they target the same I2C controller IP on AST2600.
>
> You may want to describe the differing requirements of the old vs new driver,
> in terms of DT node detection. More on that below, but probably something
> like:
>
> - the new driver is selected for DT nodes that comply with the current
> aspeed,ast2600-i2c-bus binding spec (specifically: where the required
> aspeed,global-regs property is present)
>
> - for backwards compatibility with old DTs, the old driver is selected
> for DT nodes that do not have the aspeed,global-regs property
>
Will update.
> > However, AST2600 SoCs may configure
> > the controller instances to operate either in the legacy register
> > layout or the new layout (via global register).
> > The new register mode support following.
> >
> > - Add new clock divider option for more flexible and accurate
> > clock rate generation
> > - Add tCKHighMin timing to guarantee SCL high pulse width.
> > - Add support dual pool buffer mode, split 32 bytes pool buffer
> > of each device into 2 x 16 bytes for Tx and Rx individually.
> > - Increase DMA buffer size to 4096 bytes and support byte alignment.
> > - Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> > - Re-define registers for separating controller and target
> > mode control.
> > - Support 4 individual DMA buffers for controller Tx and Rx,
> > target Tx and Rx.
> >
> > And following is new register set for package transfer sequence.
>
> s/package/packet/
Will update following.
>
> > - New Master operation mode:
> > S -> Aw -> P
S -> Aw -> packet TxD -> P
S -> Ar -> packet RxD -> P
S -> Aw -> packet TxD -> Sr -> Ar -> packet RxD -> P
> > - Bus SDA lock auto-release capability for new controller DMA
> > command mode.
> > - Bus auto timeout for new controller/target DMA mode.
> >
> > Since the register layout is selected via a global register at runtime
> > and both drivers bind to the same compatible string, this patch
> > defines the driver selection at build-time using Kconfig, ensuring
> > that only one driver is compiled into the kernel.
>
> This doesn't look to be correct, there are no Kconfig changes here.
Will remove it.
>
> Instead, you have introduced a top-level driver that selects one of the new or
> old drivers based on the compatible and presence of the aspeed,global-regs
> property.
Yes, will update
>
> You can simplify this into two independent drivers, with no need for the
> multiplexing. The overlap on the aspeed,ast2600-i2c-bus compatible can be
> resolved by ->probe() returning -ENODEV conditionally on the presence of
> aspeed,global-regs (as your intended method of distinguishing between new
> and backwards-compatible bindings).
>
> With that approach, all you need to change in the old driver is the
> (sufficiently-commented) -ENODEV return from ->probe().
>
> > This approach avoids ambiguity and ensures consistent behavior for
> > each platform configuration.
Will add in i2c-aspeed.c aspeed_i2c_probe_bus
if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2600-i2c-bus") &&
device_property_present(&pdev->dev, "aspeed,global-regs"))
return -ENODEV;
> >
> > The following is two versus register layout.
> > Old register mode:
> > {I2CD00}: Function Control Register
> > {I2CD04}: Clock and AC Timing Control Register
> > {I2CD08}: Clock and AC Timing Control Register
> > {I2CD0C}: Interrupt Control Register
> > {I2CD10}: Interrupt Status Register
> > {I2CD14}: Command/Status Register
> > {I2CD18}: Slave Device Address Register
> > {I2CD1C}: Pool Buffer Control Register
> > {I2CD20}: Transmit/Receive Byte Buffer Register
> > {I2CD24}: DMA Mode Buffer Address Register
> > {I2CD28}: DMA Transfer Length Register
> > {I2CD2C}: Original DMA Mode Buffer Address Setting
> > {I2CD30}: Original DMA Transfer Length Setting and Final Status
> >
> > New Register mode
> > {I2CC00}: Master/Slave Function Control Register
> > {I2CC04}: Master/Slave Clock and AC Timing Control Register
> > {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
> > {I2CC0C}: Master/Slave Pool Buffer Control Register
> > {I2CM10}: Master Interrupt Control Register
> > {I2CM14}: Master Interrupt Status Register
> > {I2CM18}: Master Command/Status Register
> > {I2CM1C}: Master DMA Buffer Length Register
> > {I2CS20}: Slave~ Interrupt Control Register
> > {I2CS24}: Slave~ Interrupt Status Register
> > {I2CS28}: Slave~ Command/Status Register
> > {I2CS2C}: Slave~ DMA Buffer Length Register
> > {I2CM30}: Master DMA Mode Tx Buffer Base Address
> > {I2CM34}: Master DMA Mode Rx Buffer Base Address
> > {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
> > {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
> > {I2CS40}: Save Device Address Register
> > {I2CM48}: Master DMA Length Status Register
> > {I2CS4C}: Slave DMA Length Status Register
> > {I2CC50}: Current DMA Operating Address Status
> > {I2CC54}: Current DMA Operating Length Status
>
> ... I'm not sure we need a full register description in the changelog.
Will remove it .
>
> > Add a new core file(i2c-aspeed-core.c) to avoid device tree ABI break,
> > allow both old and new device trees using the same compatible string
> > "aspeed,ast2600-i2c-bus" to function correctly.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@...eedtech.com>
> > ---
> > drivers/i2c/busses/Makefile | 2 +-
> > drivers/i2c/busses/i2c-aspeed-core.c | 89 +++
> > drivers/i2c/busses/i2c-aspeed-core.h | 19 +
> > drivers/i2c/busses/i2c-aspeed.c | 43 +-
> > drivers/i2c/busses/i2c-ast2600.c | 1018
> > ++++++++++++++++++++++++++
> > 5 files changed, 1136 insertions(+), 35 deletions(-)
> > create mode 100644 drivers/i2c/busses/i2c-aspeed-core.c
> > create mode 100644 drivers/i2c/busses/i2c-aspeed-core.h
> > create mode 100644 drivers/i2c/busses/i2c-ast2600.c
> >
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index fb985769f5ff..606b35196960 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -37,7 +37,7 @@ obj-$(CONFIG_I2C_POWERMAC) +=
> i2c-powermac.o
> > obj-$(CONFIG_I2C_ALTERA) += i2c-altera.o
> > obj-$(CONFIG_I2C_AMD_MP2) += i2c-amd-mp2-pci.o
> > i2c-amd-mp2-plat.o
> > obj-$(CONFIG_I2C_AMD_ASF) += i2c-amd-asf-plat.o
> > -obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
> > +obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o i2c-ast2600.o
> > +i2c-aspeed-core.o
> > obj-$(CONFIG_I2C_AT91) += i2c-at91.o
> > i2c-at91-y := i2c-at91-core.o
> i2c-at91-master.o
> > i2c-at91-$(CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL) += i2c-at91-slave.o
> > diff --git a/drivers/i2c/busses/i2c-aspeed-core.c
> > b/drivers/i2c/busses/i2c-aspeed-core.c
> > new file mode 100644
> > index 000000000000..a2878e1273ed
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-aspeed-core.c
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ASPEED I2C core driver
> > + *
> > + * Copyright (C) ASPEED Technology Inc.
> > + */
>
> As above, this "multiplexing" core driver may not be necessary.
Will remove i2c-aspeed-core.c
>
> > +
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_platform.h>
> > +
> > +#include "i2c-aspeed-core.h"
> > +
> > +struct aspeed_i2c_core_priv {
> > + void (*remove)(struct platform_device *pdev);
> > + void *bus_data;
> > +};
> > +
> > +static const struct of_device_id aspeed_i2c_of_match[] = {
> > + {
> > + .compatible = "aspeed,ast2400-i2c-bus",
> > + .data = (const void *)AST2400_I2C
> > + },
> > + {
> > + .compatible = "aspeed,ast2500-i2c-bus",
> > + .data = (const void *)AST2500_I2C
> > + },
> > + {
> > + .compatible = "aspeed,ast2600-i2c-bus",
> > + .data = (const void *)AST2600_I2C
> > + },
> > + { }
> > +};
>
> Just being enum members, the .data fields seem pointless; you could just check
> against the actual compatible string to determine the device type?
Yes, no need. Will update.
>
> > +
> > +MODULE_DEVICE_TABLE(of, aspeed_i2c_of_match);
> > +
> > +static int aspeed_i2c_core_probe(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct aspeed_i2c_core_priv *priv;
> > + const struct of_device_id *match;
> > + int ret;
> > +
> > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + match = of_match_device(aspeed_i2c_of_match, dev);
> > + if (!match)
> > + return -ENODEV;
> > +
> > + if (device_is_compatible(dev, "aspeed,ast2600-i2c-bus") &&
> > + device_property_present(dev, "aspeed,global-regs")) {
> > + ret = ast2600_i2c_probe(match, pdev);
> > + priv->remove = ast2600_i2c_remove;
> > + } else {
> > + ret = aspeed_i2c_probe_bus(match, pdev);
> > + priv->remove = aspeed_i2c_remove_bus;
> > + }
> > +
> > + priv->bus_data = platform_get_drvdata(pdev);
> > + platform_set_drvdata(pdev, priv);
> > + return ret;
> > +}
> > +
> > +static void aspeed_i2c_core_remove(struct platform_device *pdev) {
> > + struct aspeed_i2c_core_priv *priv =
> > +platform_get_drvdata(pdev);
> > +
> > + if (!priv || !priv->remove)
> > + return;
> > +
> > + platform_set_drvdata(pdev, priv->bus_data);
> > + return priv->remove(pdev);
> > +}
> > +
> > +static struct platform_driver aspeed_i2c_driver = {
> > + .probe = aspeed_i2c_core_probe,
> > + .remove = aspeed_i2c_core_remove,
> > + .driver = {
> > + .name = "i2c-aspeed-core",
> > + .of_match_table = aspeed_i2c_of_match,
> > + },
> > +};
> > +module_platform_driver(aspeed_i2c_driver);
> > +
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@...eedtech.com>");
> > +MODULE_DESCRIPTION("Unified ASPEED I2C driver
> > +(AST24xx/AST25xx/AST2600)"); MODULE_LICENSE("GPL");
> > diff --git a/drivers/i2c/busses/i2c-aspeed-core.h
> > b/drivers/i2c/busses/i2c-aspeed-core.h
> > new file mode 100644
> > index 000000000000..6e0091018985
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-aspeed-core.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */ #ifndef
> > +_I2C_ASPEED_CORE_H #define _I2C_ASPEED_CORE_H
> > +
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +enum i2c_version {
> > + AST2400_I2C,
> > + AST2500_I2C,
> > + AST2600_I2C,
> > + AST2700_I2C,
> > +};
> > +
> > +int aspeed_i2c_probe_bus(const struct of_device_id *match, struct
> > +platform_device *pdev); void aspeed_i2c_remove_bus(struct
> > +platform_device *pdev); int ast2600_i2c_probe(const struct
> > +of_device_id *match, struct platform_device *pdev); void
> > +ast2600_i2c_remove(struct platform_device *pdev); #endif
> > diff --git a/drivers/i2c/busses/i2c-aspeed.c
> > b/drivers/i2c/busses/i2c-aspeed.c index a26b74c71206..8466c98b6c7b
> > 100644
> > --- a/drivers/i2c/busses/i2c-aspeed.c
> > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > @@ -25,6 +25,8 @@
> > #include <linux/reset.h>
> > #include <linux/slab.h>
> >
> > +#include "i2c-aspeed-core.h"
> > +
> > /* I2C Register */
> > #define
> ASPEED_I2C_FUN_CTRL_REG
> 0x00
> > #define
> ASPEED_I2C_AC_TIMING_REG1 0x04 @@
> > -978,26 +980,9 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus
> > *bus)
> > return ret;
> > }
> >
> > -static const struct of_device_id aspeed_i2c_bus_of_table[] = {
> > - {
> > - .compatible = "aspeed,ast2400-i2c-bus",
> > - .data = aspeed_i2c_24xx_get_clk_reg_val,
> > - },
> > - {
> > - .compatible = "aspeed,ast2500-i2c-bus",
> > - .data = aspeed_i2c_25xx_get_clk_reg_val,
> > - },
> > - {
> > - .compatible = "aspeed,ast2600-i2c-bus",
> > - .data = aspeed_i2c_25xx_get_clk_reg_val,
> > - },
> > - { }
> > -};
> > -MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
> > -
> > -static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> > +int aspeed_i2c_probe_bus(const struct of_device_id *match,
> > + struct platform_device *pdev)
> > {
> > - const struct of_device_id *match;
> > struct aspeed_i2c_bus *bus;
> > struct clk *parent_clk;
> > int irq, ret;
> > @@ -1033,12 +1018,10 @@ static int aspeed_i2c_probe_bus(struct
> > platform_device *pdev)
> > bus->bus_frequency =
> I2C_MAX_STANDARD_MODE_FREQ;
> > }
> >
> > - match = of_match_node(aspeed_i2c_bus_of_table,
> > pdev->dev.of_node);
> > - if (!match)
> > + if ((enum i2c_version)(uintptr_t)match->data == AST2400_I2C)
> > bus->get_clk_reg_val =
> > aspeed_i2c_24xx_get_clk_reg_val;
> > else
> > - bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> > - match->data;
> > + bus->get_clk_reg_val =
> > +aspeed_i2c_25xx_get_clk_reg_val;
> >
> > /* Initialize the I2C adapter */
> > spin_lock_init(&bus->lock);
> > @@ -1081,8 +1064,9 @@ static int aspeed_i2c_probe_bus(struct
> > platform_device *pdev)
> >
> > return 0;
> > }
> > +EXPORT_SYMBOL_GPL(aspeed_i2c_probe_bus);
> >
> > -static void aspeed_i2c_remove_bus(struct platform_device *pdev)
> > +void aspeed_i2c_remove_bus(struct platform_device *pdev)
> > {
> > struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
> > unsigned long flags;
> > @@ -1099,16 +1083,7 @@ static void aspeed_i2c_remove_bus(struct
> > platform_device *pdev)
> >
> > i2c_del_adapter(&bus->adap);
> > }
> > -
> > -static struct platform_driver aspeed_i2c_bus_driver = {
> > - .probe = aspeed_i2c_probe_bus,
> > - .remove = aspeed_i2c_remove_bus,
> > - .driver = {
> > - .name = "aspeed-i2c-bus",
> > - .of_match_table = aspeed_i2c_bus_of_table,
> > - },
> > -};
> > -module_platform_driver(aspeed_i2c_bus_driver);
> > +EXPORT_SYMBOL_GPL(aspeed_i2c_remove_bus);
> >
> > MODULE_AUTHOR("Brendan Higgins <brendanhiggins@...gle.com>");
> > MODULE_DESCRIPTION("Aspeed I2C Bus Driver"); diff --git
> > a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> > new file mode 100644
> > index 000000000000..04cb38e018a6
>
> To prevent painting yourself into a corner (and the reference to 2700 in this
> same file): is this named appropriately for future SoC revisions?
I intent to use i2c-aspeedv2.c or i2cv2-aspeed.c or i2c-v2-aspeed.c
Or you have any suggest?
>
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-ast2600.c
> > @@ -0,0 +1,1018 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ASPEED AST2600 new register set I2C controller driver
> > + *
> > + * Copyright (C) ASPEED Technology Inc.
>
> You'll need a year here.
Will update Copyright (C) 2026 ASPEED Technology Inc.
>
> > +/* 0x10 : I2CM Controller Interrupt Control Register */ #define
> > +AST2600_I2CM_IER 0x10
> > +/* 0x14 : I2CM Controller Interrupt Status Register : WC */ #define
> > +AST2600_I2CM_ISR 0x14
> > +
> > +#define
> AST2600_I2CM_PKT_TIMEOUT BIT(18)
> > +#define AST2600_I2CM_PKT_ERROR BIT(17)
> #define
> > +AST2600_I2CM_PKT_DONE BIT(16)
> > +
> > +#define AST2600_I2CM_BUS_RECOVER_FAIL BIT(15) #define
> > +AST2600_I2CM_SDA_DL_TO BIT(14) #define
> > +AST2600_I2CM_BUS_RECOVER BIT(13)
> #define
> > +AST2600_I2CM_SMBUS_ALT BIT(12)
>
> _ALT implies "alternate", can you make this _ALERT?
Will Update AST2600_I2CM_SMBUS_ALERT
>
> > +enum xfer_mode {
> > + BYTE_MODE,
> > + BUFF_MODE,
> > + DMA_MODE,
> > +};
> > +
> > +struct ast2600_i2c_bus {
> > + struct i2c_adapter adap;
> > + struct device *dev;
> > + void __iomem *reg_base;
> > + struct regmap *global_regs;
> > + struct clk *clk;
> > + struct i2c_timings timing_info;
> > + struct completion cmd_complete;
> > + struct i2c_msg *msgs;
> > + u8 *controller_dma_safe_buf;
> > + dma_addr_t controller_dma_addr;
> > + u32 apb_clk;
> > + u32 timeout;
> > + int irq;
> > + int cmd_err;
> > + int msgs_index;
> > + int msgs_count;
> > + int controller_xfer_cnt;
> > + size_t buf_index;
> > + size_t buf_size;
> > + enum xfer_mode mode;
> > + bool multi_master;
> > + /* Buffer mode */
> > + void __iomem *buf_base;
> > + struct i2c_smbus_alert_setup alert_data;
>
> You don't seem to use alert_data?
Will remove it.
>
> > +static int ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) {
> > + u32 state = readl(i2c_bus->reg_base +
> > +AST2600_I2CC_STS_AND_BUFF);
> > + int ret = 0;
> > + u32 ctrl;
> > + int r;
> > +
> > + dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n",
> > +i2c_bus->adap.nr, state);
> > +
> > + ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +
> > + /* Disable controller */
> > + writel(ctrl & ~(AST2600_I2CC_MASTER_EN |
> > +AST2600_I2CC_SLAVE_EN),
> > + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +
> > + writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) |
> > +AST2600_I2CC_MASTER_EN,
> > + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
>
> Maybe split the readl out to make this simpler (and not wrapped).
Will update it.
>
> > +
> > + reinit_completion(&i2c_bus->cmd_complete);
> > + i2c_bus->cmd_err = 0;
> > +
> > + /* Check 0x14's SDA and SCL status */
>
> What is 0x14?
Will update /* Check SDA and SCL status */
>
> > + state = readl(i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF);
> > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state &
> > +AST2600_I2CC_SCL_LINE_STS)) {
> > + writel(AST2600_I2CM_RECOVER_CMD_EN,
> i2c_bus->reg_base
> > ++ AST2600_I2CM_CMD_STS);
> > + r =
> > +wait_for_completion_timeout(&i2c_bus->cmd_complete,
> > +i2c_bus->adap.timeout);
> > + if (r == 0) {
> > + dev_dbg(i2c_bus->dev, "recovery timed
> out\n");
> > + writel(ctrl, i2c_bus->reg_base +
> > +AST2600_I2CC_FUN_CTRL);
> > + return -ETIMEDOUT;
> > + } else if (i2c_bus->cmd_err) {
> > + dev_dbg(i2c_bus->dev, "recovery
> error\n");
> > + ret = -EPROTO;
> > + }
>
> One of these paths restores controller state, the other does not. Is that okay?
>
> (if both should be restoring it, you may want to just set ret and use the
> common exit path)
>
Will modify
out_restore:
writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
return ret;
> > + }
> > +
> > + /* Recovery done */
> > + state = readl(i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF);
> > + if (state & AST2600_I2CC_BUS_BUSY_STS) {
> > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n",
> > +state);
> > + ret = -EPROTO;
> > + }
>
> This (including the SCL/SDA state detection above) only seems sensible if this
> controller is the only controller on the bus, as your conditions depend on
> sampled bus state.
>
> If the recovery command completed successfully, but the bus is coincidentally
> busy due to some other controller, this will fail.
Yes, that is expected, when after the recovery command completed still need return
the bus status, and the ast2600_i2c_controller_xfer will return to upper layer.
>
> > +
> > + /* restore original controller setting */
> > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > + return ret;
> > +}
>
> [...]
>
> > +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) {
> > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> > +
> > + /* send start */
> > + dev_dbg(i2c_bus->dev, "[%d] %s %d byte%s %s 0x%02x\n",
> > + i2c_bus->msgs_index, str_read_write(msg->flags &
> > +I2C_M_RD),
> > + msg->len, str_plural(msg->len),
> > + msg->flags & I2C_M_RD ? "from" : "to", msg->addr);
> > +
> > + i2c_bus->controller_xfer_cnt = 0;
> > + i2c_bus->buf_index = 0;
> > +
> > + if (msg->flags & I2C_M_RD) {
> > + if (i2c_bus->mode == DMA_MODE)
> > + return
> > +ast2600_i2c_setup_dma_rx(AST2600_I2CM_START_CMD, i2c_bus);
> > + else if (i2c_bus->mode == BUFF_MODE)
> > + return
> > +ast2600_i2c_setup_buff_rx(AST2600_I2CM_START_CMD, i2c_bus);
> > + else
> > + return
> > +ast2600_i2c_setup_byte_rx(AST2600_I2CM_START_CMD, i2c_bus);
> > + } else {
> > + if (i2c_bus->mode == DMA_MODE)
> > + return
> > +ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus);
> > + else if (i2c_bus->mode == BUFF_MODE)
> > + return
> > +ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus);
> > + else
> > + return
> > +ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus);
>
> Would ->setup_rx() and ->setup_tx() ops be helpful? You have quite a few
> instances of this same three-branch condition.
>
> If so, you may also want symmetric cleanup ops too.
Will add with following
static void ast2600_i2c_set_xfer_ops(struct ast2600_i2c_bus *i2c_bus)
{
switch (i2c_bus->mode) {
case DMA_MODE:
i2c_bus->setup_tx = ast2600_i2c_setup_dma_tx;
i2c_bus->setup_rx = ast2600_i2c_setup_dma_rx;
break;
case BUFF_MODE:
i2c_bus->setup_tx = ast2600_i2c_setup_buff_tx;
i2c_bus->setup_rx = ast2600_i2c_setup_buff_rx;
break;
case BYTE_MODE:
default:
i2c_bus->setup_tx = ast2600_i2c_setup_byte_tx;
i2c_bus->setup_rx = ast2600_i2c_setup_byte_rx;
break;
}
}
>
> > + }
> > +}
> > +
> > +static int ast2600_i2c_irq_err_to_errno(u32 irq_status) {
> > + if (irq_status & AST2600_I2CM_ARBIT_LOSS)
> > + return -EAGAIN;
> > + if (irq_status & (AST2600_I2CM_SDA_DL_TO |
> > +AST2600_I2CM_SCL_LOW_TO))
> > + return -EBUSY;
> > + if (irq_status & (AST2600_I2CM_ABNORMAL))
> > + return -EPROTO;
> > +
> > + return 0;
> > +}
> > +
> > +static void ast2600_i2c_controller_package_irq(struct ast2600_i2c_bus
> > +*i2c_bus, u32 sts)
>
> minor: package -> packet
Will modify.
>
> > +{
> > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> > + int xfer_len;
> > + int i;
> > +
> > + sts &= ~AST2600_I2CM_PKT_DONE;
> > + writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base +
> > +AST2600_I2CM_ISR);
> > + switch (sts) {
> > + case AST2600_I2CM_PKT_ERROR:
> > + i2c_bus->cmd_err = -EAGAIN;
> > + complete(&i2c_bus->cmd_complete);
> > + break;
> > + case AST2600_I2CM_PKT_ERROR | AST2600_I2CM_TX_NAK: /*
> a0 fix
> > +for issue */
> > + fallthrough;
> > + case AST2600_I2CM_PKT_ERROR | AST2600_I2CM_TX_NAK |
> AST2600_I2CM_NORMAL_STOP:
> > + i2c_bus->cmd_err = -ENXIO;
> > + complete(&i2c_bus->cmd_complete);
> > + break;
> > + case AST2600_I2CM_NORMAL_STOP:
> > + /* write 0 byte only have stop isr */
> > + i2c_bus->msgs_index++;
> > + if (i2c_bus->msgs_index < i2c_bus->msgs_count) {
> > + if (ast2600_i2c_do_start(i2c_bus)) {
> > + i2c_bus->cmd_err =
> -ENOMEM;
> >
> + complete(&i2c_bus->cmd_co
> mplete);
> > + }
> > + } else {
> > + i2c_bus->cmd_err =
> i2c_bus->msgs_index;
> > + complete(&i2c_bus->cmd_complete);
> > + }
> > + break;
> > + case AST2600_I2CM_TX_ACK:
> > + case AST2600_I2CM_TX_ACK | AST2600_I2CM_NORMAL_STOP:
> > + if (i2c_bus->mode == DMA_MODE)
> > + xfer_len =
> > +AST2600_I2C_GET_TX_DMA_LEN(readl(i2c_bus->reg_base +
> >
> +
>
> > +AST2600_I2CM_DMA_LEN_STS));
> > + else if (i2c_bus->mode == BUFF_MODE)
> > + xfer_len =
> > +AST2600_I2CC_GET_TX_BUF_LEN(readl(i2c_bus->reg_base +
> >
> +
>
> > +AST2600_I2CC_BUFF_CTRL));
> > + else
> > + xfer_len = 1;
> > +
> > + i2c_bus->controller_xfer_cnt += xfer_len;
> > +
> > + if (i2c_bus->controller_xfer_cnt == msg->len) {
> > + if (i2c_bus->mode == DMA_MODE) {
> >
> + dma_unmap_single(i2c_bus->
> dev,
> > +i2c_bus->controller_dma_addr,
> >
> + msg
> ->len,
> > +DMA_TO_DEVICE);
> > +
> > +i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg,
> >
> +
> true);
> >
> + i2c_bus->controller_dma_saf
> e_buf =
> > +NULL;
> > + }
> > + i2c_bus->msgs_index++;
> > + if (i2c_bus->msgs_index ==
> > +i2c_bus->msgs_count) {
> > + i2c_bus->cmd_err =
> > +i2c_bus->msgs_index;
> >
> + complete(&i2c_bus->cmd_co
> mplete);
> > + } else {
> > + if
> (ast2600_i2c_do_start(i2c_bus)) {
> >
> + i2c_bus->cmd_er
> r = -ENOMEM;
> > +
> > +complete(&i2c_bus->cmd_complete);
> > + }
> > + }
> > + } else {
> > + if (i2c_bus->mode == DMA_MODE)
> >
> + ast2600_i2c_setup_dma_tx(0,
> i2c_bus);
> > + else if (i2c_bus->mode == BUFF_MODE)
> >
> + ast2600_i2c_setup_buff_tx(0,
> i2c_bus);
> > + else
> >
> + ast2600_i2c_setup_byte_tx(0,
> i2c_bus);
> > + }
>
> You have quite a bit of repetition across the start and the re-start cases.
> Perhaps you can combine these into helpers?
Yes will update
i2c_bus->setup_rx
>
> > + break;
> > + case AST2600_I2CM_RX_DONE:
> > + case AST2600_I2CM_RX_DONE |
> AST2600_I2CM_NORMAL_STOP:
> > + /* do next rx */
> > + if (i2c_bus->mode == DMA_MODE) {
> > + xfer_len =
> > +AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
> >
> +
>
> > +AST2600_I2CM_DMA_LEN_STS));
> > + } else if (i2c_bus->mode == BUFF_MODE) {
> > + xfer_len =
> > +AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> >
> +
>
> > +AST2600_I2CC_BUFF_CTRL));
> > + for (i = 0; i < xfer_len; i++)
> >
> + msg->buf[i2c_bus->controller
> _xfer_cnt
> > ++ i] =
> >
> + readb(i2c_bus->b
> uf_base + 0x10
> > ++ i);
> > + } else {
> > + xfer_len = 1;
> > + msg->buf[i2c_bus->controller_xfer_cnt]
> =
> > +
> > +AST2600_I2CC_GET_RX_BUFF(readl(i2c_bus->reg_base +
> >
> +
>
> > +AST2600_I2CC_STS_AND_BUFF));
> > + }
> > +
> > + if (msg->flags & I2C_M_RECV_LEN) {
> > + u8 recv_len =
> > +AST2600_I2CC_GET_RX_BUFF(readl(i2c_bus->reg_base
> >
> +
> +
> > +AST2600_I2CC_STS_AND_BUFF));
> > + msg->len = min_t(unsigned int,
> recv_len,
> > +I2C_SMBUS_BLOCK_MAX);
> > + msg->len += ((msg->flags &
> I2C_CLIENT_PEC) ? 2
> > +: 1);
> > + msg->flags &= ~I2C_M_RECV_LEN;
> > + if (!recv_len)
> >
> + i2c_bus->controller_xfer_cnt
> = 0;
> > + else
> >
> + i2c_bus->controller_xfer_cnt
> = 1;
> > + } else {
> > + i2c_bus->controller_xfer_cnt +=
> xfer_len;
> > + }
> > +
> > + if (i2c_bus->controller_xfer_cnt == msg->len) {
> > + if (i2c_bus->mode == DMA_MODE) {
> >
> + dma_unmap_single(i2c_bus->
> dev,
> > +i2c_bus->controller_dma_addr,
> >
> + msg
> ->len,
> > +DMA_FROM_DEVICE);
> > +
> > +i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf,
> >
> +
> msg, true);
> >
> + i2c_bus->controller_dma_saf
> e_buf =
> > +NULL;
> > + }
> > +
> > + i2c_bus->msgs_index++;
> > + if (i2c_bus->msgs_index ==
> > +i2c_bus->msgs_count) {
> > + i2c_bus->cmd_err =
> > +i2c_bus->msgs_index;
> >
> + complete(&i2c_bus->cmd_co
> mplete);
> > + } else {
> > + if
> (ast2600_i2c_do_start(i2c_bus)) {
> >
> + i2c_bus->cmd_er
> r = -ENOMEM;
> > +
> > +complete(&i2c_bus->cmd_complete);
> > + }
> > + }
> > + } else {
> > + if (i2c_bus->mode == DMA_MODE)
> >
> + ast2600_i2c_setup_dma_rx(0,
> i2c_bus);
> > + else if (i2c_bus->mode == BUFF_MODE)
> >
> + ast2600_i2c_setup_buff_rx(0,
> i2c_bus);
> > + else
> >
> + ast2600_i2c_setup_byte_rx(0,
> i2c_bus);
> > + }
> > + break;
> > + default:
> > + dev_dbg(i2c_bus->dev, "unhandled sts %x\n", sts);
> > + break;
> > + }
> > +}
> > +
> > +static int ast2600_i2c_controller_irq(struct ast2600_i2c_bus
> > +*i2c_bus) {
> > + u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
> > + u32 ctrl;
> > +
> > + sts &= ~AST2600_I2CM_SMBUS_ALT;
> > +
> > + if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
>
> minor: these conditions seem inverted. 'sts & ...' would be more clear.
Will update.
>
> > + writel(AST2600_I2CM_BUS_RECOVER_FAIL,
> > +i2c_bus->reg_base + AST2600_I2CM_ISR);
> > + ctrl = readl(i2c_bus->reg_base +
> > +AST2600_I2CC_FUN_CTRL);
> > + writel(0, i2c_bus->reg_base +
> AST2600_I2CC_FUN_CTRL);
> > + writel(ctrl, i2c_bus->reg_base +
> > +AST2600_I2CC_FUN_CTRL);
> > + i2c_bus->cmd_err = -EPROTO;
> > + complete(&i2c_bus->cmd_complete);
> > + return 1;
> > + }
> > +
> > + if (AST2600_I2CM_BUS_RECOVER & sts) {
> > + writel(AST2600_I2CM_BUS_RECOVER,
> i2c_bus->reg_base +
> > +AST2600_I2CM_ISR);
> > + i2c_bus->cmd_err = 0;
> > + complete(&i2c_bus->cmd_complete);
> > + return 1;
> > + }
> > +
> > + i2c_bus->cmd_err = ast2600_i2c_irq_err_to_errno(sts);
> > + if (i2c_bus->cmd_err) {
> > + writel(AST2600_I2CM_PKT_DONE,
> i2c_bus->reg_base +
> > +AST2600_I2CM_ISR);
> > + complete(&i2c_bus->cmd_complete);
> > + return 1;
> > + }
> > +
> > + if (AST2600_I2CM_PKT_DONE & sts) {
> > + ast2600_i2c_controller_package_irq(i2c_bus, sts);
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t ast2600_i2c_bus_irq(int irq, void *dev_id) {
> > + struct ast2600_i2c_bus *i2c_bus = dev_id;
> > +
> > + return IRQ_RETVAL(ast2600_i2c_controller_irq(i2c_bus));
> > +}
> > +
> > +static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap,
> > +struct i2c_msg *msgs, int num) {
> > + struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(adap);
> > + unsigned long timeout;
> > + int ret;
> > +
> > + if (!i2c_bus->multi_master &&
> > + (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF)
> &
> > +AST2600_I2CC_BUS_BUSY_STS)) {
> > + ret = ast2600_i2c_recover_bus(i2c_bus);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + i2c_bus->cmd_err = 0;
> > + i2c_bus->msgs = msgs;
> > + i2c_bus->msgs_index = 0;
> > + i2c_bus->msgs_count = num;
> > + reinit_completion(&i2c_bus->cmd_complete);
> > + ret = ast2600_i2c_do_start(i2c_bus);
> > + if (ret)
> > + goto controller_out;
> > + timeout =
> wait_for_completion_timeout(&i2c_bus->cmd_complete,
> > +i2c_bus->adap.timeout);
> > + if (timeout == 0) {
> > + u32 ctrl = readl(i2c_bus->reg_base +
> > +AST2600_I2CC_FUN_CTRL);
> > +
> > + dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n",
> > + readl(i2c_bus->reg_base +
> AST2600_I2CM_ISR),
> > + readl(i2c_bus->reg_base +
> > +AST2600_I2CC_STS_AND_BUFF));
> > + writel(0, i2c_bus->reg_base +
> AST2600_I2CC_FUN_CTRL);
> > + writel(ctrl, i2c_bus->reg_base +
> > +AST2600_I2CC_FUN_CTRL);
> > +
> > + if (i2c_bus->multi_master &&
> > + (readl(i2c_bus->reg_base +
> > +AST2600_I2CC_STS_AND_BUFF) &
> > + AST2600_I2CC_BUS_BUSY_STS))
> > + ast2600_i2c_recover_bus(i2c_bus);
>
> Can you add a comment about how this is safe (mainly: what conditions would
> have lead to this timeout)? Doing a bus recovery - by performing additional
> SCL cycles - may not be considered good controller behaviour on an
> entirely-local error condition.
/*
* A slave holding SCL low can stall the transfer and trigger
* a master timeout, then attempt bus recovery if the bus is
* still busy.
*/
>
> > +
> > + ret = -ETIMEDOUT;
> > + } else {
> > + ret = i2c_bus->cmd_err;
> > + }
> > +
> > + dev_dbg(i2c_bus->dev, "bus%d-m: %d end\n", i2c_bus->adap.nr,
> > +i2c_bus->cmd_err);
> > +
> > +controller_out:
> > + if (i2c_bus->mode == DMA_MODE) {
> > + if (i2c_bus->controller_dma_safe_buf) {
>
> Minor: you can combine these conditions. You also seem to only set
> controller_dma_safe_buf in DMA_MODE anyway, so maybe you can just check
> that.
Will update
>
>
> > + struct i2c_msg *msg =
> > +&i2c_bus->msgs[i2c_bus->msgs_index];
> > +
> > + if (msg->flags & I2C_M_RD)
> >
> + dma_unmap_single(i2c_bus->
> dev,
> > +i2c_bus->controller_dma_addr,
> >
> + msg
> ->len,
> > +DMA_FROM_DEVICE);
> > + else
> >
> + dma_unmap_single(i2c_bus->
> dev,
> > +i2c_bus->controller_dma_addr,
> >
> + msg
> ->len,
> > +DMA_TO_DEVICE);
> > +
> > +i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg,
> > +true);
> > + i2c_bus->controller_dma_safe_buf =
> NULL;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus) {
> > + struct platform_device *pdev =
> > +to_platform_device(i2c_bus->dev);
> > + u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE |
> > +AST2600_I2CC_MASTER_EN;
> > +
> > + /* I2C Reset */
> > + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +
> > + i2c_bus->multi_master =
> device_property_read_bool(&pdev->dev,
> > +"multi-master");
>
> I would suggest doing this in the same place as the other DT parsing.
Will update ~
>
> > + if (!i2c_bus->multi_master)
> > + fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;
> > +
> > + /* Enable Controller Mode */
> > + writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > + /* disable target address */
> > + writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> > +
> > + /* Set AC Timing */
> > + ast2600_i2c_ac_timing_config(i2c_bus);
> > +
> > + /* Clear Interrupt */
> > + writel(GENMASK(27, 0), i2c_bus->reg_base +
> AST2600_I2CM_ISR);
> > +}
> > +
> > +static u32 ast2600_i2c_functionality(struct i2c_adapter *adap) {
> > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> > +I2C_FUNC_SMBUS_BLOCK_DATA; }
> > +
> > +static const struct i2c_algorithm i2c_ast2600_algorithm = {
> > + .xfer = ast2600_i2c_controller_xfer,
> > + .functionality = ast2600_i2c_functionality, };
> > +
> > +int ast2600_i2c_probe(const struct of_device_id *match, struct
> > +platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct ast2600_i2c_bus *i2c_bus;
> > + struct reset_control *rst;
> > + const char *xfer_mode;
> > + struct resource *res;
> > + u32 global_ctrl;
> > + int ret;
> > +
> > + i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL);
> > + if (!i2c_bus)
> > + return -ENOMEM;
> > +
> > + i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(i2c_bus->reg_base))
> > + return PTR_ERR(i2c_bus->reg_base);
> > +
> > + rst = devm_reset_control_get_shared_deasserted(dev, NULL);
> > + if (IS_ERR(rst))
> > + return dev_err_probe(dev, PTR_ERR(rst), "Missing
> reset
> > +ctrl\n");
> > +
> > + i2c_bus->global_regs =
> >
> + syscon_regmap_lookup_by_phandle(dev_of_node(dev
> ),
> > +"aspeed,global-regs");
> > + if (IS_ERR(i2c_bus->global_regs))
> > + return PTR_ERR(i2c_bus->global_regs);
> > +
> > + regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL,
> > +&global_ctrl);
> > + if ((global_ctrl & AST2600_GLOBAL_INIT) !=
> > +AST2600_GLOBAL_INIT) {
> > + regmap_write(i2c_bus->global_regs,
> AST2600_I2CG_CTRL,
> > +AST2600_GLOBAL_INIT);
> > + regmap_write(i2c_bus->global_regs,
> > +AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
> > + }
> > +
> > + i2c_bus->dev = dev;
> > + i2c_bus->mode = BUFF_MODE;
> > + if (!device_property_read_string(dev, "aspeed,transfer-mode",
> > +&xfer_mode)) {
> > + if (!strcmp(xfer_mode, "dma"))
> > + i2c_bus->mode = DMA_MODE;
> > + else if (!strcmp(xfer_mode, "byte"))
> > + i2c_bus->mode = BYTE_MODE;
> > + else
> > + i2c_bus->mode = BUFF_MODE;
> > + }
> > +
> > + if (i2c_bus->mode == BUFF_MODE) {
> > + i2c_bus->buf_base =
> > +devm_platform_get_and_ioremap_resource(pdev, 1, &res);
> > + if (IS_ERR(i2c_bus->buf_base))
> > + i2c_bus->mode = BYTE_MODE;
> > + else
> > + i2c_bus->buf_size = resource_size(res) /
> 2;
> > + }
> > +
> > + /*
> > + * i2c timeout counter: use base clk4 1Mhz,
> > + * per unit: 1/(1000/1024) = 1024us
> > + */
> > + ret = device_property_read_u32(dev,
> > +"i2c-scl-clk-low-timeout-us", &i2c_bus->timeout);
> > + if (!ret)
> > + i2c_bus->timeout /= 1024;
> > +
> > + init_completion(&i2c_bus->cmd_complete);
> > +
> > + i2c_bus->irq = platform_get_irq(pdev, 0);
> > + if (i2c_bus->irq < 0)
> > + return i2c_bus->irq;
> > +
> > + platform_set_drvdata(pdev, i2c_bus);
> > +
> > + i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
> > + if (IS_ERR(i2c_bus->clk))
> > + return dev_err_probe(i2c_bus->dev,
> > +PTR_ERR(i2c_bus->clk), "Can't get clock\n");
> > +
> > + i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
> > +
> > + i2c_parse_fw_timings(i2c_bus->dev, &i2c_bus->timing_info,
> > +true);
> > +
> > + /* Initialize the I2C adapter */
> > + i2c_bus->adap.owner = THIS_MODULE;
> > + i2c_bus->adap.algo = &i2c_ast2600_algorithm;
> > + i2c_bus->adap.retries = 0;
> > + i2c_bus->adap.dev.parent = i2c_bus->dev;
> > + device_set_node(&i2c_bus->adap.dev, dev_fwnode(dev));
> > + i2c_bus->adap.algo_data = i2c_bus;
> > + strscpy(i2c_bus->adap.name, pdev->name);
> > + i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
> > +
> > + ast2600_i2c_init(i2c_bus);
>
> If anything past this point fails, we'll leave the controller enabled.
> Is that a problem?
Will add with err exit
err:
writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
return ret;
>
> > +
> > + ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq,
> > +0,
> > + dev_name(dev), i2c_bus);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "Unable to request irq
> > +%d\n", i2c_bus->irq);
> > +
> > + writel(AST2600_I2CM_PKT_DONE |
> AST2600_I2CM_BUS_RECOVER,
> > + i2c_bus->reg_base + AST2600_I2CM_IER);
> > +
> > + ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(ast2600_i2c_probe);
> > +
> > +void ast2600_i2c_remove(struct platform_device *pdev) {
> > + struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
> > +
> > + /* Disable everything. */
> > + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > + writel(0, i2c_bus->reg_base + AST2600_I2CM_IER); }
> > +EXPORT_SYMBOL_GPL(ast2600_i2c_remove);
> > +
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@...eedtech.com>");
> > +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
> > +MODULE_LICENSE("GPL");
>
> Cheers,
>
>
> Jeremy
Powered by blists - more mailing lists