RE: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Yeh, Andy]

Okay. All comments are addressed in v7.  
(https://patchwork.linuxtv.org/patch/47869/)

Thanks for all the comments and suggestions.

Regards, Andy

-Original Message-
From: Tomasz Figa [mailto:tf...@chromium.org] 
Sent: Friday, March 2, 2018 11:44 PM
To: Yeh, Andy <andy@intel.com>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus 
<sakari.ai...@linux.intel.com>; Chen, JasonX Z <jasonx.z.c...@intel.com>; 
Chiang, AlanX <alanx.chi...@intel.com>
Subject: Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

Hi Andy,

Thanks for the patch. Let me post some comments inline.

On Fri, Mar 2, 2018 at 11:55 PM, Andy Yeh <andy@intel.com> wrote:
> Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
> Signed-off-by: Jason Chen <jasonx.z.c...@intel.com>
> Signed-off-by: Alan Chiang <alanx.chi...@intel.com>
> ---
> since v2:
> -- Update the streaming function to remove SW_STANDBY in the beginning.
> -- Adjust the delay time from 1ms to 12ms before set stream-on register.
> since v3:
> -- fix the sd.entity to make code be compiled on the mainline kernel.
> since v4:
> -- Enabled AG, DG, and Exposure time control correctly.
> since v5:
> -- Sensor vendor provided a new setting to fix different CLK issue
> -- Add one more resolution for 1048x780, used for VGA streaming

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Sakari Ailus]

On Tue, Mar 06, 2018 at 06:52:16PM +0900, Tomasz Figa wrote:
> On Tue, Mar 6, 2018 at 6:46 PM, Sakari Ailus
>  wrote:
> > On Tue, Mar 06, 2018 at 06:28:43PM +0900, Tomasz Figa wrote:
> >> On Tue, Mar 6, 2018 at 6:18 PM, Sakari Ailus
> >>  wrote:
> >> > On Tue, Mar 06, 2018 at 05:51:36PM +0900, Tomasz Figa wrote:
> >> >> On Tue, Mar 6, 2018 at 5:40 PM, Sakari Ailus
> >> >>  wrote:
> >> >> > Hi Tomasz and Andy,
> >> >> >
> >> >> > On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
> >> >> > ...
> >> >> >> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> >> >> >> > +{
> >> >> >> > +   struct imx258 *imx258 =
> >> >> >> > +   container_of(ctrl->handler, struct imx258, 
> >> >> >> > ctrl_handler);
> >> >> >> > +   struct i2c_client *client = 
> >> >> >> > v4l2_get_subdevdata(>sd);
> >> >> >> > +   int ret = 0;
> >> >> >> > +   /*
> >> >> >> > +* Applying V4L2 control value only happens
> >> >> >> > +* when power is up for streaming
> >> >> >> > +*/
> >> >> >> > +   if (pm_runtime_get_if_in_use(>dev) <= 0)
> >> >> >> > +   return 0;
> >> >> >>
> >> >> >> I thought we decided to fix this to handle disabled runtime PM 
> >> >> >> properly.
> >> >> >
> >> >> > Good point. I bet this is a problem in a few other drivers, too. How 
> >> >> > would
> >> >> > you fix that? Check for zero only?
> >> >> >
> >> >>
> >> >> bool need_runtime_put;
> >> >>
> >> >> ret = pm_runtime_get_if_in_use(>dev);
> >> >> if (ret <= 0 && ret != -EINVAL)
> >> >> return ret;
> >> >> need_runtime_put = ret > 0;
> >> >>
> >> >> // Do stuff ...
> >> >>
> >> >> if (need_runtime_put)
> >> >>pm_runtime_put(>dev);
> >> >>
> >> >> I don't like how ugly it is, but it appears to be the only way to
> >> >> handle this correctly.
> >> >
> >> > The driver enables runtime PM so if runtime PM is enabled in kernel
> >> > configuration, it is enabled here. In that case 
> >> > pm_runtime_get_if_in_use()
> >> > will return either 0 or 1. So as far as I can see, changing the lines to:
> >> >
> >> > if (!pm_runtime_get_if_in_use(>dev))
> >> > return 0;
> >> >
> >> > is enough.
> >>
> >> Right, my bad. Somehow I was convinced that enable status can change at
> >> runtime.
> >
> > Good point. I guess in principle this could happen although I can't see a
> > reason to do so, other than to break things --- quoting
> > Documentation/power/runtime_pm.txt:
> >
> > The user space can effectively disallow the driver of the device to
> > power manage it at run time by changing the value of its
> > /sys/devices/.../power/control attribute to "on", which causes
> > pm_runtime_forbid() to be called. In principle, this mechanism may
> > also be used by the driver to effectively turn off the runtime
> > power management of the device until the user space turns it on.
> > Namely, during the initialization the driver can make sure that the
> > runtime PM status of the device is 'active' and call
> > pm_runtime_forbid(). It should be noted, however, that if the user
> > space has already intentionally changed the value of
> > /sys/devices/.../power/control to "auto" to allow the driver to
> > power manage the device at run time, the driver may confuse it by
> > using pm_runtime_forbid() this way.
> >
> > So that comes with a warning that things might not work well after doing
> > so.
> >
> > What comes to the driver code, I still wouldn't complicate it by attempting
> > to make a driver work in such a case.
> 
> I think pm_runtime_forbid() and pm_runtime_enable() operate on
> complete different data. That was exactly the source of my confusion
> earlier. Looking at the code [1], even if runtime PM is "forbidden",
> it is still "enabled" and just the usage count is incremented.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L1314

Ah, right. Thanks for the correction. Then indeed this is very clear.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Tomasz Figa]

