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

[Rui Miguel Silva <rmfrfs@gmail.com>]

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