On Tue, Mar 6, 2018 at 6:18 PM, Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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
