Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

2013-09-26 Thread Uwe Kleine-König 
Hi Libin,

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.
(BTW, calling clk_get_rate on a disabled clk is another thing you
shouldn't do.)

 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 ...)

  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

2013-09-26 Thread Russell King - ARM Linux 
On Thu, Sep 26, 2013 at 10:13:56AM +0200, Uwe Kleine-König wrote:
 Hi Libin,
 
 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.
 (BTW, calling clk_get_rate on a disabled clk is another thing you
 shouldn't do.)

It's a bug for another reason.  Consider this:

clk = devm_clk_get(...);

Now, as the CLK API defines only IS_ERR(clk) to be errors, if clk is NULL
then the devm API will allocate a tracking structure for the allocated
clock.  If you then do:

if (!IS_ERR(clk)) {
if (clk)
devm_clk_put(clk);
clk = NULL;
}

Then this structure won't get freed.  Next time you call devm_clk_get(),
you'll allocate another tracking structure.  If the driver does this a
lot, it will spawn lots of these tracking structures which will only get
cleaned up when the device is unbound (possibly never.)

So, what this driver is doing with its NULL checks against clocks is
buggy, no two ways about it.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

2013-09-26 Thread Libin Yang 
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

--

RE: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

2013-09-26 Thread Libin Yang 
Hi Russell,


-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: Thursday, September 26, 2013 4:24 PM
To: Uwe Kleine-König
Cc: Libin Yang; Jonathan Corbet; Mauro Carvalho Chehab; 
linux-media@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; ker...@pengutronix.de
Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a 
bit)

On Thu, Sep 26, 2013 at 10:13:56AM +0200, Uwe Kleine-König wrote:
 Hi Libin,

 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.
 (BTW, calling clk_get_rate on a disabled clk is another thing you
 shouldn't do.)

It's a bug for another reason.  Consider this:

   clk = devm_clk_get(...);

Now, as the CLK API defines only IS_ERR(clk) to be errors, if clk is NULL then 
the devm
API will allocate a tracking structure for the allocated
clock.  If you then do:

   if (!IS_ERR(clk)) {
   if (clk)
   devm_clk_put(clk);
   clk = NULL;
   }

Then this structure won't get freed.  Next time you call devm_clk_get(), 
you'll allocate another
tracking structure.  If the driver does this a lot, it will spawn lots of 
these tracking structures
which will only get cleaned up when the device is unbound (possibly never.)

So, what this driver is doing with its NULL checks against clocks is buggy, no 
two ways
about it. 

[Libin] Yes, you are right. it will not release the clk tracking structure if 
it is NULL and may allocate again later. It is a bug.

Regards,
Libin
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

2013-09-25 Thread Jonathan Corbet 
On Tue, 24 Sep 2013 20:59:47 +0200
Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote:

 The marvell-ccic does several things wrong or ineffectively in the clock
 handling and it's usage of the devm_* stuff
 
  - it assumes clk_get doesn't return NULL
  - it explicitly calls devm_clk_put instead just keeping the reference
during it's lifetime and let the driver core call it
  - it calls kfree, gpio_free and free_irq for resources it requested
using devm_kzalloc, devm_gpio_request and devm_request_irq
respectively.
  - it mixes devm_ and unmanaged resources which probably results in a
race condition during remove

OK, all of that stuff was added this time around by Libin; my
understanding of that particular hardware is ... minimal.  The basic
idea of the patch seems sound.  I do note, though, that you've changed
the behavior of the driver somewhat.  The MIPI clock is current
obtained at power-up time and released on power-down; you've moved it
to probe time instead, and it's held for the lifetime of the driver.
Perhaps that's even better, I don't know...Libin, what do you say on
that?

The free_irq() call is also removed by a patch previously submitted by
Wei Yongjun.

 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?

Thanks,

jon
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

2013-09-25 Thread Libin Yang 
Hi Jonathan  Uwe,

In the clk enable and prepare function, we will check the NULL pointer. So it 
should be no problem.

For the mipi_clk, it is shared between other components, so we put the clk it 
we don't use it.

For the free_irq, it's my fault. Out before patch really removed this code 
together with gpio free  It missed the last part of the original patch.


Regards,
Libin 

-Original Message-
From: Jonathan Corbet [mailto:cor...@lwn.net]
Sent: Wednesday, September 25, 2013 3:15 PM
To: Uwe Kleine-König
Cc: Mauro Carvalho Chehab; linux-media@vger.kernel.org; linux-arm-
ker...@lists.infradead.org; Russell King; ker...@pengutronix.de; Libin Yang
Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a 
bit)

On Tue, 24 Sep 2013 20:59:47 +0200
Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote:

 The marvell-ccic does several things wrong or ineffectively in the
 clock handling and it's usage of the devm_* stuff

  - it assumes clk_get doesn't return NULL
  - it explicitly calls devm_clk_put instead just keeping the reference
