| Message ID | 1506953613-23875-3-git-send-email-todor.tomov@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote: > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c [..] > +#define NUM_MASTERS 1 So you will only register one of the masters? [..] > +enum cci_i2c_queue_t { > + QUEUE_0, > + QUEUE_1 Could these be called QUEUE_READ and QUEUE_WRITE instead? > +}; > + > +enum cci_i2c_master_t { > + MASTER_0, > + MASTER_1 Just use 0 and 1 when denoting the two masters. > +}; > + > +struct resources { This struct name is not awesome, name it something like cci_res instead. > + char *clock[CCI_RES_MAX]; > + u32 clock_rate[CCI_RES_MAX]; > + char *reg[CCI_RES_MAX]; > + char *interrupt[CCI_RES_MAX]; > +}; [..] > + > +struct cci_master { > + u32 status; You use this to pass 0 or negative errno between functions, so make it int. > + u8 complete_pending; bool > + struct completion irq_complete; > +}; > + > +struct cci { > + struct device *dev; > + struct i2c_adapter adap; > + void __iomem *base; > + u32 irq; "irq" is generally supposed to be a "int". > + char irq_name[30]; > + struct cci_clock *clock; If you set the rate of your clocks initially you can replace this array with clk_bulk_data and use the clk_bulk_prepare_enable() and clk_bulk_disable_unprepare(). > + int nclocks; > + u8 mode; > + u16 queue_size[NUM_QUEUES]; > + struct cci_master master[NUM_MASTERS]; > +}; > + > +static const struct resources res_v1_0_8 = { > + .clock = { "camss_top_ahb", > + "cci_ahb", > + "camss_ahb", > + "cci" }, The code consuming this will read until NULL, so please add terminator. It happens to work as the first clock_rate is 0 in both cases. > + .clock_rate = { 0, > + 80000000, > + 0, > + 19200000 }, > + .reg = { "cci" }, > + .interrupt = { "cci" } No need to specify reg and interrupt here until they actually need to be configured; just put the strings in the code. > +}; > + [..] > + > +/* > + * cci_enable_clocks - Enable multiple clocks Correct kerneldoc would be: /** * cci_enable_clocks() - Enable multiple clocks > + * @nclocks: Number of clocks in clock array > + * @clock: Clock array > + * @dev: Device > + * > + * Return 0 on success or a negative error code otherwise > + */ > +int cci_enable_clocks(int nclocks, struct cci_clock *clock, struct device *dev) Overall this is clk_bulk_prepare_enable(), please use that. [..] > +/* > + * cci_disable_clocks - Disable multiple clocks > + * @nclocks: Number of clocks in clock array > + * @clock: Clock array > + */ > +void cci_disable_clocks(int nclocks, struct cci_clock *clock) Overall this is clk_bulk_disable_unprepare(), so please use that. > +{ > + int i; > + > + for (i = nclocks - 1; i >= 0; i--) > + clk_disable_unprepare(clock[i].clk); > +} > + [..] > + > +static int cci_init(struct cci *cci, const struct hw_params *hw) > +{ > + u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE | > + CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT | > + CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT | > + CCI_IRQ_MASK_0_I2C_M1_RD_DONE | > + CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT | > + CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT | > + CCI_IRQ_MASK_0_RST_DONE_ACK | > + CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK | > + CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK | > + CCI_IRQ_MASK_0_I2C_M0_ERROR | > + CCI_IRQ_MASK_0_I2C_M1_ERROR; > + int i; > + > + writel(val, cci->base + CCI_IRQ_MASK_0); > + > + for (i = 0; i < NUM_MASTERS; i++) { > + val = hw->thigh << 16 | hw->tlow; > + writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i)); > + > + val = hw->tsu_sto << 16 | hw->tsu_sta; > + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_0(i)); > + > + val = hw->thd_dat << 16 | hw->thd_sta; > + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_1(i)); > + > + val = hw->tbuf; > + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i)); > + > + val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp; > + writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i)); > + > + cci->master[i].status = 0; I don't think you need to initialize status here, it's always written before read in the rest of the driver. > + } > + > + return 0; > +} > + > +static int cci_run_queue(struct cci *cci, u8 master, u8 queue) > +{ > + unsigned long time; > + u32 val; > + int ret; > + > + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue)); > + writel(val, cci->base + CCI_I2C_Mm_Qn_EXEC_WORD_CNT(master, queue)); > + > + val = 1 << ((master * 2) + queue); BIT(master * 2 + queue) > + writel(val, cci->base + CCI_QUEUE_START); > + > + time = wait_for_completion_timeout(&cci->master[master].irq_complete, > + CCI_TIMEOUT_MS); > + if (!time) { > + dev_err(cci->dev, "master %d queue %d timeout\n", > + master, queue); > + return -ETIMEDOUT; > + } > + > + ret = cci->master[master].status; > + if (ret < 0) > + dev_err(cci->dev, "master %d queue %d error %d\n", > + master, queue, ret); > + > + return ret; > +} > + > +static int cci_validate_queue(struct cci *cci, u32 len, u8 master, u8 queue) > +{ > + int ret = 0; > + u32 val; > + > + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue)); > + > + if (val + len + 1 > cci->queue_size[queue]) { if (val + len >= cci->queue_size[queue]) Or even better, flip this around and do: if (val + len < cci->queue_size[queue]) return 0; val = .. writel() return cci_run_queue(); That said, this function is always called with len == cci->queue_size[queue]. So this will always "run the queue" unless val == 0. So you can simplify this function slightly by dropping the "len". > + val = CCI_I2C_REPORT | (1 << 8); > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); > + > + ret = cci_run_queue(cci, master, queue); > + } > + > + return ret; > +} > + > +static int cci_i2c_read(struct cci *cci, u16 addr, u8 *buf, u16 len) > +{ > + u8 master = MASTER_0; > + u8 queue = QUEUE_1; > + u32 val; > + u32 words_read, words_exp; > + int i, index, first; > + int ret; > + > + if (len > cci->adap.quirks->max_read_len) > + return -EOPNOTSUPP; > + The core already checks each entry against the max length quirks. But are these actually the max amount of data you can read in one operation? Generally one has to drain the fifo but the actual transfers can be larger... [..] > +} > + > +static int cci_i2c_write(struct cci *cci, u16 addr, u8 *buf, u16 len) > +{ > + u8 master = MASTER_0; > + u8 queue = QUEUE_0; > + u8 load[12] = { 0 }; > + u8 i, j; Use int for counters. > + u32 val; > + int ret; > + > + if (len > cci->adap.quirks->max_write_len) > + return -EOPNOTSUPP; > + This is taken care of by the core. [..] > +} > + > +static int cci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > +{ > + struct cci *cci = i2c_get_adapdata(adap); > + int i; > + int ret = 0; > + > + if (!num) > + return -EOPNOTSUPP; I don't think you need to handle this case explicitly. > + > + for (i = 0; i < num; i++) { > + if (msgs[i].flags & I2C_M_RD) > + ret = cci_i2c_read(cci, msgs[i].addr, msgs[i].buf, > + msgs[i].len); > + else > + ret = cci_i2c_write(cci, msgs[i].addr, msgs[i].buf, > + msgs[i].len); > + > + if (ret < 0) { > + dev_err(cci->dev, "cci i2c xfer error %d", ret); > + break; > + } > + } > + > + return ret; This should return num on success. [..] > +static int cci_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; [..] > + cci = devm_kzalloc(&pdev->dev, sizeof(*cci), GFP_KERNEL); Use dev instead &pdev->dev here, of just use pdev->dev throughout the function. [..] > + cci->mode = I2C_MODE_STANDARD; cci->mode is a local variable, please don't put it in the struct. > + ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &val); > + if (!ret) { > + if (val == 400000) > + cci->mode = I2C_MODE_FAST; > + else if (val == 1000000) > + cci->mode = I2C_MODE_FAST_PLUS; > + } > + > + /* Memory */ > + > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]); You only have a single reg, so please just use r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + cci->base = devm_ioremap_resource(dev, r); > + if (IS_ERR(cci->base)) { > + dev_err(dev, "could not map memory\n"); > + return PTR_ERR(cci->base); > + } > + > + /* Interrupt */ > + > + r = platform_get_resource_byname(pdev, IORESOURCE_IRQ, > + res->interrupt[0]); > + if (!r) { > + dev_err(dev, "missing IRQ\n"); > + return -EINVAL; > + } > + > + cci->irq = r->start; Please replace this with: cci->irq = platform_get_irq(pdev, 0); > + snprintf(cci->irq_name, sizeof(cci->irq_name), "%s", dev_name(dev)); And just hard code this in the function call. > + ret = devm_request_irq(dev, cci->irq, cci_isr, > + IRQF_TRIGGER_RISING, cci->irq_name, cci); > + if (ret < 0) { > + dev_err(dev, "request_irq failed, ret: %d\n", ret); > + return ret; > + } > + > + disable_irq(cci->irq); The time between devm_request_irq() and disable_irq() is still long enough for cci_isr() to be called, before irq_complete is initialized. Don't request irqs until you're ready to receive the interrupts. > + > + /* Clocks */ > + > + cci->nclocks = 0; > + while (res->clock[cci->nclocks]) > + cci->nclocks++; > + > + cci->clock = devm_kzalloc(dev, cci->nclocks * > + sizeof(*cci->clock), GFP_KERNEL); This can max be CCI_RES_MAX (i.e. 6) so just make the array fixed size and skip the kzalloc(). > + if (!cci->clock) > + return -ENOMEM; > + > + for (i = 0; i < cci->nclocks; i++) { > + struct cci_clock *clock = &cci->clock[i]; > + > + clock->clk = devm_clk_get(dev, res->clock[i]); > + if (IS_ERR(clock->clk)) > + return PTR_ERR(clock->clk); > + > + clock->name = res->clock[i]; > + clock->freq = res->clock_rate[i]; > + } > + > + ret = cci_enable_clocks(cci->nclocks, cci->clock, dev); > + if (ret < 0) > + return ret; > + > + val = readl_relaxed(cci->base + CCI_HW_VERSION); > + dev_info(dev, "%s: CCI HW version = 0x%08x", __func__, val); Please don't "spam" the kernel log by default, if this is useful make it dev_dbg() and people can easily enable the print. > + > + init_completion(&cci->master[0].irq_complete); > + init_completion(&cci->master[1].irq_complete); > + > + enable_irq(cci->irq); > + > + ret = cci_reset(cci); > + if (ret < 0) Clocks needs to be disabled if this fails. > + return ret; > + > + ret = cci_init(cci, &hw[cci->mode]); > + if (ret < 0) Dito > + return ret; > + > + ret = i2c_add_adapter(&cci->adap); Dito > + > + return ret; > +} [..] > +MODULE_ALIAS("platform:i2c-qcom-cci"); You don't need to specify this alias. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Thank you for the review. There are a lot of important suggestions. On 6.10.2017 08:20, Bjorn Andersson wrote: > On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote: >> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c > [..] >> +#define NUM_MASTERS 1 > > So you will only register one of the masters? Yes, in this version of the driver - only one. Probably I will extend the driver later to support also the second master on 8096. > > [..] >> +enum cci_i2c_queue_t { >> + QUEUE_0, >> + QUEUE_1 > > Could these be called QUEUE_READ and QUEUE_WRITE instead? There isn't anything read or write specific in the queues, this is just how we assign them. Both of them can do both operations. So I think that we should not narrow the names to read and write. > >> +}; >> + >> +enum cci_i2c_master_t { >> + MASTER_0, >> + MASTER_1 > > Just use 0 and 1 when denoting the two masters. Ok. > >> +}; >> + >> +struct resources { > > This struct name is not awesome, name it something like cci_res instead. Ok. > >> + char *clock[CCI_RES_MAX]; >> + u32 clock_rate[CCI_RES_MAX]; >> + char *reg[CCI_RES_MAX]; >> + char *interrupt[CCI_RES_MAX]; >> +}; > [..] >> + >> +struct cci_master { >> + u32 status; > > You use this to pass 0 or negative errno between functions, so make it > int. Ops. > >> + u8 complete_pending; > > bool Ok. > >> + struct completion irq_complete; >> +}; >> + >> +struct cci { >> + struct device *dev; >> + struct i2c_adapter adap; >> + void __iomem *base; >> + u32 irq; > > "irq" is generally supposed to be a "int". Ok. > >> + char irq_name[30]; >> + struct cci_clock *clock; > > If you set the rate of your clocks initially you can replace this array > with clk_bulk_data and use the clk_bulk_prepare_enable() and > clk_bulk_disable_unprepare(). Ok, I'll switch to the bulk API. > >> + int nclocks; >> + u8 mode; >> + u16 queue_size[NUM_QUEUES]; >> + struct cci_master master[NUM_MASTERS]; >> +}; >> + >> +static const struct resources res_v1_0_8 = { >> + .clock = { "camss_top_ahb", >> + "cci_ahb", >> + "camss_ahb", >> + "cci" }, > > The code consuming this will read until NULL, so please add terminator. The fields which are not explicitely initialized are set to NULL, right? I may add a comment that a space in the array for the terminator must be left. > It happens to work as the first clock_rate is 0 in both cases. I think that it works because it reaches the terminator. > >> + .clock_rate = { 0, >> + 80000000, >> + 0, >> + 19200000 }, >> + .reg = { "cci" }, >> + .interrupt = { "cci" } > > No need to specify reg and interrupt here until they actually need to be > configured; just put the strings in the code. > Ok. >> +}; >> + > [..] >> + >> +/* >> + * cci_enable_clocks - Enable multiple clocks > > Correct kerneldoc would be: > > /** > * cci_enable_clocks() - Enable multiple clocks Ok. > >> + * @nclocks: Number of clocks in clock array >> + * @clock: Clock array >> + * @dev: Device >> + * >> + * Return 0 on success or a negative error code otherwise >> + */ >> +int cci_enable_clocks(int nclocks, struct cci_clock *clock, struct device *dev) > > Overall this is clk_bulk_prepare_enable(), please use that. Yes, I will. > > [..] >> +/* >> + * cci_disable_clocks - Disable multiple clocks >> + * @nclocks: Number of clocks in clock array >> + * @clock: Clock array >> + */ >> +void cci_disable_clocks(int nclocks, struct cci_clock *clock) > > Overall this is clk_bulk_disable_unprepare(), so please use that. Yes. > >> +{ >> + int i; >> + >> + for (i = nclocks - 1; i >= 0; i--) >> + clk_disable_unprepare(clock[i].clk); >> +} >> + > [..] >> + >> +static int cci_init(struct cci *cci, const struct hw_params *hw) >> +{ >> + u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE | >> + CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT | >> + CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT | >> + CCI_IRQ_MASK_0_I2C_M1_RD_DONE | >> + CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT | >> + CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT | >> + CCI_IRQ_MASK_0_RST_DONE_ACK | >> + CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK | >> + CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK | >> + CCI_IRQ_MASK_0_I2C_M0_ERROR | >> + CCI_IRQ_MASK_0_I2C_M1_ERROR; >> + int i; >> + >> + writel(val, cci->base + CCI_IRQ_MASK_0); >> + >> + for (i = 0; i < NUM_MASTERS; i++) { >> + val = hw->thigh << 16 | hw->tlow; >> + writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i)); >> + >> + val = hw->tsu_sto << 16 | hw->tsu_sta; >> + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_0(i)); >> + >> + val = hw->thd_dat << 16 | hw->thd_sta; >> + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_1(i)); >> + >> + val = hw->tbuf; >> + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i)); >> + >> + val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp; >> + writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i)); >> + >> + cci->master[i].status = 0; > > I don't think you need to initialize status here, it's always written > before read in the rest of the driver. > Ok. >> + } >> + >> + return 0; >> +} >> + >> +static int cci_run_queue(struct cci *cci, u8 master, u8 queue) >> +{ >> + unsigned long time; >> + u32 val; >> + int ret; >> + >> + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue)); >> + writel(val, cci->base + CCI_I2C_Mm_Qn_EXEC_WORD_CNT(master, queue)); >> + >> + val = 1 << ((master * 2) + queue); > > BIT(master * 2 + queue) Ok. Will use BIT() also on the rest of the places where it can be used. > >> + writel(val, cci->base + CCI_QUEUE_START); >> + >> + time = wait_for_completion_timeout(&cci->master[master].irq_complete, >> + CCI_TIMEOUT_MS); >> + if (!time) { >> + dev_err(cci->dev, "master %d queue %d timeout\n", >> + master, queue); >> + return -ETIMEDOUT; >> + } >> + >> + ret = cci->master[master].status; >> + if (ret < 0) >> + dev_err(cci->dev, "master %d queue %d error %d\n", >> + master, queue, ret); >> + >> + return ret; >> +} >> + >> +static int cci_validate_queue(struct cci *cci, u32 len, u8 master, u8 queue) >> +{ >> + int ret = 0; >> + u32 val; >> + >> + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue)); >> + >> + if (val + len + 1 > cci->queue_size[queue]) { > > if (val + len >= cci->queue_size[queue]) > > > Or even better, flip this around and do: > > if (val + len < cci->queue_size[queue]) > return 0; > > val = .. > writel() > > return cci_run_queue(); > > > That said, this function is always called with len == > cci->queue_size[queue]. So this will always "run the queue" unless val > == 0. So you can simplify this function slightly by dropping the "len". Yes, I'll rework this and remove "len". > >> + val = CCI_I2C_REPORT | (1 << 8); >> + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); >> + >> + ret = cci_run_queue(cci, master, queue); >> + } >> + >> + return ret; >> +} >> + >> +static int cci_i2c_read(struct cci *cci, u16 addr, u8 *buf, u16 len) >> +{ >> + u8 master = MASTER_0; >> + u8 queue = QUEUE_1; >> + u32 val; >> + u32 words_read, words_exp; >> + int i, index, first; >> + int ret; >> + >> + if (len > cci->adap.quirks->max_read_len) >> + return -EOPNOTSUPP; >> + > > The core already checks each entry against the max length quirks. > > But are these actually the max amount of data you can read in one > operation? Generally one has to drain the fifo but the actual transfers > can be larger... Yes, the maximum data which you can read in one operation. And this is the meaning of quirks->max_read_len, right? Not the i2c transfer size. So the check is correct but I'll remove it as it is already done in i2c_check_for_quirks(), as you have pointed out. > > [..] >> +} >> + >> +static int cci_i2c_write(struct cci *cci, u16 addr, u8 *buf, u16 len) >> +{ >> + u8 master = MASTER_0; >> + u8 queue = QUEUE_0; >> + u8 load[12] = { 0 }; >> + u8 i, j; > > Use int for counters. Ok. > >> + u32 val; >> + int ret; >> + >> + if (len > cci->adap.quirks->max_write_len) >> + return -EOPNOTSUPP; >> + > > This is taken care of by the core. Yes, I'll remove it. > > [..] >> +} >> + >> +static int cci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >> +{ >> + struct cci *cci = i2c_get_adapdata(adap); >> + int i; >> + int ret = 0; >> + >> + if (!num) >> + return -EOPNOTSUPP; > > I don't think you need to handle this case explicitly. Ok. > >> + >> + for (i = 0; i < num; i++) { >> + if (msgs[i].flags & I2C_M_RD) >> + ret = cci_i2c_read(cci, msgs[i].addr, msgs[i].buf, >> + msgs[i].len); >> + else >> + ret = cci_i2c_write(cci, msgs[i].addr, msgs[i].buf, >> + msgs[i].len); >> + >> + if (ret < 0) { >> + dev_err(cci->dev, "cci i2c xfer error %d", ret); >> + break; >> + } >> + } >> + >> + return ret; > > This should return num on success. Yes. > > [..] >> +static int cci_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; > [..] >> + cci = devm_kzalloc(&pdev->dev, sizeof(*cci), GFP_KERNEL); > > Use dev instead &pdev->dev here, of just use pdev->dev throughout the > function. Ok. > > [..] >> + cci->mode = I2C_MODE_STANDARD; > > cci->mode is a local variable, please don't put it in the struct. Ok. > >> + ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &val); >> + if (!ret) { >> + if (val == 400000) >> + cci->mode = I2C_MODE_FAST; >> + else if (val == 1000000) >> + cci->mode = I2C_MODE_FAST_PLUS; >> + } >> + >> + /* Memory */ >> + >> + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]); > > You only have a single reg, so please just use > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > Ok. > >> + cci->base = devm_ioremap_resource(dev, r); >> + if (IS_ERR(cci->base)) { >> + dev_err(dev, "could not map memory\n"); >> + return PTR_ERR(cci->base); >> + } >> + >> + /* Interrupt */ >> + >> + r = platform_get_resource_byname(pdev, IORESOURCE_IRQ, >> + res->interrupt[0]); >> + if (!r) { >> + dev_err(dev, "missing IRQ\n"); >> + return -EINVAL; >> + } >> + >> + cci->irq = r->start; > > Please replace this with: > > cci->irq = platform_get_irq(pdev, 0); Ok. > >> + snprintf(cci->irq_name, sizeof(cci->irq_name), "%s", dev_name(dev)); > > And just hard code this in the function call. Ok. > >> + ret = devm_request_irq(dev, cci->irq, cci_isr, >> + IRQF_TRIGGER_RISING, cci->irq_name, cci); >> + if (ret < 0) { >> + dev_err(dev, "request_irq failed, ret: %d\n", ret); >> + return ret; >> + } >> + >> + disable_irq(cci->irq); > > The time between devm_request_irq() and disable_irq() is still long > enough for cci_isr() to be called, before irq_complete is initialized. > > Don't request irqs until you're ready to receive the interrupts. This makes sense however at this point the clocks to the device are still not enabled, doesn't this avoid any problems? > >> + >> + /* Clocks */ >> + >> + cci->nclocks = 0; >> + while (res->clock[cci->nclocks]) >> + cci->nclocks++; >> + >> + cci->clock = devm_kzalloc(dev, cci->nclocks * >> + sizeof(*cci->clock), GFP_KERNEL); > > This can max be CCI_RES_MAX (i.e. 6) so just make the array fixed size > and skip the kzalloc(). Does it matter? > >> + if (!cci->clock) >> + return -ENOMEM; >> + >> + for (i = 0; i < cci->nclocks; i++) { >> + struct cci_clock *clock = &cci->clock[i]; >> + >> + clock->clk = devm_clk_get(dev, res->clock[i]); >> + if (IS_ERR(clock->clk)) >> + return PTR_ERR(clock->clk); >> + >> + clock->name = res->clock[i]; >> + clock->freq = res->clock_rate[i]; >> + } >> + >> + ret = cci_enable_clocks(cci->nclocks, cci->clock, dev); >> + if (ret < 0) >> + return ret; >> + >> + val = readl_relaxed(cci->base + CCI_HW_VERSION); >> + dev_info(dev, "%s: CCI HW version = 0x%08x", __func__, val); > > Please don't "spam" the kernel log by default, if this is useful make it > dev_dbg() and people can easily enable the print. Ok. > >> + >> + init_completion(&cci->master[0].irq_complete); >> + init_completion(&cci->master[1].irq_complete); >> + >> + enable_irq(cci->irq); >> + >> + ret = cci_reset(cci); >> + if (ret < 0) > > Clocks needs to be disabled if this fails. Ok. > >> + return ret; >> + >> + ret = cci_init(cci, &hw[cci->mode]); >> + if (ret < 0) > > Dito Ok. > >> + return ret; >> + >> + ret = i2c_add_adapter(&cci->adap); > > Dito Ok. > >> + >> + return ret; >> +} > [..] >> +MODULE_ALIAS("platform:i2c-qcom-cci"); > > You don't need to specify this alias. Ok. > > Regards, > Bjorn >
On Thu 12 Oct 07:47 PDT 2017, Todor Tomov wrote: > Hi Bjorn, > > Thank you for the review. There are a lot of important suggestions. > > On 6.10.2017 08:20, Bjorn Andersson wrote: > > On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote: > >> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c > > [..] > >> +#define NUM_MASTERS 1 > > > > So you will only register one of the masters? > > Yes, in this version of the driver - only one. Probably I will extend the driver later > to support also the second master on 8096. > That's perfectly fine, as long as there's a plan of how to add the second master that is backwards compatible (DT wise). [..] > >> +static const struct resources res_v1_0_8 = { > >> + .clock = { "camss_top_ahb", > >> + "cci_ahb", > >> + "camss_ahb", > >> + "cci" }, > > > > The code consuming this will read until NULL, so please add terminator. > > The fields which are not explicitely initialized are set to NULL, right? > I may add a comment that a space in the array for the terminator must be left. > Right, I didn't think of this list being a statically sized array, then there are implicit NULLs here. > > It happens to work as the first clock_rate is 0 in both cases. > > I think that it works because it reaches the terminator. > Adding a NULL after the list will make this more obvious and continue to work as intended. [..] > >> + if (len > cci->adap.quirks->max_read_len) > >> + return -EOPNOTSUPP; > >> + > > > > The core already checks each entry against the max length quirks. > > > > But are these actually the max amount of data you can read in one > > operation? Generally one has to drain the fifo but the actual transfers > > can be larger... > > Yes, the maximum data which you can read in one operation. And this is > the meaning of quirks->max_read_len, right? Not the i2c transfer size. > So the check is correct but I'll remove it as it is already done in > i2c_check_for_quirks(), as you have pointed out. > AFAICT you're doing it correct. I just find it surprising that these limits are so low (in the hardware) - in the qup driver people complained that the max is 255. [..] > >> + ret = devm_request_irq(dev, cci->irq, cci_isr, > >> + IRQF_TRIGGER_RISING, cci->irq_name, cci); > >> + if (ret < 0) { > >> + dev_err(dev, "request_irq failed, ret: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + disable_irq(cci->irq); > > > > The time between devm_request_irq() and disable_irq() is still long > > enough for cci_isr() to be called, before irq_complete is initialized. > > > > Don't request irqs until you're ready to receive the interrupts. > > This makes sense however at this point the clocks to the device are > still not enabled, doesn't this avoid any problems? > You're probably right, but unless it's too much hassle it's better to be on the safe side - if nothing else for the times when someone copy-paste your code for a new driver that doesn't follow this premise. > > > >> + > >> + /* Clocks */ > >> + > >> + cci->nclocks = 0; > >> + while (res->clock[cci->nclocks]) > >> + cci->nclocks++; > >> + > >> + cci->clock = devm_kzalloc(dev, cci->nclocks * > >> + sizeof(*cci->clock), GFP_KERNEL); > > > > This can max be CCI_RES_MAX (i.e. 6) so just make the array fixed size > > and skip the kzalloc(). > > Does it matter? > At the cost of making the list statically sized in the struct you can drop this call and error check. But other than that, no. > > > >> + if (!cci->clock) > >> + return -ENOMEM; > >> + Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index c06dce2..5841094 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -832,6 +832,16 @@ config I2C_PXA_SLAVE is necessary for systems where the PXA may be a target on the I2C bus. +config I2C_QCOM_CCI + tristate "Qualcomm Camera Control Interface" + depends on ARCH_QCOM + help + If you say yes to this option, support will be included for the + built-in camera control interface on the Qualcomm SoCs. + + This driver can also be built as a module. If so, the module + will be called i2c-qcom-cci. + config I2C_QUP tristate "Qualcomm QUP based I2C controller" depends on ARCH_QCOM diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 47f3ac9..c55e57c 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -83,6 +83,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o obj-$(CONFIG_I2C_PXA) += i2c-pxa.o obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o +obj-$(CONFIG_I2C_QCOM_CCI) += i2c-qcom-cci.o obj-$(CONFIG_I2C_QUP) += i2c-qup.o obj-$(CONFIG_I2C_RIIC) += i2c-riic.o obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c new file mode 100644 index 0000000..080f0ca --- /dev/null +++ b/drivers/i2c/busses/i2c-qcom-cci.c @@ -0,0 +1,793 @@ +/* Copyright (c) 2012-2016, The Linux Foundation. All rights reserved. + * Copyright (C) 2017 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/i2c.h> +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of.h> + +#define CCI_HW_VERSION 0x0 +#define CCI_RESET_CMD 0x004 +#define CCI_RESET_CMD_MASK 0x0f73f3f7 +#define CCI_RESET_CMD_M0_MASK 0x000003f1 +#define CCI_RESET_CMD_M1_MASK 0x0003f001 +#define CCI_QUEUE_START 0x008 +#define CCI_HALT_REQ 0x034 +#define CCI_HALT_REQ_I2C_M0_Q0Q1 (1 << 0) +#define CCI_HALT_REQ_I2C_M1_Q0Q1 (1 << 1) + +#define CCI_I2C_Mm_SCL_CTL(m) (0x100 + 0x100 * (m)) +#define CCI_I2C_Mm_SDA_CTL_0(m) (0x104 + 0x100 * (m)) +#define CCI_I2C_Mm_SDA_CTL_1(m) (0x108 + 0x100 * (m)) +#define CCI_I2C_Mm_SDA_CTL_2(m) (0x10c + 0x100 * (m)) +#define CCI_I2C_Mm_MISC_CTL(m) (0x110 + 0x100 * (m)) + +#define CCI_I2C_Mm_READ_DATA(m) (0x118 + 0x100 * (m)) +#define CCI_I2C_Mm_READ_BUF_LEVEL(m) (0x11c + 0x100 * (m)) +#define CCI_I2C_Mm_Qn_EXEC_WORD_CNT(m, n) (0x300 + 0x200 * (m) + 0x100 * (n)) +#define CCI_I2C_Mm_Qn_CUR_WORD_CNT(m, n) (0x304 + 0x200 * (m) + 0x100 * (n)) +#define CCI_I2C_Mm_Qn_CUR_CMD(m, n) (0x308 + 0x200 * (m) + 0x100 * (n)) +#define CCI_I2C_Mm_Qn_REPORT_STATUS(m, n) (0x30c + 0x200 * (m) + 0x100 * (n)) +#define CCI_I2C_Mm_Qn_LOAD_DATA(m, n) (0x310 + 0x200 * (m) + 0x100 * (n)) + +#define CCI_IRQ_GLOBAL_CLEAR_CMD 0xc00 +#define CCI_IRQ_MASK_0 0xc04 +#define CCI_IRQ_MASK_0_I2C_M0_RD_DONE (1 << 0) +#define CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT (1 << 4) +#define CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT (1 << 8) +#define CCI_IRQ_MASK_0_I2C_M1_RD_DONE (1 << 12) +#define CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT (1 << 16) +#define CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT (1 << 20) +#define CCI_IRQ_MASK_0_RST_DONE_ACK (1 << 24) +#define CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK (1 << 25) +#define CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK (1 << 26) +#define CCI_IRQ_MASK_0_I2C_M0_ERROR 0x18000ee6 +#define CCI_IRQ_MASK_0_I2C_M1_ERROR 0x60ee6000 +#define CCI_IRQ_CLEAR_0 0xc08 +#define CCI_IRQ_STATUS_0 0xc0c +#define CCI_IRQ_STATUS_0_I2C_M0_RD_DONE (1 << 0) +#define CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT (1 << 4) +#define CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT (1 << 8) +#define CCI_IRQ_STATUS_0_I2C_M1_RD_DONE (1 << 12) +#define CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT (1 << 16) +#define CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT (1 << 20) +#define CCI_IRQ_STATUS_0_RST_DONE_ACK (1 << 24) +#define CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK (1 << 25) +#define CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK (1 << 26) +#define CCI_IRQ_STATUS_0_I2C_M0_ERROR 0x18000ee6 +#define CCI_IRQ_STATUS_0_I2C_M1_ERROR 0x60ee6000 + +#define CCI_TIMEOUT_MS 100 +#define NUM_MASTERS 1 +#define NUM_QUEUES 2 + +#define CCI_RES_MAX 6 + +enum cci_i2c_cmd { + CCI_I2C_SET_PARAM = 1, + CCI_I2C_WAIT, + CCI_I2C_WAIT_SYNC, + CCI_I2C_WAIT_GPIO_EVENT, + CCI_I2C_TRIG_I2C_EVENT, + CCI_I2C_LOCK, + CCI_I2C_UNLOCK, + CCI_I2C_REPORT, + CCI_I2C_WRITE, + CCI_I2C_READ, + CCI_I2C_WRITE_DISABLE_P, + CCI_I2C_READ_DISABLE_P, +}; + +enum { + I2C_MODE_STANDARD, + I2C_MODE_FAST, + I2C_MODE_FAST_PLUS, +}; + +enum cci_i2c_queue_t { + QUEUE_0, + QUEUE_1 +}; + +enum cci_i2c_master_t { + MASTER_0, + MASTER_1 +}; + +struct resources { + char *clock[CCI_RES_MAX]; + u32 clock_rate[CCI_RES_MAX]; + char *reg[CCI_RES_MAX]; + char *interrupt[CCI_RES_MAX]; +}; + +struct hw_params { + u16 thigh; + u16 tlow; + u16 tsu_sto; + u16 tsu_sta; + u16 thd_dat; + u16 thd_sta; + u16 tbuf; + u8 scl_stretch_en; + u16 trdhld; + u16 tsp; +}; + +struct cci_clock { + struct clk *clk; + const char *name; + u32 freq; +}; + +struct cci_master { + u32 status; + u8 complete_pending; + struct completion irq_complete; +}; + +struct cci { + struct device *dev; + struct i2c_adapter adap; + void __iomem *base; + u32 irq; + char irq_name[30]; + struct cci_clock *clock; + int nclocks; + u8 mode; + u16 queue_size[NUM_QUEUES]; + struct cci_master master[NUM_MASTERS]; +}; + +static const struct resources res_v1_0_8 = { + .clock = { "camss_top_ahb", + "cci_ahb", + "camss_ahb", + "cci" }, + .clock_rate = { 0, + 80000000, + 0, + 19200000 }, + .reg = { "cci" }, + .interrupt = { "cci" } +}; + +static const struct resources res_v1_4_0 = { + .clock = { "mmss_mmagic_ahb", + "camss_top_ahb", + "cci_ahb", + "camss_ahb", + "cci" }, + .clock_rate = { 0, + 0, + 0, + 0, + 37500000 }, + .reg = { "cci" }, + .interrupt = { "cci" } +}; + +static const struct hw_params hw_params_v1_0_8[3] = { + { /* I2C_MODE_STANDARD */ + .thigh = 78, + .tlow = 114, + .tsu_sto = 28, + .tsu_sta = 28, + .thd_dat = 10, + .thd_sta = 77, + .tbuf = 118, + .scl_stretch_en = 0, + .trdhld = 6, + .tsp = 1 + }, + { /* I2C_MODE_FAST */ + .thigh = 20, + .tlow = 28, + .tsu_sto = 21, + .tsu_sta = 21, + .thd_dat = 13, + .thd_sta = 18, + .tbuf = 32, + .scl_stretch_en = 0, + .trdhld = 6, + .tsp = 3 + } +}; + +static const struct hw_params hw_params_v1_4_0[3] = { + { /* I2C_MODE_STANDARD */ + .thigh = 201, + .tlow = 174, + .tsu_sto = 204, + .tsu_sta = 231, + .thd_dat = 22, + .thd_sta = 162, + .tbuf = 227, + .scl_stretch_en = 0, + .trdhld = 6, + .tsp = 3 + }, + { /* I2C_MODE_FAST */ + .thigh = 38, + .tlow = 56, + .tsu_sto = 40, + .tsu_sta = 40, + .thd_dat = 22, + .thd_sta = 35, + .tbuf = 62, + .scl_stretch_en = 0, + .trdhld = 6, + .tsp = 3 + }, + { /* I2C_MODE_FAST_PLUS */ + .thigh = 16, + .tlow = 22, + .tsu_sto = 17, + .tsu_sta = 18, + .thd_dat = 16, + .thd_sta = 15, + .tbuf = 24, + .scl_stretch_en = 0, + .trdhld = 3, + .tsp = 3 + } +}; + +static const u16 queue_0_size_v1_0_8 = 64; +static const u16 queue_1_size_v1_0_8 = 16; + +static const u16 queue_0_size_v1_4_0 = 64; +static const u16 queue_1_size_v1_4_0 = 16; + +/* + * cci_enable_clocks - Enable multiple clocks + * @nclocks: Number of clocks in clock array + * @clock: Clock array + * @dev: Device + * + * Return 0 on success or a negative error code otherwise + */ +int cci_enable_clocks(int nclocks, struct cci_clock *clock, struct device *dev) +{ + int ret; + int i; + + for (i = 0; i < nclocks; i++) { + if (clock[i].freq) { + long rate; + + rate = clk_round_rate(clock[i].clk, clock[i].freq); + if (rate < 0) { + dev_err(dev, "clk round rate failed: %ld\n", + rate); + goto error; + } + + ret = clk_set_rate(clock[i].clk, clock[i].freq); + if (ret < 0) { + dev_err(dev, "clk set rate failed: %d\n", ret); + goto error; + } + } + + ret = clk_prepare_enable(clock[i].clk); + if (ret) { + dev_err(dev, "clock enable failed, ret: %d\n", ret); + goto error; + } + } + + return 0; + +error: + for (i--; i >= 0; i--) + clk_disable_unprepare(clock[i].clk); + + return ret; +} + +/* + * cci_disable_clocks - Disable multiple clocks + * @nclocks: Number of clocks in clock array + * @clock: Clock array + */ +void cci_disable_clocks(int nclocks, struct cci_clock *clock) +{ + int i; + + for (i = nclocks - 1; i >= 0; i--) + clk_disable_unprepare(clock[i].clk); +} + +static irqreturn_t cci_isr(int irq, void *dev) +{ + struct cci *cci = dev; + u32 val; + + val = readl(cci->base + CCI_IRQ_STATUS_0); + writel(val, cci->base + CCI_IRQ_CLEAR_0); + writel(0x1, cci->base + CCI_IRQ_GLOBAL_CLEAR_CMD); + + if (val & CCI_IRQ_STATUS_0_RST_DONE_ACK) { + if (cci->master[MASTER_0].complete_pending) { + cci->master[MASTER_0].complete_pending = 0; + complete(&cci->master[MASTER_0].irq_complete); + } + + if (cci->master[MASTER_1].complete_pending) { + cci->master[MASTER_1].complete_pending = 0; + complete(&cci->master[MASTER_1].irq_complete); + } + } + + if (val & CCI_IRQ_STATUS_0_I2C_M0_RD_DONE || + val & CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT || + val & CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT) { + cci->master[MASTER_0].status = 0; + complete(&cci->master[MASTER_0].irq_complete); + } + + if (val & CCI_IRQ_STATUS_0_I2C_M1_RD_DONE || + val & CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT || + val & CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT) { + cci->master[MASTER_1].status = 0; + complete(&cci->master[MASTER_1].irq_complete); + } + + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK)) { + cci->master[MASTER_0].complete_pending = 1; + writel(CCI_RESET_CMD_M0_MASK, cci->base + CCI_RESET_CMD); + } + + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK)) { + cci->master[MASTER_1].complete_pending = 1; + writel(CCI_RESET_CMD_M1_MASK, cci->base + CCI_RESET_CMD); + } + + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_ERROR)) { + dev_err_ratelimited(cci->dev, "MASTER_0 error 0x%08x\n", val); + cci->master[MASTER_0].status = -EIO; + writel(CCI_HALT_REQ_I2C_M0_Q0Q1, cci->base + CCI_HALT_REQ); + } + + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_ERROR)) { + dev_err_ratelimited(cci->dev, "MASTER_1 error 0x%08x\n", val); + cci->master[MASTER_1].status = -EIO; + writel(CCI_HALT_REQ_I2C_M1_Q0Q1, cci->base + CCI_HALT_REQ); + } + + return IRQ_HANDLED; +} + +static int cci_reset(struct cci *cci) +{ + unsigned long time; + + cci->master[MASTER_0].complete_pending = 1; + writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD); + time = wait_for_completion_timeout( + &cci->master[MASTER_0].irq_complete, + msecs_to_jiffies(CCI_TIMEOUT_MS)); + if (!time) { + dev_err(cci->dev, "CCI reset timeout\n"); + return -ETIMEDOUT; + } + + return 0; +} + +static int cci_init(struct cci *cci, const struct hw_params *hw) +{ + u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE | + CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT | + CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT | + CCI_IRQ_MASK_0_I2C_M1_RD_DONE | + CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT | + CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT | + CCI_IRQ_MASK_0_RST_DONE_ACK | + CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK | + CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK | + CCI_IRQ_MASK_0_I2C_M0_ERROR | + CCI_IRQ_MASK_0_I2C_M1_ERROR; + int i; + + writel(val, cci->base + CCI_IRQ_MASK_0); + + for (i = 0; i < NUM_MASTERS; i++) { + val = hw->thigh << 16 | hw->tlow; + writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i)); + + val = hw->tsu_sto << 16 | hw->tsu_sta; + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_0(i)); + + val = hw->thd_dat << 16 | hw->thd_sta; + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_1(i)); + + val = hw->tbuf; + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i)); + + val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp; + writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i)); + + cci->master[i].status = 0; + } + + return 0; +} + +static int cci_run_queue(struct cci *cci, u8 master, u8 queue) +{ + unsigned long time; + u32 val; + int ret; + + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue)); + writel(val, cci->base + CCI_I2C_Mm_Qn_EXEC_WORD_CNT(master, queue)); + + val = 1 << ((master * 2) + queue); + writel(val, cci->base + CCI_QUEUE_START); + + time = wait_for_completion_timeout(&cci->master[master].irq_complete, + CCI_TIMEOUT_MS); + if (!time) { + dev_err(cci->dev, "master %d queue %d timeout\n", + master, queue); + return -ETIMEDOUT; + } + + ret = cci->master[master].status; + if (ret < 0) + dev_err(cci->dev, "master %d queue %d error %d\n", + master, queue, ret); + + return ret; +} + +static int cci_validate_queue(struct cci *cci, u32 len, u8 master, u8 queue) +{ + int ret = 0; + u32 val; + + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue)); + + if (val + len + 1 > cci->queue_size[queue]) { + val = CCI_I2C_REPORT | (1 << 8); + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); + + ret = cci_run_queue(cci, master, queue); + } + + return ret; +} + +static int cci_i2c_read(struct cci *cci, u16 addr, u8 *buf, u16 len) +{ + u8 master = MASTER_0; + u8 queue = QUEUE_1; + u32 val; + u32 words_read, words_exp; + int i, index, first; + int ret; + + if (len > cci->adap.quirks->max_read_len) + return -EOPNOTSUPP; + + /* + * Call validate queue to make sure queue is empty before starting. + * This is to avoid overflow / underflow of queue. + */ + ret = cci_validate_queue(cci, cci->queue_size[queue], master, queue); + if (ret < 0) + return ret; + + val = CCI_I2C_SET_PARAM | ((addr >> 1) & 0x7f) << 4; + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); + + val = CCI_I2C_READ | len << 4; + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); + + ret = cci_run_queue(cci, master, queue); + if (ret < 0) + return ret; + + words_read = readl(cci->base + CCI_I2C_Mm_READ_BUF_LEVEL(master)); + words_exp = len / 4 + 1; + if (words_read != words_exp) { + dev_err(cci->dev, "words read = %d, words expected = %d\n", + words_read, words_exp); + return -EIO; + } + + index = 0; + first = 1; + do { + val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master)); + + for (i = 0; i < 4 && index < len; i++) { + if (first) { + first = 0; + continue; + } + buf[index++] = (val >> (i * 8)) & 0xff; + } + } while (--words_read); + + return 0; +} + +static int cci_i2c_write(struct cci *cci, u16 addr, u8 *buf, u16 len) +{ + u8 master = MASTER_0; + u8 queue = QUEUE_0; + u8 load[12] = { 0 }; + u8 i, j; + u32 val; + int ret; + + if (len > cci->adap.quirks->max_write_len) + return -EOPNOTSUPP; + + /* + * Call validate queue to make sure queue is empty before starting. + * This is to avoid overflow / underflow of queue. + */ + ret = cci_validate_queue(cci, cci->queue_size[queue], master, queue); + if (ret < 0) + return ret; + + val = CCI_I2C_SET_PARAM | ((addr >> 1) & 0x7f) << 4; + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); + + i = 0; + load[i++] = CCI_I2C_WRITE | len << 4; + + for (j = 0; j < len; j++) + load[i++] = buf[j]; + + for (j = 0; j < i; j += 4) { + val = load[j]; + val |= load[j + 1] << 8; + val |= load[j + 2] << 16; + val |= load[j + 3] << 24; + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); + } + + val = CCI_I2C_REPORT | 1 << 8; + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue)); + + return cci_run_queue(cci, master, queue); +} + +static int cci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) +{ + struct cci *cci = i2c_get_adapdata(adap); + int i; + int ret = 0; + + if (!num) + return -EOPNOTSUPP; + + for (i = 0; i < num; i++) { + if (msgs[i].flags & I2C_M_RD) + ret = cci_i2c_read(cci, msgs[i].addr, msgs[i].buf, + msgs[i].len); + else + ret = cci_i2c_write(cci, msgs[i].addr, msgs[i].buf, + msgs[i].len); + + if (ret < 0) { + dev_err(cci->dev, "cci i2c xfer error %d", ret); + break; + } + } + + return ret; +} + +static u32 cci_func(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C; +} + +static const struct i2c_algorithm cci_algo = { + .master_xfer = cci_xfer, + .functionality = cci_func, +}; + +static const struct i2c_adapter_quirks cci_quirks_v1_0_8 = { + .max_write_len = 10, + .max_read_len = 12, +}; + +static const struct i2c_adapter_quirks cci_quirks_v1_4_0 = { + .max_write_len = 11, + .max_read_len = 12, +}; + +/* + * cci_probe - Probe CCI platform device + * @pdev: Pointer to CCI platform device + * + * Return 0 on success or a negative error code on failure + */ +static int cci_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + const struct resources *res; + const struct hw_params *hw; + struct cci *cci; + struct resource *r; + int ret = 0; + u32 val; + int i; + + cci = devm_kzalloc(&pdev->dev, sizeof(*cci), GFP_KERNEL); + if (!cci) + return -ENOMEM; + + cci->dev = dev; + platform_set_drvdata(pdev, cci); + + if (of_device_is_compatible(dev->of_node, "qcom,cci-v1.0.8")) { + res = &res_v1_0_8; + hw = hw_params_v1_0_8; + cci->queue_size[0] = queue_0_size_v1_0_8; + cci->queue_size[1] = queue_1_size_v1_0_8; + cci->adap.quirks = &cci_quirks_v1_0_8; + } else if (of_device_is_compatible(dev->of_node, "qcom,cci-v1.4.0")) { + res = &res_v1_4_0; + hw = hw_params_v1_4_0; + cci->queue_size[0] = queue_0_size_v1_4_0; + cci->queue_size[1] = queue_1_size_v1_4_0; + cci->adap.quirks = &cci_quirks_v1_4_0; + } else { + return -EINVAL; + } + + cci->adap.algo = &cci_algo; + cci->adap.dev.parent = cci->dev; + cci->adap.dev.of_node = dev->of_node; + i2c_set_adapdata(&cci->adap, cci); + + strlcpy(cci->adap.name, "Qualcomm Camera Control Interface", + sizeof(cci->adap.name)); + + cci->mode = I2C_MODE_STANDARD; + ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &val); + if (!ret) { + if (val == 400000) + cci->mode = I2C_MODE_FAST; + else if (val == 1000000) + cci->mode = I2C_MODE_FAST_PLUS; + } + + /* Memory */ + + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]); + cci->base = devm_ioremap_resource(dev, r); + if (IS_ERR(cci->base)) { + dev_err(dev, "could not map memory\n"); + return PTR_ERR(cci->base); + } + + /* Interrupt */ + + r = platform_get_resource_byname(pdev, IORESOURCE_IRQ, + res->interrupt[0]); + if (!r) { + dev_err(dev, "missing IRQ\n"); + return -EINVAL; + } + + cci->irq = r->start; + snprintf(cci->irq_name, sizeof(cci->irq_name), "%s", dev_name(dev)); + ret = devm_request_irq(dev, cci->irq, cci_isr, + IRQF_TRIGGER_RISING, cci->irq_name, cci); + if (ret < 0) { + dev_err(dev, "request_irq failed, ret: %d\n", ret); + return ret; + } + + disable_irq(cci->irq); + + /* Clocks */ + + cci->nclocks = 0; + while (res->clock[cci->nclocks]) + cci->nclocks++; + + cci->clock = devm_kzalloc(dev, cci->nclocks * + sizeof(*cci->clock), GFP_KERNEL); + if (!cci->clock) + return -ENOMEM; + + for (i = 0; i < cci->nclocks; i++) { + struct cci_clock *clock = &cci->clock[i]; + + clock->clk = devm_clk_get(dev, res->clock[i]); + if (IS_ERR(clock->clk)) + return PTR_ERR(clock->clk); + + clock->name = res->clock[i]; + clock->freq = res->clock_rate[i]; + } + + ret = cci_enable_clocks(cci->nclocks, cci->clock, dev); + if (ret < 0) + return ret; + + val = readl_relaxed(cci->base + CCI_HW_VERSION); + dev_info(dev, "%s: CCI HW version = 0x%08x", __func__, val); + + init_completion(&cci->master[0].irq_complete); + init_completion(&cci->master[1].irq_complete); + + enable_irq(cci->irq); + + ret = cci_reset(cci); + if (ret < 0) + return ret; + + ret = cci_init(cci, &hw[cci->mode]); + if (ret < 0) + return ret; + + ret = i2c_add_adapter(&cci->adap); + + return ret; +} + +/* + * cci_remove - Remove CCI platform device + * @pdev: Pointer to CCI platform device + * + * Always returns 0. + */ +static int cci_remove(struct platform_device *pdev) +{ + struct cci *cci = platform_get_drvdata(pdev); + + disable_irq(cci->irq); + cci_disable_clocks(cci->nclocks, cci->clock); + + i2c_del_adapter(&cci->adap); + + return 0; +} + +static const struct of_device_id cci_dt_match[] = { + { .compatible = "qcom,cci-v1.0.8" }, + { .compatible = "qcom,cci-v1.4.0" }, + {} +}; +MODULE_DEVICE_TABLE(of, cci_dt_match); + +static struct platform_driver qcom_cci_driver = { + .probe = cci_probe, + .remove = cci_remove, + .driver = { + .name = "i2c-qcom-cci", + .of_match_table = cci_dt_match, + }, +}; + +module_platform_driver(qcom_cci_driver); + +MODULE_ALIAS("platform:i2c-qcom-cci"); +MODULE_DESCRIPTION("Qualcomm Camera Control Interface driver"); +MODULE_AUTHOR("Todor Tomov <todor.tomov@linaro.org>"); +MODULE_LICENSE("GPL v2");
This commit adds basic I2C bus support for the Camera Control Interface (CCI) controller found on the Qualcomm SoC processors. CCI versions supported: - 1.0.8 - found on MSM8916/APQ8016 - support for the only CCI I2C bus; - 1.4.0 - found on MSM8996/APQ8096 - support for first (out of two) I2C bus. Signed-off-by: Todor Tomov <todor.tomov@linaro.org> --- drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-qcom-cci.c | 793 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 804 insertions(+) create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c