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.pe...@nuvoton.com > + */ 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