Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
Hi Raj, On Wed, Oct 03, 2018 at 10:56:19PM +, Mani, Rajmohan wrote: ... > > From some comment you had later, > > I guess you're meaning that only 3 or 7 are the valid values. > > > > Yet, you're listing from 2^3 to 2^7, and that's confusing. Perhaps > > you want to say, instead, that the valid values are at the 3..7 range? > > If so, please use something like "values at the [3..7] range". > > > > As Sakari pointed / preferred in the other thread, we will use the format > [3, 7] to represent all integers between 3 and 7, including 3 and 7. Feel free to add a reference to this in the format documentation: https://en.wikipedia.org/wiki/Interval_(mathematics)> I guess the right place would be the top parameter format ReST document. ... > > > > + * All above has precision u0.4, range [0..0xF]. > > > > again, what do you mean by u0.4? > > unsigned integer with 0 bits used for representing whole number, > with 4 least significant bits used to represent the fractional part. You could refer to this: https://en.wikipedia.org/wiki/Q_(number_format)> The ux.y notation is more common in the context of software but I couldn't find any decent document to refer to. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
Hi Raj,
My apologies for the delayed reply.
On Fri, Aug 31, 2018 at 11:39:54PM +, Mani, Rajmohan wrote:
...
> > > +struct ipu3_uapi_af_meta_data {
> > > + __u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] IPU3_ALIGN;
> >
> > Here IPU3_ALIGN is put at the end...
> >
> > > +} __packed;
> > > +
> > > +/**
> > > + * struct ipu3_uapi_af_raw_buffer - AF raw buffer
> > > + *
> > > + * @meta_data: raw buffer _uapi_af_meta_data for auto focus meta
> > data.
> > > + */
> > > +struct ipu3_uapi_af_raw_buffer {
> > > + IPU3_ALIGN struct ipu3_uapi_af_meta_data meta_data;
> >
> > ... and here at the start. Is that due to the difference between an array
> > and a
> > struct?
> >
>
> No.
>
> When preparing uAPI kernelDoc using "make htmldocs",
> the kernel-doc encounters two type of error/warnings
> caused by IPU3_ALIGN.
>
> case 1:
> struct IPU3_ALIGN ipu3_uapi_dummy {
> ...
> } __packed;
>
> "error: Cannot parse struct or union!"
>
> case 2:
> struct ipu3_uapi_dummy {
> struct ipu3_uapi_x x IPU3_ALIGN;
> } __packed;
>
> "warning: Function parameter or member 'IPU3_ALIGN' not
> described in 'ipu3_uapi_dummy'"
>
> Positioned the attribute syntax without altering the
> mem layout of the structs, while also making "make htmldocs" to
> compile fine.
>
> Let us know if it's okay to ignore Sphinx warnings.
I looked a bit at different options for handling this in scripts/kernel-doc
but the difficulty in macro substitution comes in determining where to do
the substitution and where not to. That seems unaddressable in the kernel-doc
script; most of the time you want the definitions as-is while this is
likely the only case where something else might be appropriate. Making
IPU3_ALIGN a special case probably wouldn't really fly either.
In this particular case I'd just write open the alignment requirement so
kernel-doc can correctly parse it.
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH] media: imx208: Add imx208 camera sensor driver
On Tue, Jul 10, 2018 at 07:37:54AM +, Yeh, Andy wrote:
> Hi PC,
>
> Thanks for the patch.
>
> Cc in Grant, and Intel Jim/Jason
>
> > -Original Message-
> > From: Chen, Ping-chung
> > Sent: Tuesday, July 10, 2018 3:16 PM
> > To: linux-media@vger.kernel.org
> > Cc: sakari.ai...@linux.intel.com; Yeh, Andy ;
> > tf...@chromium.org; Chen, Ping-chung
> > Subject: [PATCH] media: imx208: Add imx208 camera sensor driver
> > +};
> > +
> > +static int imx208_enum_mbus_code(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_mbus_code_enum *code) {
> > + /* Only one bayer order(GRBG) is supported */
> > + if (code->index > 0)
> > + return -EINVAL;
> > +
>
> There is no limitation on using GRBG bayer order now. You can refer to imx355
> driver as well.
It seems the rest of the driver uses RGGB.
The enumeration should only contain what is possible using the current
flipping configuration.
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: RESEND[PATCH v6 2/2] media: dw9807: Add dw9807 vcm driver
Hi Andy, On Wed, Mar 21, 2018 at 03:58:42PM +, Yeh, Andy wrote: > Thanks for the comments. A quick question first. For the reset we need some > time to address. > > -Original Message- > From: jacopo mondi [mailto:jac...@jmondi.org] > Sent: Tuesday, March 20, 2018 6:28 PM > To: Yeh, Andy <andy@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; > devicet...@vger.kernel.org; Chiang, AlanX <alanx.chi...@intel.com> > Subject: Re: RESEND[PATCH v6 2/2] media: dw9807: Add dw9807 vcm driver > > Hi Andy, >a few comments on you patch below... > > On Sat, Mar 17, 2018 at 01:05:26AM +0800, Andy Yeh wrote: > > From: Alan Chiang <alanx.chi...@intel.com> > > a/drivers/media/i2c/dw9807.c b/drivers/media/i2c/dw9807.c new file > > mode 100644 index 000..95626e9 > > --- /dev/null > > +++ b/drivers/media/i2c/dw9807.c > > @@ -0,0 +1,320 @@ > > +// Copyright (C) 2018 Intel Corporation // SPDX-License-Identifier: > > +GPL-2.0 > > + > > Nit: my understanding is that the SPDX identifier goes first > > https://lwn.net/Articles/739183/ > > I checked this site. And it says Copyright should be before SPDX identifier. > == file01.c == > // Copyright (c) 2012-2016 Joe Random Hacker // SPDX-License-Identifier: > BSD-2-Clause ... > == file02.c == > // Copyright (c) 2017 Jon Severinsson > // SPDX-License-Identifier: BSD-2-Clause ... > == file03.c == > // Copyright (c) 2008 The NetBSD Foundation, Inc. > // SPDX-License-Identifier: BSD-2-Clause-NetBSD This is an example which is AFAIU purported to show the problem with various BSD licenses in a comment from someone. The order of the copyright holder and license lines might be just random. The practice in kernel at least appears to be SPDX license first. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v4 04/12] intel-ipu3: Add user space ABI definitions
Hi Rajmohan,
My apologies for the late reply.
On Sat, Nov 11, 2017 at 04:07:22AM +, Mani, Rajmohan wrote:
> Hi Sakari,
>
> > -Original Message-
> > From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> > Sent: Friday, October 20, 2017 2:30 AM
> > To: Zhi, Yong <yong@intel.com>
> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> > Xu
> > <jian.xu.zh...@intel.com>; Mani, Rajmohan <rajmohan.m...@intel.com>;
> > Toivonen, Tuukka <tuukka.toivo...@intel.com>; Hu, Jerry W
> > <jerry.w...@intel.com>
> > Subject: Re: [PATCH v4 04/12] intel-ipu3: Add user space ABI definitions
> >
> > Hi Yong,
> >
> > On Tue, Oct 17, 2017 at 10:54:49PM -0500, Yong Zhi wrote:
...
> > > +struct ipu3_uapi_params {
> > > + __u32 fourcc; /* V4L2_PIX_FMT_IPU3_PARAMS */
> > > + __u32 version; /* Must be 0x100 */
> >
> > These were called padding1 and padding2 in the previous version. What
> > happened?
> >
> > I'd just call them reserved, and maybe also make the use field the first
> > member of the struct.
> >
>
> These fields were repurposed after v3 of this patch series. Please see the
> user space code that uses these fields.
> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/workers/IPU3AicToFwEncoder.cpp
They were fourcc and version in the beginning, and then replaced by
padding1 and padding 2. Is there a particular reason for changing them
back?
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2 08/12] intel-ipu3: params: compute and program ccs
On Wed, Oct 11, 2017 at 04:14:37AM +, Zhi, Yong wrote:
> Hi, Andy,
>
> > -Original Message-
> > From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> > ow...@vger.kernel.org] On Behalf Of Andy Shevchenko
> > Sent: Friday, June 16, 2017 3:53 PM
> > To: Zhi, Yong <yong@intel.com>
> > Cc: Linux Media Mailing List <linux-media@vger.kernel.org>;
> > sakari.ai...@linux.intel.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> > tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> > Toivonen, Tuukka <tuukka.toivo...@intel.com>
> > Subject: Re: [PATCH v2 08/12] intel-ipu3: params: compute and program ccs
> >
> > On Thu, Jun 15, 2017 at 1:19 AM, Yong Zhi <yong@intel.com> wrote:
> > > A collection of routines that are mainly responsible to calculate the
> > > acc parameters.
> >
> > > +static unsigned int ipu3_css_scaler_get_exp(unsigned int counter,
> > > + unsigned int divider) {
> > > + unsigned int i = 0;
> > > +
> > > + while (counter <= divider / 2) {
> > > + divider /= 2;
> > > + i++;
> > > + }
> > > +
> > > + return i;
> >
> > We have a lot of different helpers including one you may use instead of this
> > function.
> >
> > It's *highly* recommended you learn what we have under lib/ (and not only
> > there) in kernel bewfore submitting a new version.
> >
>
> Tried to identify more places that could be re-implemented with lib
> helpers or more generic method, but we failed to spot any obvious
> candidate thus far.
How about:
return (!counter || divider < counter) ?
0 : fls(divider / counter) - 1;
--
Sakari Ailus
sakari.ai...@linux.intel.com
