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

[Tomasz Figa <tfiga@chromium.org>]

On Tue, Mar 6, 2018 at 6:18 PM, Sakari Ailus
<sakari.ailus@linux.intel.com> 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
>> <sakari.ailus@linux.intel.com> 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(&imx258->sd);
>> >> > +       int ret = 0;
>> >> > +       /*
>> >> > +        * Applying V4L2 control value only happens
>> >> > +        * when power is up for streaming
>> >> > +        */
>> >> > +       if (pm_runtime_get_if_in_use(&client->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(&client->dev);
>> if (ret <= 0 && ret != -EINVAL)
>>         return ret;
>> need_runtime_put = ret > 0;
>>
>> // Do stuff ...
>>
>> if (need_runtime_put)
>>        pm_runtime_put(&client->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(&client->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(&imx258->mutex);
>> >> > +       ctrl_hdlr->lock = &imx258->mutex;
>> >> > +       imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> >> > +                               &imx258_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