Hi,
On Tue, May 19, 2020 at 01:47:16PM +0100, Naushir Patuck wrote:
[snip]
> > > +++ b/drivers/staging/vc04_services/include/uapi/linux/bcm2835-isp.h
> > > @@ -0,0 +1,333 @@
> > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
> > > +/*
> > > + * bcm2835-isp.h
> > > + *
> > > + * BCM2835 ISP driver - user space header file.
> > > + *
> > > + * Copyright © 2019-2020 Raspberry Pi (Trading) Ltd.
> > > + *
> > > + * Author: Naushir Patuck (naush@xxxxxxxxxxxxxxx)
> > > + *
> > > + */
> > > +
> > > +#ifndef __BCM2835_ISP_H_
> > > +#define __BCM2835_ISP_H_
> > > +
> > > +#include <linux/v4l2-controls.h>
> > > +
> > > +/* TODO: move the control IDs definitions to v4l2-controls.h */
> > > +#define V4L2_CID_USER_BCM2835_ISP_BASE (V4L2_CID_USER_BASE + 0x10c0)
> >
> > As the TODO says: move this to v4l2-controls.h. Currently the 0x10c0 offset
> > clashes with V4L2_CID_USER_ATMEL_ISC_BASE, so that certainly should be fixed.
> >
>
> Unfortunately, there seems to be a mixup here. Laurent, we have
> accidentally mailed a WIP revision of this patch. The final version
> does have V4L2_CID_USER_BCM2835_ISP_BASE with a unique id in
> v4l2-controls.h. I will talk with Laurent separately to get the
> correct revison included in the next patch-set.
I have created this file in this directory with the intention not to
pollute the kernel headers with definitions for a staging driver,
like in example, the IPU3 definitions in
drivers/staging/media/ipu3/include/intel-ipu3.h
Which, by the way, has another conflicting definition for its control
base
define V4L2_CID_INTEL_IPU3_BASE (V4L2_CID_USER_BASE + 0x10c0)
>
> > > +
> > > +/* TODO: move the formats definitions to videodev2.h */
> > > +/* 12 Y/CbCr 4:2:0 128 pixel wide column */
> > > +#define V4L2_PIX_FMT_NV12_COL128 v4l2_fourcc('N', 'C', '1', '2')
> > > +/* Y/CbCr 4:2:0 10bpc, 3x10 packed as 4 bytes in a 128 bytes / 96 pixel wide column */
> > > +#define V4L2_PIX_FMT_NV12_10_COL128 v4l2_fourcc('N', 'C', '3', '0')
> > > +/* Sensor Ancillary metadata */
> > > +#define V4L2_META_FMT_SENSOR_DATA v4l2_fourcc('S', 'E', 'N', 'S')
> > > +/* BCM2835 ISP image statistics output */
> > > +#define V4L2_META_FMT_BCM2835_ISP_STATS v4l2_fourcc('B', 'S', 'T', 'A')
> > > +
>
> Similarly, these have also been moved to the right header files.
Same. I'm not sure now what the right policy for a staging driver
should be then.
>
> > > +#define V4L2_CID_USER_BCM2835_ISP_CC_MATRIX \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0001)
> > > +#define V4L2_CID_USER_BCM2835_ISP_LENS_SHADING \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0002)
> > > +#define V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0003)
> > > +#define V4L2_CID_USER_BCM2835_ISP_GEQ \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0004)
> > > +#define V4L2_CID_USER_BCM2835_ISP_GAMMA \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0005)
> > > +#define V4L2_CID_USER_BCM2835_ISP_DENOISE \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0006)
> > > +#define V4L2_CID_USER_BCM2835_ISP_SHARPEN \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0007)
> > > +#define V4L2_CID_USER_BCM2835_ISP_DPC \
> > > + (V4L2_CID_USER_BCM2835_ISP_BASE + 0x0008)
> >
> > There is no documentation for these controls. Specifically, it doesn't
> > tell you which struct should be used.
>
> As above, the documentaiton is available in the newer patch.
>
related: the documentation for this driver is in
drivers/staging/vc04_services/Documentation/
should this be moved to Documentation already ?
(as Nuash said, there's no documentation for controls in this version)
Thanks
j
