Re: [PATCH v3 2/2] media: atmel: atmel-sama5d2-isc: fixed checkpatch warnings

[Eugen.Hristev]

On 06.06.2019 11:34, Sakari Ailus wrote:

> Hi Eugen,
> 
> On Thu, Jun 06, 2019 at 07:43:47AM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>> From: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>>
>> Checkpatch complaining that locks do not have comments,
>> unaligned code and macro reuse of same argument in to_isc_clk.
>> Fixed them by renaming, realigning and adding struct comments
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>> ---
>> Changes in v3:
>> - new patch, addresses the checkpatch issues that Hans asked to fix
>>
>>   drivers/media/platform/atmel/atmel-isc.h         | 51 +++++++++++++++++++++---
>>   drivers/media/platform/atmel/atmel-sama5d2-isc.c |  4 +-
>>   2 files changed, 48 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
>> index 0bd5dff..6ff9fa8 100644
>> --- a/drivers/media/platform/atmel/atmel-isc.h
>> +++ b/drivers/media/platform/atmel/atmel-isc.h
>> @@ -24,14 +24,14 @@ struct isc_clk {
>>   	struct clk_hw   hw;
>>   	struct clk      *clk;
>>   	struct regmap   *regmap;
>> -	spinlock_t	lock;
>> +	spinlock_t	lock;	/* synchronize access to clock registers */
> 
> You probably want to serialise the access, not synchronise it (i.e. happen
> at the same time). So, s/synchronise/serialise/ ?

Hello Sakari,

For sure, what I meant is to use access synchronization : do not access 
in the same time (synchronize one with another to avoid corruption of 
data, etc.)

You believe serialize is a better term ? I considered 'synchronization' 
to be more generic : this definition which I found online :
Data synchronization is the process of maintaining the consistency and 
uniformity of data instances across all consuming applications and 
storing devices. It ensures that the same copy or version of data is 
used in all devices - from source to destination.

If you think serialize is better I can change it

Eugen
> 
> Same on the isc_device fields below.
> 
> With that,
> 
> Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> 
>>   	u8		id;
>>   	u8		parent_id;
>>   	u32		div;
>>   	struct device	*dev;
>>   };
>>   
>> -#define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
>> +#define to_isc_clk(v) container_of(v, struct isc_clk, hw)
>>   
>>   struct isc_buffer {
>>   	struct vb2_v4l2_buffer  vb;
>> @@ -146,6 +146,47 @@ struct isc_ctrls {
>>   
>>   #define ISC_PIPE_LINE_NODE_NUM	11
>>   
>> +/*
>> + * struct isc_device - ISC device driver data/config struct
>> + * @regmap:		Register map
>> + * @hclock:		Hclock clock input (refer datasheet)
>> + * @ispck:		iscpck clock (refer datasheet)
>> + * @isc_clks:		ISC clocks
>> + *
>> + * @dev:		Registered device driver
>> + * @v4l2_dev:		v4l2 registered device
>> + * @video_dev:		registered video device
>> + *
>> + * @vb2_vidq:		video buffer 2 video queue
>> + * @dma_queue_lock:	lock to synchronize the dma buffer queue
>> + * @dma_queue:		the queue for dma buffers
>> + * @cur_frm:		current isc frame/buffer
>> + * @sequence:		current frame number
>> + * @stop:		true if isc is not streaming, false if streaming
>> + * @comp:		completion reference that signals frame completion
>> + *
>> + * @fmt:		current v42l format
>> + * @user_formats:	list of formats that are supported and agreed with sd
>> + * @num_user_formats:	how many formats are in user_formats
>> + *
>> + * @config:		current ISC format configuration
>> + * @try_config:		the current ISC try format , not yet activated
>> + *
>> + * @ctrls:		holds information about ISC controls
>> + * @do_wb_ctrl:		control regarding the DO_WHITE_BALANCE button
>> + * @awb_work:		workqueue reference for autowhitebalance histogram
>> + *			analysis
>> + *
>> + * @lock:		lock for synchronizing userspace file operations
>> + *			with ISC operations
>> + * @awb_lock:		lock for synchronizing awb work queue operations
>> + *			with DMA/buffer operations
>> + *
>> + * @pipeline:		configuration of the ISC pipeline
>> + *
>> + * @current_subdev:	current subdevice: the sensor
>> + * @subdev_entities:	list of subdevice entitites
>> + */
>>   struct isc_device {
>>   	struct regmap		*regmap;
>>   	struct clk		*hclock;
>> @@ -157,7 +198,7 @@ struct isc_device {
>>   	struct video_device	video_dev;
>>   
>>   	struct vb2_queue	vb2_vidq;
>> -	spinlock_t		dma_queue_lock;
>> +	spinlock_t		dma_queue_lock; /* sync access to dma queue */
>>   	struct list_head	dma_queue;
>>   	struct isc_buffer	*cur_frm;
>>   	unsigned int		sequence;
>> @@ -175,8 +216,8 @@ struct isc_device {
>>   	struct v4l2_ctrl	*do_wb_ctrl;
>>   	struct work_struct	awb_work;
>>   
>> -	struct mutex		lock;
>> -	spinlock_t		awb_lock;
>> +	struct mutex		lock; /* sync access to file operations */
>> +	spinlock_t		awb_lock; /* sync access to DMA buffers from awb work queue */
>>   
>>   	struct regmap_field	*pipeline[ISC_PIPE_LINE_NODE_NUM];
>>   
>> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>> index 69faaaf..299243f 100644
>> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>> @@ -87,8 +87,8 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
>>   			break;
>>   		}
>>   
>> -		subdev_entity = devm_kzalloc(dev,
>> -					  sizeof(*subdev_entity), GFP_KERNEL);
>> +		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
>> +					     GFP_KERNEL);
>>   		if (!subdev_entity) {
>>   			of_node_put(rem);
>>   			ret = -ENOMEM;
>> -- 
>> 2.7.4
>>
>