On Tue, 27 Nov 2012, Libin Yang wrote: > Hi Guennadi, > > >-----Original Message----- > >From: Guennadi Liakhovetski [mailto:g.liakhovetski@xxxxxx] > >Sent: Wednesday, November 28, 2012 3:14 PM > >To: Libin Yang > >Cc: Albert Wang; corbet@xxxxxxx; linux-media@xxxxxxxxxxxxxxx > >Subject: RE: [PATCH 02/15] [media] marvell-ccic: add MIPI support for marvell-ccic driver > > > >On Tue, 27 Nov 2012, Libin Yang wrote: > > > >> Hello Guennadi, > >> > >> Please see my comments below. > >> > >> Best Regards, > >> Libin > >> > >> >-----Original Message----- > >> >From: Albert Wang > >> >Sent: Tuesday, November 27, 2012 7:21 PM > >> >To: Guennadi Liakhovetski > >> >Cc: corbet@xxxxxxx; linux-media@xxxxxxxxxxxxxxx; Libin Yang > >> >Subject: RE: [PATCH 02/15] [media] marvell-ccic: add MIPI support for marvell-ccic > >driver > >> > > >> >Hi, Guennadi > >> > > >> >We will update the patch by following your good suggestion! :) > >> > > >> > >> [snip] > >> > >> >>> + pll1 = clk_get(dev, "pll1"); > >> >>> + if (IS_ERR(pll1)) { > >> >>> + dev_err(dev, "Could not get pll1 clock\n"); > >> >>> + return; > >> >>> + } > >> >>> + > >> >>> + tx_clk_esc = clk_get_rate(pll1) / 1000000 / 12; > >> >>> + clk_put(pll1); > >> >> > >> >>Once you release your clock per "clk_put()" its rate can be changed by some other user, > >> >>so, your tx_clk_esc becomes useless. Better keep the reference to the clock until clean > >up. > >> >>Maybe you can also use > >> >>devm_clk_get() to simplify the clean up. > >> >> > >> >That's a good suggestion. > >> > > >> [Libin] In our code design, the pll1 will never be changed after the system boots up. > >Camera and other components can only get the clk without modifying it. > > > >This doesn't matter. We have a standard API and we have to abide to its > >rules. Your driver can be reused or its code can be copied by others. I > >don't think it should be too difficult to just issue devm_clk_get() once > >and then just forget about it. > > [Libin] Yes, you are right. We should consider the driver may be reused. > I didn't realize it. Another question is: If we use devm_clk_get(), what > I understand, the clk will be put when the device is being released. It > means the driver will hold the clk all the time the driver is in the > kernel. What do you think if we get the clk when opening the camera, and > put it in the close? Sure, that's fine too. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.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