On Tue, Mar 6, 2018 at 6:46 PM, Sakari Ailus
 wrote:
> On Tue, Mar 06, 2018 at 06:28:43PM +0900, Tomasz Figa wrote:
>> On Tue, Mar 6, 2018 at 6:18 PM, Sakari Ailus
>>  wrote:
>> > On Tue, Mar 06, 2018 at 05:51:36PM +0900, Tomasz Figa wrote:
>> >> On Tue, Mar 6, 2018 at 5:40 PM, Sakari Ailus
>> >>  wrote:
>> >> > Hi Tomasz and Andy,
>> >> >
>> >> > On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
>> >> > ...
>> >> >> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>> >> >> > +{
>> >> >> > +   struct imx258 *imx258 =
>> >> >> > +   container_of(ctrl->handler, struct imx258, 
>> >> >> > ctrl_handler);
>> >> >> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> >> >> > +   int ret = 0;
>> >> >> > +   /*
>> >> >> > +* Applying V4L2 control value only happens
>> >> >> > +* when power is up for streaming
>> >> >> > +*/
>> >> >> > +   if (pm_runtime_get_if_in_use(>dev) <= 0)
>> >> >> > +   return 0;
>> >> >>
>> >> >> I thought we decided to fix this to handle disabled runtime PM 
>> >> >> properly.
>> >> >
>> >> > Good point. I bet this is a problem in a few other drivers, too. How 
>> >> > would
>> >> > you fix that? Check for zero only?
>> >> >
>> >>
>> >> bool need_runtime_put;
>> >>
>> >> ret = pm_runtime_get_if_in_use(>dev);
>> >> if (ret <= 0 && ret != -EINVAL)
>> >> return ret;
>> >> need_runtime_put = ret > 0;
>> >>
>> >> // Do stuff ...
>> >>
>> >> if (need_runtime_put)
>> >>pm_runtime_put(>dev);
>> >>
>> >> I don't like how ugly it is, but it appears to be the only way to
>> >> handle this correctly.
>> >
>> > The driver enables runtime PM so if runtime PM is enabled in kernel
>> > configuration, it is enabled here. In that case pm_runtime_get_if_in_use()
>> > will return either 0 or 1. So as far as I can see, changing the lines to:
>> >
>> > if (!pm_runtime_get_if_in_use(>dev))
>> > return 0;
>> >
>> > is enough.
>>
>> Right, my bad. Somehow I was convinced that enable status can change at
>> runtime.
>
> Good point. I guess in principle this could happen although I can't see a
> reason to do so, other than to break things --- quoting
> Documentation/power/runtime_pm.txt:
>
> The user space can effectively disallow the driver of the device to
> power manage it at run time by changing the value of its
> /sys/devices/.../power/control attribute to "on", which causes
> pm_runtime_forbid() to be called. In principle, this mechanism may
> also be used by the driver to effectively turn off the runtime
> power management of the device until the user space turns it on.
> Namely, during the initialization the driver can make sure that the
> runtime PM status of the device is 'active' and call
> pm_runtime_forbid(). It should be noted, however, that if the user
> space has already intentionally changed the value of
> /sys/devices/.../power/control to "auto" to allow the driver to
> power manage the device at run time, the driver may confuse it by
> using pm_runtime_forbid() this way.
>
> So that comes with a warning that things might not work well after doing
> so.
>
> What comes to the driver code, I still wouldn't complicate it by attempting
> to make a driver work in such a case.

I think pm_runtime_forbid() and pm_runtime_enable() operate on
complete different data. That was exactly the source of my confusion
earlier. Looking at the code [1], even if runtime PM is "forbidden",
it is still "enabled" and just the usage count is incremented.

https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L1314

Best regards,
Tomasz

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Sakari Ailus]

