Re: [PATCH 2/4] i2c: Add Qualcomm Camera Control Interface driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 

-- 
Best regards,
Todor Tomov
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux