| Message ID | 1559806756-16683-2-git-send-email-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hi Eugen, On Thu, Jun 06, 2019 at 07:43:47AM +0000, Eugen.Hristev@microchip.com wrote: > From: Eugen Hristev <eugen.hristev@microchip.com> > > 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@microchip.com> > --- > 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/ ? Same on the isc_device fields below. With that, Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > 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 >
On 06.06.2019 11:34, Sakari Ailus wrote: > Hi Eugen, > > On Thu, Jun 06, 2019 at 07:43:47AM +0000, Eugen.Hristev@microchip.com wrote: >> From: Eugen Hristev <eugen.hristev@microchip.com> >> >> 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@microchip.com> >> --- >> 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@linux.intel.com> > >> 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 >> >
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 */ 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;