| Message ID | 1344103941-23047-1-git-send-email-develkernel412222@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Devendra, On Sat, Aug 4, 2012 at 3:12 PM, Devendra Naga <develkernel412222@gmail.com> wrote: > > mutex_init(&ci->lock); > memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); While you're still looking at this driver, perhaps you can change the memcpy with a plain struct assignment (if you feel like). It's really pointless to use a memcpy here. Something like this: - memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); + ci->cfg = *cfg; Regards, Ezequiel. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Ezequiel, On Sun, Aug 5, 2012 at 12:24 AM, Ezequiel Garcia <elezegarcia@gmail.com> wrote: > Hi Devendra, > > On Sat, Aug 4, 2012 at 3:12 PM, Devendra Naga > <develkernel412222@gmail.com> wrote: >> >> mutex_init(&ci->lock); >> memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); > > While you're still looking at this driver, perhaps you can change the memcpy > with a plain struct assignment (if you feel like). > It's really pointless to use a memcpy here. > > Something like this: > > - memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); > + ci->cfg = *cfg; > Correct, and also one more thing like this is - memcpy(&ci->en, &en_templ, sizeof(en_templ)); + ci->en = en_templ; Is it ok if i change ci->cfg and ci->en? > Regards, > Ezequiel. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Devendra, On Sun, Aug 5, 2012 at 1:04 AM, Devendra Naga <develkernel412222@gmail.com> wrote: > Hello Ezequiel, > > On Sun, Aug 5, 2012 at 12:24 AM, Ezequiel Garcia <elezegarcia@gmail.com> wrote: >> Hi Devendra, >> >> On Sat, Aug 4, 2012 at 3:12 PM, Devendra Naga >> <develkernel412222@gmail.com> wrote: >>> >>> mutex_init(&ci->lock); >>> memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); >> >> While you're still looking at this driver, perhaps you can change the memcpy >> with a plain struct assignment (if you feel like). >> It's really pointless to use a memcpy here. >> >> Something like this: >> >> - memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); >> + ci->cfg = *cfg; >> > Correct, and also one more thing like this is > > - memcpy(&ci->en, &en_templ, sizeof(en_templ)); > + ci->en = en_templ; > > Is it ok if i change ci->cfg and ci->en? Yes, I believe it is ok. A few more remarks I would like to add. 1. When sending patches for staging/media, it's not necessary to put staging list/maintainer (Greg) on Cc. I guess, it doesn't hurt, though. But it's media list / Mauro who will decide on the patches. 2. You could also change the order in "struct cxd". Currently it's like this struct cxd { struct dvb_ca_en50221 en; struct i2c_adapter *i2c; struct cxd2099_cfg cfg; But it would be better to put it like this struct cxd { struct i2c_adapter *i2c; struct cxd2099_cfg cfg; struct dvb_ca_en50221 en; It's more logical, and ci->i2c and ci->cfg are used more frequently, so it makes sense to put it near the top of the struct. (You may think I'm being too paranoid: I am). 3. You don't have hw to test, uh? In that case, don't forget to always add a "Tested by compilation only" inside the commit message. That way the maintainer (Mauro) are free to _not_ pick the patch, if he feels it's not safe/clear enough. Hope this helps and thanks for your work, Ezequiel. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Ezequiel, Thanks, you wrote a full description of what i need to do.. i will definitely follow this. On Sun, Aug 5, 2012 at 11:57 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote: > Hi Devendra, > > On Sun, Aug 5, 2012 at 1:04 AM, Devendra Naga > <develkernel412222@gmail.com> wrote: >> Hello Ezequiel, >> >> On Sun, Aug 5, 2012 at 12:24 AM, Ezequiel Garcia <elezegarcia@gmail.com> wrote: >>> Hi Devendra, >>> >>> On Sat, Aug 4, 2012 at 3:12 PM, Devendra Naga >>> <develkernel412222@gmail.com> wrote: >>>> >>>> mutex_init(&ci->lock); >>>> memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); >>> >>> While you're still looking at this driver, perhaps you can change the memcpy >>> with a plain struct assignment (if you feel like). >>> It's really pointless to use a memcpy here. >>> >>> Something like this: >>> >>> - memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg)); >>> + ci->cfg = *cfg; >>> >> Correct, and also one more thing like this is >> >> - memcpy(&ci->en, &en_templ, sizeof(en_templ)); >> + ci->en = en_templ; >> >> Is it ok if i change ci->cfg and ci->en? > > Yes, I believe it is ok. > > A few more remarks I would like to add. > > 1. When sending patches for staging/media, it's not necessary to put > staging list/maintainer > (Greg) on Cc. I guess, it doesn't hurt, though. > But it's media list / Mauro who will decide on the patches. > Yeah, for some patches i have done ccing to Greg, but i think you told me not to cc at that time itself so not cc'ed now. > 2. You could also change the order in "struct cxd". > Currently it's like this > > struct cxd { > struct dvb_ca_en50221 en; > struct i2c_adapter *i2c; > struct cxd2099_cfg cfg; > > But it would be better to put it like this > > struct cxd { > struct i2c_adapter *i2c; > struct cxd2099_cfg cfg; > struct dvb_ca_en50221 en; > > It's more logical, and ci->i2c and ci->cfg are used more frequently, so it makes > sense to put it near the top of the struct. > (You may think I'm being too paranoid: I am). > I am afraid i may not be doing those, if re-ordering may cause some ambiguous problems. sorry... > 3. You don't have hw to test, uh? > In that case, don't forget to always add a "Tested by compilation only" > inside the commit message. That way the maintainer (Mauro) are free to > _not_ pick > the patch, if he feels it's not safe/clear enough. > Ok . i will definitely put that message in commit. Thanks > Hope this helps and thanks for your work, > Ezequiel. Since the changes are different than what this patch does, i will do the changes you proposed in a new patch and will send it out. Thanks for your time, -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/media/cxd2099/cxd2099.c b/drivers/staging/media/cxd2099/cxd2099.c index 1678503..4f2235f 100644 --- a/drivers/staging/media/cxd2099/cxd2099.c +++ b/drivers/staging/media/cxd2099/cxd2099.c @@ -691,10 +691,9 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg, return NULL; } - ci = kmalloc(sizeof(struct cxd), GFP_KERNEL); + ci = kzalloc(sizeof(struct cxd), GFP_KERNEL); if (!ci) return NULL; - memset(ci, 0, sizeof(*ci)); mutex_init(&ci->lock); memcpy(&ci->cfg, cfg, sizeof(struct cxd2099_cfg));
this does kmalloc and followed by memset, calling kzalloc will actually sets the allocated memory to zero, use kzalloc instead Signed-off-by: Devendra Naga <develkernel412222@gmail.com> --- drivers/staging/media/cxd2099/cxd2099.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)