Hi Devendra,
On Sun, Aug 5, 2012 at 1:04 AM, Devendra Naga
<develkernel412222@xxxxxxxxx> wrote:
> Hello Ezequiel,
>
> On Sun, Aug 5, 2012 at 12:24 AM, Ezequiel Garcia <elezegarcia@xxxxxxxxx> wrote:
>> Hi Devendra,
>>
>> On Sat, Aug 4, 2012 at 3:12 PM, Devendra Naga
>> <develkernel412222@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
