From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sylwester Nawrocki Date: Tue, 02 Oct 2012 10:13:20 +0000 Subject: Re: [PATCH 05/14] media: add a V4L2 OF parser Message-Id: <506ABE40.9070504@samsung.com> List-Id: References: <1348754853-28619-1-git-send-email-g.liakhovetski@gmx.de> <1348754853-28619-6-git-send-email-g.liakhovetski@gmx.de> <506A0D28.10505@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Guennadi Liakhovetski Cc: linux-sh-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Mark Brown , Magnus Damm , Hans Verkuil , Laurent Pinchart , Sylwester Nawrocki , linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Hi Guennadi, On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, >>> + ARRAY_SIZE(data_lanes))) { >>> + int i; >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; >> >> It doesn't look like what we aimed for. The data-lanes array is supposed >> to be of variable length, thus I don't think it can be parsed like that. >> Or am I missing something ? I think we need more something like below >> (based on of_property_read_u32_array(), not tested): > > Ok, you're right, that my version only was suitable for fixed-size arrays, > which wasn't our goal. But I don't think we should go down to manually > parsing property data. How about (tested;-)) > > data = of_find_property(node, "data-lanes", NULL); > if (data) { > int i = 0; > const __be32 *lane = NULL; > do { > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > link->mipi_csi_2.num_data_lanes = i; > while (i--) > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > } Yes, that looks neat and does what it is supposed to do. :) Thanks! For now, I'll trust you it works ;) With regards to the remaining patches, it looks a bit scary to me how complicated it got, perhaps mostly because of requirement to reference host devices from subdevs. And it seems to rely on the existing SoC camera infrastructure, which might imply lot's of work need to be done for non soc-camera drivers. But I'm going to take a closer look and comment more on the details at the corresponding patches. -- Regards, Sylwester From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sylwester Nawrocki Subject: Re: [PATCH 05/14] media: add a V4L2 OF parser Date: Tue, 02 Oct 2012 12:13:20 +0200 Message-ID: <506ABE40.9070504@samsung.com> References: <1348754853-28619-1-git-send-email-g.liakhovetski@gmx.de> <1348754853-28619-6-git-send-email-g.liakhovetski@gmx.de> <506A0D28.10505@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-reply-to: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Guennadi Liakhovetski Cc: linux-sh-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Mark Brown , Magnus Damm , Hans Verkuil , Laurent Pinchart , Sylwester Nawrocki , linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Guennadi, On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, >>> + ARRAY_SIZE(data_lanes))) { >>> + int i; >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; >> >> It doesn't look like what we aimed for. The data-lanes array is supposed >> to be of variable length, thus I don't think it can be parsed like that. >> Or am I missing something ? I think we need more something like below >> (based on of_property_read_u32_array(), not tested): > > Ok, you're right, that my version only was suitable for fixed-size arrays, > which wasn't our goal. But I don't think we should go down to manually > parsing property data. How about (tested;-)) > > data = of_find_property(node, "data-lanes", NULL); > if (data) { > int i = 0; > const __be32 *lane = NULL; > do { > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > link->mipi_csi_2.num_data_lanes = i; > while (i--) > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > } Yes, that looks neat and does what it is supposed to do. :) Thanks! For now, I'll trust you it works ;) With regards to the remaining patches, it looks a bit scary to me how complicated it got, perhaps mostly because of requirement to reference host devices from subdevs. And it seems to rely on the existing SoC camera infrastructure, which might imply lot's of work need to be done for non soc-camera drivers. But I'm going to take a closer look and comment more on the details at the corresponding patches. -- Regards, Sylwester From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mailout2.w1.samsung.com ([210.118.77.12]:31965 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753659Ab2JBKNX (ORCPT ); Tue, 2 Oct 2012 06:13:23 -0400 Message-id: <506ABE40.9070504@samsung.com> Date: Tue, 02 Oct 2012 12:13:20 +0200 From: Sylwester Nawrocki MIME-version: 1.0 To: Guennadi Liakhovetski Cc: Sylwester Nawrocki , linux-media@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, Laurent Pinchart , Hans Verkuil , Magnus Damm , linux-sh@vger.kernel.org, Mark Brown , Stephen Warren , Arnd Bergmann , Grant Likely Subject: Re: [PATCH 05/14] media: add a V4L2 OF parser References: <1348754853-28619-1-git-send-email-g.liakhovetski@gmx.de> <1348754853-28619-6-git-send-email-g.liakhovetski@gmx.de> <506A0D28.10505@gmail.com> In-reply-to: Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Guennadi, On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, >>> + ARRAY_SIZE(data_lanes))) { >>> + int i; >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; >> >> It doesn't look like what we aimed for. The data-lanes array is supposed >> to be of variable length, thus I don't think it can be parsed like that. >> Or am I missing something ? I think we need more something like below >> (based on of_property_read_u32_array(), not tested): > > Ok, you're right, that my version only was suitable for fixed-size arrays, > which wasn't our goal. But I don't think we should go down to manually > parsing property data. How about (tested;-)) > > data = of_find_property(node, "data-lanes", NULL); > if (data) { > int i = 0; > const __be32 *lane = NULL; > do { > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > link->mipi_csi_2.num_data_lanes = i; > while (i--) > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > } Yes, that looks neat and does what it is supposed to do. :) Thanks! For now, I'll trust you it works ;) With regards to the remaining patches, it looks a bit scary to me how complicated it got, perhaps mostly because of requirement to reference host devices from subdevs. And it seems to rely on the existing SoC camera infrastructure, which might imply lot's of work need to be done for non soc-camera drivers. But I'm going to take a closer look and comment more on the details at the corresponding patches. -- Regards, Sylwester