linux-kernel - Re: [PATCH v3 2/2] [media] imx214: Add imx214 camera sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPybu_1B+zAasFuiRrhByXaVGz6KfuBxXJYWRwBM-CL8Bh6C=g@mail.gmail.com>
Date:   Wed, 3 Oct 2018 09:17:56 +0200
From:   Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Hans Verkuil <hans.verkuil@...co.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-media <linux-media@...r.kernel.org>, jacopo@...ndi.org
Subject: Re: [PATCH v3 2/2] [media] imx214: Add imx214 camera sensor driver

HI Sakari

Thanks a lot for your review!

On Tue, Oct 2, 2018 at 6:27 PM Sakari Ailus
<sakari.ailus@...ux.intel.com> wrote:
> > +static int imx214_s_power(struct v4l2_subdev *sd, int on)
> > +{
> > +     struct imx214 *imx214 = to_imx214(sd);
> > +     int ret = 0;
> > +
> > +     on = !!on;
> > +
> > +     if (imx214->power_on == on)
> > +             return 0;
> > +
> > +     if (on)
> > +             ret = imx214_set_power_on(imx214);
> > +     else
> > +             imx214_set_power_off(imx214);
> > +
> > +     imx214->power_on = on;
> > +
> > +     return 0;
>
> Using runtime PM would relieve you of this function.

Tried using runtime PM, but did not manage to get it working with the
qcom i2c driver. Will try again when this is merged on a separated
patch.
>
> > +static int imx214_find_mode(u32 height)
> > +{
> > +     int i;
>
> Unsigned int.
>
> > +
> > +     for (i = 0; (i < ARRAY_SIZE(imx214_modes) - 1) ; i++)
>
> Extra parentheses.
>
> How about using v4l2_find_nearest_size()?
>
> > +static int imx214_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct imx214 *imx214;
> > +     int ret;
> > +     static const s64 link_freq[] = {
> > +             IMX214_DEFAULT_LINK_FREQ,
>
> This should come from firmware.

Thanks for clarifying this on the IRC. The link frequencies should
come from the firmware, but since we only support one and is probed it
is fine to get a static list.

> Kind regards,
>
> Sakari Ailus
> sakari.ailus@...ux.intel.com

Thanks again

--
Ricardo Ribalda