On Tue, Mar 06, 2018 at 06:28:43PM +0900, Tomasz Figa wrote:
> On Tue, Mar 6, 2018 at 6:18 PM, Sakari Ailus
>  wrote:
> > On Tue, Mar 06, 2018 at 05:51:36PM +0900, Tomasz Figa wrote:
> >> On Tue, Mar 6, 2018 at 5:40 PM, Sakari Ailus
> >>  wrote:
> >> > Hi Tomasz and Andy,
> >> >
> >> > On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
> >> > ...
> >> >> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> >> >> > +{
> >> >> > +   struct imx258 *imx258 =
> >> >> > +   container_of(ctrl->handler, struct imx258, 
> >> >> > ctrl_handler);
> >> >> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> >> >> > +   int ret = 0;
> >> >> > +   /*
> >> >> > +* Applying V4L2 control value only happens
> >> >> > +* when power is up for streaming
> >> >> > +*/
> >> >> > +   if (pm_runtime_get_if_in_use(>dev) <= 0)
> >> >> > +   return 0;
> >> >>
> >> >> I thought we decided to fix this to handle disabled runtime PM properly.
> >> >
> >> > Good point. I bet this is a problem in a few other drivers, too. How 
> >> > would
> >> > you fix that? Check for zero only?
> >> >
> >>
> >> bool need_runtime_put;
> >>
> >> ret = pm_runtime_get_if_in_use(>dev);
> >> if (ret <= 0 && ret != -EINVAL)
> >> return ret;
> >> need_runtime_put = ret > 0;
> >>
> >> // Do stuff ...
> >>
> >> if (need_runtime_put)
> >>pm_runtime_put(>dev);
> >>
> >> I don't like how ugly it is, but it appears to be the only way to
> >> handle this correctly.
> >
> > The driver enables runtime PM so if runtime PM is enabled in kernel
> > configuration, it is enabled here. In that case pm_runtime_get_if_in_use()
> > will return either 0 or 1. So as far as I can see, changing the lines to:
> >
> > if (!pm_runtime_get_if_in_use(>dev))
> > return 0;
> >
> > is enough.
> 
> Right, my bad. Somehow I was convinced that enable status can change at
> runtime.

Good point. I guess in principle this could happen although I can't see a
reason to do so, other than to break things --- quoting
Documentation/power/runtime_pm.txt:

The user space can effectively disallow the driver of the device to
power manage it at run time by changing the value of its
/sys/devices/.../power/control attribute to "on", which causes
pm_runtime_forbid() to be called. In principle, this mechanism may
also be used by the driver to effectively turn off the runtime
power management of the device until the user space turns it on.
Namely, during the initialization the driver can make sure that the
runtime PM status of the device is 'active' and call
pm_runtime_forbid(). It should be noted, however, that if the user
space has already intentionally changed the value of
/sys/devices/.../power/control to "auto" to allow the driver to
power manage the device at run time, the driver may confuse it by
using pm_runtime_forbid() this way.

So that comes with a warning that things might not work well after doing
so.

What comes to the driver code, I still wouldn't complicate it by attempting
to make a driver work in such a case.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Tomasz Figa]

