Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720853 > unrolled thread

Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver Wolfram Sang <wsa@the-dreams.de> - 2017-08-27 17:40 +0200
    Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver Baolin Wang <baolin.wang@linaro.org> - 2017-08-28 05:30 +0200
      Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver Wolfram Sang <wsa@the-dreams.de> - 2017-08-28 17:20 +0200
        Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver Baolin Wang <baolin.wang@linaro.org> - 2017-08-29 04:00 +0200

#1720853 — Re: [RESEND PATCH v4 2/2] i2c: Add Spreadtrum I2C controller driver

[Multipart message — attachments visible in raw view] — view raw

Hi,

thanks for your submission.

> +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)
> +{
> +	dev_err(&i2c_dev->adap.dev, ": ======dump i2c-%d reg=======\n",
> +		i2c_dev->adap.nr);
> +	dev_err(&i2c_dev->adap.dev, ": I2C_CTRL:0x%x\n",
> +		readl(i2c_dev->base + I2C_CTL));
> +	dev_err(&i2c_dev->adap.dev, ": I2C_ADDR_CFG:0x%x\n",
> +		readl(i2c_dev->base + I2C_ADDR_CFG));
> +	dev_err(&i2c_dev->adap.dev, ": I2C_COUNT:0x%x\n",
> +		readl(i2c_dev->base + I2C_COUNT));
> +	dev_err(&i2c_dev->adap.dev, ": I2C_RX:0x%x\n",
> +		readl(i2c_dev->base + I2C_RX));
> +	dev_err(&i2c_dev->adap.dev, ": I2C_STATUS:0x%x\n",
> +		readl(i2c_dev->base + I2C_STATUS));
> +	dev_err(&i2c_dev->adap.dev, ": ADDR_DVD0:0x%x\n",
> +		readl(i2c_dev->base + ADDR_DVD0));
> +	dev_err(&i2c_dev->adap.dev, ": ADDR_DVD1:0x%x\n",
> +		readl(i2c_dev->base + ADDR_DVD1));
> +	dev_err(&i2c_dev->adap.dev, ": ADDR_STA0_DVD:0x%x\n",
> +		readl(i2c_dev->base + ADDR_STA0_DVD));
> +	dev_err(&i2c_dev->adap.dev, ": ADDR_RST:0x%x\n",
> +		readl(i2c_dev->base + ADDR_RST));

I really thing register dumps should be dev_dbg().

> +}
> +
> +static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
> +{
> +	writel(count, i2c_dev->base + I2C_COUNT);
> +}
> +
> +static void sprd_i2c_send_stop(struct sprd_i2c *i2c_dev, int stop)
> +{
> +	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);

u32? Here and in many other places?

...

> +static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> +{
> +	struct sprd_i2c *i2c_dev = dev_id;
> +	struct i2c_msg *msg = i2c_dev->msg;
> +	int ack = readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK;
> +	u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);
> +	u32 i2c_tran;
> +
> +	if (msg->flags & I2C_M_RD)
> +		i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;
> +	else
> +		i2c_tran = i2c_count;
> +
> +	/*
> +	 * If we got one ACK from slave when writing data, and we did not

Here you say: "If we get ack..."

> +	 * finish this transmission (i2c_tran is not zero), then we should
> +	 * continue to write data.
> +	 *
> +	 * For reading data, ack is always 0, if i2c_tran is not 0 which
> +	 * means we still need to contine to read data from slave.
> +	 */
> +	if (i2c_tran && !ack) {

... but the code gives the assumption you did NOT get an ack. So, either
rename the variable to 'ack_err' or keep it 'ack' and invert the logic
when initializing the variable.

> +		sprd_i2c_data_transfer(i2c_dev);
> +		return IRQ_HANDLED;
> +	}
> +
> +	i2c_dev->err = 0;
> +
> +	/*
> +	 * If we did not get one ACK from slave when writing data, we should
> +	 * dump all registers to check I2C status.

Why? I would say no. NACK from a slave can always happen, e.g. when an
EEPROM is busy erasing a page.

> +	 */
> +	if (ack) {
> +		i2c_dev->err = -EIO;
> +		sprd_i2c_dump_reg(i2c_dev);
> +	} else if (msg->flags & I2C_M_RD && i2c_dev->count) {
> +		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> +	}
> +
> +	/* Transmission is done and clear ack and start operation */
> +	sprd_i2c_clear_ack(i2c_dev);
> +	sprd_i2c_clear_start(i2c_dev);
> +	complete(&i2c_dev->complete);
> +
> +	return IRQ_HANDLED;
> +}

...

> +
> +	pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);
> +	pm_runtime_use_autosuspend(i2c_dev->dev);
> +	pm_runtime_set_active(i2c_dev->dev);
> +	pm_runtime_enable(i2c_dev->dev);
> +
> +	ret = pm_runtime_get_sync(i2c_dev->dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "i2c%d pm runtime resume failed!\n",
> +			pdev->id);

Error message has wrong text.

