Re: [PATCH 05/15] media: staging/imx7: add MIPI CSI-2 receiver subdev for i.MX7

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dan,
On Thu 19 Apr 2018 at 12:45, Dan Carpenter wrote:
+static int mipi_csis_clk_get(struct csi_state *state)
+{
+	struct device *dev = &state->pdev->dev;
+	int ret = true;

Better to leave ret unitialized.

ack.


+
+	state->mipi_clk = devm_clk_get(dev, "mipi");
+	if (IS_ERR(state->mipi_clk)) {
+		dev_err(dev, "Could not get mipi csi clock\n");
+		return -ENODEV;
+	}
+
+	state->phy_clk = devm_clk_get(dev, "phy");
+	if (IS_ERR(state->phy_clk)) {
+		dev_err(dev, "Could not get mipi phy clock\n");
+		return -ENODEV;
+	}
+
+	/* Set clock rate */
+	if (state->clk_frequency)
+ ret = clk_set_rate(state->mipi_clk, state->clk_frequency);
+	else
+		dev_warn(dev, "No clock frequency specified!\n");
+	if (ret < 0) {
+ dev_err(dev, "set rate=%d failed: %d\n", state->clk_frequency,
+			ret);
+		return -EINVAL;

Preserve the error code.

agree.


+	}
+
+	return ret;

This could be "return 0;" (let's not return true). It might be nicer
like:

	if (!state->clk_frequency) {

looking back again to my code ;), this can never happen, because if clock-frequency is not given by dts I give it a default value. So, this
error path will never happen. I will take this in account in v2.

		dev_warn(dev, "No clock frequency specified!\n");
		return 0;
	}

	ret = clk_set_rate(state->mipi_clk, state->clk_frequency);
	if (ret < 0)
dev_err(dev, "set rate=%d failed: %d\n", state->clk_frequency,
			ret);

	return ret;


+}
+

[ snip ]

+static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
+{
+	struct csi_state *state = dev_id;
+	unsigned long flags;
+	u32 status;
+	int i;
+
+	status = mipi_csis_read(state, MIPI_CSIS_INTSRC);
+
+	spin_lock_irqsave(&state->slock, flags);
+
+	/* Update the event/error counters */
+	if ((status & MIPI_CSIS_INTSRC_ERRORS) || 1) {
                                                 ^^^
Was this supposed to make it into the published code?

No... ;). Only for my debug purpose... Good catch.

---
Cheers,
	Rui

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux