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
