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