On Tue, Mar 6, 2018 at 6:18 PM, Sakari Ailus
 wrote:
> On Tue, Mar 06, 2018 at 05:51:36PM +0900, Tomasz Figa wrote:
>> On Tue, Mar 6, 2018 at 5:40 PM, Sakari Ailus
>>  wrote:
>> > Hi Tomasz and Andy,
>> >
>> > On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
>> > ...
>> >> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>> >> > +{
>> >> > +   struct imx258 *imx258 =
>> >> > +   container_of(ctrl->handler, struct imx258, 
>> >> > ctrl_handler);
>> >> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> >> > +   int ret = 0;
>> >> > +   /*
>> >> > +* Applying V4L2 control value only happens
>> >> > +* when power is up for streaming
>> >> > +*/
>> >> > +   if (pm_runtime_get_if_in_use(>dev) <= 0)
>> >> > +   return 0;
>> >>
>> >> I thought we decided to fix this to handle disabled runtime PM properly.
>> >
>> > Good point. I bet this is a problem in a few other drivers, too. How would
>> > you fix that? Check for zero only?
>> >
>>
>> bool need_runtime_put;
>>
>> ret = pm_runtime_get_if_in_use(>dev);
>> if (ret <= 0 && ret != -EINVAL)
>> return ret;
>> need_runtime_put = ret > 0;
>>
>> // Do stuff ...
>>
>> if (need_runtime_put)
>>pm_runtime_put(>dev);
>>
>> I don't like how ugly it is, but it appears to be the only way to
>> handle this correctly.
>
> The driver enables runtime PM so if runtime PM is enabled in kernel
> configuration, it is enabled here. In that case pm_runtime_get_if_in_use()
> will return either 0 or 1. So as far as I can see, changing the lines to:
>
> if (!pm_runtime_get_if_in_use(>dev))
> return 0;
>
> is enough.

Right, my bad. Somehow I was convinced that enable status can change at runtime.

>
>> >> > +   ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
>> >> > +   if (ret)
>> >>
>> >> Missing error message.
>
> Same for this actually, printing an error message here isn't useful. It'd
> be just waiting for someone to clean it up. :-)

Fair enough.

>
>> >>
>> >> > +   return ret;
>> >> > +
>> >> > +   mutex_init(>mutex);
>> >> > +   ctrl_hdlr->lock = >mutex;
>> >> > +   imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> >> > +   _ctrl_ops,
>> >> > +   V4L2_CID_LINK_FREQ,
>> >> > +   ARRAY_SIZE(link_freq_menu_items) - 1,
>> >> > +   0,
>> >> > +   link_freq_menu_items);
>> >> > +
>> >> > +   if (!imx258->link_freq) {
>> >> > +   ret = -EINVAL;
>> >>
>> >> Missing error message.
>> >
>> > I wouldn't add an error message here. Typically you'd need that information
>> > at development time only, never after that. v4l2_ctrl_new_int_menu(), as
>> > other control framework functions creating new controls, can fail due to
>> > memory allocation failure (which is already vocally reported) or due to bad
>> > parameters (that are constants).
>> >
>> > I'd rather do:
>> >
>> > if (!imx258->link_freq)
>> > ... |= ...;
>> >
>> > It simplifies error handling and removes the need for goto.
>>
>> Hmm, I'm not sure I understand your suggestion. Do you perhaps mean
>>
>> if (imx258->link_freq) {
>> // Do stuff that dereferences imx258->link_freq
>> }
>>
>> // ...
>>
>> if (ctrl_hdlr->error) {
>> // Check for error only here, at the end of control initialization.
>> }
>>
>> then it would be better indeed.
>
> Yes, indeed.

SGTM.

Best regards,
Tomasz

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Sakari Ailus]