during it's lifetime and let the driver core call it
  - it calls kfree, gpio_free and free_irq for resources it requested
using devm_kzalloc, devm_gpio_request and devm_request_irq
respectively.
  - it mixes devm_ and unmanaged resources which probably results in a
race condition during remove

OK, all of that stuff was added this time around by Libin; my understanding of 
that particular
hardware is ... minimal.  The basic idea of the patch seems sound.  I do note, 
though, that
you've changed the behavior of the driver somewhat.  The MIPI clock is current 
obtained at
power-up time and released on power-down; you've moved it to probe time 
instead, and it's
held for the lifetime of the driver.
Perhaps that's even better, I don't know...Libin, what do you say on that?

The free_irq() call is also removed by a patch previously submitted by Wei 
Yongjun.

 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?

Thanks,

jon

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

2013-09-24 Thread Uwe Kleine-König 
The marvell-ccic does several things wrong or ineffectively in the clock
handling and it's usage of the devm_* stuff

 - it assumes clk_get doesn't return NULL
 - it explicitly calls devm_clk_put instead just keeping the reference
   during it's lifetime and let the driver core call it
 - it calls kfree, gpio_free and free_irq for resources it requested
   using devm_kzalloc, devm_gpio_request and devm_request_irq
   respectively.
 - it mixes devm_ and unmanaged resources which probably results in a
   race condition during remove

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.

Signed-off-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
---
Cc: Libin Yang lby...@marvell.com

Changes since (implicit) v1:
 - really fix the third issue in the list.
 - drop whitespace noise hunk

---
 drivers/media/platform/marvell-ccic/mmp-driver.c | 29 ++--
 1 file changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c 
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index b5a19af..ed16f81e 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -143,7 +143,6 @@ static int mmpcam_power_up(struct mcam_camera *mcam)
struct mmp_camera_platform_data *pdata;
 
if (mcam-bus_type == V4L2_MBUS_CSI2) {
-   cam-mipi_clk = devm_clk_get(mcam-dev, mipi);
if ((IS_ERR(cam-mipi_clk)  mcam-dphy[2] == 0))
return PTR_ERR(cam-mipi_clk);
}
@@ -186,12 +185,6 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
gpio_set_value(pdata-sensor_power_gpio, 0);
gpio_set_value(pdata-sensor_reset_gpio, 0);
 
-   if (mcam-bus_type == V4L2_MBUS_CSI2  !IS_ERR(cam-mipi_clk)) {
-   if (cam-mipi_clk)
-   devm_clk_put(mcam-dev, cam-mipi_clk);
-   cam-mipi_clk = NULL;
-   }
-
mcam_clk_disable(mcam);
 }
 
@@ -325,19 +318,6 @@ static irqreturn_t mmpcam_irq(int irq, void *data)
return IRQ_RETVAL(handled);
 }
 
-static void mcam_deinit_clk(struct mcam_camera *mcam)
-{
-   unsigned int i;
-
-   for (i = 0; i  NR_MCAM_CLK; i++) {
-   if (!IS_ERR(mcam-clk[i])) {
-   if (mcam-clk[i])
-   devm_clk_put(mcam-dev, mcam-clk[i]);
-   }
-   mcam-clk[i] = NULL;
-   }
-}
-
 static void mcam_init_clk(struct mcam_camera *mcam)
 {
unsigned int i;
@@ -371,7 +351,7 @@ static int mmpcam_probe(struct platform_device *pdev)
if (cam == NULL)
return -ENOMEM;
cam-pdev = pdev;
-   cam-mipi_clk = NULL;
+   cam-mipi_clk = devm_clk_get(pdev-dev, mipi);
INIT_LIST_HEAD(cam-devlist);
 
mcam = cam-mcam;
@@ -442,6 +422,7 @@ static int mmpcam_probe(struct platform_device *pdev)
/*
 * Power the device up and hand it off to the core.
 */
+
ret = mmpcam_power_up(mcam);
if (ret)
goto out_deinit_clk;
@@ -470,7 +451,6 @@ out_unregister:
 out_power_down:
mmpcam_power_down(mcam);
 out_deinit_clk:
-   mcam_deinit_clk(mcam);
return ret;
 }
 
@@ -481,16 +461,11 @@ static int mmpcam_remove(struct mmp_camera *cam)
struct mmp_camera_platform_data *pdata;
 
mmpcam_remove_device(cam);
-   free_irq(cam-irq, mcam);
mccic_shutdown(mcam);
mmpcam_power_down(mcam);
pdata = cam-pdev-dev.platform_data;
-   gpio_free(pdata-sensor_reset_gpio);
-   gpio_free(pdata-sensor_power_gpio);
-   mcam_deinit_clk(mcam);
iounmap(cam-power_regs);
iounmap(mcam-regs);
-   kfree(cam);
return 0;
 }
 
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html