[PATCH 1/2] V4L/DVB: radio-si4713: Release i2c adapter in driver cleanup paths

[PATCH 1/2] V4L/DVB: radio-si4713: Release i2c adapter in driver cleanup paths

[Jarkko Nikula]

Call to i2c_put_adapter was missing in radio_si4713_pdriver_probe and
radio_si4713_pdriver_remove.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 drivers/media/radio/radio-si4713.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index 13554ab..0a9fc4d 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -296,14 +296,14 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 	if (!sd) {
 		dev_err(&pdev->dev, "Cannot get v4l2 subdevice\n");
 		rval = -ENODEV;
-		goto unregister_v4l2_dev;
+		goto put_adapter;
 	}
 
 	rsdev->radio_dev = video_device_alloc();
 	if (!rsdev->radio_dev) {
 		dev_err(&pdev->dev, "Failed to alloc video device.\n");
 		rval = -ENOMEM;
-		goto unregister_v4l2_dev;
+		goto put_adapter;
 	}
 
 	memcpy(rsdev->radio_dev, &radio_si4713_vdev_template,
@@ -320,6 +320,8 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 
 free_vdev:
 	video_device_release(rsdev->radio_dev);
+put_adapter:
+	i2c_put_adapter(adapter);
 unregister_v4l2_dev:
 	v4l2_device_unregister(&rsdev->v4l2_dev);
 free_rsdev:
@@ -335,8 +337,12 @@ static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
 	struct radio_si4713_device *rsdev = container_of(v4l2_dev,
 						struct radio_si4713_device,
 						v4l2_dev);
+	struct v4l2_subdev *sd = list_entry(v4l2_dev->subdevs.next,
+					    struct v4l2_subdev, list);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
 	video_unregister_device(rsdev->radio_dev);
+	i2c_put_adapter(client->adapter);
 	v4l2_device_unregister(&rsdev->v4l2_dev);
 	kfree(rsdev);
 
-- 
1.7.1

[PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support

[Jarkko Nikula]

Convert the driver to use regulator framework instead of set_power callback.
This with gpio_reset platform data provide cleaner way to manage chip VIO,
VDD and reset signal inside the driver.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
---
I don't have specifications for this chip so I don't know how long the
reset signal must be active after power-up. I used 50 us from Maemo
kernel sources for Nokia N900 and I can successfully enable-disable
transmitter on N900 with vdd power cycling.
---
 drivers/media/radio/radio-si4713.c |   20 ++++++++++++++-
 drivers/media/radio/si4713-i2c.c   |   48 ++++++++++++++++++++++++++++-------
 drivers/media/radio/si4713-i2c.h   |    3 +-
 include/media/si4713.h             |    3 +-
 4 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index 0a9fc4d..c666012 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -28,6 +28,7 @@
 #include <linux/i2c.h>
 #include <linux/videodev2.h>
 #include <linux/slab.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ioctl.h>
@@ -48,6 +49,7 @@ MODULE_VERSION("0.0.1");
 struct radio_si4713_device {
 	struct v4l2_device		v4l2_dev;
 	struct video_device		*radio_dev;
+	struct regulator		*reg_vio;
 };
 
 /* radio_si4713_fops - file operations interface */
@@ -283,12 +285,22 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 		goto free_rsdev;
 	}
 
+	rsdev->reg_vio = regulator_get(&pdev->dev, "vio");
+	if (IS_ERR(rsdev->reg_vio)) {
+		dev_err(&pdev->dev, "Cannot get vio regulator\n");
+		rval = PTR_ERR(rsdev->reg_vio);
+		goto unregister_v4l2_dev;
+	}
+	rval = regulator_enable(rsdev->reg_vio);
+	if (rval)
+		goto reg_put;
+
 	adapter = i2c_get_adapter(pdata->i2c_bus);
 	if (!adapter) {
 		dev_err(&pdev->dev, "Cannot get i2c adapter %d\n",
 							pdata->i2c_bus);
 		rval = -ENODEV;
-		goto unregister_v4l2_dev;
+		goto reg_disable;
 	}
 
 	sd = v4l2_i2c_new_subdev_board(&rsdev->v4l2_dev, adapter, "si4713_i2c",
@@ -322,6 +334,10 @@ free_vdev:
 	video_device_release(rsdev->radio_dev);
 put_adapter:
 	i2c_put_adapter(adapter);
+reg_disable:
+	regulator_disable(rsdev->reg_vio);
+reg_put:
+	regulator_put(rsdev->reg_vio);
 unregister_v4l2_dev:
 	v4l2_device_unregister(&rsdev->v4l2_dev);
 free_rsdev:
@@ -343,6 +359,8 @@ static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
 
 	video_unregister_device(rsdev->radio_dev);
 	i2c_put_adapter(client->adapter);
+	regulator_disable(rsdev->reg_vio);
+	regulator_put(rsdev->reg_vio);
 	v4l2_device_unregister(&rsdev->v4l2_dev);
 	kfree(rsdev);
 
diff --git a/drivers/media/radio/si4713-i2c.c b/drivers/media/radio/si4713-i2c.c
index ab63dd5..4b5470c 100644
--- a/drivers/media/radio/si4713-i2c.c
+++ b/drivers/media/radio/si4713-i2c.c
@@ -27,6 +27,8 @@
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-common.h>
@@ -369,7 +371,12 @@ static int si4713_powerup(struct si4713_device *sdev)
 	if (sdev->power_state)
 		return 0;
 
-	sdev->platform_data->set_power(1);
+	regulator_enable(sdev->reg_vdd);
+	if (gpio_is_valid(sdev->gpio_reset)) {
+		udelay(50);
+		gpio_set_value(sdev->gpio_reset, 1);
+	}
+
 	err = si4713_send_command(sdev, SI4713_CMD_POWER_UP,
 					args, ARRAY_SIZE(args),
 					resp, ARRAY_SIZE(resp),
@@ -384,7 +391,9 @@ static int si4713_powerup(struct si4713_device *sdev)
 		err = si4713_write_property(sdev, SI4713_GPO_IEN,
 						SI4713_STC_INT | SI4713_CTS);
 	} else {
-		sdev->platform_data->set_power(0);
+		if (gpio_is_valid(sdev->gpio_reset))
+			gpio_set_value(sdev->gpio_reset, 0);
+		regulator_disable(sdev->reg_vdd);
 	}
 
 	return err;
@@ -411,7 +420,9 @@ static int si4713_powerdown(struct si4713_device *sdev)
 		v4l2_dbg(1, debug, &sdev->sd, "Power down response: 0x%02x\n",
 				resp[0]);
 		v4l2_dbg(1, debug, &sdev->sd, "Device in reset mode\n");
-		sdev->platform_data->set_power(0);
+		if (gpio_is_valid(sdev->gpio_reset))
+			gpio_set_value(sdev->gpio_reset, 0);
+		regulator_disable(sdev->reg_vdd);
 		sdev->power_state = POWER_OFF;
 	}
 
@@ -1959,6 +1970,7 @@ static int si4713_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
 	struct si4713_device *sdev;
+	struct si4713_platform_data *pdata = client->dev.platform_data;
 	int rval;
 
 	sdev = kzalloc(sizeof *sdev, GFP_KERNEL);
@@ -1968,11 +1980,20 @@ static int si4713_probe(struct i2c_client *client,
 		goto exit;
 	}
 
-	sdev->platform_data = client->dev.platform_data;
-	if (!sdev->platform_data) {
-		v4l2_err(&sdev->sd, "No platform data registered.\n");
-		rval = -ENODEV;
-		goto free_sdev;
+	sdev->gpio_reset = -1;
+	if (pdata && gpio_is_valid(pdata->gpio_reset)) {
+		rval = gpio_request(pdata->gpio_reset, "si4713 reset");
+		if (rval)
+			goto free_sdev;
+		sdev->gpio_reset = pdata->gpio_reset;
+		gpio_direction_output(sdev->gpio_reset, 0);
+	}
+
+	sdev->reg_vdd = regulator_get(&client->dev, "vdd");
+	if (IS_ERR(sdev->reg_vdd)) {
+		dev_err(&client->dev, "Cannot get vdd regulator\n");
+		rval = PTR_ERR(sdev->reg_vdd);
+		goto free_gpio;
 	}
 
 	v4l2_i2c_subdev_init(&sdev->sd, client, &si4713_subdev_ops);
@@ -1986,7 +2007,7 @@ static int si4713_probe(struct i2c_client *client,
 			client->name, sdev);
 		if (rval < 0) {
 			v4l2_err(&sdev->sd, "Could not request IRQ\n");
-			goto free_sdev;
+			goto put_reg;
 		}
 		v4l2_dbg(1, debug, &sdev->sd, "IRQ requested.\n");
 	} else {
@@ -2004,6 +2025,11 @@ static int si4713_probe(struct i2c_client *client,
 free_irq:
 	if (client->irq)
 		free_irq(client->irq, sdev);
+put_reg:
+	regulator_put(sdev->reg_vdd);
+free_gpio:
+	if (gpio_is_valid(sdev->gpio_reset))
+		gpio_free(sdev->gpio_reset);
 free_sdev:
 	kfree(sdev);
 exit:
@@ -2023,7 +2049,9 @@ static int si4713_remove(struct i2c_client *client)
 		free_irq(client->irq, sdev);
 
 	v4l2_device_unregister_subdev(sd);
-
+	regulator_put(sdev->reg_vdd);
+	if (gpio_is_valid(sdev->gpio_reset))
+		gpio_free(sdev->gpio_reset);
 	kfree(sdev);
 
 	return 0;
diff --git a/drivers/media/radio/si4713-i2c.h b/drivers/media/radio/si4713-i2c.h
index faf8cff..cf79f6e 100644
--- a/drivers/media/radio/si4713-i2c.h
+++ b/drivers/media/radio/si4713-i2c.h
@@ -220,11 +220,12 @@ struct si4713_device {
 	/* private data structures */
 	struct mutex mutex;
 	struct completion work;
-	struct si4713_platform_data *platform_data;
 	struct rds_info rds_info;
 	struct limiter_info limiter_info;
 	struct pilot_info pilot_info;
 	struct acomp_info acomp_info;
+	struct regulator *reg_vdd;
+	int gpio_reset;
 	u32 frequency;
 	u32 preemphasis;
 	u32 mute;
diff --git a/include/media/si4713.h b/include/media/si4713.h
index 99850a5..ed7353e 100644
--- a/include/media/si4713.h
+++ b/include/media/si4713.h
@@ -23,8 +23,7 @@
  * Platform dependent definition
  */
 struct si4713_platform_data {
-	/* Set power state, zero is off, non-zero is on. */
-	int (*set_power)(int power);
+	int gpio_reset; /* < 0 if not used */
 };
 
 /*
-- 
1.7.1

Re: [PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support

[Jarkko Nikula]

On Sun, 13 Jun 2010 21:09:28 +0300
Jarkko Nikula <jhnikula@gmail.com> wrote:

> Convert the driver to use regulator framework instead of set_power callback.
> This with gpio_reset platform data provide cleaner way to manage chip VIO,
> VDD and reset signal inside the driver.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
> I don't have specifications for this chip so I don't know how long the
> reset signal must be active after power-up. I used 50 us from Maemo
> kernel sources for Nokia N900 and I can successfully enable-disable
> transmitter on N900 with vdd power cycling.
> ---

Ping? Any comments to these two Si4713 patches?


-- 
Jarkko

Re: [PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support

[Eduardo Valentin]

Hello Jarkko,

On Sun, Jun 13, 2010 at 08:09:28PM +0200, Jarkko Nikula wrote:
> Convert the driver to use regulator framework instead of set_power callback.
> This with gpio_reset platform data provide cleaner way to manage chip VIO,
> VDD and reset signal inside the driver.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
> I don't have specifications for this chip so I don't know how long the
> reset signal must be active after power-up. I used 50 us from Maemo
> kernel sources for Nokia N900 and I can successfully enable-disable
> transmitter on N900 with vdd power cycling.
> ---
>  drivers/media/radio/radio-si4713.c |   20 ++++++++++++++-
>  drivers/media/radio/si4713-i2c.c   |   48 ++++++++++++++++++++++++++++-------
>  drivers/media/radio/si4713-i2c.h   |    3 +-
>  include/media/si4713.h             |    3 +-

Could you please elaborate a bit more on the fact that you have put vio on
the platform driver and vdd on the i2c driver?

>  4 files changed, 60 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
> index 0a9fc4d..c666012 100644
> --- a/drivers/media/radio/radio-si4713.c
> +++ b/drivers/media/radio/radio-si4713.c
> @@ -28,6 +28,7 @@
>  #include <linux/i2c.h>
>  #include <linux/videodev2.h>
>  #include <linux/slab.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ioctl.h>
> @@ -48,6 +49,7 @@ MODULE_VERSION("0.0.1");
>  struct radio_si4713_device {
>  	struct v4l2_device		v4l2_dev;
>  	struct video_device		*radio_dev;
> +	struct regulator		*reg_vio;
>  };
>  
>  /* radio_si4713_fops - file operations interface */
> @@ -283,12 +285,22 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  		goto free_rsdev;
>  	}
>  
> +	rsdev->reg_vio = regulator_get(&pdev->dev, "vio");
> +	if (IS_ERR(rsdev->reg_vio)) {
> +		dev_err(&pdev->dev, "Cannot get vio regulator\n");
> +		rval = PTR_ERR(rsdev->reg_vio);
> +		goto unregister_v4l2_dev;
> +	}
> +	rval = regulator_enable(rsdev->reg_vio);
> +	if (rval)
> +		goto reg_put;
> +
>  	adapter = i2c_get_adapter(pdata->i2c_bus);
>  	if (!adapter) {
>  		dev_err(&pdev->dev, "Cannot get i2c adapter %d\n",
>  							pdata->i2c_bus);
>  		rval = -ENODEV;
> -		goto unregister_v4l2_dev;
> +		goto reg_disable;
>  	}
>  
>  	sd = v4l2_i2c_new_subdev_board(&rsdev->v4l2_dev, adapter, "si4713_i2c",
> @@ -322,6 +334,10 @@ free_vdev:
>  	video_device_release(rsdev->radio_dev);
>  put_adapter:
>  	i2c_put_adapter(adapter);
> +reg_disable:
> +	regulator_disable(rsdev->reg_vio);
> +reg_put:
> +	regulator_put(rsdev->reg_vio);
>  unregister_v4l2_dev:
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  free_rsdev:
> @@ -343,6 +359,8 @@ static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
>  
>  	video_unregister_device(rsdev->radio_dev);
>  	i2c_put_adapter(client->adapter);
> +	regulator_disable(rsdev->reg_vio);
> +	regulator_put(rsdev->reg_vio);
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  	kfree(rsdev);
>  
> diff --git a/drivers/media/radio/si4713-i2c.c b/drivers/media/radio/si4713-i2c.c
> index ab63dd5..4b5470c 100644
> --- a/drivers/media/radio/si4713-i2c.c
> +++ b/drivers/media/radio/si4713-i2c.c
> @@ -27,6 +27,8 @@
>  #include <linux/interrupt.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-common.h>
> @@ -369,7 +371,12 @@ static int si4713_powerup(struct si4713_device *sdev)
>  	if (sdev->power_state)
>  		return 0;
>  
> -	sdev->platform_data->set_power(1);
> +	regulator_enable(sdev->reg_vdd);
> +	if (gpio_is_valid(sdev->gpio_reset)) {
> +		udelay(50);
> +		gpio_set_value(sdev->gpio_reset, 1);
> +	}
> +
>  	err = si4713_send_command(sdev, SI4713_CMD_POWER_UP,
>  					args, ARRAY_SIZE(args),
>  					resp, ARRAY_SIZE(resp),
> @@ -384,7 +391,9 @@ static int si4713_powerup(struct si4713_device *sdev)
>  		err = si4713_write_property(sdev, SI4713_GPO_IEN,
>  						SI4713_STC_INT | SI4713_CTS);
>  	} else {
> -		sdev->platform_data->set_power(0);
> +		if (gpio_is_valid(sdev->gpio_reset))
> +			gpio_set_value(sdev->gpio_reset, 0);
> +		regulator_disable(sdev->reg_vdd);
>  	}
>  
>  	return err;
> @@ -411,7 +420,9 @@ static int si4713_powerdown(struct si4713_device *sdev)
>  		v4l2_dbg(1, debug, &sdev->sd, "Power down response: 0x%02x\n",
>  				resp[0]);
>  		v4l2_dbg(1, debug, &sdev->sd, "Device in reset mode\n");
> -		sdev->platform_data->set_power(0);
> +		if (gpio_is_valid(sdev->gpio_reset))
> +			gpio_set_value(sdev->gpio_reset, 0);
> +		regulator_disable(sdev->reg_vdd);
>  		sdev->power_state = POWER_OFF;
>  	}
>  
> @@ -1959,6 +1970,7 @@ static int si4713_probe(struct i2c_client *client,
>  					const struct i2c_device_id *id)
>  {
>  	struct si4713_device *sdev;
> +	struct si4713_platform_data *pdata = client->dev.platform_data;
>  	int rval;
>  
>  	sdev = kzalloc(sizeof *sdev, GFP_KERNEL);
> @@ -1968,11 +1980,20 @@ static int si4713_probe(struct i2c_client *client,
>  		goto exit;
>  	}
>  
> -	sdev->platform_data = client->dev.platform_data;
> -	if (!sdev->platform_data) {
> -		v4l2_err(&sdev->sd, "No platform data registered.\n");
> -		rval = -ENODEV;
> -		goto free_sdev;
> +	sdev->gpio_reset = -1;
> +	if (pdata && gpio_is_valid(pdata->gpio_reset)) {
> +		rval = gpio_request(pdata->gpio_reset, "si4713 reset");
> +		if (rval)
> +			goto free_sdev;
> +		sdev->gpio_reset = pdata->gpio_reset;
> +		gpio_direction_output(sdev->gpio_reset, 0);
> +	}
> +
> +	sdev->reg_vdd = regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(sdev->reg_vdd)) {
> +		dev_err(&client->dev, "Cannot get vdd regulator\n");
> +		rval = PTR_ERR(sdev->reg_vdd);
> +		goto free_gpio;
>  	}
>  
>  	v4l2_i2c_subdev_init(&sdev->sd, client, &si4713_subdev_ops);
> @@ -1986,7 +2007,7 @@ static int si4713_probe(struct i2c_client *client,
>  			client->name, sdev);
>  		if (rval < 0) {
>  			v4l2_err(&sdev->sd, "Could not request IRQ\n");
> -			goto free_sdev;
> +			goto put_reg;
>  		}
>  		v4l2_dbg(1, debug, &sdev->sd, "IRQ requested.\n");
>  	} else {
> @@ -2004,6 +2025,11 @@ static int si4713_probe(struct i2c_client *client,
>  free_irq:
>  	if (client->irq)
>  		free_irq(client->irq, sdev);
> +put_reg:
> +	regulator_put(sdev->reg_vdd);
> +free_gpio:
> +	if (gpio_is_valid(sdev->gpio_reset))
> +		gpio_free(sdev->gpio_reset);
>  free_sdev:
>  	kfree(sdev);
>  exit:
> @@ -2023,7 +2049,9 @@ static int si4713_remove(struct i2c_client *client)
>  		free_irq(client->irq, sdev);
>  
>  	v4l2_device_unregister_subdev(sd);
> -
> +	regulator_put(sdev->reg_vdd);
> +	if (gpio_is_valid(sdev->gpio_reset))
> +		gpio_free(sdev->gpio_reset);
>  	kfree(sdev);
>  
>  	return 0;
> diff --git a/drivers/media/radio/si4713-i2c.h b/drivers/media/radio/si4713-i2c.h
> index faf8cff..cf79f6e 100644
> --- a/drivers/media/radio/si4713-i2c.h
> +++ b/drivers/media/radio/si4713-i2c.h
> @@ -220,11 +220,12 @@ struct si4713_device {
>  	/* private data structures */
>  	struct mutex mutex;
>  	struct completion work;
> -	struct si4713_platform_data *platform_data;
>  	struct rds_info rds_info;
>  	struct limiter_info limiter_info;
>  	struct pilot_info pilot_info;
>  	struct acomp_info acomp_info;
> +	struct regulator *reg_vdd;
> +	int gpio_reset;
>  	u32 frequency;
>  	u32 preemphasis;
>  	u32 mute;
> diff --git a/include/media/si4713.h b/include/media/si4713.h
> index 99850a5..ed7353e 100644
> --- a/include/media/si4713.h
> +++ b/include/media/si4713.h
> @@ -23,8 +23,7 @@
>   * Platform dependent definition
>   */
>  struct si4713_platform_data {
> -	/* Set power state, zero is off, non-zero is on. */
> -	int (*set_power)(int power);
> +	int gpio_reset; /* < 0 if not used */
>  };
>  
>  /*
> -- 
> 1.7.1

-- 
---
Eduardo Valentin

Re: [PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support

[Jarkko Nikula]

Hi

On Tue, 7 Sep 2010 22:49:49 +0300
Eduardo Valentin <eduardo.valentin@nokia.com> wrote:

> Hello Jarkko,
> 
> On Sun, Jun 13, 2010 at 08:09:28PM +0200, Jarkko Nikula wrote:
> > Convert the driver to use regulator framework instead of set_power callback.
> > This with gpio_reset platform data provide cleaner way to manage chip VIO,
> > VDD and reset signal inside the driver.
> > 
> > Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> > Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> > ---
> > I don't have specifications for this chip so I don't know how long the
> > reset signal must be active after power-up. I used 50 us from Maemo
> > kernel sources for Nokia N900 and I can successfully enable-disable
> > transmitter on N900 with vdd power cycling.
> > ---
> >  drivers/media/radio/radio-si4713.c |   20 ++++++++++++++-
> >  drivers/media/radio/si4713-i2c.c   |   48 ++++++++++++++++++++++++++++-------
> >  drivers/media/radio/si4713-i2c.h   |    3 +-
> >  include/media/si4713.h             |    3 +-
> 
> Could you please elaborate a bit more on the fact that you have put vio on
> the platform driver and vdd on the i2c driver?
> 
This is good question and let me explain. The regulator management
division between these two files were based on my assumption that only
VIO is needed and must be on before probing the chip on I2C bus.

Another assumption was that only VDD can realistically be managed
runtime in si4713_powerup function. I think usually IO voltages cannot
be shutdown even in suspend while the system is powered so I let the
driver to keep the VIO enabled as long as the module is loaded.


-- 
Jarkko

Re: [PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support

[Eduardo Valentin]

Hello,

On Wed, Sep 08, 2010 at 07:59:38AM +0200, Jarkko Nikula wrote:
> Hi
> 
> On Tue, 7 Sep 2010 22:49:49 +0300
> Eduardo Valentin <eduardo.valentin@nokia.com> wrote:
> 
> > Hello Jarkko,
> > 
> > On Sun, Jun 13, 2010 at 08:09:28PM +0200, Jarkko Nikula wrote:
> > > Convert the driver to use regulator framework instead of set_power callback.
> > > This with gpio_reset platform data provide cleaner way to manage chip VIO,
> > > VDD and reset signal inside the driver.
> > > 
> > > Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> > > Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> > > ---
> > > I don't have specifications for this chip so I don't know how long the
> > > reset signal must be active after power-up. I used 50 us from Maemo
> > > kernel sources for Nokia N900 and I can successfully enable-disable
> > > transmitter on N900 with vdd power cycling.
> > > ---
> > >  drivers/media/radio/radio-si4713.c |   20 ++++++++++++++-
> > >  drivers/media/radio/si4713-i2c.c   |   48 ++++++++++++++++++++++++++++-------
> > >  drivers/media/radio/si4713-i2c.h   |    3 +-
> > >  include/media/si4713.h             |    3 +-
> > 
> > Could you please elaborate a bit more on the fact that you have put vio on
> > the platform driver and vdd on the i2c driver?
> > 
> This is good question and let me explain. The regulator management
> division between these two files were based on my assumption that only
> VIO is needed and must be on before probing the chip on I2C bus.
> 
> Another assumption was that only VDD can realistically be managed
> runtime in si4713_powerup function. I think usually IO voltages cannot
> be shutdown even in suspend while the system is powered so I let the
> driver to keep the VIO enabled as long as the module is loaded.

OK. I kinda agree with you in this reasoning.

My concern here is that splitting the regulator usage into two entities
could cause some troubles.

The background here you are probably missing is that the split between
i2c and platform drivers. That has been done because we were thinking also
in the situation where the si4713 i2c driver could be used without the
platform driver. I mean, the i2c code could be re-used for instance by
other v4l2 driver, if that is driving a device which has also si4713.
So, in this sense, the current platform is essentially a wrapper.
And if you split the regulator usage in that way,
we would probably be loosing that.

And apart from that, it is also bad from the regfw point of view as well.
I believe the idea is that the driver itself must take care of all needed
regulators. The way you have done, looks like the platform driver needs only
VIO and the i2c needs only VDD. And to my understanding, the i2c needs both
in order to work. So, my suggestion is to move everything to the i2c driver.

> 
> 
> -- 
> Jarkko

---
Eduardo Valentin

Re: [PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support

[Jarkko Nikula]

On Wed, 8 Sep 2010 15:11:36 +0300
Eduardo Valentin <eduardo.valentin@nokia.com> wrote:

> The background here you are probably missing is that the split between
> i2c and platform drivers. That has been done because we were thinking also
> in the situation where the si4713 i2c driver could be used without the
> platform driver. I mean, the i2c code could be re-used for instance by
> other v4l2 driver, if that is driving a device which has also si4713.
> So, in this sense, the current platform is essentially a wrapper.
> And if you split the regulator usage in that way,
> we would probably be loosing that.
> 
This is good to know. In that sense it would be good to have some
common place for managing the VIO here.

> And apart from that, it is also bad from the regfw point of view as well.
> I believe the idea is that the driver itself must take care of all needed
> regulators. The way you have done, looks like the platform driver needs only
> VIO and the i2c needs only VDD. And to my understanding, the i2c needs both
> in order to work. So, my suggestion is to move everything to the i2c driver.
> 
Problem of course is that the chip cannot be probed if the VIO is
missing so it must be on before the chip is probed. Quite many i2c
drivers seems to rely that the VIO is on before probing. Therefore I
did here the VIO enable in platform driver as there were this instance
on top of i2c driver. I think perfect solution would require some sort
of support to i2c core.


-- 
Jarkko

Re: [PATCH 1/2] V4L/DVB: radio-si4713: Release i2c adapter in driver cleanup paths

[Mauro Carvalho Chehab]

Hi Eduardo,

Could you please review those two patches?

Thanks,
Mauro.

Em 13-06-2010 15:09, Jarkko Nikula escreveu:
> Call to i2c_put_adapter was missing in radio_si4713_pdriver_probe and
> radio_si4713_pdriver_remove.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  drivers/media/radio/radio-si4713.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
> index 13554ab..0a9fc4d 100644
> --- a/drivers/media/radio/radio-si4713.c
> +++ b/drivers/media/radio/radio-si4713.c
> @@ -296,14 +296,14 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  	if (!sd) {
>  		dev_err(&pdev->dev, "Cannot get v4l2 subdevice\n");
>  		rval = -ENODEV;
> -		goto unregister_v4l2_dev;
> +		goto put_adapter;
>  	}
>  
>  	rsdev->radio_dev = video_device_alloc();
>  	if (!rsdev->radio_dev) {
>  		dev_err(&pdev->dev, "Failed to alloc video device.\n");
>  		rval = -ENOMEM;
> -		goto unregister_v4l2_dev;
> +		goto put_adapter;
>  	}
>  
>  	memcpy(rsdev->radio_dev, &radio_si4713_vdev_template,
> @@ -320,6 +320,8 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  
>  free_vdev:
>  	video_device_release(rsdev->radio_dev);
> +put_adapter:
> +	i2c_put_adapter(adapter);
>  unregister_v4l2_dev:
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  free_rsdev:
> @@ -335,8 +337,12 @@ static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
>  	struct radio_si4713_device *rsdev = container_of(v4l2_dev,
>  						struct radio_si4713_device,
>  						v4l2_dev);
> +	struct v4l2_subdev *sd = list_entry(v4l2_dev->subdevs.next,
> +					    struct v4l2_subdev, list);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
>  	video_unregister_device(rsdev->radio_dev);
> +	i2c_put_adapter(client->adapter);
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  	kfree(rsdev);
>  

Re: [PATCH 1/2] V4L/DVB: radio-si4713: Release i2c adapter in driver cleanup paths

[Jarkko Nikula]

On Mon, 05 Jul 2010 16:28:30 -0300
Mauro Carvalho Chehab <maurochehab@gmail.com> wrote:

> Hi Eduardo,
> 
> Could you please review those two patches?
> 
Hmm.. are these two patches already late for 2.6.36? I have two another
patches to arch/arm/mach-omap2/board-rx51-peripherals.c and
sound/soc/omap/rx51.c that are pending from patch 2/2.


-- 
Jarkko

Re: [PATCH 1/2] V4L/DVB: radio-si4713: Release i2c adapter in driver cleanup paths

[Jarkko Nikula]

Hi

On Mon, 2 Aug 2010 13:09:52 +0300
Jarkko Nikula <jhnikula@gmail.com> wrote:

> On Mon, 05 Jul 2010 16:28:30 -0300
> Mauro Carvalho Chehab <maurochehab@gmail.com> wrote:
> 
> > Hi Eduardo,
> > 
> > Could you please review those two patches?
> > 
> Hmm.. are these two patches already late for 2.6.36? I have two another
> patches to arch/arm/mach-omap2/board-rx51-peripherals.c and
> sound/soc/omap/rx51.c that are pending from patch 2/2.
> 
Is there anything I can do with these two patches? Would reposting to
lkml make any sense (like getting nak/ack about regulator framework
conversion etc)? At least I would like to drive these patches to some
conclusion :-)

https://patchwork.kernel.org/patch/105846/
https://patchwork.kernel.org/patch/105847/


-- 
Jarkko

Re: [PATCH 1/2] V4L/DVB: radio-si4713: Release i2c adapter in driver cleanup paths

[Eduardo Valentin]

Jarkko and Mauro,

Apologies for the long delay.

On Sun, Jun 13, 2010 at 08:09:27PM +0200, Jarkko Nikula wrote:
> Call to i2c_put_adapter was missing in radio_si4713_pdriver_probe and
> radio_si4713_pdriver_remove.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>

Acked-by: Eduardo Valentin <eduardo.valentin@nokia.com>

> ---
>  drivers/media/radio/radio-si4713.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
> index 13554ab..0a9fc4d 100644
> --- a/drivers/media/radio/radio-si4713.c
> +++ b/drivers/media/radio/radio-si4713.c
> @@ -296,14 +296,14 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  	if (!sd) {
>  		dev_err(&pdev->dev, "Cannot get v4l2 subdevice\n");
>  		rval = -ENODEV;
> -		goto unregister_v4l2_dev;
> +		goto put_adapter;
>  	}
>  
>  	rsdev->radio_dev = video_device_alloc();
>  	if (!rsdev->radio_dev) {
>  		dev_err(&pdev->dev, "Failed to alloc video device.\n");
>  		rval = -ENOMEM;
> -		goto unregister_v4l2_dev;
> +		goto put_adapter;
>  	}
>  
>  	memcpy(rsdev->radio_dev, &radio_si4713_vdev_template,
> @@ -320,6 +320,8 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  
>  free_vdev:
>  	video_device_release(rsdev->radio_dev);
> +put_adapter:
> +	i2c_put_adapter(adapter);
>  unregister_v4l2_dev:
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  free_rsdev:
> @@ -335,8 +337,12 @@ static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
>  	struct radio_si4713_device *rsdev = container_of(v4l2_dev,
>  						struct radio_si4713_device,
>  						v4l2_dev);
> +	struct v4l2_subdev *sd = list_entry(v4l2_dev->subdevs.next,
> +					    struct v4l2_subdev, list);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
>  	video_unregister_device(rsdev->radio_dev);
> +	i2c_put_adapter(client->adapter);
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  	kfree(rsdev);
>  
> -- 
> 1.7.1

-- 
---
Eduardo Valentin