On Tue, Feb 11, 2014 at 12:53 PM, Guennadi Liakhovetski
<g.liakhovetski@xxxxxx> wrote:
> On Tue, 11 Feb 2014, Bryan Wu wrote:
>
>> On Mon, Feb 10, 2014 at 10:37 PM, Guennadi Liakhovetski
>> <g.liakhovetski@xxxxxx> wrote:
>> > Hi Bryan,
>> >
>> > On Mon, 10 Feb 2014, Bryan Wu wrote:
>> >
>> >> On Sun, Feb 9, 2014 at 2:20 PM, Guennadi Liakhovetski
>> >> <g.liakhovetski@xxxxxx> wrote:
>> >> > Hi Bryan,
>> >> >
>> >> > Thanks for reiterating this patch!
>> >> >
>> >>
>> >> Sure, my pleasure. I basically assembled your patches together and
>> >> change them to use latest V4L2 soc_camera API.
>> >>
>> >> > On Fri, 7 Feb 2014, Bryan Wu wrote:
>> >
>> > [snip]
>> >
>> >> >> @@ -67,6 +81,8 @@ struct soc_camera_async_client {
>> >> >>
>> >> >> static int soc_camera_video_start(struct soc_camera_device *icd);
>> >> >> static int video_dev_create(struct soc_camera_device *icd);
>> >> >> +static void soc_camera_of_i2c_info(struct device_node *node,
>> >> >> + struct soc_camera_of_client *sofc);
>> >> >
>> >> > If you have to resubmit this patch, plase, make sure the second line of
>> >> > the above declaration is aligned af usual - under the first character
>> >> > _after_ the opening bracket.
>> >> >
>> >>
>> >> No problem, I will update this.
>> >> Hmmm, something weird on my side. I did put the second line starting
>> >> under the first character after the opening bracket. But in git show
>> >> and git format-patch I got this
>> >> ---
>> >> static int soc_camera_video_start(struct soc_camera_device *icd);
>> >> static int video_dev_create(struct soc_camera_device *icd);
>> >> +static void soc_camera_of_i2c_info(struct device_node *node,
>> >> + struct soc_camera_of_client *sofc);
>> >> ---
>> >>
>> >> But I think that's what you want, right?
>> >
>> > Don't know - now aöö TABs above are replaced with spaces, so, cannot say.
>> >
>> > [snip]
>> >
>> >> >> +{
>> >> >> + struct soc_camera_of_client *sofc;
>> >> >> + struct soc_camera_desc *sdesc;
>> >> >
>> >> > I'm really grateful, that you decided to use my original patch and
>> >> > preserve my authorship! But then, I think, it'd be also better to avoid
>> >> > unnecessary changes to it. What was wrong with allocation of *sofc in the
>> >> > definition line?
>> >> >
>> >>
>> >> Oh, this is really I want to bring up. It's a very subtle bug here.
>> >>
>> >> If we use local variable sofc instead of zalloc, fields of sofc have
>> >> undetermined None NULL value.
>> >
>> > No. If you initialise some members of a struct in its definition line, the
>> > rest will be initialised to 0 / NULL. I.e. in
>> >
>> > struct foo y = {.x = 1,};
>> >
>> > all other fields of y will be initialised to 0.
>>
>> I see, but original one is soc_camera_link which is simple in this
>> case. right now we move to soc_camera_desc. I think following line is
>> not very straight forward in a local function.
>>
>> struct soc_camera_desc sdesc = { .host_desc = { .host_wait = true,},};
>
> I usually do
>
> struct soc_camera_desc sdesc = {.host_desc.host_wait = true,};
>
>> What about a) struct soc_camera_desc sdesc and use memset to all 0. b)
>> use kzalloc() and kfree() in this function.
>>
>> I think b) is more straight forward and easy to understand.
>
> With error handling for a failed kzalloc() - don't think so.
>
OK deal. I will update this.
More questions here:
1. Why do we need a notifier for soc_camera_pdrv_probe()?
I think we can call platform_device_register() which will finish call
the soc_camera_pdrv_probe() and before we start to call.
icd = platform_get_drvdata(sofc->pdev);
I removed those bus notifier code, it works fine on my platform for probing.
2. How to use v4l2-async API properly?
I think basically we need to call
v4l2_async_notifier_register(&ici->v4l2_dev, &sofc->notifier);
And move the code in soc_camera_i2c_notify() into
.bound/.unbind/.complete call function.
I just treat the scan_async_group() as an example.
Thanks,
-Bryan
--
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
