Re: [PATCH v3 2/3] media: i2c: Add IMX290 CMOS image sensor driver

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: devicetree@vger.kernel.org, c.barrett@framos.com,
	linux-kernel@vger.kernel.org, a.brela@framos.com,
	robh+dt@kernel.org, mchehab@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v3 2/3] media: i2c: Add IMX290 CMOS image sensor driver
Date: Thu, 03 Oct 2019 12:56:48 +0530	[thread overview]
Message-ID: <3FAB5E91-9FD2-4052-881B-E4B18D44D33B@linaro.org> (raw)
In-Reply-To: <20191003071646.GZ896@valkosipuli.retiisi.org.uk>

Hi Sakari, 

On 3 October 2019 12:46:46 PM IST, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>Hi Manivannan,
>
>On Thu, Oct 03, 2019 at 11:03:38AM +0530, Manivannan Sadhasivam wrote:
>....
>> > > > > +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.
>

Hmm, okay. Will just pass the value directly. 

>> 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.
>

Yes. It would be good to have the units passed onto the userspace somehow like it is done in IIO. Then we don't need to fiddle in the driver for mismatch. Something consider in future... 

>> 
>> > > 
>> > > > > +
>> > > > > +	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.

Okay. 

Thanks, 
Mani

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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