Re: [PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support
On Wed, 8 Sep 2010 15:11:36 +0300 Eduardo Valentin 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 -- 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 2/2] V4L/DVB: radio-si4713: Add regulator framework support
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 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 > > > Cc: Eduardo Valentin > > > --- > > > 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 -- 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 2/2] V4L/DVB: radio-si4713: Add regulator framework support
Hi On Tue, 7 Sep 2010 22:49:49 +0300 Eduardo Valentin 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 > > Cc: Eduardo Valentin > > --- > > 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 -- 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 2/2] V4L/DVB: radio-si4713: Add regulator framework support
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
> Cc: Eduardo Valentin
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -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))
> +
Re: [PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support
On Sun, 13 Jun 2010 21:09:28 +0300 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 > Cc: Eduardo Valentin > --- > 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 -- 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
[PATCH 2/2] V4L/DVB: radio-si4713: Add regulator framework support
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
Cc: Eduardo Valentin
---
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
#include
#include
+#include
#include
#include
#include
@@ -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
#include
#include
+#include
+#include
#include
#include
#include
@@ -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;
+
