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

Groups > linux.kernel > #1411253 > unrolled thread

Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

Started by"M'boumba Cedric Madianga" <cedric.madianga@gmail.com>
First post2016-06-01 16:10 +0200
Last post2016-06-03 09:50 +0200
Articles 5 — 2 participants

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: [PATCH 2/5] i2c: Add STM32F4 I2C driver "M'boumba Cedric Madianga" <cedric.madianga@gmail.com> - 2016-06-01 16:10 +0200
    Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver Maxime Coquelin <mcoquelin.stm32@gmail.com> - 2016-06-01 16:20 +0200
      Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver "M'boumba Cedric Madianga" <cedric.madianga@gmail.com> - 2016-06-02 17:40 +0200
        Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver Maxime Coquelin <mcoquelin.stm32@gmail.com> - 2016-06-02 18:10 +0200
          Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver "M'boumba Cedric Madianga" <cedric.madianga@gmail.com> - 2016-06-03 09:50 +0200

#1411253 — Re: [PATCH 2/5] i2c: Add STM32F4 I2C driver

From"M'boumba Cedric Madianga" <cedric.madianga@gmail.com>
Date2016-06-01 16:10 +0200
SubjectRe: [PATCH 2/5] i2c: Add STM32F4 I2C driver
Message-ID<rFf0l-Xx-13@gated-at.bofh.it>
Hi Maxime,

>>> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +       struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
>>> +       u32 ccr, val, clk_rate;
>>> +
>>> +       ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
>>> +       ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
>>> +                STM32F4_I2C_CCR_CCR_MASK);
>>> +
>>> +       clk_rate = clk_get_rate(i2c_dev->clk);
>>> +
>>> +       switch (i2c_dev->speed) {
>>> +       case STM32F4_I2C_SPEED_STANDARD:
>>> +               val = clk_rate / t->rate * 2;
>>> +               if (val < STM32F4_I2C_MIN_CCR)
>>> +                       ccr |= STM32F4_I2C_CCR_CCR(STM32F4_I2C_MIN_CCR);
>>> +               else
>>> +                       ccr |= STM32F4_I2C_CCR_CCR(val);
>>> +               break;
>>> +       case STM32F4_I2C_SPEED_FAST:
>>> +               ccr |= STM32F4_I2C_CCR_FS;
>>> +               if (t->duty) {
>>> +                       ccr |= STM32F4_I2C_CCR_DUTY;
>>> +                       ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 25);
>>> +               } else {
>>> +                       ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 3);
>>> +               }
>> Is it really useful since duty seems to always be 0?
> Agree, I will rework it by directly set duty at 0 in the register.

Contrary to what I wrote previously, the duty has to be set for FAST
Mode to reach 400khz.
So, I am going to keep the timing struct and set duty to 1 for FAST mode

[toc] | [next] | [standalone]

#1411261

FromMaxime Coquelin <mcoquelin.stm32@gmail.com>
Date2016-06-01 16:20 +0200
Message-ID<rFfa1-11j-1@gated-at.bofh.it>
In reply to#1411253
2016-06-01 16:01 GMT+02:00 M'boumba Cedric Madianga <cedric.madianga@gmail.com>:
> Hi Maxime,
>
>>>> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
>>>> +{
>>>> +       struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
>>>> +       u32 ccr, val, clk_rate;
>>>> +
>>>> +       ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
>>>> +       ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
>>>> +                STM32F4_I2C_CCR_CCR_MASK);
>>>> +
>>>> +       clk_rate = clk_get_rate(i2c_dev->clk);
>>>> +
>>>> +       switch (i2c_dev->speed) {
>>>> +       case STM32F4_I2C_SPEED_STANDARD:
>>>> +               val = clk_rate / t->rate * 2;
>>>> +               if (val < STM32F4_I2C_MIN_CCR)
>>>> +                       ccr |= STM32F4_I2C_CCR_CCR(STM32F4_I2C_MIN_CCR);
>>>> +               else
>>>> +                       ccr |= STM32F4_I2C_CCR_CCR(val);
>>>> +               break;
>>>> +       case STM32F4_I2C_SPEED_FAST:
>>>> +               ccr |= STM32F4_I2C_CCR_FS;
>>>> +               if (t->duty) {
>>>> +                       ccr |= STM32F4_I2C_CCR_DUTY;
>>>> +                       ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 25);
>>>> +               } else {
>>>> +                       ccr |= STM32F4_I2C_CCR_CCR(clk_rate / t->rate * 3);
>>>> +               }
>>> Is it really useful since duty seems to always be 0?
>> Agree, I will rework it by directly set duty at 0 in the register.
>
> Contrary to what I wrote previously, the duty has to be set for FAST
> Mode to reach 400khz.
> So, I am going to keep the timing struct and set duty to 1 for FAST mode