On Tue, Mar 06, 2018 at 05:51:36PM +0900, Tomasz Figa wrote:
> On Tue, Mar 6, 2018 at 5:40 PM, Sakari Ailus
>  wrote:
> > Hi Tomasz and Andy,
> >
> > On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
> > ...
> >> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> >> > +{
> >> > +   struct imx258 *imx258 =
> >> > +   container_of(ctrl->handler, struct imx258, ctrl_handler);
> >> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> >> > +   int ret = 0;
> >> > +   /*
> >> > +* Applying V4L2 control value only happens
> >> > +* when power is up for streaming
> >> > +*/
> >> > +   if (pm_runtime_get_if_in_use(>dev) <= 0)
> >> > +   return 0;
> >>
> >> I thought we decided to fix this to handle disabled runtime PM properly.
> >
> > Good point. I bet this is a problem in a few other drivers, too. How would
> > you fix that? Check for zero only?
> >
> 
> bool need_runtime_put;
> 
> ret = pm_runtime_get_if_in_use(>dev);
> if (ret <= 0 && ret != -EINVAL)
> return ret;
> need_runtime_put = ret > 0;
> 
> // Do stuff ...
> 
> if (need_runtime_put)
>pm_runtime_put(>dev);
> 
> I don't like how ugly it is, but it appears to be the only way to
> handle this correctly.

The driver enables runtime PM so if runtime PM is enabled in kernel
configuration, it is enabled here. In that case pm_runtime_get_if_in_use()
will return either 0 or 1. So as far as I can see, changing the lines to:

if (!pm_runtime_get_if_in_use(>dev))
return 0;

is enough.

> >> > +   ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> >> > +   if (ret)
> >>
> >> Missing error message.

Same for this actually, printing an error message here isn't useful. It'd
be just waiting for someone to clean it up. :-)

> >>
> >> > +   return ret;
> >> > +
> >> > +   mutex_init(>mutex);
> >> > +   ctrl_hdlr->lock = >mutex;
> >> > +   imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >> > +   _ctrl_ops,
> >> > +   V4L2_CID_LINK_FREQ,
> >> > +   ARRAY_SIZE(link_freq_menu_items) - 1,
> >> > +   0,
> >> > +   link_freq_menu_items);
> >> > +
> >> > +   if (!imx258->link_freq) {
> >> > +   ret = -EINVAL;
> >>
> >> Missing error message.
> >
> > I wouldn't add an error message here. Typically you'd need that information
> > at development time only, never after that. v4l2_ctrl_new_int_menu(), as
> > other control framework functions creating new controls, can fail due to
> > memory allocation failure (which is already vocally reported) or due to bad
> > parameters (that are constants).
> >
> > I'd rather do:
> >
> > if (!imx258->link_freq)
> > ... |= ...;
> >
> > It simplifies error handling and removes the need for goto.
> 
> Hmm, I'm not sure I understand your suggestion. Do you perhaps mean
> 
> if (imx258->link_freq) {
> // Do stuff that dereferences imx258->link_freq
> }
> 
> // ...
> 
> if (ctrl_hdlr->error) {
> // Check for error only here, at the end of control initialization.
> }
> 
> then it would be better indeed.

Yes, indeed.

-- 
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Tomasz Figa]

On Tue, Mar 6, 2018 at 5:40 PM, Sakari Ailus
 wrote:
