Hi Bjorn, On Sat, 7 Mar 2020 at 03:05, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote: > > On Fri 06 Mar 09:19 PST 2020, Loic Poulain wrote: > > > This commit adds I2C bus support for the Camera Control Interface > > (CCI) I2C controller found on the Qualcomm SoC processors. This I2C > > controller supports two masters and they are registered to the core. > > > > CCI versions supported in the driver are MSM8916 and MSM8996. > > +SDM845 > > > > > This is a rework of the patch posted by Vinod: > > https://patchwork.kernel.org/patch/10569961/ > > > > With additional fixes + most of the comments addressed. > > > > Signed-off-by: Todor Tomov <todor.tomov@xxxxxxxxxx> > > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx> > > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx> > > Tested-by: Robert Foss <robert.foss@xxxxxxxxxx> > > --- > > v2: Remove clock rates config from driver (done via assigned clock) > > Added CCI timeout recovery from Ricardo's patch: > > https://www.spinics.net/lists/linux-i2c/msg36973.html > > v3: add sdm845 support > > rework cci_init function > > v4: fix checkpatch issue (double semi-colon) > > > > drivers/i2c/busses/Kconfig | 10 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-qcom-cci.c | 787 ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 798 insertions(+) > > create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 6a0aa76..807a052 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -894,6 +894,16 @@ config I2C_QCOM_GENI > > This driver can also be built as a module. If so, the module > > will be called i2c-qcom-geni. > > > > +config I2C_QCOM_CCI > > Please move this above GENI, to keep sort order. ack > > > + tristate "Qualcomm Camera Control Interface" > > + depends on ARCH_QCOM || COMPILE_TEST > > + 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 3ab8aeb..9028b77 100644 > > --- a/drivers/i2c/busses/Makefile > > +++ b/drivers/i2c/busses/Makefile > > @@ -92,6 +92,7 @@ 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_GENI) += i2c-qcom-geni.o > > +obj-$(CONFIG_I2C_QCOM_CCI) += i2c-qcom-cci.o > > Sort order. ack > > > 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 > [..] > > +/* Max number of resources + 1 for a NULL terminator */ > > +#define CCI_RES_MAX 6 > > + > > + > > Extra newline? > > > +#define CCI_I2C_SET_PARAM 1 > > +#define CCI_I2C_REPORT 8 > > +#define CCI_I2C_WRITE 9 > > +#define CCI_I2C_READ 10 > [..] > > +static int cci_i2c_read(struct cci *cci, u16 master, > > + u16 addr, u8 *buf, u16 len) > > +{ > > + u32 val, words_read, words_exp; > > + u8 queue = QUEUE_1; > > + int i, index = 0, ret; > > + bool first = true; > > + > > + /* > > + * Call validate queue to make sure queue is empty before starting. > > + * This is to avoid overflow / underflow of queue. > > + */ > > + ret = cci_validate_queue(cci, master, queue); > > + if (ret < 0) > > + return ret; > > + > > + val = CCI_I2C_SET_PARAM | (addr & 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; > > + } > > + > > + do { > > + val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master)); > > + > > + for (i = 0; i < 4 && index < len; i++) { > > + if (first) { > > So lower 8 bits of the first word read should always be discarded? Do we > know why? Can we have a comment describing this behavior? Yes, the first byte contains the SID (slave ID), I'll add a comment. > > + cci->master[i].master = i; > > + cci->master[i].cci = cci; > > + > > + i2c_set_adapdata(&cci->master[i].adap, &cci->master[i]); > > + snprintf(cci->master[i].adap.name, > > + sizeof(cci->master[i].adap.name), > > + "Qualcomm Camera Control Interface: %d", i); > > + > > + /* find the child node for i2c-bus as we are on cci node */ > > + of_node = of_get_next_available_child(dev->of_node, of_node); > > + if (!of_node) { > > + dev_err(dev, "Missing i2c-bus@%d child node\n", i); > > + return -EINVAL; > > + } > > + cci->master[i].adap.dev.of_node = of_node; > > Won't this break if the two masters are provided in reverse order in the > DT? Indeed, I'm going to rework that. > > + /* Interrupt */ > > + > > + ret = platform_get_irq(pdev, 0); > > + if (ret < 0) { > > + dev_err(dev, "missing IRQ: %d\n", ret); > > + return ret; > > + } > > + cci->irq = ret; > > + > > + ret = devm_request_irq(dev, cci->irq, cci_isr, > > + IRQF_TRIGGER_RISING, dev_name(dev), cci); > > Wouldn't it be better to request the irq after the clocks are enabled? > Presumably it won't fire here, but if it does then the isr might access > unclocked registers? > > At least you shouldn't have to temporarily disable the irqs until later > in the probe? Yes we can move the request at a later time in the probe. Regards, Loic
