| Message ID | 20200520095113.185414-1-tali.perry1@gmail.com |
|---|---|
| Headers | show |
| Series | i2c: npcm7xx: add NPCM i2c controller driver | expand |
On Wed, May 20, 2020 at 12:51:12PM +0300, Tali Perry wrote: > Add Nuvoton NPCM BMC I2C controller driver. ... > +#ifdef CONFIG_DEBUG_FS Why?! > +#include <linux/debugfs.h> > +#endif ... > +/* Status of one I2C module */ > +struct npcm_i2c { > + struct i2c_adapter adap; > + struct device *dev; Isn't it adap.dev->parent? > +}; ... > +static void npcm_i2c_master_abort(struct npcm_i2c *bus) > +{ > + /* Only current master is allowed to issue a stop condition */ > + if (npcm_i2c_is_master(bus)) { if (!npcm_i2c_is_master(bus)) return; ? > + npcm_i2c_eob_int(bus, true); > + npcm_i2c_master_stop(bus); > + npcm_i2c_clear_master_status(bus); > + } > +} ... > +/* SDA status is set - TX or RX, master */ > +static void npcm_i2c_irq_handle_sda(struct npcm_i2c *bus, u8 i2cst) > +{ > + u8 fif_cts; > + if (bus->state == I2C_IDLE) { > + if (npcm_i2c_is_master(bus)) { if (a) { if (b) { ... } } == if (a && b) { ... } Check whole code for such pattern. > + } > + > + /* SDA interrupt, after start\restart */ > + } else { > + if (NPCM_I2CST_XMIT & i2cst) { > + bus->operation = I2C_WRITE_OPER; > + npcm_i2c_irq_master_handler_write(bus); > + } else { > + bus->operation = I2C_READ_OPER; > + npcm_i2c_irq_master_handler_read(bus); > + } > + } > +} ... > + } > + + /* 1MHz */ ? > + else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) { > + } > + > + /* Frequency larger than 1 MHZ is not supported */ > + else > + return -EINVAL; ... > + // master and slave modes share a single irq. It's again being inconsistent with comment style. Choose one and fix all comments accordingly (SPDX is another story, though) ... > +static int i2c_debugfs_get(void *data, u64 *val) > +{ > + *val = *(u64 *)(data); > + return 0; > +} > +DEFINE_DEBUGFS_ATTRIBUTE(i2c_debugfs_ops, i2c_debugfs_get, NULL, "0x%02llx\n"); Why not to use debugfs_create_u64(), or how is it called? > +static void i2c_init_debugfs(struct platform_device *pdev, struct npcm_i2c *bus) > +{ > + if (!npcm_i2c_debugfs_dir) > + return; > + > + if (!pdev || !bus) > + return; How is it possible? > + bus->debugfs = debugfs_create_dir(dev_name(&pdev->dev), > + npcm_i2c_debugfs_dir); > + if (IS_ERR_OR_NULL(bus->debugfs)) { > + bus->debugfs = NULL; > + return; > + } struct dentry *d; d = create(...); if (IS_ERR_OR_NULL(d)) return; bus->... = d; > + > + debugfs_create_file("ber_count", 0444, bus->debugfs, > + &bus->ber_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("rec_succ_count", 0444, bus->debugfs, > + &bus->rec_succ_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("rec_fail_count", 0444, bus->debugfs, > + &bus->rec_fail_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("nack_count", 0444, bus->debugfs, > + &bus->nack_count, > + &i2c_debugfs_ops); > + > + debugfs_create_file("timeout_count", 0444, bus->debugfs, > + &bus->timeout_count, > + &i2c_debugfs_ops); > +} ... > +#ifdef CONFIG_DEBUG_FS Why?! > + i2c_init_debugfs(pdev, bus); > +#endif ... > +#ifdef CONFIG_DEBUG_FS Ditto. > + debugfs_remove_recursive(bus->debugfs); > +#endif > +static int __init npcm_i2c_init(void) > +{ > + npcm_i2c_debugfs_dir = debugfs_create_dir("i2c", NULL); You didn't compile this with !CONFIG_DEBUG_FS? > + if (IS_ERR_OR_NULL(npcm_i2c_debugfs_dir)) { > + pr_warn("i2c init of debugfs failed\n"); > + npcm_i2c_debugfs_dir = NULL; > + } See above for the better pattern. Why do you need noisy warning? What does it say to user? Can they use device or not? > + return 0; > +}
Thanks Andy, Question below: On Wed, May 20, 2020 at 1:24 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, May 20, 2020 at 12:51:12PM +0300, Tali Perry wrote: > > Add Nuvoton NPCM BMC I2C controller driver. > > ... > > > +#ifdef CONFIG_DEBUG_FS > > Why?! It is made to save code size if CONFIG_DEBUG_FS is not defined? We see a lot of kernel code that is doing it. So could you elaborate what is the problem? > > > +#include <linux/debugfs.h> > > +#endif >
On Wed, May 20, 2020 at 02:37:13PM +0300, Avi Fishman wrote: > On Wed, May 20, 2020 at 1:24 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Wed, May 20, 2020 at 12:51:12PM +0300, Tali Perry wrote: > > > Add Nuvoton NPCM BMC I2C controller driver. > > > > ... > > > > > +#ifdef CONFIG_DEBUG_FS > > > > Why?! > > It is made to save code size if CONFIG_DEBUG_FS is not defined? Nope (in cases I have commented on). Try again. > We see a lot of kernel code that is doing it. Cargo cult, okay. So, somebody should try to understand what they are doing. > So could you elaborate what is the problem? Problem 1: ugly code. Problem 2: some of the code is not guarded (seems never been tested with disabled debugfs). Problem 3: it's not needed. > > > +#include <linux/debugfs.h> > > > +#endif
This patch set adds i2c controller support for the Nuvoton NPCM Baseboard Management Controller (BMC). NPCM7xx includes 16 I2C controllers. This driver operates the controller. This module also includes a slave mode. --- v11 -> v10: - Fix according to maintainer comments. - Init clk simplified. - Comments in c99 - Split master irq function. - debugfs not mandatory. - yaml file fix. v10 -> v9: - Fix according to maintainer comments. - binding file changed to yaml format. - Shorten recovery flow. - Add support for health monitoring counters. v9 -> v8: - Fix according to maintainer comments. - Split lines of iowrite..(ioread..) to separate lines. - Use readx_poll_timeout_atomic - resolve various style issues. v8 -> v7: - Split to two commits, one for master, one for slave. - Rename smb to i2c. - Remove global vars. v7 -> v6: - Rebased on Linux 5.4-rc8 (was Linux 5.4-rc7). - Fix issue found by kbuild test robot (redundant include). - Note: left a warning related to fall through. This fall through is intentional. v6 -> v5: - Update documentation v5 -> v4: - support recovery - master-slave switch support needed for IPMB v4 -> v3: - typo on cover letter. v3 -> v2: - fix dt binding: compatible name: omit "bus" v2 -> v1: - run check patch in strict mode. - use linux crc. - define regs in constant offset without base. - remove debug prints. - no declarations for local functions. v1: initial version Signed-off-by: Tali Perry <tali.perry1@gmail.com> Reported-by: kbuild test robot <lkp@intel.com> --- Tali Perry (3): dt-bindings: i2c: npcm7xx: add NPCM I2C controller documentation i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver i2c: npcm7xx: Add support for slave mode for Nuvoton NPCM BMC I2C controller driver. .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 62 + drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-npcm7xx.c | 2480 +++++++++++++++++ 4 files changed, 2552 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c base-commit: b9bbe6ed63b2b9f2c9ee5cbd0f2c946a2723f4ce