> Hi Tomasz and Andy,
>
> On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
> ...
>> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>> > +{
>> > +   struct imx258 *imx258 =
>> > +   container_of(ctrl->handler, struct imx258, ctrl_handler);
>> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> > +   int ret = 0;
>> > +   /*
>> > +* Applying V4L2 control value only happens
>> > +* when power is up for streaming
>> > +*/
>> > +   if (pm_runtime_get_if_in_use(>dev) <= 0)
>> > +   return 0;
>>
>> I thought we decided to fix this to handle disabled runtime PM properly.
>
> Good point. I bet this is a problem in a few other drivers, too. How would
> you fix that? Check for zero only?
>

bool need_runtime_put;

ret = pm_runtime_get_if_in_use(>dev);
if (ret <= 0 && ret != -EINVAL)
return ret;
need_runtime_put = ret > 0;

// Do stuff ...

if (need_runtime_put)
   pm_runtime_put(>dev);

I don't like how ugly it is, but it appears to be the only way to
handle this correctly.

> ...
>
>> [snip]
>> > +/* Initialize control handlers */
>> > +static int imx258_init_controls(struct imx258 *imx258)
>> > +{
>> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> > +   struct v4l2_ctrl_handler *ctrl_hdlr;
>> > +   s64 exposure_max;
>> > +   s64 vblank_def;
>> > +   s64 vblank_min;
>> > +   s64 pixel_rate_min;
>> > +   s64 pixel_rate_max;
>> > +   int ret;
>> > +
>> > +   ctrl_hdlr = >ctrl_handler;
>> > +   ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
>> > +   if (ret)
>>
>> Missing error message.
>>
>> > +   return ret;
>> > +
>> > +   mutex_init(>mutex);
>> > +   ctrl_hdlr->lock = >mutex;
>> > +   imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> > +   _ctrl_ops,
>> > +   V4L2_CID_LINK_FREQ,
>> > +   ARRAY_SIZE(link_freq_menu_items) - 1,
>> > +   0,
>> > +   link_freq_menu_items);
>> > +
>> > +   if (!imx258->link_freq) {
>> > +   ret = -EINVAL;
>>
>> Missing error message.
>
> I wouldn't add an error message here. Typically you'd need that information
> at development time only, never after that. v4l2_ctrl_new_int_menu(), as
> other control framework functions creating new controls, can fail due to
> memory allocation failure (which is already vocally reported) or due to bad
> parameters (that are constants).
>
> I'd rather do:
>
> if (!imx258->link_freq)
> ... |= ...;
>
> It simplifies error handling and removes the need for goto.

Hmm, I'm not sure I understand your suggestion. Do you perhaps mean

if (imx258->link_freq) {
// Do stuff that dereferences imx258->link_freq
}

// ...

if (ctrl_hdlr->error) {
// Check for error only here, at the end of control initialization.
}

then it would be better indeed.

>
>>
>> > +   goto error;
>> > +   }
>> > +
>> > +   imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> > +
>> > +   pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
>> > +   pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
>> > +   /* By default, PIXEL_RATE is read only */
>> > +   imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> > +   V4L2_CID_PIXEL_RATE,
>> > +   pixel_rate_min, pixel_rate_max,
>> > +   1, pixel_rate_max);
>> > +
>> > +
>> > +   vblank_def = imx258->cur_mode->vts_def - imx258->cur_mode->height;
>> > +   vblank_min = imx258->cur_mode->vts_min - imx258->cur_mode->height;
>> > +   imx258->vblank = v4l2_ctrl_new_std(
>> > +   ctrl_hdlr, _ctrl_ops, 
>> > V4L2_CID_VBLANK,
>> > +   vblank_min,
>> > +   IMX258_VTS_MAX - imx258->cur_mode->height, 
>> > 1,
>> > +   vblank_def);
>> > +
>> > +   imx258->hblank = v4l2_ctrl_new_std(
>> > +   ctrl_hdlr, _ctrl_ops, 
>> > V4L2_CID_HBLANK,
>> > +   IMX258_PPL_650MHZ - 
>> > imx258->cur_mode->width,
>> > +   IMX258_PPL_650MHZ - 
>> > imx258->cur_mode->width,
>> > +   1,
>> > +   IMX258_PPL_650MHZ - 
>> > imx258->cur_mode->width);
>> > +
>> > +   if (!imx258->hblank) {
>> > +   ret = -EINVAL;
>> > +   goto error;
>> > +   }
>>
>> Why checking hblank, but not other controls? I think in this case just
>> the general check for ctrl_hdlr->error should be enough.
>
> There's no need to access most other controls (except blank and 

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Sakari Ailus]

Hi Tomasz and Andy,

On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
...
> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +   struct imx258 *imx258 =
> > +   container_of(ctrl->handler, struct imx258, ctrl_handler);
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   int ret = 0;
> > +   /*
> > +* Applying V4L2 control value only happens
> > +* when power is up for streaming
> > +*/
> > +   if (pm_runtime_get_if_in_use(>dev) <= 0)
> > +   return 0;
> 
> I thought we decided to fix this to handle disabled runtime PM properly.

Good point. I bet this is a problem in a few other drivers, too. How would
you fix that? Check for zero only?

...

