Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

[Felipe Balbi]

Hi,

On Thu, Jul 18, 2013 at 05:15:45PM +0530, Sourav Poddar wrote:
> >>+	list_for_each_entry(t,&m->transfers, transfer_list) {
> >>+		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
> >>+		qspi->cmd |= QSPI_WC_CMD_INT_EN;
> >>+
> >>+		ret = qspi_transfer_msg(qspi, t);
> >>+		if (ret) {
> >>+			dev_dbg(qspi->dev, "transfer message failed\n");
> >>+			return -EINVAL;
> >>+		}
> >>+
> >>+		m->actual_length += t->len;
> >>+
> >>+		if (list_is_last(&t->transfer_list,&m->transfers))
> >>+			goto out;
> >>+	}
> >The use of list_is_last() here is *realy* strange - what's going on
> >there?
> >
> We are checking if there is any transfer left, if no we are signalling the
> flash device about the end of transfer.

I'll quote your diff below because I wanted to show where your 'out'
label lies:

+       list_for_each_entry(t, &m->transfers, transfer_list) {
+               qspi->cmd |= QSPI_WLEN(t->bits_per_word);
+               qspi->cmd |= QSPI_WC_CMD_INT_EN;
+
+               ret = qspi_transfer_msg(qspi, t);
+               if (ret) {
+                       dev_dbg(qspi->dev, "transfer message failed\n");
+                       return -EINVAL;
+               }
+
+               m->actual_length += t->len;
+
+               if (list_is_last(&t->transfer_list, &m->transfers))
+                       goto out;
+       }
+
+out:
+       m->status = status;
+       spi_finalize_current_message(master);

see how it's place right after the closing curly brackets ? In that
case, why do you need it ? list_for_each_entry() will do exactly what it
says: iterate over each entry on the entry. When it reaches the last
one, the loop will end.

This renders your list_is_last() check useless. In fact, you could've
figured that out if you just spent the time to read
list_for_each_entry() carefully.

-- 
balbi

Attachment: signature.asc
Description: Digital signature