Hi Manivannan,

On Thu, Oct 03, 2019 at 11:03:38AM +0530, Manivannan Sadhasivam wrote:
....

quoted

quoted

quoted

quoted

+static int imx290_set_gain(struct imx290 *imx290, u32 value)
+{
+	int ret;
+
+	u32 adjusted_value = (value * 10) / 3;

What's the purpose of this? Why not to use the value directly?

The gain register accepts the value 10/3 of the actual gain required. Hence,
we need to manually do the calculation before updating the value. I can
add a comment here to clarify.

It's better to use the register value directly. Otherwise the granularity
won't be available to the user space.

The sensor datasheet clearly defines that the 10/3'rd of the expected gain
should be set to this register. So, IMO we should be setting the value as

The unit of that gain is decibels, but the controls do not have a unit.
Register value is really preferred here.

mentioned in the datasheet. I agree that we are missing the userspace
granularity here but sticking to the device limitation shouldn't be a problem.
As I said, I'll add a comment here to clarify.

The comment isn't visible in the uAPI.

quoted

quoted

quoted

quoted

+
+	ret = imx290_write_buffered_reg(imx290, IMX290_GAIN, 1, adjusted_value);
+	if (ret)
+		dev_err(imx290->dev, "Unable to write gain\n");
+
+	return ret;
+}
+
+static int imx290_set_power_on(struct imx290 *imx290)
+{
+	int ret;
+
+	ret = clk_prepare_enable(imx290->xclk);

Please move the code from this function to the runtime PM runtime suspend
callback. The same for imx290_set_power_off().

May I know why? I think since this is being used only once, you're suggesting
to move to the callback function itself but please see the comment below. I
will reuse this function to power on the device during probe.

Yes, you can call the same function from probe, even if it's used as a
runtime PM callback.

There's no need to have a function that acts as a wrapper for calling it
with a different type of an argument.

You mean directly calling imx290_runtime_resume() from probe is fine?

Yes. Feel free to call it e.g. imx290_power_on or something.

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel