On 6/5/19 2:08 PM, Eugen.Hristev@xxxxxxxxxxxxx wrote: > > > On 05.06.2019 14:56, Hans Verkuil wrote: > >> >> Hi Eugen, >> >> On 5/30/19 9:50 AM, Eugen.Hristev@xxxxxxxxxxxxx wrote: >>> From: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> >>> >>> This splits the Atmel ISC driver into a common base: atmel-isc-base.c >>> and the driver probe/dt part , atmel-sama5d2-isc.c >>> This is needed to keep a common ground for the sensor controller which will >>> be reused. >>> The atmel-isc will use the common symbols inside the atmel-isc-base >>> Future driver will also use the same symbols and redefine different aspects, >>> for a different version of the ISC. >>> This is done to avoid complete code duplication by creating a totally >>> different driver for the new variant of the ISC. >>> >>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> >>> --- >>> >>> Hello Hans, >>> >>> I am resending this with the required fixes. >>> You asked me about the #ifdef around ATMEL_ISC_NAME, it's because I was >>> thinking to have the ATMEL_ISC_NAME different by each driver that use >>> the atmel-isc.h, but for now I removed any ifdef around it, and will >>> consider a different define for new drivers. >>> >>> Changes in v2: >>> - renamed atmel-isc.c to atmel-sama5d2-isc.c as sama5d2 is the SoC that >>> includes this hardware block. The module will still be named atmel-isc.ko >>> - removed ifdef around definition of ATMEL_ISC_NAME >>> - moved external declarations in the specific files, this was breaking >>> module loading >>> >>> v4l2-compliance SHA: 0d61ddede7d340ffa1c75a2882e30c455ef3d8b8, 32 bitatmel-isc f0008000.isc: ================= START STATUS ================= >>> >>> atmelpliance test for atmel-isc device /dev/video0: >>> >>> Driver Info: >>> Driver name : atmel-isc >>> Card type : Atmel Image Sensor Controller >>> Bus info : platform:atmel-isc f0008000.isc >>> Driver version : 5.2.0 >>> Capabilities : 0x84200001 >>> Video Capture >>> Streaming >>> Extended Pix Format >>> Device Capabilities >>> Device Caps : 0x04200001 >>> Video Capture >>> Streaming >>> Extended Pix Format >>> >>> Required ioctls: >>> test VIDIOC_QUERYCAP: OK >>> >>> Allow for multiple opens: >>> test second /dev/video0 open: OK >>> test VIDIOC_QUERYCAP: OK >>> test VIDIOC_G/S_PRIORITY: OK >>> test for unlimited opens: OK >>> >>> Debug ioctls: >>> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) >>> -isc f0008000.isc: Brightness: 0 >>> atmel-isc f0008000.isc: Contrast: 256 >>> atmel-isc f0008000.isc: Gamma: 2 >>> atmel-isc f0008000.isc: White Balance, Automatic: true >>> atmel-isc f0008000.isc: ================== END STATUS ================== >>> test VIDIOC_LOG_STATUS: OK >>> >>> Input ioctls: >>> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) >>> test VIDIOC_G/S_FREQUENCY: OK (Not Supported) >>> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) >>> test VIDIOC_ENUMAUDIO: OK (Not Supported) >>> test VIDIOC_G/S/ENUMINPUT: OK >>> test VIDIOC_G/S_AUDIO: OK (Not Supported) >>> Inputs: 1 Audio Inputs: 0 Tuners: 0 >>> >>> Output ioctls: >>> test VIDIOC_G/S_MODULATOR: OK (Not Supported) >>> test VIDIOC_G/S_FREQUENCY: OK (Not Supported) >>> test VIDIOC_ENUMAUDOUT: OK (Not Supported) >>> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) >>> test VIDIOC_G/S_AUDOUT: OK (Not Supported) >>> Outputs: 0 Audio Outputs: 0 Modulators: 0 >>> >>> Input/Output configuration ioctls: >>> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) >>> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) >>> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) >>> test VIDIOC_G/S_EDID: OK (Not Supported) >>> >>> Control ioctls (Input 0): >>> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK >>> test VIDIOC_QUERYCTRL: OK >>> test VIDIOC_G/S_CTRL: OK >>> test VIDIOC_G/S/TRY_EXT_CTRLS: OK >>> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK >>> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) >>> Standard Controls: 6 Private Controls: 0 >>> >>> Format ioctls (Input 0): >>> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK >>> test VIDIOC_G/S_PARM: OK >>> test VIDIOC_G_FBUF: OK (Not Supported) >>> test VIDIOC_G_FMT: OK >>> test VIDIOC_TRY_FMT: OK >>> test VIDIOC_S_FMT: OK >>> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) >>> test Cropping: OK (Not Supported) >>> test Composing: OK (Not Supported) >>> test Scaling: OK (Not Supported) >>> >>> Codec ioctls (Input 0): >>> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) >>> test VIDIOC_G_ENC_INDEX: OK (Not Supported) >>> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) >>> >>> Buffer ioctls (Input 0): >>> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK >>> test VIDIOC_EXPBUF: OK >>> test Requests: OK (Not Supported) >>> >>> Total for atmel-isc device /dev/video0: 44, Succeeded: 44, Failed: 0, Warnings: 0 >>> >>> >>> MAINTAINERS | 4 +- >>> drivers/media/platform/atmel/Makefile | 4 +- >>> drivers/media/platform/atmel/atmel-isc-base.c | 2148 ++++++++++++++++++ >>> drivers/media/platform/atmel/atmel-isc.c | 2634 ---------------------- >>> drivers/media/platform/atmel/atmel-isc.h | 192 ++ >>> drivers/media/platform/atmel/atmel-sama5d2-isc.c | 365 +++ >>> 6 files changed, 2711 insertions(+), 2636 deletions(-) >>> create mode 100644 drivers/media/platform/atmel/atmel-isc-base.c >>> delete mode 100644 drivers/media/platform/atmel/atmel-isc.c >>> create mode 100644 drivers/media/platform/atmel/atmel-isc.h >>> create mode 100644 drivers/media/platform/atmel/atmel-sama5d2-isc.c >>> >> >> Checkpatch gave me various warnings that I think should be addressed: > > > Hello Hans, > > I saw the checkpatch issues as well, but most of them are inherited and > appear now because of code move... > I would think it would be cleaner to solve these issues in a an a-priori > patch, and then do the split as a "clean,just-split" patch afterwards. > What do you think? > > And for the externs, I can handle it, but I would create a separate > header file for them that would be included in new atmel-sama5d2-isc.c > > Are you ok with this approach ? The extern issues are introduced by this patch, so they should be solved here. The other issues can be fixed in a separate follow-up patch. Regards, Hans > > Thanks for review, > Eugen > >> >> WARNING: externs should be avoided in .c files >> #249: FILE: drivers/media/platform/atmel/atmel-isc-base.c:40: >> +extern unsigned int sensor_preferred; >> >> It looks like these module parameters can be moved to atmel-isc-base.c >> and only an extern unsigned int debug should be added to the atmel-isc.h. >> >> Externs in a source is indeed dubious. >> >> CHECK: spinlock_t definition without comment >> #681: FILE: drivers/media/platform/atmel/atmel-isc.h:27: >> + spinlock_t lock; >> >> I know there was no comment before, but it might be nice to add one >> now. >> >> CHECK: Macro argument reuse 'hw' - possible side-effects? >> #688: FILE: drivers/media/platform/atmel/atmel-isc.h:34: >> +#define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) >> >> This seems really wrong. One hw refers to the argument, the >> other is the name of a field in a struct. Use a different >> name as the macro argument to avoid this confusion. >> >> >> CHECK: spinlock_t definition without comment >> #814: FILE: drivers/media/platform/atmel/atmel-isc.h:160: >> + spinlock_t dma_queue_lock; >> >> CHECK: struct mutex definition without comment >> #832: FILE: drivers/media/platform/atmel/atmel-isc.h:178: >> + struct mutex lock; >> >> CHECK: spinlock_t definition without comment >> #833: FILE: drivers/media/platform/atmel/atmel-isc.h:179: >> + spinlock_t awb_lock; >> >> Comments would be nice. >> >> WARNING: externs should be avoided in .c files >> #909: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:57: >> +extern struct isc_format formats_list[]; >> >> WARNING: externs should be avoided in .c files >> #910: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:58: >> +extern struct isc_format controller_formats[]; >> >> WARNING: externs should be avoided in .c files >> #911: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:59: >> +extern const u32 isc_gamma_table[GAMMA_MAX + 1][GAMMA_ENTRIES]; >> >> WARNING: externs should be avoided in .c files >> #912: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:60: >> +extern const struct regmap_config isc_regmap_config; >> >> WARNING: externs should be avoided in .c files >> #913: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:61: >> +extern const struct v4l2_async_notifier_operations isc_async_ops; >> >> This should be in a header. >> >> WARNING: externs should be avoided in .c files >> #915: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:63: >> +extern irqreturn_t isc_interrupt(int irq, void *dev_id); >> >> WARNING: externs should be avoided in .c files >> #916: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:64: >> +extern int isc_pipeline_init(struct isc_device *isc); >> >> WARNING: externs should be avoided in .c files >> #917: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:65: >> +extern int isc_clk_init(struct isc_device *isc); >> >> WARNING: externs should be avoided in .c files >> #918: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:66: >> +extern void isc_subdev_cleanup(struct isc_device *isc); >> >> WARNING: externs should be avoided in .c files >> #919: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:67: >> +extern void isc_clk_cleanup(struct isc_device *isc); >> >> Should also be in a header. No need to extern when just declaring the >> function prototype, BTW. >> >> CHECK: Alignment should match open parenthesis >> #964: FILE: drivers/media/platform/atmel/atmel-sama5d2-isc.c:112: >> + subdev_entity = devm_kzalloc(dev, >> + sizeof(*subdev_entity), GFP_KERNEL); >> >> Please fix this as well. >> >> Regards, >> >> Hans >>
