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 ? 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 >
