On Sun, Apr 30, 2023 at 12:17:12PM +0800, Ryan Chen wrote:
> Add i2c new register mode driver to support AST2600 i2c
> new register mode. AST2600 i2c controller have legacy and
> new register mode. The new register mode have global register
> support 4 base clock for scl clock selection, and new clock
> divider mode. The i2c new register mode have separate register
> set to control i2c master and slave.
...
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/reset.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/of_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/i2c-smbus.h>
Ordered?
...
> +#define AST2600_GLOBAL_INIT \
> + (AST2600_I2CG_CTRL_NEW_REG | \
> + AST2600_I2CG_CTRL_NEW_CLK_DIV)
Make just a one TAB and put the last two lines on the single one.
...
> +#define I2CCG_DIV_CTRL 0xC6411208
Is it decimal? Is it combination of bitfields? Can you add a comment what is
this magic?
...
> +struct ast2600_i2c_timing_table {
> + u32 divisor;
> + u32 timing;
> +};
Is it even used?
...
> +enum xfer_mode {
> + BYTE_MODE = 0,
Why explicit assignment?
> + BUFF_MODE,
> + DMA_MODE,
> +};
...
> + base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2);
> + base_clk2 = (i2c_bus->apb_clk * 10) /
> + (((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2);
> + base_clk3 = (i2c_bus->apb_clk * 10) /
> + (((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2);
> + base_clk4 = (i2c_bus->apb_clk * 10) /
> + (((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2);
The same equation is used per each byte of clk_div_reg? Can it be rewritten to
avoid this and using loop, so you will have an array of base_clk to be filled?
Don't forget to use GENMASK().
...
> + if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) {
> + baseclk_idx = 0;
> + divisor = DIV_ROUND_UP(i2c_bus->apb_clk, i2c_bus->bus_frequency);
> + } else if ((base_clk1 / i2c_bus->bus_frequency) <= 32) {
> + baseclk_idx = 1;
> + divisor = DIV_ROUND_UP(base_clk1, i2c_bus->bus_frequency);
> + } else if ((base_clk2 / i2c_bus->bus_frequency) <= 32) {
> + baseclk_idx = 2;
> + divisor = DIV_ROUND_UP(base_clk2, i2c_bus->bus_frequency);
> + } else if ((base_clk3 / i2c_bus->bus_frequency) <= 32) {
> + baseclk_idx = 3;
> + divisor = DIV_ROUND_UP(base_clk3, i2c_bus->bus_frequency);
Will be optimized with above suggestion, I believe.
> + } else {
> + baseclk_idx = 4;
> + divisor = DIV_ROUND_UP(base_clk4, i2c_bus->bus_frequency);
> + inc = 0;
> + while ((divisor + inc) > 32) {
> + inc |= divisor & 0x1;
> + divisor >>= 1;
> + baseclk_idx++;
> + }
> + divisor += inc;
I think the above loop can be rewritten to have better representation.
> + }
...
> + baseclk_idx &= 0xf;
GENMASK()?
...
> + scl_low = ((divisor * 9) / 16) - 1;
> + scl_low = min_t(u32, scl_low, 0xf);
This can be done in one line. Also, why not 15?
...
> + scl_high = (divisor - scl_low - 2) & 0xf;
GENMASK()?
...
> + data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | (baseclk_idx);
Too many parentheses.
...
> + /* due to master slave is common buffer, so need force the master stop not issue */
> + if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) {
GENMASK() ?
> + writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> + i2c_bus->cmd_err = -EBUSY;
> + writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> + complete(&i2c_bus->cmd_complete);
> + }
...
> + /* send start */
> + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
> + i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write",
str_read_write() ?
> + msg->len, msg->len > 1 ? "s" : "",
> + msg->flags & I2C_M_RD ? "from" : "to", msg->addr);
...
> + for (i = 0; i < xfer_len; i++) {
> + wbuf[i % 4] = msg->buf[i];
> + if (i % 4 == 3)
> + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
Err.. There can be alignment issues.
> + }
> + if (--i % 4 != 3)
> + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
The above code is ugly. Can you think about it and write in a better way?
...
> + for (i = 0; i < xfer_len; i++) {
> + wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
> + if (i % 4 == 3)
> + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
> + }
> + if (--i % 4 != 3)
> + writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
Ditto.
...
Do you have similar code pieces? Why not doing it in a separate function with
parameters?
...
> + return ast2600_i2c_master_irq(i2c_bus) ? IRQ_HANDLED : IRQ_NONE;
IRQ_RETVAL() ?
...
> + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR);
GENMASK()
...
> + writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR);
Ditto.
> + if (i2c_bus->mode == BYTE_MODE) {
> + writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER);
Ditto.
> + } else {
> + /* Set interrupt generation of I2C slave controller */
> + writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
> + }
...
> + WARN_ON(!i2c_bus->slave);
Why?
...
> +static const struct of_device_id ast2600_i2c_bus_of_table[] = {
> + {
> + .compatible = "aspeed,ast2600-i2cv2",
> + },
> + {}
> +};
> +
Redundant blank line.
> +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table);
...
> + i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL);
> + if (!i2c_bus)
> + return dev_err_probe(dev, -ENOMEM, "no memory allocated\n");
No. We do not print error message for ENOMEM.
You homework to find why.
...
> + if (of_property_read_bool(pdev->dev.of_node, "aspeed,enable-dma"))
device_property_read_bool() ?
> + i2c_bus->mode = DMA_MODE;
...
> + if (i2c_bus->mode == BUFF_MODE) {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (res && resource_size(res) >= 2) {
> + i2c_bus->buf_base = devm_ioremap_resource(dev, res);
> +
> + if (!IS_ERR_OR_NULL(i2c_bus->buf_base))
> + i2c_bus->buf_size = resource_size(res) / 2;
> + } else {
> + i2c_bus->mode = BYTE_MODE;
> + }
> + }
Can be done without additional checks and with a simple call to
devm_platform_ioremap_resource(). No?
...
> + /* i2c timeout counter: use base clk4 1Mhz,
> + * per unit: 1/(1000/4096) = 4096us
> + */
Wrong multi-line style of the comment.
...
> + ret = of_property_read_u32(dev->of_node,
> + "i2c-scl-clk-low-timeout-us",
> + &i2c_bus->timeout);
> + if (!ret)
> + i2c_bus->timeout /= 4096;
What is this and why I2C core timings (standard) can't be utilized here?
...
> + ret = of_property_read_u32(dev->of_node, "clock-frequency", &i2c_bus->bus_frequency);
> + if (ret < 0) {
> + dev_warn(dev, "Could not read clock-frequency property\n");
> + i2c_bus->bus_frequency = 100000;
> + }
There are macro for standard speeds. Moreover, there is a function to parse
properties, no need to open code.
...
> + i2c_bus->adap.dev.of_node = dev->of_node;
device_set_node()
...
> + if (of_property_read_bool(dev->of_node, "smbus-alert")) {
Doesn't core have already support for this?
> + i2c_bus->alert_enable = true;
> + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data);
> + if (!i2c_bus->ara)
> + dev_warn(dev, "Failed to register ARA client\n");
> +
> + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT,
> + i2c_bus->reg_base + AST2600_I2CM_IER);
> + } else {
> + i2c_bus->alert_enable = false;
> + /* Set interrupt generation of I2C master controller */
> + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> + i2c_bus->reg_base + AST2600_I2CM_IER);
> + }
...
> + dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n",
> + dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency / 1000,
> + i2c_bus->mode);
Useless noise.
--
With Best Regards,
Andy Shevchenko
