Re: [PATCH v2 3/3] [media] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Tomasz Figa <tfiga@chromium.org>]

On Mon, Jun 26, 2017 at 11:51 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Mon, Jun 12, 2017 at 06:59:18PM +0900, Tomasz Figa wrote:
>
>>
>> > +       if (WARN_ON(freq <= 0))
>> > +               return -EINVAL;
>>
>> It generally doesn't make sense for the frequency to be negative, so
>> maybe the argument should have been unsigned to start with? (And
>> 32-bit if we don't expect frequencies higher than 4 GHz anyway.)
>
> The value comes from a 64-bit integer V4L2 control so that implies the value
> range of s64 as well.

Okay, if there is no way to enforce this at control level, then I
guess we have to keep this here.

>
>>
>> > +
>> > +       /* b could be 0, -2 or -8, so r < 500000000 */
>>
>> Definitely. Anything <= 0 is also less than 500000000. Let's take a
>> look at the computation below again:
>>
>> 1) accinv is multiplied by b,
>> 2) 500000000 is divided by 256 (=== shift right by 8 bits) = 1953125,
>> 3) accinv*b is multiplied by 1953125 to form the value of r.
>>
>> Now let's see at possible maximum absolute values for particular steps:
>> 1) 16 * -8 = -128 (signed 8 bits),
>> 2) 1953125 (unsigned 21 bits),
>> 3) -128 * 1953125 = -249999872 (signed 29 bits).
>>
>> So I think the important thing to note in the comment is:
>>
>> /* b could be 0, -2 or -8, so |accinv * b| is always less than (1 <<
>> ds) and thus |r| < 500000000. */
>>
>> > +       r = accinv * b * (500000000 >> ds);
>>
>> On the other hand, you lose some precision here. If you used s64
>> instead and did the divide shift at the end ((accinv * b * 500000000)
>> >> ds), for the example above you would get -250007629. (Depending on
>> how big freq is, it might not matter, though.)
>>
>
> The frequency is typically hundreds of mega-Hertz.

I think it still would make sense to have the calculation a bit more precise.

>
>> Also nit: What is 500000000? We have local constants defined above, I
>> think it could also make sense to do the same for this one. The
>> compiler should do constant propagation and simplify respective
>> calculations anyway.
>
> COUNT_ACC in the formula in the comment a few decalines above is in
> nanoseconds. Performing the calculations in integer arithmetics results in
> having 500000000 in the resulting formula.
>
> So this is actually a constant related to the hardware but it does not have
> a pre-determined name because it is derived from COUNT_ACC.

Which, I believe, doesn't stop us from naming it.

>> > +static int cio2_v4l2_querycap(struct file *file, void *fh,
>> > +                             struct v4l2_capability *cap)
>> > +{
>> > +       struct cio2_device *cio2 = video_drvdata(file);
>> > +
>> > +       strlcpy(cap->driver, CIO2_NAME, sizeof(cap->driver));
>> > +       strlcpy(cap->card, CIO2_DEVICE_NAME, sizeof(cap->card));
>> > +       snprintf(cap->bus_info, sizeof(cap->bus_info),
>> > +                "PCI:%s", pci_name(cio2->pci_dev));
>> > +       cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>>
>> Hmm, I thought single plane queue type was deprecated these days and
>> _MPLANE recommended for all new drivers. I'll defer this to other
>> reviewers, though.
>
> If the device supports single plane formats only, I don't see a reason to
> use MPLANE buffer types.

On the other hand, if a further new revision of the hardware (or
amendment of supported feature set of current hardware) actually adds
support for multiple planes, changing it to MPLANE will require
keeping a non-MPLANE variant of the code, due to userspace
compatibility concerns...

Best regards,
Tomasz