On 06/07/2011 12:02 PM, Guennadi Liakhovetski wrote:
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.
And copy paste "client" into "sd" is even quicker, isn't it ?
A general question to you: from your comments I haven't understood: have
you also tested the patches or only reviewed them?
Only reviewed so far. Test will come Monday.
Cheers.
--
Robert
--
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
