Messages in this thread | | | Subject | Re: [PATCH 3/3] [media] au0828-video: Move two assignments in au0828_init_isoc() | From | Hans Verkuil <> | Date | Fri, 4 Nov 2016 09:09:15 +0100 |
| |
On 03/11/16 20:56, SF Markus Elfring wrote: >>> From: Markus Elfring <elfring@users.sourceforge.net> >>> Date: Mon, 24 Oct 2016 22:44:02 +0200 >>> >>> Move the assignment for the data structure members "isoc_copy" >>> and "num_bufs" behind the source code for memory allocations >>> by this function. >> >> I don't see the point, > > Another explanation try … > > >> dropping this patch. > > I proposed that these assignments should only be performed after the required > memory allocations succeeded. Is this detail worth for further considerations?
I understand why you think it is better, but I disagree :-) I prefer the current approach, that way I know as a reviewer that these fields are correctly set and I can forget about them. Not worth spending more time on this.
Regards,
Hans
> > >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >>> --- >>> drivers/media/usb/au0828/au0828-video.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c >>> index b5c88a7..5ebda64 100644 >>> --- a/drivers/media/usb/au0828/au0828-video.c >>> +++ b/drivers/media/usb/au0828/au0828-video.c >>> @@ -218,9 +218,6 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >>> int rc; >>> >>> au0828_isocdbg("au0828: called au0828_prepare_isoc\n"); >>> - >>> - dev->isoc_ctl.isoc_copy = isoc_copy; >>> - dev->isoc_ctl.num_bufs = num_bufs; >>> dev->isoc_ctl.urb = kcalloc(num_bufs, >>> sizeof(*dev->isoc_ctl.urb), >>> GFP_KERNEL); >>> @@ -240,6 +237,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >>> dev->isoc_ctl.buf = NULL; >>> >>> sb_size = max_packets * dev->isoc_ctl.max_pkt_size; >>> + dev->isoc_ctl.num_bufs = num_bufs; >>> >>> /* allocate urbs and transfer buffers */ >>> for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { >>> @@ -276,6 +274,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >>> k += dev->isoc_ctl.max_pkt_size; >>> } >>> } >>> + dev->isoc_ctl.isoc_copy = isoc_copy; >>> >>> /* submit urbs and enables IRQ */ >>> for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { >>> >
| |