> +		goto err_rpm_put;
> +	}
> +
> +static int sprd_i2c_init(void)
> +{
> +	return platform_driver_register(&sprd_i2c_driver);
> +}
> +arch_initcall_sync(sprd_i2c_init);

arch_initcall? and no exit() function? Why is it that way and/or why
can't you use platform_module_driver()?

Rest looks good. I like the comments you added to the code.

Regards,

   Wolfram

[toc] | [next] | [standalone]

#1720984

Hi Wolfram,

On 27 August 2017 at 23:30, Wolfram Sang <wsa@the-dreams.de> wrote:
> Hi,
>
> thanks for your submission.
>
>> +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)
>> +{
>> +     dev_err(&i2c_dev->adap.dev, ": ======dump i2c-%d reg=======\n",
>> +             i2c_dev->adap.nr);
>> +     dev_err(&i2c_dev->adap.dev, ": I2C_CTRL:0x%x\n",
>> +             readl(i2c_dev->base + I2C_CTL));
>> +     dev_err(&i2c_dev->adap.dev, ": I2C_ADDR_CFG:0x%x\n",
>> +             readl(i2c_dev->base + I2C_ADDR_CFG));
>> +     dev_err(&i2c_dev->adap.dev, ": I2C_COUNT:0x%x\n",
>> +             readl(i2c_dev->base + I2C_COUNT));
>> +     dev_err(&i2c_dev->adap.dev, ": I2C_RX:0x%x\n",
>> +             readl(i2c_dev->base + I2C_RX));
>> +     dev_err(&i2c_dev->adap.dev, ": I2C_STATUS:0x%x\n",
>> +             readl(i2c_dev->base + I2C_STATUS));
>> +     dev_err(&i2c_dev->adap.dev, ": ADDR_DVD0:0x%x\n",
>> +             readl(i2c_dev->base + ADDR_DVD0));
>> +     dev_err(&i2c_dev->adap.dev, ": ADDR_DVD1:0x%x\n",
>> +             readl(i2c_dev->base + ADDR_DVD1));
>> +     dev_err(&i2c_dev->adap.dev, ": ADDR_STA0_DVD:0x%x\n",
>> +             readl(i2c_dev->base + ADDR_STA0_DVD));
>> +     dev_err(&i2c_dev->adap.dev, ": ADDR_RST:0x%x\n",
>> +             readl(i2c_dev->base + ADDR_RST));
>
> I really thing register dumps should be dev_dbg().

OK. Will fix in next version.

>
>> +}
>> +
>> +static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
>> +{
>> +     writel(count, i2c_dev->base + I2C_COUNT);
>> +}
>> +
>> +static void sprd_i2c_send_stop(struct sprd_i2c *i2c_dev, int stop)
>> +{
>> +     unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
>
> u32? Here and in many other places?

OK.

>
> ...
>
>> +static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>> +{
>> +     struct sprd_i2c *i2c_dev = dev_id;
>> +     struct i2c_msg *msg = i2c_dev->msg;
>> +     int ack = readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK;
>> +     u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);
>> +     u32 i2c_tran;
>> +
>> +     if (msg->flags & I2C_M_RD)
>> +             i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;
>> +     else
>> +             i2c_tran = i2c_count;
>> +
>> +     /*
>> +      * If we got one ACK from slave when writing data, and we did not
>
> Here you say: "If we get ack..."
>
>> +      * finish this transmission (i2c_tran is not zero), then we should
>> +      * continue to write data.
>> +      *
>> +      * For reading data, ack is always 0, if i2c_tran is not 0 which
>> +      * means we still need to contine to read data from slave.
>> +      */
>> +     if (i2c_tran && !ack) {
>
> ... but the code gives the assumption you did NOT get an ack. So, either
> rename the variable to 'ack_err' or keep it 'ack' and invert the logic
> when initializing the variable.

If ack == 0 means we got one ack. I will invert the logic as you suggested.

>
>> +             sprd_i2c_data_transfer(i2c_dev);
>> +             return IRQ_HANDLED;
>> +     }
>> +
>> +     i2c_dev->err = 0;
>> +
>> +     /*
>> +      * If we did not get one ACK from slave when writing data, we should
>> +      * dump all registers to check I2C status.
>
> Why? I would say no. NACK from a slave can always happen, e.g. when an
> EEPROM is busy erasing a page.

For our I2C controller databook, if the master did not get one ACK
from slave when writing data to salve, we should send one STOP signal
to abort this data transfer or generate one repeated START signal to
start one new data transfer cycle. Considering our I2C usage
scenarios, we should dump registers to analyze I2C status and notify
to user to re-start new data transfer.

>
>> +      */
>> +     if (ack) {
>> +             i2c_dev->err = -EIO;
>> +             sprd_i2c_dump_reg(i2c_dev);
>> +     } else if (msg->flags & I2C_M_RD && i2c_dev->count) {
>> +             sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
>> +     }
>> +
>> +     /* Transmission is done and clear ack and start operation */
>> +     sprd_i2c_clear_ack(i2c_dev);
>> +     sprd_i2c_clear_start(i2c_dev);
>> +     complete(&i2c_dev->complete);
>> +
>> +     return IRQ_HANDLED;
>> +}
>
> ...
>
>> +
>> +     pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);
>> +     pm_runtime_use_autosuspend(i2c_dev->dev);
>> +     pm_runtime_set_active(i2c_dev->dev);
>> +     pm_runtime_enable(i2c_dev->dev);
>> +
>> +     ret = pm_runtime_get_sync(i2c_dev->dev);
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev, "i2c%d pm runtime resume failed!\n",
>> +                     pdev->id);
>
> Error message has wrong text.