> [snip]
> > +/* Initialize control handlers */
> > +static int imx258_init_controls(struct imx258 *imx258)
> > +{
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   struct v4l2_ctrl_handler *ctrl_hdlr;
> > +   s64 exposure_max;
> > +   s64 vblank_def;
> > +   s64 vblank_min;
> > +   s64 pixel_rate_min;
> > +   s64 pixel_rate_max;
> > +   int ret;
> > +
> > +   ctrl_hdlr = >ctrl_handler;
> > +   ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> > +   if (ret)
> 
> Missing error message.
> 
> > +   return ret;
> > +
> > +   mutex_init(>mutex);
> > +   ctrl_hdlr->lock = >mutex;
> > +   imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > +   _ctrl_ops,
> > +   V4L2_CID_LINK_FREQ,
> > +   ARRAY_SIZE(link_freq_menu_items) - 1,
> > +   0,
> > +   link_freq_menu_items);
> > +
> > +   if (!imx258->link_freq) {
> > +   ret = -EINVAL;
> 
> Missing error message.

I wouldn't add an error message here. Typically you'd need that information
at development time only, never after that. v4l2_ctrl_new_int_menu(), as
other control framework functions creating new controls, can fail due to
memory allocation failure (which is already vocally reported) or due to bad
parameters (that are constants).

I'd rather do:

if (!imx258->link_freq)
... |= ...;

It simplifies error handling and removes the need for goto.

> 
> > +   goto error;
> > +   }
> > +
> > +   imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +   pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
> > +   pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
> > +   /* By default, PIXEL_RATE is read only */
> > +   imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> > +   V4L2_CID_PIXEL_RATE,
> > +   pixel_rate_min, pixel_rate_max,
> > +   1, pixel_rate_max);
> > +
> > +
> > +   vblank_def = imx258->cur_mode->vts_def - imx258->cur_mode->height;
> > +   vblank_min = imx258->cur_mode->vts_min - imx258->cur_mode->height;
> > +   imx258->vblank = v4l2_ctrl_new_std(
> > +   ctrl_hdlr, _ctrl_ops, 
> > V4L2_CID_VBLANK,
> > +   vblank_min,
> > +   IMX258_VTS_MAX - imx258->cur_mode->height, 
> > 1,
> > +   vblank_def);
> > +
> > +   imx258->hblank = v4l2_ctrl_new_std(
> > +   ctrl_hdlr, _ctrl_ops, 
> > V4L2_CID_HBLANK,
> > +   IMX258_PPL_650MHZ - imx258->cur_mode->width,
> > +   IMX258_PPL_650MHZ - imx258->cur_mode->width,
> > +   1,
> > +   IMX258_PPL_650MHZ - 
> > imx258->cur_mode->width);
> > +
> > +   if (!imx258->hblank) {
> > +   ret = -EINVAL;
> > +   goto error;
> > +   }
> 
> Why checking hblank, but not other controls? I think in this case just
> the general check for ctrl_hdlr->error should be enough.

There's no need to access most other controls (except blank and link_freq).
The flags field is set for hblank below, therefore the check.

> 
> > +
> > +   imx258->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +   exposure_max = imx258->cur_mode->vts_def - 8;
> > +   imx258->exposure = v4l2_ctrl_new_std(
> > +   ctrl_hdlr, _ctrl_ops,
> > +   V4L2_CID_EXPOSURE, IMX258_EXPOSURE_MIN,
> > +   IMX258_EXPOSURE_MAX, IMX258_EXPOSURE_STEP,
> > +   IMX258_EXPOSURE_DEFAULT);
> > +
> > +   v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops, 
> > V4L2_CID_ANALOGUE_GAIN,
> > +   IMX258_ANA_GAIN_MIN, IMX258_ANA_GAIN_MAX,
> > +   IMX258_ANA_GAIN_STEP, 
> > IMX258_ANA_GAIN_DEFAULT);
> > +
> > +   

Re: [PATCH v6] media: imx258: Add imx258 camera sensor driver

[Tomasz Figa]

Hi Andy,

Thanks for the patch. Let me post some comments inline.

