Re: [PATCH v10 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
From: Andy Shevchenko
Date: Mon May 11 2020 - 05:18:02 EST
On Sun, May 10, 2020 at 01:23:29PM +0300, Tali Perry wrote:
> Add Nuvoton NPCM BMC I2C controller driver.
Some cosmetic changes needs to be done.
...
> +/*
> + * Nuvoton NPCM7xx I2C Controller driver
> + *
> + * Copyright (C) 2020 Nuvoton Technologies tali.perry@xxxxxxxxxxx
> + */
So, entire file has C99 comment style, but this and few other places.
Any reason of inconsistency?
...
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
Why?
> +#include <linux/debugfs.h>
> +#endif
...
> +//#define _I2C_DEBUG_
Leftover, remove.
...
> +// Common regs
> +#define NPCM_I2CSDA 0x00
> +#define NPCM_I2CST 0x02
> +#define NPCM_I2CCST 0x04
> +#define NPCM_I2CCTL1 0x06
Indentation issue. And it's better to use TABs over spaces here and
in the similar places above and below.
> +#define NPCM_I2CADDR1 0x08
> +#define NPCM_I2CCTL2 0x0A
> +#define NPCM_I2CADDR2 0x0C
> +#define NPCM_I2CCTL3 0x0E
> +#define NPCM_I2CCST2 0x18
> +#define NPCM_I2CCST3 0x19
...
> +// supported clk settings. values in KHz.
> +#define I2C_FREQ_MIN 10
> +#define I2C_FREQ_MAX 1000
_KHZ to both.
...
> +#define I2C_NUM_OF_ADDR 10
Is it 10-bit address support or what?
...
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static struct dentry *npcm_i2c_debugfs_dir; /* i2c debugfs directory */
> +static const char *ber_cnt_name = "ber_count";
> +static const char *rec_succ_cnt_name = "rec_succ_count";
> +static const char *rec_fail_cnt_name = "rec_fail_count";
> +static const char *nack_cnt_name = "nack_count";
> +static const char *timeout_cnt_name = "timeout_count";
> +#endif
Why are these global?
...
> +static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus,
> + u16 max_bytes_to_send)
> +{
> + // Fill the FIFO, while the FIFO is not full and there are more bytes to
> + // write
> + while ((max_bytes_to_send--) &&
Inner parentheses are redundant.
> + (I2C_HW_FIFO_SIZE - npcm_i2c_fifo_usage(bus))) {
> + if (bus->wr_ind < bus->wr_size)
> + npcm_i2c_wr_byte(bus, bus->wr_buf[bus->wr_ind++]);
> + else
> + npcm_i2c_wr_byte(bus, 0xFF);
> + }
> +}
...
> + // Clear stall only after setting STOP
> + iowrite8(NPCM_I2CST_STASTR, bus->reg + NPCM_I2CST);
> +
> + ret = IRQ_HANDLED;
Indentation issue.
...
> + if (bus->wr_size)
> + npcm_i2c_set_fifo(bus, -1,
> + bus->wr_size);
> + else
> + npcm_i2c_set_fifo(bus, bus->rd_size,
> + -1);
These two looks much better on one line.
...
> + if (npcm_i2c_is_quick(bus) || bus->wr_size)
> + npcm_i2c_wr_byte(bus, bus->dest_addr);
> + else
> + npcm_i2c_wr_byte(bus, bus->dest_addr |
> + 0x01);
0x01 has its definition, hasn't it?
...
> + // Repeat the following sequence until SDA is released
> + do {
> + // Issue a single SCL toggle
> + iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST);
> + udelay(20);
> + // If SDA line is inactive (high), stop
> + if (npcm_i2c_get_SDA(_adap)) {
> + done = true;
> + status = 0;
> + }
> + } while (!done && iter--);
readx_poll_timeout() ?
...
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> + if (!status) {
Why not positive condition?
if (status) {
...
} else {
...
}
> + } else {
> + }
> +#endif
...
> +static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq)
> +{
> + u32 k1 = 0;
> + u32 k2 = 0;
> + u8 dbnct = 0;
> + u32 sclfrq = 0;
> + u8 hldt = 7;
> + bool fast_mode = false;
> + u32 src_clk_freq; // in KHz
src_clk_khz ?
> +
> + src_clk_freq = bus->apb_clk / 1000;
> + bus->bus_freq = bus_freq;
> +
> + // 100KHz and below:
> + if (bus_freq <= (I2C_MAX_STANDARD_MODE_FREQ / 1000)) {
Instead of all these / 1000 can't you use bus frequency in Hz and do division
when it's really needed?
> + sclfrq = src_clk_freq / (bus_freq * 4);
> +
> + if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> + return -EDOM;
> +
> + if (src_clk_freq >= 40000)
> + hldt = 17;
> + else if (src_clk_freq >= 12500)
> + hldt = 15;
> + else
> + hldt = 7;
> + }
> +
> + // 400KHz:
> + else if (bus_freq <= (I2C_MAX_FAST_MODE_FREQ / 1000)) {
> + sclfrq = 0;
> + fast_mode = true;
> +
> + if (src_clk_freq < 7500)
> + // 400KHZ cannot be supported for core clock < 7.5 MHZ
> + return -EDOM;
> +
> + else if (src_clk_freq >= 50000) {
> + k1 = 80;
> + k2 = 48;
> + hldt = 12;
> + dbnct = 7;
> + }
> +
> + // Master or Slave with frequency > 25 MHZ
> + else if (src_clk_freq > 25000) {
> + hldt = (DIV_ROUND_UP(src_clk_freq * 300,
> + 1000000) + 7) & 0xFF;
How ' & 0xFF' is not no-op here and in the similar cases?
> +
> + k1 = DIV_ROUND_UP(src_clk_freq * 1600,
> + 1000000);
> + k2 = DIV_ROUND_UP(src_clk_freq * 900,
> + 1000000);
Perhaps,
#define clk_coef(freq, mul) DIV_ROUND_UP((freq) * (mul), 1000000)
?
> + k1 = round_up(k1, 2);
> + k2 = round_up(k2 + 1, 2);
> + if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
> + k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
> + return -EDOM;
> + }
> + }
> +
> + else if (bus_freq <= (I2C_MAX_FAST_MODE_PLUS_FREQ / 1000)) {
> + sclfrq = 0;
> + fast_mode = true;
> +
> + if (src_clk_freq < 24000)
> + // 1MHZ cannot be supported for master core clock < 15 MHZ
> + // or slave core clock < 24 MHZ
> + return -EDOM;
> +
> + k1 = round_up((DIV_ROUND_UP(src_clk_freq * 620,
> + 1000000)), 2);
> + k2 = round_up((DIV_ROUND_UP(src_clk_freq * 380,
> + 1000000) + 1), 2);
> + if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
> + k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
> + return -EDOM;
> +
> + // Core clk > 40 MHZ
> + if (src_clk_freq > 40000) {
> + // Set HLDT:
> + // SDA hold time: (HLDT-7) * T(CLK) >= 120
> + // HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> + hldt = (DIV_ROUND_UP(src_clk_freq * 120,
> + 1000000) + 7) & 0xFF;
> + } else {
> + hldt = 7;
> + dbnct = 2;
> + }
> + }
> +
> + // Frequency larger than 1 MHZ is not supported
> + else
> + return false;
> + return true;
> +}
...
> + ret = device_property_read_u32(&pdev->dev, "bus-frequency", &clk_freq);
> + if (ret < 0) {
> + dev_info(&pdev->dev, "Could not read bus-frequency property\n");
> + clk_freq = 100000;
We have define for this, don't we?
> + }
Wolfram, we discussed this simplified timings property parser,
any news about it?
...
> +static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)
> +{
> + irqreturn_t ret;
> + struct npcm_i2c *bus = dev_id;
> +
> + bus->int_cnt++;
> +
> + if (npcm_i2c_is_master(bus))
> + bus->master_or_slave = I2C_MASTER;
> +
> + if (bus->master_or_slave == I2C_MASTER) {
> + bus->int_time_stamp = jiffies;
> + ret = npcm_i2c_int_master_handler(bus);
> + if (ret == IRQ_HANDLED)
> + return ret;
What's the point?
> + }
> + return IRQ_HANDLED;
> +}
...
> + bus->dest_addr = (slave_addr << 1) & 0xFE;
How ' & 0xFE' part is not a no-op?
...
> + time_left = jiffies +
> + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
It's perfectly one line. Fix here and in any other place with similar issue.
...
> +static int i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus)
Should be void.
...
> + bus->irq = platform_get_irq(pdev, 0);
> + if (bus->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ number\n");
Duplicate message.
> + return bus->irq;
> + }
...
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
Why? Okay, why here instead of making a stub?
> + ret = i2c_init_debugfs(pdev, bus);
> + if (ret < 0)
> + return ret;
Wrong. Should not fail the probe.
> +#endif
...
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
Why? Just make it always present in the structure.
> + if (!!bus->debugfs) {
!! ???
> + debugfs_remove_recursive(bus->debugfs);
> + bus->debugfs = NULL;
> + }
> +#endif
...
> + npcm_i2c_debugfs_dir = debugfs_create_dir("i2c", NULL);
> + if (IS_ERR_OR_NULL(npcm_i2c_debugfs_dir)) {
> + pr_warn("i2c init of debugfs failed\n");
> + npcm_i2c_debugfs_dir = NULL;
> + return -ENOMEM;
> + }
Shouldn't prevent driver to work.
...
> + if (!!npcm_i2c_debugfs_dir) {
!! ???
> + debugfs_remove_recursive(npcm_i2c_debugfs_dir);
> + npcm_i2c_debugfs_dir = NULL;
What's the point?
> + }
--
With Best Regards,
Andy Shevchenko