Re: [PATCH 1/2] V4L: mt9m111: propagate higher level abstraction down in functions

[Robert Jarzmik <robert.jarzmik@free.fr>]

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