On Mon, 6 Jun 2011, Robert Jarzmik wrote:
> On 06/06/2011 07:20 PM, Guennadi Liakhovetski wrote:
> > It is more convenient to propagate the higher level abstraction - the
> > struct mt9m111 object into functions and then retrieve a pointer to
> > the i2c client, if needed, than to do the reverse.
> Agreed.
>
> One minor point, you ofter replace :
> > - struct mt9m111 *mt9m111 = to_mt9m111(client);
> > + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
>
> Why haven't you replaced the signature of to_mt9m111() into :
> static struct mt9m111 *to_mt9m111(const struct v4l2_subdev *sd)
> {
> return container_of(sd, struct mt9m111, subdev);
> }
>
> This way, each to_mt9m111(client) will become to_mt9m111(sd), and the purpose
> of to_mt9m111() will be kept. Wouldn't that be better ?
Because "container_of(sd, struct mt9m111, subdev)" is still easy enough to
write (copy-paste, of course:)) and understand, whereas
"container_of(i2c_get_clientdata(client), struct mt9m111, subdev)" is
already too awkward to look at, even though it is now only used at 4
locations.
A general question to you: from your comments I haven't understood: have
you also tested the patches or only reviewed them?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
