Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1683288 > unrolled thread
| Started by | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| First post | 2017-07-07 18:30 +0200 |
| Last post | 2017-07-13 00:40 +0200 |
| Articles | 4 — 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.
Re: [PATCHv4 3/3] i2c: altera: Add Altera I2C Controller driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-07 18:30 +0200
Re: [PATCHv4 3/3] i2c: altera: Add Altera I2C Controller driver Thor Thayer <thor.thayer@linux.intel.com> - 2017-07-07 23:10 +0200
Re: [PATCHv4 3/3] i2c: altera: Add Altera I2C Controller driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-08 23:50 +0200
Re: [PATCHv4 3/3] i2c: altera: Add Altera I2C Controller driver Thor Thayer <thor.thayer@linux.intel.com> - 2017-07-13 00:40 +0200
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-07-07 18:30 +0200 |
| Subject | Re: [PATCHv4 3/3] i2c: altera: Add Altera I2C Controller driver |
| Message-ID | <u0EiJ-6wG-3@gated-at.bofh.it> |
On Mon, Jun 19, 2017 at 11:36 PM, <thor.thayer@linux.intel.com> wrote:
Either...
> +#include <linux/init.h>
...or...
> +#include <linux/module.h>
...choose one.
> +#define ALTR_I2C_THRESHOLD 0 /*IRQ Threshold at 1 element */
Space missed.
> +/**
> + * altr_i2c_empty_rx_fifo - Fetch data from RX FIFO until end of
> + * transfer. Send a Stop bit on the last byte.
> + */
> +static void altr_i2c_empty_rx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t rx_fifo_avail = readl(idev->base + ALTR_I2C_RX_FIFO_LVL);
> + int bytes_to_transfer = min(rx_fifo_avail, idev->msg_len);
> +
> + while (bytes_to_transfer-- > 0) {
> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
> + if (idev->msg_len == 1)
> + altr_i2c_stop(idev);
> + else
> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
> +
> + idev->msg_len--;
> + }
Move out invariant from the loop (and I see a bug, you might go over
boundaries).
while (bytes_to_transfer-- > 0) {
*idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
if (idev->msg_len-- == 1)
break;
writel(0, idev->base + ALTR_I2C_TFR_CMD);
}
altr_i2c_stop(idev);
> +}
> +
> +/**
> + * altr_i2c_fill_tx_fifo - Fill TX FIFO from current message buffer.
> + * @return: Number of bytes left to transfer.
> + */
> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
> + ALTR_I2C_TC_FIFO_LVL);
> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
> + int ret = idev->msg_len - bytes_to_transfer;
> +
> + while (bytes_to_transfer-- > 0) {
> + if (idev->msg_len == 1)
> + writel(ALTR_I2C_TFR_CMD_STO | *idev->buf++,
> + idev->base + ALTR_I2C_TFR_CMD);
> + else
> + writel(*idev->buf++, idev->base + ALTR_I2C_TFR_CMD);
> + idev->msg_len--;
> + }
Ditto.
> +
> + return ret;
> +}
> +/**
> + * altr_i2c_wait_for_core_idle - After TX, check core idle for all bytes TX.
> + * @return: 0 on success or -ETIMEDOUT on timeout.
> + */
> +static int altr_i2c_wait_for_core_idle(struct altr_i2c_dev *idev)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(ALTR_I2C_TIMEOUT);
> +
> + do {
> + if (time_after(jiffies, timeout)) {
> + dev_err(idev->dev, "Core Idle timeout\n");
> + return -ETIMEDOUT;
> + }
> + } while (readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE);
> +
> + return 0;
> +}
readl_poll_timeout[_atomic]() please.
> +static irqreturn_t altr_i2c_isr(int irq, void *_dev)
> +{
> + struct altr_i2c_dev *idev = _dev;
> + /* Read IRQ status but only interested in Enabled IRQs. */
> + u32 status = readl(idev->base + ALTR_I2C_ISR) &
> + readl(idev->base + ALTR_I2C_ISER);
Don't you have cached value for mask?
> +}
> +
> +static int altr_i2c_xfer_msg(struct altr_i2c_dev *idev, struct i2c_msg *msg)
> +{
> + u32 int_mask = ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | ALTR_I2C_ISR_NACK;
> + u32 addr = (msg->addr << 1) & 0xFF;
i2c_8bit_addr_from_msg() ?
> + unsigned long time_left;
> +
> + if (i2c_m_rd(msg)) {
> + /* Dummy read to ensure RX FIFO is empty */
> + readl(idev->base + ALTR_I2C_RX_DATA);
> + addr |= ALTR_I2C_TFR_CMD_RW_D;
> + }
> +
> + writel(ALTR_I2C_TFR_CMD_STA | addr, idev->base + ALTR_I2C_TFR_CMD);
> +
> + if (i2c_m_rd(msg)) {
> + /* write the first byte to start the RX */
> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
> + int_mask |= ALTR_I2C_ISER_RXOF_EN | ALTR_I2C_ISER_RXRDY_EN;
> + } else {
> + altr_i2c_fill_tx_fifo(idev);
> + int_mask |= ALTR_I2C_ISR_TXRDY;
> + }
It's hard to follow. Perhaps
if (read) {
...do for read...
} else {
...do for write...
}
> + if (readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE)
> + dev_err(idev->dev, "%s: Core Status not IDLE...\n", __func__);
Better to
val = readl();
if (val)
...
> +static int
> +altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
Why [] ?!
> + struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
> + int i;
> + int ret = 0;
> +
> + for (i = 0; ret == 0 && i < num; ++i)
> + ret = altr_i2c_xfer_msg(idev, &msgs[i]);
> +
> + return ret ? : i;
Oy vey...
Perhaps
static int altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
*msgs, int num)
{
struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
int ret;
while (num--) {
ret = altr_i2c_xfer_msg(idev, msgs++);
if (ret)
return ret;
}
return 0;
}
> +static u32 altr_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
> +}
Useless. Use value in place.
> +static int altr_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct altr_i2c_dev *idev = NULL;
> + struct resource *res;
> + int irq;
> + int ret = 0;
Redundant assignment.
> + if (of_property_read_u32(np, "fifo-size", &idev->fifo_size))
> + idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;
Shouldn't be possible to auto detect?
> + if (of_property_read_u32(np, "clock-frequency", &idev->bus_clk_rate)) {
device_property_*() ?
> + dev_err(&pdev->dev, "Default to 100kHz\n");
> + idev->bus_clk_rate = 100000; /* default clock rate */
> + }
--
With Best Regards,
Andy Shevchenko
[toc] | [next] | [standalone]
| From | Thor Thayer <thor.thayer@linux.intel.com> |
|---|---|
| Date | 2017-07-07 23:10 +0200 |
| Message-ID | <u0IFI-17R-13@gated-at.bofh.it> |
| In reply to | #1683288 |
Hi Andy,
On 07/07/2017 11:25 AM, Andy Shevchenko wrote:
> On Mon, Jun 19, 2017 at 11:36 PM, <thor.thayer@linux.intel.com> wrote:
>
> Either...
>> +#include <linux/init.h>
>
> ...or...
>> +#include <linux/module.h>
>
> ...choose one.
>
>> +#define ALTR_I2C_THRESHOLD 0 /*IRQ Threshold at 1 element */
>
> Space missed.
>
Got it. Thanks!
>> +/**
>> + * altr_i2c_empty_rx_fifo - Fetch data from RX FIFO until end of
>> + * transfer. Send a Stop bit on the last byte.
>> + */
>> +static void altr_i2c_empty_rx_fifo(struct altr_i2c_dev *idev)
>> +{
>> + size_t rx_fifo_avail = readl(idev->base + ALTR_I2C_RX_FIFO_LVL);
>> + int bytes_to_transfer = min(rx_fifo_avail, idev->msg_len);
>> +
>
>> + while (bytes_to_transfer-- > 0) {
>> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
>> + if (idev->msg_len == 1)
>> + altr_i2c_stop(idev);
>> + else
>> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
>> +
>> + idev->msg_len--;
>> + }
>
> Move out invariant from the loop (and I see a bug, you might go over
> boundaries).
>
> while (bytes_to_transfer-- > 0) {
> *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
> if (idev->msg_len-- == 1)
> break;
> writel(0, idev->base + ALTR_I2C_TFR_CMD);
> }
>
> altr_i2c_stop(idev);
>
I see your point on the boundary. However your change is slightly
different from what I'm trying to do.
I think you assumed the alt_i2c_stop() call can cause a stop condition.
This soft IP can't send just a start or a stop condition by itself -
both of these conditions need to be paired with a byte.
The other subtle side effect is the start condition + byte write is the
first write which is why the last write is skipped.
I need to send a byte with a stop condition on the last expected byte
(idev->msg_len == 1) while this change would send it after the FIFO is
empty or after (msg_len == 1).
Your version is cleaner so I'll just add the alt_i2c_stop(idev) call
inside the (msg_len == 1) condition and before the break.
>> +}
>> +
>> +/**
>> + * altr_i2c_fill_tx_fifo - Fill TX FIFO from current message buffer.
>> + * @return: Number of bytes left to transfer.
>> + */
>> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
>> +{
>> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
>> + ALTR_I2C_TC_FIFO_LVL);
>> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
>> + int ret = idev->msg_len - bytes_to_transfer;
>> +
>> + while (bytes_to_transfer-- > 0) {
>> + if (idev->msg_len == 1)
>> + writel(ALTR_I2C_TFR_CMD_STO | *idev->buf++,
>> + idev->base + ALTR_I2C_TFR_CMD);
>> + else
>> + writel(*idev->buf++, idev->base + ALTR_I2C_TFR_CMD);
>> + idev->msg_len--;
>> + }
>
> Ditto.
>
See above but I will move the msg_len-- inside the condition check like
you had.
>> +
>> + return ret;
>> +}
>
>> +/**
>> + * altr_i2c_wait_for_core_idle - After TX, check core idle for all bytes TX.
>> + * @return: 0 on success or -ETIMEDOUT on timeout.
>> + */
>> +static int altr_i2c_wait_for_core_idle(struct altr_i2c_dev *idev)
>> +{
>> + unsigned long timeout = jiffies + msecs_to_jiffies(ALTR_I2C_TIMEOUT);
>> +
>> + do {
>> + if (time_after(jiffies, timeout)) {
>> + dev_err(idev->dev, "Core Idle timeout\n");
>> + return -ETIMEDOUT;
>> + }
>> + } while (readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE);
>> +
>> + return 0;
>> +}
>
> readl_poll_timeout[_atomic]() please.
>
>> +static irqreturn_t altr_i2c_isr(int irq, void *_dev)
>> +{
>> + struct altr_i2c_dev *idev = _dev;
>
>> + /* Read IRQ status but only interested in Enabled IRQs. */
>> + u32 status = readl(idev->base + ALTR_I2C_ISR) &
>> + readl(idev->base + ALTR_I2C_ISER);
>
> Don't you have cached value for mask?
>
Not right now, but it may be good to add that to the altr_i2c_dev structure.
>> +}
>
>> +
>> +static int altr_i2c_xfer_msg(struct altr_i2c_dev *idev, struct i2c_msg *msg)
>> +{
>> + u32 int_mask = ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | ALTR_I2C_ISR_NACK;
>
>> + u32 addr = (msg->addr << 1) & 0xFF;
>
> i2c_8bit_addr_from_msg() ?
>
Nice! I missed that function when writing this but I like it since it
also simplifies the code below.
>> + unsigned long time_left;
>> +
>
>> + if (i2c_m_rd(msg)) {
>> + /* Dummy read to ensure RX FIFO is empty */
>> + readl(idev->base + ALTR_I2C_RX_DATA);
>> + addr |= ALTR_I2C_TFR_CMD_RW_D;
>> + }
>> +
>> + writel(ALTR_I2C_TFR_CMD_STA | addr, idev->base + ALTR_I2C_TFR_CMD);
>> +
>> + if (i2c_m_rd(msg)) {
>> + /* write the first byte to start the RX */
>> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
>> + int_mask |= ALTR_I2C_ISER_RXOF_EN | ALTR_I2C_ISER_RXRDY_EN;
>> + } else {
>> + altr_i2c_fill_tx_fifo(idev);
>> + int_mask |= ALTR_I2C_ISR_TXRDY;
>> + }
>
> It's hard to follow. Perhaps
>
> if (read) {
> ...do for read...
> } else {
> ...do for write...
> }
Will do. This will be much cleaner, especially with the
i2c_8bit_addr_from_msg() function you pointed out. Thanks!
>
>> + if (readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE)
>> + dev_err(idev->dev, "%s: Core Status not IDLE...\n", __func__);
>
> Better to
>
> val = readl();
> if (val)
> ...
Got it. Thanks!
>
>> +static int
>> +altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>> +{
>
> Why [] ?!
>
>> + struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
>
>> + int i;
>> + int ret = 0;
>> +
>> + for (i = 0; ret == 0 && i < num; ++i)
>> + ret = altr_i2c_xfer_msg(idev, &msgs[i]);
>> +
>> + return ret ? : i;
>
> Oy vey...
> Perhaps
>
> static int altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
> *msgs, int num)
> {
> struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
> int ret;
>
> while (num--) {
> ret = altr_i2c_xfer_msg(idev, msgs++);
> if (ret)
> return ret;
> }
> return 0;
> }
>
Yes, I just copied this from the axxia driver but I'll clean this up for
my re-write.
>> +static u32 altr_i2c_func(struct i2c_adapter *adap)
>> +{
>> + return I2C_FUNC_I2C;
>> +}
>
> Useless. Use value in place.
Got it. Thanks!
>
>> +static int altr_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct altr_i2c_dev *idev = NULL;
>> + struct resource *res;
>> + int irq;
>
>> + int ret = 0;
>
> Redundant assignment.
>
>> + if (of_property_read_u32(np, "fifo-size", &idev->fifo_size))
>> + idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;
>
> Shouldn't be possible to auto detect?
>
I agree. That would have been SO much better but the hardware designers
released this without capturing the size in a register - they come from
a bare-metal project perspective. Since the FIFO size is configurable
in the FPGA from 4 to 256 levels deep, I need to capture this with the
device tree.
>> + if (of_property_read_u32(np, "clock-frequency", &idev->bus_clk_rate)) {
>
> device_property_*() ?
OK. This is another function I wasn't aware of but I see the
i2c-designware-platdrv.c uses it too.
IIUC, it seems like this falls back to the device tree if the device
properties aren't defined.
>
>> + dev_err(&pdev->dev, "Default to 100kHz\n");
>> + idev->bus_clk_rate = 100000; /* default clock rate */
>> + }
>
Great comments! Thanks for reviewing!
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-07-08 23:50 +0200 |
| Message-ID | <u15LX-7rv-1@gated-at.bofh.it> |
| In reply to | #1683428 |
On Sat, Jul 8, 2017 at 12:08 AM, Thor Thayer
<thor.thayer@linux.intel.com> wrote:
> On 07/07/2017 11:25 AM, Andy Shevchenko wrote:
>> On Mon, Jun 19, 2017 at 11:36 PM, <thor.thayer@linux.intel.com> wrote:
>>> + while (bytes_to_transfer-- > 0) {
>>> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
>>> + if (idev->msg_len == 1)
>>> + altr_i2c_stop(idev);
>>> + else
>>> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
>>> +
>>> + idev->msg_len--;
>>> + }
>>
>>
>> Move out invariant from the loop (and I see a bug, you might go over
>> boundaries).
>>
>> while (bytes_to_transfer-- > 0) {
>> *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
>> if (idev->msg_len-- == 1)
>> break;
>> writel(0, idev->base + ALTR_I2C_TFR_CMD);
>> }
>>
>> altr_i2c_stop(idev);
>>
> I see your point on the boundary.
Actually I didn't notice min() call above. So, you may ignore that part.
> However your change is slightly different
> from what I'm trying to do.
I don't see how it differs to what you wrote.
> I think you assumed the alt_i2c_stop() call can cause a stop condition. This
> soft IP can't send just a start or a stop condition by itself - both of
> these conditions need to be paired with a byte.
OK.
> The other subtle side effect is the start condition + byte write is the
> first write which is why the last write is skipped.
> I need to send a byte with a stop condition on the last expected byte
> (idev->msg_len == 1) while this change would send it after the FIFO is empty
> or after (msg_len == 1).
Consider the corner case when msg_len _is_ 1 at the beginning.
Then continue with 2. I really didn't see the difference in two
snippets. Perhaps you have a bug there.
> Your version is cleaner so I'll just add the alt_i2c_stop(idev) call inside
> the (msg_len == 1) condition and before the break.
>>> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
>>> +{
>>> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
>>> +
>>> ALTR_I2C_TC_FIFO_LVL);
>>> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
>>> + int ret = idev->msg_len - bytes_to_transfer;
>>> +
>>> + while (bytes_to_transfer-- > 0) {
>>> + if (idev->msg_len == 1)
>>> + writel(ALTR_I2C_TFR_CMD_STO | *idev->buf++,
>>> + idev->base + ALTR_I2C_TFR_CMD);
>>> + else
>>> + writel(*idev->buf++, idev->base +
>>> ALTR_I2C_TFR_CMD);
>>> + idev->msg_len--;
>>> + }
>>
>>
>> Ditto.
>>
> See above but I will move the msg_len-- inside the condition check like you
> had.
Ditto.
>>> +static int altr_i2c_wait_for_core_idle(struct altr_i2c_dev *idev)
>>> +{
>>> + unsigned long timeout = jiffies +
>>> msecs_to_jiffies(ALTR_I2C_TIMEOUT);
>>> +
>>> + do {
>>> + if (time_after(jiffies, timeout)) {
>>> + dev_err(idev->dev, "Core Idle timeout\n");
>>> + return -ETIMEDOUT;
>>> + }
>>> + } while (readl(idev->base + ALTR_I2C_STATUS) &
>>> ALTR_I2C_STAT_CORE);
>>> +
>>> + return 0;
>>> +}
>> readl_poll_timeout[_atomic]() please.
You ignored some of my comments including this one. Why? Can you go
again through my rreview and answer the rest?
>>> + if (of_property_read_u32(np, "fifo-size", &idev->fifo_size))
>>> + idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;
>> Shouldn't be possible to auto detect?
>>
> I agree. That would have been SO much better but the hardware designers
> released this without capturing the size in a register - they come from a
> bare-metal project perspective. Since the FIFO size is configurable in the
> FPGA from 4 to 256 levels deep, I need to capture this with the device tree.
You may do it manually, right? There are examples for similar cases
like writing the offset into FIFO until it returns the written value
and FIFO maximum possible size is not achieved.
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Thor Thayer <thor.thayer@linux.intel.com> |
|---|---|
| Date | 2017-07-13 00:40 +0200 |
| Message-ID | <u2ysy-5Ke-7@gated-at.bofh.it> |
| In reply to | #1683657 |
Hi Andy,
On 07/08/2017 04:41 PM, Andy Shevchenko wrote:
> On Sat, Jul 8, 2017 at 12:08 AM, Thor Thayer
> <thor.thayer@linux.intel.com> wrote:
>> On 07/07/2017 11:25 AM, Andy Shevchenko wrote:
>>> On Mon, Jun 19, 2017 at 11:36 PM, <thor.thayer@linux.intel.com> wrote:
>
>>>> + while (bytes_to_transfer-- > 0) {
>>>> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
>>>> + if (idev->msg_len == 1)
>>>> + altr_i2c_stop(idev);
>>>> + else
>>>> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
>>>> +
>>>> + idev->msg_len--;
>>>> + }
>>>
>>>
>>> Move out invariant from the loop (and I see a bug, you might go over
>>> boundaries).
>>>
>>> while (bytes_to_transfer-- > 0) {
>>> *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
>>> if (idev->msg_len-- == 1)
>>> break;
>>> writel(0, idev->base + ALTR_I2C_TFR_CMD);
>>> }
>>>
>>> altr_i2c_stop(idev);
>>>
>> I see your point on the boundary.
>
> Actually I didn't notice min() call above. So, you may ignore that part.
>
>> However your change is slightly different
>> from what I'm trying to do.
>
> I don't see how it differs to what you wrote.
The stop condition is sent after every FIFO read but I only want the
stop condition when the entire message has been sent (at msg_len == 1)
which will be the last byte.
>
>> I think you assumed the alt_i2c_stop() call can cause a stop condition. This
>> soft IP can't send just a start or a stop condition by itself - both of
>> these conditions need to be paired with a byte.
>
> OK.
>
>> The other subtle side effect is the start condition + byte write is the
>> first write which is why the last write is skipped.
>
>> I need to send a byte with a stop condition on the last expected byte
>> (idev->msg_len == 1) while this change would send it after the FIFO is empty
>> or after (msg_len == 1).
>
> Consider the corner case when msg_len _is_ 1 at the beginning.
> Then continue with 2. I really didn't see the difference in two
> snippets. Perhaps you have a bug there.
I see your point about a bug. I'm sending an extra byte. I'll fix it in
the next revision. Thanks.
>
>> Your version is cleaner so I'll just add the alt_i2c_stop(idev) call inside
>> the (msg_len == 1) condition and before the break.
>
>>>> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
>>>> +{
>>>> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
>>>> +
>>>> ALTR_I2C_TC_FIFO_LVL);
>>>> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
>>>> + int ret = idev->msg_len - bytes_to_transfer;
>>>> +
>>>> + while (bytes_to_transfer-- > 0) {
>>>> + if (idev->msg_len == 1)
>>>> + writel(ALTR_I2C_TFR_CMD_STO | *idev->buf++,
>>>> + idev->base + ALTR_I2C_TFR_CMD);
>>>> + else
>>>> + writel(*idev->buf++, idev->base +
>>>> ALTR_I2C_TFR_CMD);
>>>> + idev->msg_len--;
>>>> + }
>>>
>>>
>>> Ditto.
>>>
>> See above but I will move the msg_len-- inside the condition check like you
>> had.
>
> Ditto.
I'll check this one as well.
>
>>>> +static int altr_i2c_wait_for_core_idle(struct altr_i2c_dev *idev)
>>>> +{
>>>> + unsigned long timeout = jiffies +
>>>> msecs_to_jiffies(ALTR_I2C_TIMEOUT);
>>>> +
>>>> + do {
>>>> + if (time_after(jiffies, timeout)) {
>>>> + dev_err(idev->dev, "Core Idle timeout\n");
>>>> + return -ETIMEDOUT;
>>>> + }
>>>> + } while (readl(idev->base + ALTR_I2C_STATUS) &
>>>> ALTR_I2C_STAT_CORE);
>>>> +
>>>> + return 0;
>>>> +}
>
>>> readl_poll_timeout[_atomic]() please.
>
> You ignored some of my comments including this one. Why? Can you go
> again through my rreview and answer the rest?
I wasn't aware of this function but it simplifies the code. I'll add it.
Thanks.
I addressed most of your comments. In some cases, I grouped the comments
together - for instance in the msg[], the comments addressed the same
function.
>
>>>> + if (of_property_read_u32(np, "fifo-size", &idev->fifo_size))
>>>> + idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;
>
>>> Shouldn't be possible to auto detect?
>>>
>> I agree. That would have been SO much better but the hardware designers
>> released this without capturing the size in a register - they come from a
>> bare-metal project perspective. Since the FIFO size is configurable in the
>> FPGA from 4 to 256 levels deep, I need to capture this with the device tree.
>
> You may do it manually, right? There are examples for similar cases
> like writing the offset into FIFO until it returns the written value
> and FIFO maximum possible size is not achieved.
>
While I agree that some FIFOs could be discovered that way, data pushed
into this FIFO will be transmitted which isn't good.
Reading from the device tree is quite clean. Additionally, there is a
precedence for the "fifo-size" being read from the device tree in
i2c-at91.c.
I have an addition question about a comment that isn't in this reply so
I'm copying it here to keep the thread clean.
<snip> Copying my previous reply
>>> +static u32 altr_i2c_func(struct i2c_adapter *adap)
>>> +{
>>> + return I2C_FUNC_I2C;
>>> +}
>>
>> Useless. Use value in place.
>
>Got it. Thanks!
After looking at this, I'm not clear what you mean. The
i2c_algorithm.functionality parameter requires a function pointer which
is why the altr_i2c_func() is assigned to that parameter as shown below.
Am I missing something?
+
+static const struct i2c_algorithm altr_i2c_algo = {
+ .master_xfer = altr_i2c_xfer,
+ .functionality = altr_i2c_func,
+};
</snip>
Thanks again for reviewing and for the helpful comments.
Thor
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web