Hi Clement,

On Thu, Jan 22, 2015 at 04:27:39PM +0100, clement.perrochaud@effinnov.com wrote:

From: Clément Perrochaud <redacted>

Signed-off-by: Clément Perrochaud <redacted>
Signed-off-by: Clément Perrochaud <redacted>

Are you sure you want both S-O-B lines ?

+static int nxp_nci_i2c_fw_read(struct nxp_nci_i2c_phy *phy,
+			       struct sk_buff **skb)
+{
+	struct i2c_client *client = phy->i2c_dev;
+	u16 header;
+	size_t frame_len;
+	int r;
+
+	r = i2c_master_recv(client, (u8 *) &header, NXP_NCI_FW_HDR_LEN);
+	if (r < 0) {
+		goto fw_read_exit;
+	} else if (r != NXP_NCI_FW_HDR_LEN) {
+		nfc_err(&client->dev, "Incorrect header length: %u\n", r);
+		r = -EBADMSG;
+		goto fw_read_exit;
+	}
+
+	frame_len = (get_unaligned_be16(&header) & NXP_NCI_FW_FRAME_LEN_MASK) +
+		    NXP_NCI_FW_CRC_LEN;
+
+	*skb = alloc_skb(NXP_NCI_FW_HDR_LEN + frame_len, GFP_KERNEL);
+	if (*skb == NULL) {
+		r = -ENOMEM;
+		goto fw_read_exit;
+	}
+
+	memcpy(skb_put(*skb, NXP_NCI_FW_HDR_LEN), &header, NXP_NCI_FW_HDR_LEN);
+
+	r = i2c_master_recv(client, skb_put(*skb, frame_len), frame_len);
+	if (r != frame_len) {
+		nfc_err(&client->dev,
+			"Invalid frame length: %u (expected %u)\n",
+			r, frame_len);
+		r = -EBADMSG;
+		goto fw_read_exit_free_skb;
+	}
+
+	r = 0;
+	goto fw_read_exit;

return 0; is enough.

+static int nxp_nci_i2c_nci_read(struct nxp_nci_i2c_phy *phy,
+				struct sk_buff **skb)
+{
+	struct nci_ctrl_hdr header; /* May actually be a data header */
+	struct i2c_client *client = phy->i2c_dev;
+	int r;
+
+	r = i2c_master_recv(client, (u8 *) &header, NCI_CTRL_HDR_SIZE);
+	if (r < 0) {
+		goto nci_read_exit;
+	} else if (r != NCI_CTRL_HDR_SIZE) {
+		nfc_err(&client->dev, "Incorrect header length: %u\n", r);
+		r = -EBADMSG;
+		goto nci_read_exit;
+	}
+
+	*skb = alloc_skb(NCI_CTRL_HDR_SIZE + header.plen, GFP_KERNEL);
+	if (*skb == NULL) {
+		r = -ENOMEM;
+		goto nci_read_exit;
+	}
+
+	memcpy(skb_put(*skb, NCI_CTRL_HDR_SIZE), (void *) &header,
+	       NCI_CTRL_HDR_SIZE);
+
+	r = i2c_master_recv(client, skb_put(*skb, header.plen), header.plen);
+	if (r != header.plen) {
+		nfc_err(&client->dev,
+			"Invalid frame payload length: %u (expected %u)\n",
+			r, header.plen);
+		r = -EBADMSG;
+		goto nci_read_exit_free_skb;
+	}
+
+	r = 0;
+	goto nci_read_exit;

Ditto.

+static irqreturn_t nxp_nci_i2c_irq_thread_fn(int irq, void *phy_id)
+{
+	struct nxp_nci_i2c_phy *phy = phy_id;
+	struct i2c_client *client;
+	struct nxp_nci_info *info;
+
+	struct sk_buff *skb = NULL;
+	int r = 0;
+
+	if (!phy || !phy->ndev)
+		goto exit_irq_none;
+
+	client = phy->i2c_dev;
+
+	if (!client || irq != client->irq)
+		goto exit_irq_none;
+
+	if (phy->hard_fault != 0)
+		goto exit_irq_handled;
+
+	info = nci_get_drvdata(phy->ndev);

You probably want to take the info_lock mutex here.

+	switch (info->mode) {
+	case NXP_NCI_MODE_NCI:
+		r = nxp_nci_i2c_nci_read(phy, &skb);
+		break;
+	case NXP_NCI_MODE_FW:
+		r = nxp_nci_i2c_fw_read(phy, &skb);
+		break;
+	case NXP_NCI_MODE_COLD:
+		r = -EREMOTEIO;
+		break;
+	}
+
+	if (r == -EREMOTEIO) {
+		phy->hard_fault = r;
+		skb = NULL;
+	} else if (r < 0) {
+		nfc_err(&client->dev, "Read failed with error %d\n", r);
+		goto exit_irq_handled;
+	}
+
+	switch (info->mode) {
+	case NXP_NCI_MODE_NCI:
+		nci_recv_frame(phy->ndev, skb);
+		break;
+	case NXP_NCI_MODE_FW:
+		nxp_nci_fw_recv_frame(phy->ndev, skb);
+		break;
+	case NXP_NCI_MODE_COLD:
+		break;
+	}
+
+exit_irq_handled:
+	return IRQ_HANDLED;
+exit_irq_none:
+	WARN_ON_ONCE(1);
+	return IRQ_NONE;
+}
+static int nxp_nci_i2c_request_gpios(struct nxp_nci_i2c_phy *phy)
+{
+	int r;
+
+	r = gpio_request(phy->gpio_en, "nxp_nci_en");

Please use the devm_gpiod_* API so that you don't need to free GPIOs
explicitely.

+	if (r < 0)
+		goto err_out;
+
+	r = gpio_direction_output(phy->gpio_en, 0);
+	if (r < 0)
+		goto err_gpio_en;
+
+	r = gpio_request(phy->gpio_fw, "nxp_nci_fw");
+	if (r < 0)
+		goto err_gpio_en;
+
+	r = gpio_direction_output(phy->gpio_fw, 0);
+	if (r < 0)
+		goto err_gpio_fw;
+
+	goto err_out;
+
+err_gpio_fw:
+	gpio_free(phy->gpio_fw);
+err_gpio_en:
+	gpio_free(phy->gpio_en);
+err_out:
+	return r;
+}
+
+static void nxp_nci_i2c_free_gpios(struct nxp_nci_i2c_phy *phy)
+{
+	gpio_free(phy->gpio_fw);
+	gpio_free(phy->gpio_en);
+}

You would no longer need that one with devm_gpiod_*.