Hi Uwe, Thanks for your reviewing. Please see the comments below. >On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote: >> In the clk enable and prepare function, we will check the NULL >> pointer. So it should be no problem. >I'm not sure what you mean here and unfortunately your quoting style makes your statement >appear without context. > > if (... && !IS_ERR(cam->mipi_clk)) { > if (cam->mipi_clk) > devm_clk_put(mcam->dev, cam->mipi_clk); > cam->mipi_clk = NULL; > } > >might work in your setup, but it's wrong usage of the clk API. There is no reason NULL >couldn't be a valid clk pointer. Moreover I cannot find a place in that driver that calls prepare >and/or enable for the mipi_clk. [Libin] Right. NULL could be a valid clk pointer. In the code, the clk will not be released if mipi_clk is NULL. Is below OK? + if (... && !IS_ERR(cam->mipi_clk)) { + devm_clk_put(mcam->dev, cam->mipi_clk); + cam->mipi_clk = NULL; + } Set cam->mipi_clk = NULL is let cam->mipi_clk to be the initial state just like after cam is allocated. >(BTW, calling clk_get_rate on a disabled clk is another thing you shouldn't do.) [Libin] Thanks for pointing it out. We enable the clk in other components. Yes, you are right. We should enable the clk explicitly here. > >> For the mipi_clk, it is shared between other components, so we put the clk it we don't use it. >There should be no problem if >1 driver holds a reference to the clock. It's clk_disable (+ >maybe clk_unprepare) you should call when you don't need it any more. (But even having >1 >driver enabling a clk isn't a problem ...) [Libin] So you mean we need not release the clk here and wait for devm to release it later? I will check it with my colleagues to see whether they are OK with this. > >> >> This patch fixes all but the last issue in this list. This patch >> >> doesn't introduce new reasons for not building, but there are >> >> already several bulid problems. >> > >> >Care to report those? >Sure: > > CC drivers/media/platform/marvell-ccic/mmp-driver.o > drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_calc_dphy': > drivers/media/platform/marvell-ccic/mmp-driver.c:264:15: error: 'struct >mmp_camera_platform_data' has no member named 'dphy3_algo' > drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: error: >'DPHY3_ALGO_PXA910' undeclared (first use in this function) > drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: note: each undeclared >identifier is reported only once for each function it appears in > drivers/media/platform/marvell-ccic/mmp-driver.c:269:8: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:270:17: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:271:16: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:273:7: error: >'DPHY3_ALGO_PXA2128' undeclared (first use in this function) > drivers/media/platform/marvell-ccic/mmp-driver.c:277:8: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:278:17: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:279:16: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:308:7: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:312:104: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:312:120: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:312:136: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_probe': > drivers/media/platform/marvell-ccic/mmp-driver.c:385:24: error: 'struct >mmp_camera_platform_data' has no member named 'mclk_min' > drivers/media/platform/marvell-ccic/mmp-driver.c:386:24: error: 'struct >mmp_camera_platform_data' has no member named 'mclk_src' > drivers/media/platform/marvell-ccic/mmp-driver.c:387:24: error: 'struct >mmp_camera_platform_data' has no member named 'mclk_div' > drivers/media/platform/marvell-ccic/mmp-driver.c:388:24: error: 'struct >mmp_camera_platform_data' has no member named 'bus_type' > drivers/media/platform/marvell-ccic/mmp-driver.c:389:20: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:391:20: error: 'struct >mmp_camera_platform_data' has no member named 'lane' > >Best regards >Uwe > >-- >Pengutronix e.K. | Uwe Kleine-König | >Industrial Linux Solutions | http://www.pengutronix.de/ | -- 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