Will fix it.

>
>> +             goto err_rpm_put;
>> +     }
>> +
>> +static int sprd_i2c_init(void)
>> +{
>> +     return platform_driver_register(&sprd_i2c_driver);
>> +}
>> +arch_initcall_sync(sprd_i2c_init);
>
> arch_initcall? and no exit() function? Why is it that way and/or why
> can't you use platform_module_driver()?

As I explained before, in our Spreadtrum platform, our regulator
driver will depend on I2C driver and the regulator driver uses
subsys_initcall() level to initialize. Moreover some other drivers
like GPU, they will depend on regulator to set voltage and they also
need initialization much earlier.

Since it is arch_initcall() level, Andy suggested I should get rid of
tristate (use bool) and drop module.h here and all leftovers like
MODULE_*() calls including module_exit().

>
> Rest looks good. I like the comments you added to the code.

Really appreciated for your comments.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1721731

[Multipart message — attachments visible in raw view] — view raw

> >> +     /*
> >> +      * If we did not get one ACK from slave when writing data, we should
> >> +      * dump all registers to check I2C status.
> >
> > Why? I would say no. NACK from a slave can always happen, e.g. when an
> > EEPROM is busy erasing a page.
> 
> For our I2C controller databook, if the master did not get one ACK
> from slave when writing data to salve, we should send one STOP signal
> to abort this data transfer or generate one repeated START signal to
> start one new data transfer cycle. Considering our I2C usage

Yes, so far so good.

> scenarios, we should dump registers to analyze I2C status and notify
> to user to re-start new data transfer.

I disagree here. You notify the users by returning -EIO. The upper layer
(e.g. the i2c client driver) will handle it, like the EEPROM driver
might retry after a while. This all is expected behaviour, so no need to
print the registers to the logfile.

If you really, really want to keep it, make it debug output. But I think
the sentence "we should dump all registers" needs to be rephrased.

> As I explained before, in our Spreadtrum platform, our regulator
> driver will depend on I2C driver and the regulator driver uses
> subsys_initcall() level to initialize. Moreover some other drivers
> like GPU, they will depend on regulator to set voltage and they also
> need initialization much earlier.
> 
> Since it is arch_initcall() level, Andy suggested I should get rid of
> tristate (use bool) and drop module.h here and all leftovers like
> MODULE_*() calls including module_exit().

I see. So the driver is really so essential for proper bootup that it is
not even allowed to be unloaded. I might make an exception here and
allow arch_initcall() then. But I do wonder: did you try deferred
probing all over the place?

[toc] | [prev] | [next] | [standalone]

#1722100

Hi Wolfram,

On 28 August 2017 at 23:13, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> >> +     /*
>> >> +      * If we did not get one ACK from slave when writing data, we should
>> >> +      * dump all registers to check I2C status.
>> >
>> > Why? I would say no. NACK from a slave can always happen, e.g. when an
>> > EEPROM is busy erasing a page.
>>
>> For our I2C controller databook, if the master did not get one ACK
>> from slave when writing data to salve, we should send one STOP signal
>> to abort this data transfer or generate one repeated START signal to
>> start one new data transfer cycle. Considering our I2C usage
>
> Yes, so far so good.
>
>> scenarios, we should dump registers to analyze I2C status and notify
>> to user to re-start new data transfer.
>
> I disagree here. You notify the users by returning -EIO. The upper layer
> (e.g. the i2c client driver) will handle it, like the EEPROM driver
> might retry after a while. This all is expected behaviour, so no need to
> print the registers to the logfile.
>
> If you really, really want to keep it, make it debug output. But I think
> the sentence "we should dump all registers" needs to be rephrased.

Make sense. I will remove the registers printing here.

>
>> As I explained before, in our Spreadtrum platform, our regulator
>> driver will depend on I2C driver and the regulator driver uses
>> subsys_initcall() level to initialize. Moreover some other drivers
>> like GPU, they will depend on regulator to set voltage and they also
>> need initialization much earlier.
>>
>> Since it is arch_initcall() level, Andy suggested I should get rid of
>> tristate (use bool) and drop module.h here and all leftovers like
>> MODULE_*() calls including module_exit().
>
> I see. So the driver is really so essential for proper bootup that it is
> not even allowed to be unloaded. I might make an exception here and

Yes.

> allow arch_initcall() then. But I do wonder: did you try deferred
> probing all over the place?

Many modules (like GPU) need set voltage earlier by regulator which
depends on I2C ( or we also need regulate voltage for big-cores ASAP),
if we defer to set voltage for GPU or other modules, maybe will cause
some system problems. Thanks for your comments.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web