On Fri, Mar 2, 2018 at 11:55 PM, Andy Yeh  wrote:
> Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
> Signed-off-by: Jason Chen 
> Signed-off-by: Alan Chiang 
> ---
> since v2:
> -- Update the streaming function to remove SW_STANDBY in the beginning.
> -- Adjust the delay time from 1ms to 12ms before set stream-on register.
> since v3:
> -- fix the sd.entity to make code be compiled on the mainline kernel.
> since v4:
> -- Enabled AG, DG, and Exposure time control correctly.
> since v5:
> -- Sensor vendor provided a new setting to fix different CLK issue
> -- Add one more resolution for 1048x780, used for VGA streaming
>
>  MAINTAINERS|7 +
>  drivers/media/i2c/Kconfig  |   11 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/imx258.c | 1341 
> 
>  4 files changed, 1360 insertions(+)
>  create mode 100644 drivers/media/i2c/imx258.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a339bb5..9f75510 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12646,6 +12646,13 @@ S: Maintained
>  F: drivers/ssb/
>  F: include/linux/ssb/
>
> +SONY IMX258 SENSOR DRIVER
> +M: Sakari Ailus 
> +L: linux-media@vger.kernel.org
> +T: git git://linuxtv.org/media_tree.git
> +S: Maintained
> +F: drivers/media/i2c/imx258.c
> +
>  SONY IMX274 SENSOR DRIVER
>  M: Leon Luo 
>  L: linux-media@vger.kernel.org
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index fd01842..bcd4bf1 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -565,6 +565,17 @@ config VIDEO_APTINA_PLL
>  config VIDEO_SMIAPP_PLL
> tristate
>
> +config VIDEO_IMX258
> +   tristate "Sony IMX258 sensor support"
> +   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +   depends on MEDIA_CAMERA_SUPPORT
> +   ---help---
> + This is a Video4Linux2 sensor-level driver for the Sony
> + IMX258 camera.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imx258.
> +
>  config VIDEO_IMX274
> tristate "Sony IMX274 sensor support"
> depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 1b62639..4bf7d00 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
>  obj-$(CONFIG_VIDEO_ML86V7667)  += ml86v7667.o
>  obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
>  obj-$(CONFIG_VIDEO_TC358743)   += tc358743.o
> +obj-$(CONFIG_VIDEO_IMX258) += imx258.o
>  obj-$(CONFIG_VIDEO_IMX274) += imx274.o
>
>  obj-$(CONFIG_SDR_MAX2175) += max2175.o
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> new file mode 100644
> index 000..0418edd
> --- /dev/null
> +++ b/drivers/media/i2c/imx258.c
> @@ -0,0 +1,1341 @@
> +// Copyright (C) 2018 Intel Corporation
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define IMX258_REG_VALUE_08BIT 1
> +#define IMX258_REG_VALUE_16BIT 2
> +#define IMX258_REG_VALUE_24BIT 3

Is there any need for these macros? Respective functions could just
take the number of bytes as the argument directly.

> +
> +#define IMX258_REG_MODE_SELECT 0x0100
> +#define IMX258_MODE_STANDBY0x00
> +#define IMX258_MODE_STREAMING  0x01
> +
> +/* Chip ID */
> +#define IMX258_REG_CHIP_ID 0x0016
> +#define IMX258_CHIP_ID 0x0258
> +
> +/* V_TIMING internal */
> +#define IMX258_REG_VTS 0x0340
> +#define IMX258_VTS_30FPS   0x0c98
> +#define IMX258_VTS_MAX 0x
> +
> +/*Frame Length Line*/
> +#define IMX258_FLL_MIN 0x08a6
> +#define IMX258_FLL_MAX 0x
> +#define IMX258_FLL_STEP1
> +#define IMX258_FLL_DEFAULT 0x0c98
> +
> +/* HBLANK control - read only */
> +#define IMX258_PPL_650MHZ  5352
> +#define IMX258_PPL_325MHZ  2676
> +
> +/* Exposure control */
> +#define IMX258_REG_EXPOSURE0x0202
> +#define IMX258_EXPOSURE_MIN4
> +#define IMX258_EXPOSURE_STEP   1
> +#define IMX258_EXPOSURE_DEFAULT0x640
> +#define IMX258_EXPOSURE_MAX65535
> +
> +/* Analog gain control */
> +#define IMX258_REG_ANALOG_GAIN 0x0204
> +#define IMX258_ANA_GAIN_MIN0
> +#define IMX258_ANA_GAIN_MAX0x1fff
> +#define IMX258_ANA_GAIN_STEP   1
> +#define IMX258_ANA_GAIN_DEFAULT