| 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
| ||
|
Date: Sun, 8 Sep 2013 19:03:56 +0200
From: Wolfram Sang <wsa@...-dreams.de>
To: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, w.sang@...gutronix.de,
t.figa@...sung.com, ben-linux@...ff.org, grant.likely@...retlab.ca,
devicetree-discuss@...ts.ozlabs.org, sjg@...omium.org,
naveenkrishna.ch@...il.com, broonie@...nsource.wolfsonmicro.com
Subject: Re: [PATCH] i2c: exynos5: add High Speed I2C controller driver
On Wed, Aug 21, 2013 at 02:54:37PM +0530, Naveen Krishna Ch wrote:
> Adds support for High Speed I2C driver found in Exynos5 and
> later SoCs from Samsung.
>
> Highspeed mode is a minor change in the i2c protocol.
> Starts with
> 1. start condition,
> 2. 8-bit master ID code (00001xxx)
> 3. followed by a NACK bit
> Once the above conditions are met, the bus is now operates in highspeed mode.
> The rest of the I2C protocol applies the same.
The description is correct, but it is not a change in the protocol. This
is fully specified in the I2C specs.
> Driver only supports Device Tree method.
>
> Changes since v1:
> 1. Added FIFO functionality
> 2. Added High speed mode functionality
> 3. Remove SMBUS_QUICK
> 4. Remove the debugfs functionality
> 5. Use devm_* functions where ever possible
> 6. Driver is free from GPIO configs
> 7. Use OF data string "clock-frequency" to get the bus operating frequencies
> 8. Split the clock divisor calculation function
> 9. Add resets for the failed transacton cases
> 10. Removed retries as core does retries if -EAGAIN is returned
> 11. Removed mode from device tree info (use speed to distinguish
> the mode of operation)
> 12. Use wait_for_completion_timeout as the interruptible case is not tested well
> 13. few other bug fixes and cosmetic changes
> 14. Removed the untested runtime_pm code
> 15. Removed the retries as core does that
> 16. Use adap.nr instead of alias
> 17. Added spinlocks around the irq code
> 18. Use i2c_add_numbered_adapter() instead of using aliases
Changes since v1 are irrelevant for the patch description.
>
> Signed-off-by: Taekgyun Ko <taeggyun.ko@...sung.com>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
> Reviewed-by: Simon Glass <sjg@...gle.com>
> Tested-by: Andrew Bresticker <abrestic@...gle.com>
> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@...sung.com>
> Signed-off-by: Andrew Bresticker <abrestic@...gle.com>
>
> ---
> Wolfram and Thomas Figa thanks for reviewing the code.
>
> Changes since v10:
> 1. Remove the incomplete runtime_pm code
> 2. Correct the error checks as suggested by Thomas
> 3. Use i2c_add_numbered_adapter() as suggested
> 4. Modified the irq routine to handle the specific interrupts
> 5. Added spinlocks around the irq code
> 6. Remove the "mode" of operation field from device tree node and use the
> clock-frequency to decide the mode.
>
> .../devicetree/bindings/i2c/i2c-exynos5.txt | 44 ++
> drivers/i2c/busses/Kconfig | 7 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-exynos5.c | 799 ++++++++++++++++++++
> 4 files changed, 851 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> create mode 100644 drivers/i2c/busses/i2c-exynos5.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> new file mode 100644
> index 0000000..805e018
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
> @@ -0,0 +1,44 @@
> +* Samsung's High Speed I2C controller
> +
> +The Samsung's High Speed I2C controller is used to interface with I2C devices
> +at various speeds ranging from 100khz to 3.4Mhz.
> +
> +Required properties:
> + - compatible: value should be.
> + -> "samsung,exynos5-hsi2c", for i2c compatible with exynos5 hsi2c.
> + - reg: physical base address of the controller and length of memory mapped
> + region.
> + - interrupts: interrupt number to the cpu.
> + - #address-cells: always 1 (for i2c addresses)
> + - #size-cells: always 0
> +
> + - Pinctrl:
> + - pinctrl-0: Pin control group to be used for this controller.
> + - pinctrl-names: Should contain only one value - "default".
> +
> +Optional properties:
> + - clock-frequency: Desired operating frequency in Hz of the bus.
> + -> If not specified, the default value is 100khz in fast-speed mode and
> + 1Mhz in high-speed mode.
Description doesn't match the current code.
> + -> If specified, The bus operates in high-speed mode only if the
> + clock-frequency is >= 1Mhz.
s/The/the/
...
> +/*
> + * exynos5_i2c_init: configures the controller for I2C functionality
> + * Programs I2C controller for Master mode operation
> + */
> +static void exynos5_i2c_init(struct exynos5_i2c *i2c)
> +{
> + u32 i2c_conf = readl(i2c->regs + HSI2C_CONF);
> + u32 i2c_timeout = readl(i2c->regs + HSI2C_TIMEOUT);
> +
> + /* Disable timeout */
> + i2c_timeout &= ~HSI2C_TIMEOUT_EN;
> + writel(i2c_timeout, i2c->regs + HSI2C_TIMEOUT);
Just curious: Can't you use HSI2C_TIMEOUT and wait_for_completion
instead of wait_for_completion_timeout? If so, it might save you a
little bit of overhead.
> +
> + writel((HSI2C_FUNC_MODE_I2C | HSI2C_MASTER),
> + i2c->regs + HSI2C_CTL);
> + writel(HSI2C_TRAILING_COUNT, i2c->regs + HSI2C_TRAILIG_CTL);
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD) {
> + writel(HSI2C_MASTER_ID(MASTER_ID(i2c->adap.nr)),
> + i2c->regs + HSI2C_ADDR);
> + i2c_conf |= HSI2C_HS_MODE;
> + }
> +
> + writel(i2c_conf | HSI2C_AUTO_MODE, i2c->regs + HSI2C_CONF);
> +}
> +
> +
> + if ((i2c->msg->flags & I2C_M_RD) && (int_status &
> + (HSI2C_INT_TRAILING | HSI2C_INT_RX_ALMOSTFULL))) {
> + fifo_status = readl(i2c->regs + HSI2C_FIFO_STATUS);
> + fifo_level = HSI2C_RX_FIFO_LVL(fifo_status);
> + len = min(fifo_level, i2c->msg->len - i2c->msg_ptr);
> +
> + while (len > 0) {
> + byte = (unsigned char)
> + readl(i2c->regs + HSI2C_RX_DATA);
> + i2c->msg->buf[i2c->msg_ptr++] = byte;
> + len--;
> + }
With all this copying going on, this should be threaded irq probably.
> + i2c->state = 0;
> + } else if (int_status & HSI2C_INT_TX_ALMOSTEMPTY) {
> + fifo_status = readl(i2c->regs + HSI2C_FIFO_STATUS);
> + fifo_level = HSI2C_TX_FIFO_LVL(fifo_status);
> +
> + len = HSI2C_FIFO_MAX - fifo_level;
> + if (len > (i2c->msg->len - i2c->msg_ptr))
> + len = i2c->msg->len - i2c->msg_ptr;
> +
> + while (len > 0) {
> + byte = i2c->msg->buf[i2c->msg_ptr++];
> + writel(byte, i2c->regs + HSI2C_TX_DATA);
> + len--;
> + }
> + i2c->state = 0;
> + }
> +
> + stop:
> + if ((i2c->msg_ptr == i2c->msg->len) || (i2c->state < 0)) {
> + writel(0, i2c->regs + HSI2C_INT_ENABLE);
> + exynos5_i2c_clr_pend_irq(i2c);
> + complete(&i2c->msg_complete);
> + } else {
> + exynos5_i2c_clr_pend_irq(i2c);
> + }
> +
> + spin_unlock_irqrestore(&i2c->lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
...
> +static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
> + struct i2c_msg *msgs, int stop)
> +{
> + unsigned long timeout;
> + int ret;
> +
> + INIT_COMPLETION(i2c->msg_complete);
> +
> + spin_lock_irq(&i2c->lock);
> + i2c->msg = msgs;
> + i2c->msg_ptr = 0;
> + i2c->trans_done = 0;
> +
> + exynos5_i2c_message_start(i2c, stop);
> +
> + spin_unlock_irq(&i2c->lock);
> + timeout = wait_for_completion_timeout(&i2c->msg_complete,
> + EXYNOS5_I2C_TIMEOUT);
> + if (timeout == 0)
> + ret = -ETIMEDOUT;
> + else
> + ret = i2c->state;
> +
> + if (ret < 0) {
> + exynos5_i2c_reset(i2c);
Do you really need to reset the core after a failed transaction?
> + if (ret == -ETIMEDOUT) {
> + dev_warn(i2c->dev, "%s timeout\n",
> + (msgs->flags & I2C_M_RD) ? "rx" : "tx");
> + return ret;
> + } else if (ret == -EAGAIN) {
> + return ret;
> + }
> + }
> +
> + /*
> + * If this is the last message to be transfered (stop == 1)
> + * Then check if the bus can be brought back to idle.
> + *
> + * Return -EBUSY if the bus still busy.
> + */
> + if (exynos5_i2c_wait_bus_idle(i2c, stop))
> + return -EBUSY;
Why do you need this after the transaction?
> +
> + /* Return the state as in interrupt routine */
> + return ret;
> +}
> +
> +static int exynos5_i2c_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct exynos5_i2c *i2c = (struct exynos5_i2c *)adap->algo_data;
> + int i = 0, ret = 0, stop = 0;
> +
> + if (i2c->suspended) {
> + dev_err(i2c->dev, "HS-I2C is not initialzed.\n");
> + return -EIO;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + for (i = 0; i < num; i++) {
> + stop = (i == num - 1);
> +
> + if (msgs->len == 0) {
Did you need that? It shouldn't happen since you are not advertising
SMBUS_QUICK.
> + msgs++;
> + continue;
> + }
> +
> + ret = exynos5_i2c_xfer_msg(i2c, msgs, stop);
> + if (!stop)
> + msgs++;
Probably better put in the for-body (i.e. i++, msgs++).
> +
> + if (ret < 0)
> + goto out;
> + }
> +
> + if (i == num) {
> + ret = num;
> + } else {
> + /* Only one message, cannot access the device */
> + if (i == 1)
> + ret = -EREMOTEIO;
> + else
> + ret = i;
> +
> + dev_warn(i2c->dev, "xfer message failed\n");
> + }
> +
> + out:
> + clk_disable_unprepare(i2c->clk);
> + return ret;
> +}
> +
> +static u32 exynos5_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +}
> +
> +static const struct i2c_algorithm exynos5_i2c_algorithm = {
> + .master_xfer = exynos5_i2c_xfer,
> + .functionality = exynos5_i2c_func,
> +};
> +
> +static int exynos5_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct exynos5_i2c *i2c;
> + struct resource *mem;
> + unsigned int op_clock;
> + int ret;
> +
> + if (!np) {
> + dev_err(&pdev->dev, "no device node\n");
> + return -ENOENT;
> + }
How should this happen?
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(struct exynos5_i2c), GFP_KERNEL);
> + if (!i2c) {
> + dev_err(&pdev->dev, "no memory for state\n");
> + return -ENOMEM;
> + }
> +
...
> + i2c->adap.nr = -1;
> + ret = i2c_add_numbered_adapter(&i2c->adap);
Just use i2c_add_adapter and skip setting nr to -1.
...
> +static const struct dev_pm_ops exynos5_i2c_dev_pm_ops = {
> + .suspend_noirq = exynos5_i2c_suspend_noirq,
> + .resume_noirq = exynos5_i2c_resume_noirq,
> +};
> +
> +#define EXYNOS5_DEV_PM_OPS (&exynos5_i2c_dev_pm_ops)
> +#else
> +#define EXYNOS5_DEV_PM_OPS NULL
> +#endif
Isn't there a macro for this? SIMPLE_DEV_PM_OPS*? Not sure, I always mix
them up...
Regards,
Wolfram
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists