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

From: Todor Tomov <todor.tomov@linaro.org>

This commit adds I2C driver for the Camera Control Interface
(CCI) I2C controller found on the Qualcomm SoC processors.

CCI versions supported:
 - 1.0.8 - found on MSM8916/APQ8016
 - 1.4.0 - found on MSM8996/APQ8096

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/i2c/busses/Kconfig        |  10 +
 drivers/i2c/busses/Makefile       |   1 +
 drivers/i2c/busses/i2c-qcom-cci.c | 814 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 825 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c

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.

> +	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.

> +};
> +
> +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

> +	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 {}

> +		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.


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

> +	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?

> +		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".

> +}
> +
> +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/

> +
> +	/*
> +	 * 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...

> +
> +	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() ?

> +				       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()

> +
> +	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...

> +	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?

> +
> +	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.

Regards,
Bjorn

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

On Mon 06 Aug 21:30 PDT 2018, Vinod wrote:

> 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 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?
> 

Just that unsigned int is the type used in the irq api.

[..]
> > 
> > > +
> > > +	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
> 

Right. So doing something similar to how we deal with clocks in i2c-qup
would probably make sense; we enable them during probe and then toggle
them using pm_runtime from there on.

Regards,
Bjorn

On 07-08-18, 10:00, Vinod wrote:
> On 06-08-18, 11:45, Bjorn Andersson wrote:

> > 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

So I discussed this with Todor and found that __genpd_dev_pm_attach in
drivers/base/power/domain.c handles the "power-domain" binding.

I do not think driver needs to do anything here, this seems handled in
core and we need to provide binding, FWIW, the 8996 doesn't work without
this

On Fri 17 Aug 05:41 PDT 2018, Vinod wrote:

> On 07-08-18, 10:00, Vinod wrote:
> > On 06-08-18, 11:45, Bjorn Andersson wrote:
> 
> > > 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
> 
> So I discussed this with Todor and found that __genpd_dev_pm_attach in
> drivers/base/power/domain.c handles the "power-domain" binding.
> 
> I do not think driver needs to do anything here, this seems handled in
> core and we need to provide binding, FWIW, the 8996 doesn't work without
> this
> 

Right, the power-domain will be picked up and afaict will be turned on
as it's attached to the device during probe of the platform driver. So
you don't need to do anything additionally to make it work.

But I suggest that you follow up with a patch adding runtime pm support
so that the associated power domain will be disabled when the bus is not
in use.

Regards,
Bjorn

On 17-08-18, 11:33, Bjorn Andersson wrote:
> On Fri 17 Aug 05:41 PDT 2018, Vinod wrote:
> 
> > On 07-08-18, 10:00, Vinod wrote:
> > > On 06-08-18, 11:45, Bjorn Andersson wrote:
> > 
> > > > 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
> > 
> > So I discussed this with Todor and found that __genpd_dev_pm_attach in
> > drivers/base/power/domain.c handles the "power-domain" binding.
> > 
> > I do not think driver needs to do anything here, this seems handled in
> > core and we need to provide binding, FWIW, the 8996 doesn't work without
> > this
> > 
> 
> Right, the power-domain will be picked up and afaict will be turned on
> as it's attached to the device during probe of the platform driver. So
> you don't need to do anything additionally to make it work.
> 
> But I suggest that you follow up with a patch adding runtime pm support
> so that the associated power domain will be disabled when the bus is not
> in use.

Agreed, have already started on that.