Ok, That's fine by me.

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

#1412363

From"M'boumba Cedric Madianga" <cedric.madianga@gmail.com>
Date2016-06-02 17:40 +0200
Message-ID<rFCSZ-7Dw-1@gated-at.bofh.it>
In reply to#1411261
Hi,

>> +
>> +/**
>> + * stm32f4_i2c_xfer() - Transfer combined I2C message
>> + * @i2c_adap: Adapter pointer to the controller
>> + * @msgs: Pointer to data to be written.
>> + * @num: Number of messages to be executed
>> + */
>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
>> +                           int num)
>> +{
>> +       struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> +       int ret, i;
>> +
>> +       i2c_dev->busy = true;
>> +
>> +       ret = clk_prepare_enable(i2c_dev->clk);
>> +       if (ret) {
>> +               dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
>> +               return ret;
>> +       }
>> +
>> +       stm32f4_i2c_hw_config(i2c_dev);
> Maybe you could call this only at probe and resume time?
> You would save some register accesses.
Some clarification about this point.
We need to call stm32f4_i2c_hw_config before each I2C transfer as at
the end of I2C communication the peripheral is automatically disabled
and configuration registers are reset.


BR,
Cedric

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

#1412404

FromMaxime Coquelin <mcoquelin.stm32@gmail.com>
Date2016-06-02 18:10 +0200
Message-ID<rFDm2-82K-53@gated-at.bofh.it>
In reply to#1412363
Hi Cedric,

2016-06-02 17:35 GMT+02:00 M'boumba Cedric Madianga <cedric.madianga@gmail.com>:
> Hi,
>
>>> +
>>> +/**
>>> + * stm32f4_i2c_xfer() - Transfer combined I2C message
>>> + * @i2c_adap: Adapter pointer to the controller
>>> + * @msgs: Pointer to data to be written.
>>> + * @num: Number of messages to be executed
>>> + */
>>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
>>> +                           int num)
>>> +{
>>> +       struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>>> +       int ret, i;
>>> +
>>> +       i2c_dev->busy = true;
>>> +
>>> +       ret = clk_prepare_enable(i2c_dev->clk);
>>> +       if (ret) {
>>> +               dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       stm32f4_i2c_hw_config(i2c_dev);
>> Maybe you could call this only at probe and resume time?
>> You would save some register accesses.
> Some clarification about this point.
> We need to call stm32f4_i2c_hw_config before each I2C transfer as at
> the end of I2C communication the peripheral is automatically disabled
> and configuration registers are reset.

Ok, but I wonder how the IP knows this is the last i2c message to be sent?
Or maybe it gets re-initialized as soon as the clk is disabled?

Thanks for the inputs,
Maxime

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

#1412876

From"M'boumba Cedric Madianga" <cedric.madianga@gmail.com>
Date2016-06-03 09:50 +0200
Message-ID<rFS1H-kK-23@gated-at.bofh.it>
In reply to#1412404
Hi Maxime,

2016-06-02 18:02 GMT+02:00 Maxime Coquelin <mcoquelin.stm32@gmail.com>:
> Hi Cedric,
>
> 2016-06-02 17:35 GMT+02:00 M'boumba Cedric Madianga <cedric.madianga@gmail.com>:
>> Hi,
>>
>>>> +
>>>> +/**
>>>> + * stm32f4_i2c_xfer() - Transfer combined I2C message
>>>> + * @i2c_adap: Adapter pointer to the controller
>>>> + * @msgs: Pointer to data to be written.
>>>> + * @num: Number of messages to be executed
>>>> + */
>>>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
>>>> +                           int num)
>>>> +{
>>>> +       struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>>>> +       int ret, i;
>>>> +
>>>> +       i2c_dev->busy = true;
>>>> +
>>>> +       ret = clk_prepare_enable(i2c_dev->clk);
>>>> +       if (ret) {
>>>> +               dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       stm32f4_i2c_hw_config(i2c_dev);
>>> Maybe you could call this only at probe and resume time?
>>> You would save some register accesses.
>> Some clarification about this point.
>> We need to call stm32f4_i2c_hw_config before each I2C transfer as at
>> the end of I2C communication the peripheral is automatically disabled
>> and configuration registers are reset.
>
> Ok, but I wonder how the IP knows this is the last i2c message to be sent?
When the IP is enabled we could generated a STOP at any moment by
setting a bit in CR1 register.
At this moment, the IP knows that this is the last byte to be sent.
After STOP generation, I notice that all registers are automatically
set with their reset values

> Or maybe it gets re-initialized as soon as the clk is disabled?
Not really because they are all re-initialized before disabling the clock

>
> Thanks for the inputs,
> Maxime

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web