Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Vinod <vkoul@kernel.org>]

On 06-08-18, 11:45, Bjorn Andersson wrote:
> On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
> [..]
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> [..]
> > +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 {
> 
> This is now unused.

ah yes, thanks for spotting, will remove

> 
> > +	struct clk *clk;
> > +	const char *name;
> > +	u32 freq;
> > +};
> > +
> > +struct cci;
> > +
> > +struct cci_master {
> > +	struct i2c_adapter adap;
> > +	u16 master;
> > +	u8 mode;
> > +	int status;
> > +	bool complete_pending;
> > +	struct completion irq_complete;
> > +	struct cci *cci;
> > +
> 
> Empty line.

will fix this and other style/empty lines, thanks.

> 
> > +};
> > +
> > +struct cci_data {
> > +	unsigned int num_masters;
> > +	struct i2c_adapter_quirks quirks;
> > +	u16 queue_size[NUM_QUEUES];
> > +	struct cci_res res;
> > +	struct hw_params params[3];
> > +};
> > +
> > +struct cci {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	u32 irq;
> 
> Use unsigned int rather than a type of specific size

Any specific reason for that preference?

> 
> > +	const struct cci_data *data;
> > +	struct clk_bulk_data *clock;
> > +	u32 *clock_freq;
> > +	int nclocks;
> > +	struct cci_master master[NUM_MASTERS];
> > +};
> > +
> > +/**
> > + * cci_clock_set_rate() - Set clock frequency rates
> > + * @nclocks: Number of clocks
> > + * @clock: Clock array
> > + * @clock_freq: Clock frequency rate array
> > + * @dev: Device
> > + *
> > + * Return 0 on success or a negative error code otherwise
> > + */
> > +static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
> > +			      u32 *clock_freq, struct device *dev)
> > +{
> > +	int i, ret;
> > +	long rate;
> > +
> > +	for (i = 0; i < nclocks; i++)
> 
> Multiline loop deserves {}

yes

> 
> > +		if (clock_freq[i]) {
> > +			rate = clk_round_rate(clock[i].clk, clock_freq[i]);
> > +			if (rate < 0) {
> > +				dev_err(dev, "clk round rate failed: %ld\n",
> > +					rate);
> > +				return rate;
> > +			}
> > +
> > +			ret = clk_set_rate(clock[i].clk, clock_freq[i]);
> > +			if (ret < 0) {
> > +				dev_err(dev, "clk set rate failed: %d\n", ret);
> > +				return ret;
> > +			}
> > +		}
> > +
> > +	return 0;
> > +}
> > +
> [..]
> [..]
> > +static int cci_reset(struct cci *cci)
> > +{
> > +	unsigned long time;
> > +
> > +	cci->master[0].complete_pending = true;
> > +	writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
> > +
> > +	time = wait_for_completion_timeout
> > +		(&cci->master[0].irq_complete,
> > +		 msecs_to_jiffies(CCI_TIMEOUT_MS));
> 
> Please rework the indentation of this.

and I think we should pass master to this, this looks not correct to me.

> 
> Also CCI_TIMEOUT_MS is converted to jiffies in all the places, define it
> in jiffies instead.

will do

> 
> > +	if (!time) {
> > +		dev_err(cci->dev, "CCI reset timeout\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> > +}
> [..]
> > +static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
> > +{
> > +	int ret = 0;
> > +	u32 val;
> > +
> > +	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> > +
> > +	if (val == cci->data->queue_size[queue])
> > +		return -EINVAL;
> > +
> > +	if (val) {
> > +		val = CCI_I2C_REPORT | BIT(8);
> 
> Can we get a define (or a comment) for BIT(8) as well?

lets define BIT(8)..

> 
> > +		writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > +		ret = cci_run_queue(cci, master, queue);
> > +	}
> > +
> > +	return ret;
> 
> Rather than wrapping the second half of the function in a check for val,
> return early on !val and then you can end with return cci_run_queue()
> and drop the "ret".

yes sounds better


> 
> > +}
> > +
> > +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 = 0;
> 
> s/0/false/

right

> 
> > +
> > +	/*
> > +	 * 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;
> > +	}
> 
> I thought that an i2c read could return less data than was requested...

yes that is my understanding, will check with Todor and update this

> 
> > +
> > +	do {
> > +		val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
> > +
> > +		for (i = 0; i < 4 && index < len; i++) {
> > +			if (first) {
> > +				first = false;
> > +				continue;
> > +			}
> > +			buf[index++] = (val >> (i * 8)) & 0xff;
> > +		}
> > +	} while (--words_read);
> > +
> > +	return 0;
> > +}
> [..]
> > +static int cci_probe(struct platform_device *pdev)
> > +{
> [..]
> > +	/* Clocks */
> > +
> > +	cci->nclocks = 0;
> > +	while (cci->data->res.clock[cci->nclocks])
> > +		cci->nclocks++;
> > +
> > +	cci->clock = devm_kcalloc(dev, cci->nclocks,
> > +				  sizeof(*cci->clock), GFP_KERNEL);
> > +	if (!cci->clock)
> > +		return -ENOMEM;
> > +
> > +	cci->clock_freq = devm_kcalloc(dev, cci->nclocks,
> 
> You're just using this temporarily to create a copy of the
> res.clock_rate array, how about just passing the res.clock_rate into
> cci_clock_set_rate() ?

that sound reasonable, will give it a shot

> 
> > +				       sizeof(*cci->clock_freq), GFP_KERNEL);
> > +	if (!cci->clock_freq)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < cci->nclocks; i++) {
> > +		struct clk_bulk_data *clock = &cci->clock[i];
> > +
> > +		clock->clk = devm_clk_get(dev, cci->data->res.clock[i]);
> > +		if (IS_ERR(clock->clk))
> > +			return PTR_ERR(clock->clk);
> > +
> > +		clock->id = cci->data->res.clock[i];
> > +		cci->clock_freq[i] = cci->data->res.clock_rate[i];
> > +	}
> 
> Fill out cci->clock[*].id and call clk_bulk_get()

ok

> 
> > +
> > +	ret = cci_clock_set_rate(cci->nclocks, cci->clock,
> > +				 cci->clock_freq, dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clock);
> 
> It seems a little bit excessive to keep the clocks on while the driver
> is probed, but this could be improved in a follow up patch...

okay but are they required for controller to be probed, I will check
that part and update

> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	val = readl_relaxed(cci->base + CCI_HW_VERSION);
> > +	dev_dbg(dev, "%s: CCI HW version = 0x%08x", __func__, val);
> > +
> > +	enable_irq(cci->irq);
> > +
> > +	ret = cci_reset(cci);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	for (i = 0; i < cci->data->num_masters; i++) {
> > +		ret = cci_init(cci, &cci->data->params[cci->master[i].mode]);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		ret = i2c_add_adapter(&cci->master[i].adap);
> > +		if (ret < 0)
> > +			goto error_i2c;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_i2c:
> > +	for (; i >= 0; i--)
> > +		i2c_del_adapter(&cci->master[i].adap);
> > +error:
> > +	disable_irq(cci->irq);
> > +	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int cci_remove(struct platform_device *pdev)
> > +{
> > +	struct cci *cci = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	disable_irq(cci->irq);
> > +	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
> 
> i2c clients might be communicating with their clients until you call
> i2c_del_adapter(), so better pull the resources after you have removed
> the adaptor.
> 
> Maybe even a cci_halt() call before we cut the clocks?

yes makes sense

> 
> > +
> > +	for (i = 0; i < cci->data->num_masters; i++)
> > +		i2c_del_adapter(&cci->master[i].adap);
> > +
> > +	return 0;
> > +}
> [..]
> > +static const struct cci_data cci_v1_4_0_data = {
> [..]
> > +		{
> > +			/* 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
> > +		}
> > +
> 
> Empty line.
> 
> > +	},
> > +
> > +};
> 
> The dt binding mentions that a power domain is required for v1.4, but
> there's no support for this in the driver.

I will check that, yes it is not handled in the driver we have

-- 
~Vinod