lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 5 Jun 2019 12:08:40 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <hverkuil@...all.nl>, <linux-media@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
CC:     <ksloat@...pglobal.com>
Subject: Re: [PATCH v2] media: atmel: atmel-isc: split driver into driver base
 and isc



On 05.06.2019 14:56, Hans Verkuil wrote:

> 
> Hi Eugen,
> 
> On 5/30/19 9:50 AM, Eugen.Hristev@...rochip.com wrote:
>> From: Eugen Hristev <eugen.hristev@...rochip.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@...rochip.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
> 

Powered by blists - more mailing lists