From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Dooks Date: Thu, 15 May 2014 21:01:55 +0000 Subject: Re: [PATCH v3 7/7] soc_camera: initial of code Message-Id: <53752B43.9000101@codethink.co.uk> List-Id: References: <1397471802-27216-8-git-send-email-ben.dooks@codethink.co.uk> In-Reply-To: <1397471802-27216-8-git-send-email-ben.dooks@codethink.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-sh@vger.kernel.org On 03/05/14 18:44, Guennadi Liakhovetski wrote: > On Mon, 14 Apr 2014, Ben Dooks wrote: > >> Add initial support for OF based soc-camera devices that may be used >> by any of the soc-camera drivers. The driver itself will need converting >> to use OF. >> >> These changes allow the soc-camera driver to do the connecting of any >> async capable v4l2 device to the soc-camera driver. This has currently >> been tested on the Renesas Lager board. >> >> Signed-off-by: Ben Dooks Thankyou for the feedback. I will try and get this sorted over the weekend as I have been travelling and away from code. >> --- >> Changes since v1: >> >> - Updated to make the i2c mclk name compatible with other >> drivers. The issue of having mclk which is not part of >> the drivers/clk interface is something that can be dealt >> with separately. >> --- >> drivers/media/platform/soc_camera/soc_camera.c | 117 ++++++++++++++++++++++++- >> 1 file changed, 116 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c >> index 4b8c024..c50ec5c 100644 >> --- a/drivers/media/platform/soc_camera/soc_camera.c >> +++ b/drivers/media/platform/soc_camera/soc_camera.c >> @@ -36,6 +36,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> >> @@ -1579,6 +1580,118 @@ static void scan_async_host(struct soc_camera_host *ici) >> #define scan_async_host(ici) do {} while (0) >> #endif >> >> +#ifdef CONFIG_OF >> +static int soc_of_bind(struct soc_camera_host *ici, >> + struct device_node *ep, >> + struct device_node *remote) >> +{ >> + struct soc_camera_device *icd; >> + struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,}; >> + struct soc_camera_async_client *sasc; >> + struct soc_camera_async_subdev *sasd; >> + struct v4l2_async_subdev **asd_array; >> + struct i2c_client *client; >> + char clk_name[V4L2_SUBDEV_NAME_SIZE]; >> + int ret; >> + >> + /* alloacte a new subdev and add match info to it */ >> + sasd = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasd), GFP_KERNEL); >> + if (!sasd) >> + return -ENOMEM; >> + > > I think I set a bad example to follow :) Please, have a look at this: > > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/77490 > > Maybe it would be good to do the same here? > >> + asd_array = devm_kzalloc(ici->v4l2_dev.dev, >> + sizeof(struct v4l2_async_subdev **), >> + GFP_KERNEL); > > is that a correct size?... > >> + if (!asd_array) >> + return -ENOMEM; >> + >> + sasd->asd.match.of.node = remote; >> + sasd->asd.match_type = V4L2_ASYNC_MATCH_OF; >> + asd_array[0] = &sasd->asd; >> + >> + /* Or shall this be managed by the soc-camera device? */ >> + sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL); >> + if (!sasc) >> + return -ENOMEM; > > And a third one... I think it's already worth a new struct with these > three objects in it to allocate it in one go? > >> + >> + /* HACK: just need a != NULL */ >> + sdesc.host_desc.board_info = ERR_PTR(-ENODATA); >> + >> + ret = soc_camera_dyn_pdev(&sdesc, sasc); >> + if (ret < 0) >> + return ret; >> + >> + sasc->sensor = &sasd->asd; >> + >> + icd = soc_camera_add_pdev(sasc); >> + if (!icd) { >> + platform_device_put(sasc->pdev); >> + return -ENOMEM; >> + } >> + >> + sasc->notifier.subdevs = asd_array; >> + sasc->notifier.num_subdevs = 1; > > Hm, see below... > >> + sasc->notifier.bound = soc_camera_async_bound; >> + sasc->notifier.unbind = soc_camera_async_unbind; >> + sasc->notifier.complete = soc_camera_async_complete; >> + >> + icd->sasc = sasc; >> + icd->parent = ici->v4l2_dev.dev; >> + >> + client = of_find_i2c_device_by_node(remote); >> + >> + if (client) >> + snprintf(clk_name, sizeof(clk_name), "%d-%04x", >> + client->adapter->nr, client->addr); >> + else >> + snprintf(clk_name, sizeof(clk_name), "of-%s", >> + of_node_full_name(remote)); >> + >> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd); >> + if (IS_ERR(icd->clk)) { >> + ret = PTR_ERR(icd->clk); >> + goto eclkreg; >> + } >> + >> + ret = v4l2_async_notifier_register(&ici->v4l2_dev, &sasc->notifier); >> + if (!ret) >> + return 0; >> + >> +eclkreg: >> + icd->clk = NULL; >> + platform_device_unregister(sasc->pdev); >> + dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret); >> + >> + return ret; >> +} >> + >> +static inline void scan_of_host(struct soc_camera_host *ici) > > You don't need "inline" in the C code - the compiler will decide by > itself. > >> +{ >> + struct device_node *np = ici->v4l2_dev.dev->of_node; >> + struct device_node *epn = NULL; >> + struct device_node *ren; >> + >> + while (true) { >> + epn = v4l2_of_get_next_endpoint(np, epn); >> + if (!epn) >> + break; >> + >> + ren = v4l2_of_get_remote_port(epn); > > Please, rebase on top of current -next. You'll probably use > of_graph_get_next_endpoint() and of_graph_get_remote_port() respectively. > >> + if (!ren) { >> + pr_info("%s: no remote for %s\n", >> + __func__, of_node_full_name(epn)); >> + continue; >> + } >> + >> + /* so we now have a remote node to connect */ >> + soc_of_bind(ici, epn, ren->parent); > > Hm, I think, this isn't quite correct. If the documented DT layout in > Documentation/devicetree/bindings/media/video-interfaces.txt hasn't > changed, a port can have several endpoints. And I think you have to > collect them all into your asd_array[] to form a group to have > soc_camera_async_complete() called only when all endpoints have been > bound. > > If you think it's too difficult with no real-life use case, you can limit > support to one endpoint per port, but please make this explicit and error > out with a suitable message if more than one endpoint is present. > >> + } >> +} >> + >> +#else >> +static inline void scan_of_host(struct soc_camera_host *ici) { } > > Also no need for inline. > >> +#endif >> + >> /* Called during host-driver probe */ >> static int soc_camera_probe(struct soc_camera_host *ici, >> struct soc_camera_device *icd) >> @@ -1830,7 +1943,9 @@ int soc_camera_host_register(struct soc_camera_host *ici) >> mutex_init(&ici->host_lock); >> mutex_init(&ici->clk_lock); >> >> - if (ici->asd_sizes) >> + if (ici->v4l2_dev.dev->of_node) >> + scan_of_host(ici); >> + else if (ici->asd_sizes) >> /* >> * No OF, host with a list of subdevices. Don't try to mix >> * modes by initialising some groups statically and some >> -- >> 1.9.1 >> > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius