* Re: [PATCH v2] media: atmel: atmel-isc: split driver into driver base and isc [not found] <1559202331-15397-1-git-send-email-eugen.hristev@microchip.com> @ 2019-06-05 11:56 ` Hans Verkuil 2019-06-05 12:08 ` Eugen.Hristev 0 siblings, 1 reply; 3+ messages in thread From: Hans Verkuil @ 2019-06-05 11:56 UTC (permalink / raw) To: Eugen.Hristev, linux-media, linux-arm-kernel, linux-kernel; +Cc: ksloat Hi Eugen, On 5/30/19 9:50 AM, Eugen.Hristev@microchip.com wrote: > From: Eugen Hristev <eugen.hristev@microchip.com> > > 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@microchip.com> > --- > > 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: 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] media: atmel: atmel-isc: split driver into driver base and isc 2019-06-05 11:56 ` [PATCH v2] media: atmel: atmel-isc: split driver into driver base and isc Hans Verkuil @ 2019-06-05 12:08 ` Eugen.Hristev 2019-06-05 12:28 ` Hans Verkuil 0 siblings, 1 reply; 3+ messages in thread From: Eugen.Hristev @ 2019-06-05 12:08 UTC (permalink / raw) To: hverkuil, linux-media, linux-arm-kernel, linux-kernel; +Cc: ksloat On 05.06.2019 14:56, Hans Verkuil wrote: > > Hi Eugen, > > On 5/30/19 9:50 AM, Eugen.Hristev@microchip.com wrote: >> From: Eugen Hristev <eugen.hristev@microchip.com> >> >> 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@microchip.com> >> --- >> >> 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] media: atmel: atmel-isc: split driver into driver base and isc 2019-06-05 12:08 ` Eugen.Hristev @ 2019-06-05 12:28 ` Hans Verkuil 0 siblings, 0 replies; 3+ messages in thread From: Hans Verkuil @ 2019-06-05 12:28 UTC (permalink / raw) To: Eugen.Hristev, linux-media, linux-arm-kernel, linux-kernel; +Cc: ksloat On 6/5/19 2:08 PM, Eugen.Hristev@microchip.com wrote: > > > On 05.06.2019 14:56, Hans Verkuil wrote: > >> >> Hi Eugen, >> >> On 5/30/19 9:50 AM, Eugen.Hristev@microchip.com wrote: >>> From: Eugen Hristev <eugen.hristev@microchip.com> >>> >>> 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@microchip.com> >>> --- >>> >>> 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 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-05 12:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1559202331-15397-1-git-send-email-eugen.hristev@microchip.com>
2019-06-05 11:56 ` [PATCH v2] media: atmel: atmel-isc: split driver into driver base and isc Hans Verkuil
2019-06-05 12:08 ` Eugen.Hristev
2019-06-05 12:28 ` Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).