| Message ID | 7bc5c69030b6e491cfb2519eb1b21c1b0adc70c3.1498029380.git.baolin.wang@spreadtrum.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Jun 21, 2017 at 03:23:03PM +0800, Baolin Wang wrote: > This patch adds the binding documentation for Spreadtrum I2C > controller device. > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > --- > Changes since v1: > - No updates. > --- > Documentation/devicetree/bindings/i2c/i2c-sprd.txt | 31 ++++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sprd.txt > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-sprd.txt b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt > new file mode 100644 > index 0000000..f285e5b > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt > @@ -0,0 +1,31 @@ > +I2C for Spreadtrum platforms > + > +Required properties: > +- compatible: Should be "sprd,r8p0-i2c". Compatible strings should be SoC specific. > +- reg: Specify the physical base address of the controller and length > + of memory mapped region. > +- interrupts: Should contain I2C interrupt. > +- clock-names: Should contain following entries: > + "i2c" for I2C clock, > + "source" for I2C source (parent) clock, > + "enable" for I2C module enable clock. > +- clocks: Should contain a clock specifier for each entry in clock-names. > +- clock-frequency: Constains desired I2C bus clock frequency in Hz. > +- #address-cells: Should be 1 to describe address cells for I2C device address. > +- #size-cells: Should be 0 means no size cell for I2C device address. > + > +Optional properties: > +- Child nodes conforming to I2C bus binding > + > +Examples: > +i2c0: i2c@70500000 { > + compatible = "sprd,r8p0-i2c"; > + reg = <0 0x70500000 0 0x1000>; > + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>; > + clock-names = "i2c", "source", "enable"; > + clocks = <&clk_i2c3>, <&ext_26m>, <&clk_ap_apb_gates 11>; > + clock-frequency = <400000>; > + #address-cells = <1>; > + #size-cells = <0>; > +}; > + > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 21, 2017 at 10:23 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > This patch adds the I2C controller driver for Spreadtrum platform. Needs more work. See my comments below. > +#include <linux/init.h> > +#include <linux/module.h> Since your answer to the comment about arch_initcall you perhaps need to get rid of tristate (use bool) and drop module.h here and all leftovers like MODULE_*() calls. > +#include <linux/slab.h> Should we include this still explicitly (after kernel.h)? > + > +#define I2C_CTL 0x0 > +#define I2C_ADDR_CFG 0x4 > +#define I2C_COUNT 0x8 > +#define I2C_RX 0xC > +#define I2C_TX 0x10 > +#define I2C_STATUS 0x14 > +#define I2C_HSMODE_CFG 0x18 > +#define I2C_VERSION 0x1C > +#define ADDR_DVD0 0x20 > +#define ADDR_DVD1 0x24 > +#define ADDR_STA0_DVD 0x28 > +#define ADDR_RST 0x2C 1. Please, use fixed sized values 0x00 and so on. 2. Please, low case. > +#define SPRD_I2C_TIMEOUT (msecs_to_jiffies(1000)) Redundant parens. > +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev) If you switch your driver to regmap API, you may drop this function completely. > + writel(tmp & (~STP_EN), i2c_dev->base + I2C_CTL); Redundant parens. Also pay attention to other similar places. > +static void sprd_i2c_write_bytes(struct sprd_i2c *i2c_dev, u8 *buf, u32 len) So, isn't better to provide a writesb(), readsb() to ia64 instead of doing below? > +{ > + u32 i = 0; > + > + for (i = 0; i < len; i++) { > + writel(buf[i], i2c_dev->base + I2C_TX); > + dev_dbg(&i2c_dev->adap.dev, "write bytes[%d] = %x\n", i, buf[i]); Moreover, don't waste lines in the kernel log buffer by doing this. Choose either %*ph (up to 64 bytes) or print_hex_dump() (Same for _read_bytes() below) > + } > +} > +static void sprd_i2c_set_full_thld(struct sprd_i2c *i2c_dev, u32 full_thld) > +{ > + unsigned int tmp = readl(i2c_dev->base + I2C_CTL); > + > + tmp &= ~(0xf << FIFO_AF_LVL); Magic. Define it using GENMASK() > + tmp |= (full_thld << FIFO_AF_LVL); Redundant parens. > + tmp &= ~(0xf << FIFO_AE_LVL); > + tmp |= (empty_thld << FIFO_AE_LVL); Same. > +static void sprd_i2c_opt_mode(struct sprd_i2c *i2c_dev, int rw) > +{ > + unsigned int cmd = readl(i2c_dev->base + I2C_CTL) & (~I2C_MODE); Redundant parens. > + writel((cmd | rw << 3), i2c_dev->base + I2C_CTL); Ditto. It's a C, and not a LISP :-) > +} > +static void sprd_i2c_data_transfer(struct sprd_i2c *i2c_dev) > +{ > + if ((msg->flags & I2C_M_RD)) { Redundant parens. > + /* Reset rx/tx fifos */ Noise. > + /* Set device address */ Ditto. > + /* Set I2C read or write bytes count */ Ditto. > + if ((msg->flags & I2C_M_RD)) { > + /* Set read operation */ > + sprd_i2c_opt_mode(i2c_dev, 1); > + /* Last byte transmission should excute stop operation */ > + sprd_i2c_send_stop(i2c_dev, 1); Same comments as above. > + } else { > + /* Set write operation */ Noise. > + /* Last byte transmission should excute stop operation */ > + if (is_last_msg) > + sprd_i2c_send_stop(i2c_dev, 1); > + else > + sprd_i2c_send_stop(i2c_dev, 0); sprd_i2c_send_stop(i2c_dev, !!is_last_msg); Though, consider to make is_last_msg boolean. > +static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap, > + struct i2c_msg *msgs, int num) > +{ > + struct sprd_i2c *i2c_dev = i2c_adap->algo_data; > + int im, ret; > + > + ret = pm_runtime_get_sync(i2c_dev->dev); > + if (ret < 0) > + return ret; > + > + for (im = 0; im != num; im++) im < num - 1, and... ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im], 0); ... ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im++], 1); ...and we clearly see that you have messed up with returned code on each itteration here > + pm_runtime_mark_last_busy(i2c_dev->dev); > + pm_runtime_put_autosuspend(i2c_dev->dev); > + return (ret >= 0) ? im : ret; Usual pattern is ret < 0 ? ret : im; > +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, unsigned int freq) > +{ > + u32 apb_clk = i2c_dev->src_clk; > + u32 i2c_dvd = apb_clk / (4 * freq) - 1; > + u32 high = ((i2c_dvd << 1) * 2) / 5; > + u32 low = ((i2c_dvd << 1) * 3) / 5; > + u32 div0 = (high & 0xffff) << 16 | (low & 0xffff); > + u32 div1 = (high & 0xffff0000) | ((low & 0xffff0000) >> 16); Magic masks all over. Also it needs a comment what formula is used and how. > + > + writel(div0, i2c_dev->base + ADDR_DVD0); > + writel(div1, i2c_dev->base + ADDR_DVD1); > + > + if (freq == 400000) > + writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > + else if (freq == 100000) > + writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD); What about 3400000? What about the rest of the speeds, shouldn't you return an error from here? > +} > + > +static void sprd_i2c_enable(struct sprd_i2c *i2c_dev) > +{ > + unsigned int tmp = readl(i2c_dev->base + I2C_CTL); > + > + tmp &= ~(I2C_EN | I2C_TRIM_OPT | (0xF << FIFO_AF_LVL) | > + (0xF << FIFO_AE_LVL)); Magic masks (I saw them already above). > + /* Set rx fifo full data threshold */ Drop noise comments. They don't bring any value since you have nice function names. > + sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD); > + /* Set tx fifo empty data threshold */ > + sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD); > + > + sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq); > + /* Reset rx/tx fifo */ > + sprd_i2c_reset_fifo(i2c_dev); > + sprd_i2c_clear_irq(i2c_dev); > +static int sprd_i2c_clk_init(struct sprd_i2c *i2c_dev) > +{ > + struct clk *clk_i2c, *clk_parent; > + struct device_node *np = i2c_dev->adap.dev.of_node; > + > + clk_i2c = of_clk_get_by_name(np, "i2c"); What the issue to use resource agnostic API here, i.e. devm_clk_get() ? > + clk_parent = of_clk_get_by_name(np, "source"); Ditto. > + i2c_dev->clk = of_clk_get_by_name(np, "enable"); Ditto. > + if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop)) > + i2c_dev->bus_freq = prop; > + > + sprd_i2c_clk_init(i2c_dev); > + platform_set_drvdata(pdev, i2c_dev); > + > + ret = clk_prepare_enable(i2c_dev->clk); > + if (ret) > + return ret; > + > + sprd_i2c_enable(i2c_dev); > + > +error: I would put it as err_rpm_put: > + pm_runtime_put_noidle(i2c_dev->dev); > + pm_runtime_disable(i2c_dev->dev); > + clk_disable_unprepare(i2c_dev->clk); > + return ret; > +}
Hi Rob, On 五, 6月 23, 2017 at 05:20:16下午 -0500, Rob Herring wrote: > On Wed, Jun 21, 2017 at 03:23:03PM +0800, Baolin Wang wrote: > > This patch adds the binding documentation for Spreadtrum I2C > > controller device. > > > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > > --- > > Changes since v1: > > - No updates. > > --- > > Documentation/devicetree/bindings/i2c/i2c-sprd.txt | 31 ++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sprd.txt > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-sprd.txt b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt > > new file mode 100644 > > index 0000000..f285e5b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt > > @@ -0,0 +1,31 @@ > > +I2C for Spreadtrum platforms > > + > > +Required properties: > > +- compatible: Should be "sprd,r8p0-i2c". > > Compatible strings should be SoC specific. Will fix this in next version. Thanks. > > > +- reg: Specify the physical base address of the controller and length > > + of memory mapped region. > > +- interrupts: Should contain I2C interrupt. > > +- clock-names: Should contain following entries: > > + "i2c" for I2C clock, > > + "source" for I2C source (parent) clock, > > + "enable" for I2C module enable clock. > > +- clocks: Should contain a clock specifier for each entry in clock-names. > > +- clock-frequency: Constains desired I2C bus clock frequency in Hz. > > +- #address-cells: Should be 1 to describe address cells for I2C device address. > > +- #size-cells: Should be 0 means no size cell for I2C device address. > > + > > +Optional properties: > > +- Child nodes conforming to I2C bus binding > > + > > +Examples: > > +i2c0: i2c@70500000 { > > + compatible = "sprd,r8p0-i2c"; > > + reg = <0 0x70500000 0 0x1000>; > > + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>; > > + clock-names = "i2c", "source", "enable"; > > + clocks = <&clk_i2c3>, <&ext_26m>, <&clk_ap_apb_gates 11>; > > + clock-frequency = <400000>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > +}; > > + > > -- > > 1.7.9.5 > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On 日, 6月 25, 2017 at 06:06:44下午 +0300, Andy Shevchenko wrote: > On Wed, Jun 21, 2017 at 10:23 AM, Baolin Wang > <baolin.wang@spreadtrum.com> wrote: > > This patch adds the I2C controller driver for Spreadtrum platform. > > Needs more work. > See my comments below. > > > > > +#include <linux/init.h> > > > +#include <linux/module.h> > > Since your answer to the comment about arch_initcall you perhaps need > to get rid of tristate (use bool) and drop module.h here and all > leftovers like MODULE_*() calls. Will remove them in next version. > > > +#include <linux/slab.h> > > Should we include this still explicitly (after kernel.h)? Will remove it. > > > + > > +#define I2C_CTL 0x0 > > +#define I2C_ADDR_CFG 0x4 > > +#define I2C_COUNT 0x8 > > +#define I2C_RX 0xC > > +#define I2C_TX 0x10 > > +#define I2C_STATUS 0x14 > > +#define I2C_HSMODE_CFG 0x18 > > +#define I2C_VERSION 0x1C > > +#define ADDR_DVD0 0x20 > > +#define ADDR_DVD1 0x24 > > +#define ADDR_STA0_DVD 0x28 > > +#define ADDR_RST 0x2C > > 1. Please, use fixed sized values > 0x00 and so on. > 2. Please, low case. OK. > > > +#define SPRD_I2C_TIMEOUT (msecs_to_jiffies(1000)) > > Redundant parens. OK. > > > +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev) > > If you switch your driver to regmap API, you may drop this function completely. Now we have no use to switch i2c driver to regmap API and we really need these registers info to debug I2C driver when we met something wrong. So I think I should still keep this function for debugging. > > > + writel(tmp & (~STP_EN), i2c_dev->base + I2C_CTL); > > Redundant parens. > Also pay attention to other similar places. OK, Will check the whole file. > > > > +static void sprd_i2c_write_bytes(struct sprd_i2c *i2c_dev, u8 *buf, u32 len) > > So, isn't better to provide a writesb(), readsb() to ia64 instead of > doing below? When I change to writesb/readsb, it will compile failed on my x86 platform. So I guess I should still keep writeb/readb now. > > > +{ > > + u32 i = 0; > > + > > + for (i = 0; i < len; i++) { > > + writel(buf[i], i2c_dev->base + I2C_TX); > > > + dev_dbg(&i2c_dev->adap.dev, "write bytes[%d] = %x\n", i, buf[i]); > > Moreover, don't waste lines in the kernel log buffer by doing this. > Choose either > > %*ph (up to 64 bytes) > > or > > print_hex_dump() > > (Same for _read_bytes() below) OK. > > > + } > > +} > > > +static void sprd_i2c_set_full_thld(struct sprd_i2c *i2c_dev, u32 full_thld) > > +{ > > + unsigned int tmp = readl(i2c_dev->base + I2C_CTL); > > + > > + tmp &= ~(0xf << FIFO_AF_LVL); > > Magic. > Define it using GENMASK() OK. Will use GENMASK instead of magic number. > > > + tmp |= (full_thld << FIFO_AF_LVL); > > Redundant parens. OK. > > > + tmp &= ~(0xf << FIFO_AE_LVL); > > + tmp |= (empty_thld << FIFO_AE_LVL); > > Same. OK. > > > +static void sprd_i2c_opt_mode(struct sprd_i2c *i2c_dev, int rw) > > +{ > > + unsigned int cmd = readl(i2c_dev->base + I2C_CTL) & (~I2C_MODE); > > Redundant parens. OK. > > > + writel((cmd | rw << 3), i2c_dev->base + I2C_CTL); > > Ditto. > > It's a C, and not a LISP :-) OK. > > > +} > > > +static void sprd_i2c_data_transfer(struct sprd_i2c *i2c_dev) > > +{ > > > + if ((msg->flags & I2C_M_RD)) { > > Redundant parens. Will remove it. > > > + /* Reset rx/tx fifos */ > > Noise. > > > + /* Set device address */ > > Ditto. > > > + /* Set I2C read or write bytes count */ > > Ditto. > > > + if ((msg->flags & I2C_M_RD)) { > > + /* Set read operation */ > > + sprd_i2c_opt_mode(i2c_dev, 1); > > + /* Last byte transmission should excute stop operation */ > > + sprd_i2c_send_stop(i2c_dev, 1); > > Same comments as above. Will remove theese reduandant comments. > > > + } else { > > > + /* Set write operation */ > > Noise. > > > + /* Last byte transmission should excute stop operation */ > > + if (is_last_msg) > > + sprd_i2c_send_stop(i2c_dev, 1); > > + else > > + sprd_i2c_send_stop(i2c_dev, 0); > > sprd_i2c_send_stop(i2c_dev, !!is_last_msg); > > Though, consider to make is_last_msg boolean. OK. > > > +static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap, > > + struct i2c_msg *msgs, int num) > > +{ > > + struct sprd_i2c *i2c_dev = i2c_adap->algo_data; > > + int im, ret; > > + > > + ret = pm_runtime_get_sync(i2c_dev->dev); > > + if (ret < 0) > > + return ret; > > + > > > + for (im = 0; im != num; im++) > > im < num - 1, and... > > ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im], 0); > ... ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im++], 1); > > ...and we clearly see that you have messed up with returned code on > each itteration here Yes, will re-check and modify this loop. > > > + pm_runtime_mark_last_busy(i2c_dev->dev); > > + pm_runtime_put_autosuspend(i2c_dev->dev); > > > > + return (ret >= 0) ? im : ret; > > Usual pattern is > ret < 0 ? ret : im; OK. > > > > +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, unsigned int freq) > > +{ > > + u32 apb_clk = i2c_dev->src_clk; > > + u32 i2c_dvd = apb_clk / (4 * freq) - 1; > > + u32 high = ((i2c_dvd << 1) * 2) / 5; > > + u32 low = ((i2c_dvd << 1) * 3) / 5; > > > + u32 div0 = (high & 0xffff) << 16 | (low & 0xffff); > > + u32 div1 = (high & 0xffff0000) | ((low & 0xffff0000) >> 16); > > Magic masks all over. > > Also it needs a comment what formula is used and how. Will add comments to explain the formula. > > > + > > + writel(div0, i2c_dev->base + ADDR_DVD0); > > + writel(div1, i2c_dev->base + ADDR_DVD1); > > + > > > + if (freq == 400000) > > + writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD); > > + else if (freq == 100000) > > + writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD); > > What about 3400000? > > What about the rest of the speeds, shouldn't you return an error from here? Now we only support 100000 and 400000, otherwise will be error. I will check this in probe() function. > > > +} > > + > > +static void sprd_i2c_enable(struct sprd_i2c *i2c_dev) > > +{ > > + unsigned int tmp = readl(i2c_dev->base + I2C_CTL); > > + > > + tmp &= ~(I2C_EN | I2C_TRIM_OPT | (0xF << FIFO_AF_LVL) | > > + (0xF << FIFO_AE_LVL)); > > Magic masks (I saw them already above). OK. > > > > + /* Set rx fifo full data threshold */ > > Drop noise comments. They don't bring any value since you have nice > function names. OK. > > > + sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD); > > + /* Set tx fifo empty data threshold */ > > + sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD); > > + > > + sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq); > > + /* Reset rx/tx fifo */ > > + sprd_i2c_reset_fifo(i2c_dev); > > + sprd_i2c_clear_irq(i2c_dev); > > > +static int sprd_i2c_clk_init(struct sprd_i2c *i2c_dev) > > +{ > > + struct clk *clk_i2c, *clk_parent; > > + struct device_node *np = i2c_dev->adap.dev.of_node; > > + > > > + clk_i2c = of_clk_get_by_name(np, "i2c"); > > What the issue to use resource agnostic API here, i.e. devm_clk_get() ? You are right, and will replace with devm_clk_get(). > > > + clk_parent = of_clk_get_by_name(np, "source"); > > Ditto. > > > + i2c_dev->clk = of_clk_get_by_name(np, "enable"); > > Ditto. > > > + if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop)) > > + i2c_dev->bus_freq = prop; > > + > > + sprd_i2c_clk_init(i2c_dev); > > + platform_set_drvdata(pdev, i2c_dev); > > + > > + ret = clk_prepare_enable(i2c_dev->clk); > > + if (ret) > > + return ret; > > + > > + sprd_i2c_enable(i2c_dev); > > + > > > +error: > > I would put it as > > err_rpm_put: OK. Very appreciate for your helpful comments. Thanks a lot. > > > + pm_runtime_put_noidle(i2c_dev->dev); > > + pm_runtime_disable(i2c_dev->dev); > > + clk_disable_unprepare(i2c_dev->clk); > > + return ret; > > +} > > -- > With Best Regards, > Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/i2c/i2c-sprd.txt b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt new file mode 100644 index 0000000..f285e5b --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt @@ -0,0 +1,31 @@ +I2C for Spreadtrum platforms + +Required properties: +- compatible: Should be "sprd,r8p0-i2c". +- reg: Specify the physical base address of the controller and length + of memory mapped region. +- interrupts: Should contain I2C interrupt. +- clock-names: Should contain following entries: + "i2c" for I2C clock, + "source" for I2C source (parent) clock, + "enable" for I2C module enable clock. +- clocks: Should contain a clock specifier for each entry in clock-names. +- clock-frequency: Constains desired I2C bus clock frequency in Hz. +- #address-cells: Should be 1 to describe address cells for I2C device address. +- #size-cells: Should be 0 means no size cell for I2C device address. + +Optional properties: +- Child nodes conforming to I2C bus binding + +Examples: +i2c0: i2c@70500000 { + compatible = "sprd,r8p0-i2c"; + reg = <0 0x70500000 0 0x1000>; + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "i2c", "source", "enable"; + clocks = <&clk_i2c3>, <&ext_26m>, <&clk_ap_apb_gates 11>; + clock-frequency = <400000>; + #address-cells = <1>; + #size-cells = <0>; +}; +
This patch adds the binding documentation for Spreadtrum I2C controller device. Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> --- Changes since v1: - No updates. --- Documentation/devicetree/bindings/i2c/i2c-sprd.txt | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sprd.txt