[RFCv2 0/8] [media] si4713 DT binding

[RFCv2 0/8] [media] si4713 DT binding

[Sebastian Reichel]

Hi,

This is the RFCv2 patchset adding DT support to the si4713
radio transmitter i2c driver. The changes can be summarized
as follows:

 * Move regulator information back into the driver. The
   regulators needed are documented in the chip and have
   nothing to do with boarddata. Instead devm_regulator_get_optional
   is used and errors are handled quite loosely now. Maybe the USB
   driver should provide dummy regulators.
 * GPIO handling is updated to gpiod consumer interface, resulting
   in a driver cleanup and easy DT handling
 * The driver is updated to use managed resources wherever possible

So much about the nice stuff. But there is also

 * Instantiation of the platform device from the i2c (sub-)device. Since DT
   is not supposed to contain linuxisms the device is a simple i2c node
   resulting in the i2c probe function being called. Thus registering the main
   v4l device must happen from there.

Tested:
 * Compilation on torvalds/linux.git:master (based on 52d589a)
 * Booting in DT mode
 * Some simply driver queries using v4l2-ctl

Not tested:
 * The USB driver, since I do not own the USB dongle
 * The legacy platform code (only DT boot has been tested).
   (The legacy platform code is supposed to removed in the near future anyways)

Changes since RFCv1 (requested by Hans Verkuil):
 - splitted the patchset into more patches
 - replaced dev_info with dev_dbg for missing regulators
 - check for ENOSYS value from devm_gpiod_get (disabled GPIOLIB)

-- Sebastian

Sebastian Reichel (8):
  [media] si4713: switch to devm regulator API
  [media] si4713: switch reset gpio to devm_gpiod API
  [media] si4713: use managed memory allocation
  [media] si4713: use managed irq request
  [media] si4713: add device tree support
  [media] si4713: add DT binding documentation
  ARM: OMAP2: RX-51: update si4713 platform data
  [media] si4713: cleanup platform data

 Documentation/devicetree/bindings/media/si4713.txt |  30 ++++
 arch/arm/mach-omap2/board-rx51-peripherals.c       |  69 ++++-----
 drivers/media/radio/si4713/radio-platform-si4713.c |  28 +---
 drivers/media/radio/si4713/si4713.c                | 167 +++++++++++++--------
 drivers/media/radio/si4713/si4713.h                |  15 +-
 include/media/radio-si4713.h                       |  30 ----
 include/media/si4713.h                             |   4 +-
 7 files changed, 186 insertions(+), 157 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/si4713.txt
 delete mode 100644 include/media/radio-si4713.h

-- 
2.1.1

[RFCv2 1/8] [media] si4713: switch to devm regulator API

[Sebastian Reichel]

This switches back to the normal regulator API (but use
managed variant) in preparation for device tree support.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/media/radio/si4713/si4713.c | 80 ++++++++++++++++++++++++++-----------
 drivers/media/radio/si4713/si4713.h |  6 +--
 2 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
index b576555..b335093 100644
--- a/drivers/media/radio/si4713/si4713.c
+++ b/drivers/media/radio/si4713/si4713.c
@@ -23,6 +23,7 @@
 
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
@@ -366,13 +367,22 @@ static int si4713_powerup(struct si4713_device *sdev)
 	if (sdev->power_state)
 		return 0;
 
-	if (sdev->supplies) {
-		err = regulator_bulk_enable(sdev->supplies, sdev->supply_data);
+	if (sdev->vdd) {
+		err = regulator_enable(sdev->vdd);
 		if (err) {
-			v4l2_err(&sdev->sd, "Failed to enable supplies: %d\n", err);
+			v4l2_err(&sdev->sd, "Failed to enable vdd: %d\n", err);
 			return err;
 		}
 	}
+
+	if (sdev->vio) {
+		err = regulator_enable(sdev->vio);
+		if (err) {
+			v4l2_err(&sdev->sd, "Failed to enable vio: %d\n", err);
+			return err;
+		}
+	}
+
 	if (gpio_is_valid(sdev->gpio_reset)) {
 		udelay(50);
 		gpio_set_value(sdev->gpio_reset, 1);
@@ -399,11 +409,18 @@ static int si4713_powerup(struct si4713_device *sdev)
 	}
 	if (gpio_is_valid(sdev->gpio_reset))
 		gpio_set_value(sdev->gpio_reset, 0);
-	if (sdev->supplies) {
-		err = regulator_bulk_disable(sdev->supplies, sdev->supply_data);
+
+
+	if (sdev->vdd) {
+		err = regulator_disable(sdev->vdd);
 		if (err)
-			v4l2_err(&sdev->sd,
-				 "Failed to disable supplies: %d\n", err);
+			v4l2_err(&sdev->sd, "Failed to disable vdd: %d\n", err);
+	}
+
+	if (sdev->vio) {
+		err = regulator_disable(sdev->vio);
+		if (err)
+			v4l2_err(&sdev->sd, "Failed to disable vio: %d\n", err);
 	}
 
 	return err;
@@ -432,12 +449,21 @@ static int si4713_powerdown(struct si4713_device *sdev)
 		v4l2_dbg(1, debug, &sdev->sd, "Device in reset mode\n");
 		if (gpio_is_valid(sdev->gpio_reset))
 			gpio_set_value(sdev->gpio_reset, 0);
-		if (sdev->supplies) {
-			err = regulator_bulk_disable(sdev->supplies,
-						     sdev->supply_data);
-			if (err)
+
+		if (sdev->vdd) {
+			err = regulator_disable(sdev->vdd);
+			if (err) {
+				v4l2_err(&sdev->sd,
+					"Failed to disable vdd: %d\n", err);
+			}
+		}
+
+		if (sdev->vio) {
+			err = regulator_disable(sdev->vio);
+			if (err) {
 				v4l2_err(&sdev->sd,
-					 "Failed to disable supplies: %d\n", err);
+					"Failed to disable vio: %d\n", err);
+			}
 		}
 		sdev->power_state = POWER_OFF;
 	}
@@ -1441,17 +1467,26 @@ static int si4713_probe(struct i2c_client *client,
 		}
 		sdev->gpio_reset = pdata->gpio_reset;
 		gpio_direction_output(sdev->gpio_reset, 0);
-		sdev->supplies = pdata->supplies;
 	}
 
-	for (i = 0; i < sdev->supplies; i++)
-		sdev->supply_data[i].supply = pdata->supply_names[i];
+	sdev->vdd = devm_regulator_get_optional(&client->dev, "vdd");
+	if (IS_ERR(sdev->vdd)) {
+		rval = PTR_ERR(sdev->vdd);
+		if (rval == -EPROBE_DEFER)
+			goto exit;
+
+		dev_dbg(&client->dev, "no vdd regulator found: %d\n", rval);
+		sdev->vdd = NULL;
+	}
+
+	sdev->vio = devm_regulator_get_optional(&client->dev, "vio");
+	if (IS_ERR(sdev->vio)) {
+		rval = PTR_ERR(sdev->vio);
+		if (rval == -EPROBE_DEFER)
+			goto exit;
 
-	rval = regulator_bulk_get(&client->dev, sdev->supplies,
-				  sdev->supply_data);
-	if (rval) {
-		dev_err(&client->dev, "Cannot get regulators: %d\n", rval);
-		goto free_gpio;
+		dev_dbg(&client->dev, "no vio regulator found: %d\n", rval);
+		sdev->vio = NULL;
 	}
 
 	v4l2_i2c_subdev_init(&sdev->sd, client, &si4713_subdev_ops);
@@ -1559,7 +1594,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 put_reg;
+			goto free_ctrls;
 		}
 		v4l2_dbg(1, debug, &sdev->sd, "IRQ requested.\n");
 	} else {
@@ -1579,8 +1614,6 @@ free_irq:
 		free_irq(client->irq, sdev);
 free_ctrls:
 	v4l2_ctrl_handler_free(hdl);
-put_reg:
-	regulator_bulk_free(sdev->supplies, sdev->supply_data);
 free_gpio:
 	if (gpio_is_valid(sdev->gpio_reset))
 		gpio_free(sdev->gpio_reset);
@@ -1604,7 +1637,6 @@ static int si4713_remove(struct i2c_client *client)
 
 	v4l2_device_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
-	regulator_bulk_free(sdev->supplies, sdev->supply_data);
 	if (gpio_is_valid(sdev->gpio_reset))
 		gpio_free(sdev->gpio_reset);
 	kfree(sdev);
diff --git a/drivers/media/radio/si4713/si4713.h b/drivers/media/radio/si4713/si4713.h
index ed700e3..ed28ed2 100644
--- a/drivers/media/radio/si4713/si4713.h
+++ b/drivers/media/radio/si4713/si4713.h
@@ -190,8 +190,6 @@
 #define MIN_ACOMP_THRESHOLD		(-40)
 #define MAX_ACOMP_GAIN			20
 
-#define SI4713_NUM_SUPPLIES		2
-
 /*
  * si4713_device - private data
  */
@@ -236,8 +234,8 @@ struct si4713_device {
 		struct v4l2_ctrl *tune_ant_cap;
 	};
 	struct completion work;
-	unsigned supplies;
-	struct regulator_bulk_data supply_data[SI4713_NUM_SUPPLIES];
+	struct regulator *vdd;
+	struct regulator *vio;
 	int gpio_reset;
 	u32 power_state;
 	u32 rds_enabled;
-- 
2.1.1

[RFCv2 2/8] [media] si4713: switch reset gpio to devm_gpiod API

[Sebastian Reichel]

This updates the driver to use the managed gpiod interface
instead of the unmanged old GPIO API. This is a preperation
for the introduction of device tree support.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/media/radio/si4713/si4713.c | 38 +++++++++++++++++--------------------
 drivers/media/radio/si4713/si4713.h |  3 ++-
 2 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
index b335093..e560a7e 100644
--- a/drivers/media/radio/si4713/si4713.c
+++ b/drivers/media/radio/si4713/si4713.c
@@ -383,9 +383,9 @@ static int si4713_powerup(struct si4713_device *sdev)
 		}
 	}
 
-	if (gpio_is_valid(sdev->gpio_reset)) {
+	if (!IS_ERR(sdev->gpio_reset)) {
 		udelay(50);
-		gpio_set_value(sdev->gpio_reset, 1);
+		gpiod_set_value(sdev->gpio_reset, 1);
 	}
 
 	if (client->irq)
@@ -407,8 +407,8 @@ static int si4713_powerup(struct si4713_device *sdev)
 						SI4713_STC_INT | SI4713_CTS);
 		return err;
 	}
-	if (gpio_is_valid(sdev->gpio_reset))
-		gpio_set_value(sdev->gpio_reset, 0);
+	if (!IS_ERR(sdev->gpio_reset))
+		gpiod_set_value(sdev->gpio_reset, 0);
 
 
 	if (sdev->vdd) {
@@ -447,8 +447,8 @@ 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");
-		if (gpio_is_valid(sdev->gpio_reset))
-			gpio_set_value(sdev->gpio_reset, 0);
+		if (!IS_ERR(sdev->gpio_reset))
+			gpiod_set_value(sdev->gpio_reset, 0);
 
 		if (sdev->vdd) {
 			err = regulator_disable(sdev->vdd);
@@ -1457,16 +1457,17 @@ static int si4713_probe(struct i2c_client *client,
 		goto exit;
 	}
 
-	sdev->gpio_reset = -1;
-	if (pdata && gpio_is_valid(pdata->gpio_reset)) {
-		rval = gpio_request(pdata->gpio_reset, "si4713 reset");
-		if (rval) {
-			dev_err(&client->dev,
-				"Failed to request gpio: %d\n", rval);
-			goto free_sdev;
-		}
-		sdev->gpio_reset = pdata->gpio_reset;
-		gpio_direction_output(sdev->gpio_reset, 0);
+	sdev->gpio_reset = devm_gpiod_get(&client->dev, "reset");
+	if (!IS_ERR(sdev->gpio_reset)) {
+		gpiod_direction_output(sdev->gpio_reset, 0);
+	} else if (PTR_ERR(sdev->gpio_reset) == -ENOENT) {
+		dev_dbg(&client->dev, "No reset GPIO assigned\n");
+	} else if (PTR_ERR(sdev->gpio_reset) == -ENOSYS) {
+		dev_dbg(&client->dev, "No reset GPIO support\n");
+	} else {
+		rval = PTR_ERR(sdev->gpio_reset);
+		dev_err(&client->dev, "Failed to request gpio: %d\n", rval);
+		goto free_sdev;
 	}
 
 	sdev->vdd = devm_regulator_get_optional(&client->dev, "vdd");
@@ -1614,9 +1615,6 @@ free_irq:
 		free_irq(client->irq, sdev);
 free_ctrls:
 	v4l2_ctrl_handler_free(hdl);
-free_gpio:
-	if (gpio_is_valid(sdev->gpio_reset))
-		gpio_free(sdev->gpio_reset);
 free_sdev:
 	kfree(sdev);
 exit:
@@ -1637,8 +1635,6 @@ static int si4713_remove(struct i2c_client *client)
 
 	v4l2_device_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
-	if (gpio_is_valid(sdev->gpio_reset))
-		gpio_free(sdev->gpio_reset);
 	kfree(sdev);
 
 	return 0;
diff --git a/drivers/media/radio/si4713/si4713.h b/drivers/media/radio/si4713/si4713.h
index ed28ed2..7c2479f 100644
--- a/drivers/media/radio/si4713/si4713.h
+++ b/drivers/media/radio/si4713/si4713.h
@@ -16,6 +16,7 @@
 #define SI4713_I2C_H
 
 #include <linux/regulator/consumer.h>
+#include <linux/gpio/consumer.h>
 #include <media/v4l2-subdev.h>
 #include <media/v4l2-ctrls.h>
 #include <media/si4713.h>
@@ -236,7 +237,7 @@ struct si4713_device {
 	struct completion work;
 	struct regulator *vdd;
 	struct regulator *vio;
-	int gpio_reset;
+	struct gpio_desc *gpio_reset;
 	u32 power_state;
 	u32 rds_enabled;
 	u32 frequency;
-- 
2.1.1

[RFCv2 3/8] [media] si4713: use managed memory allocation

[Sebastian Reichel]

Introduce the usage of managed memory allocation to
simplify the code slightly.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/media/radio/si4713/si4713.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
index e560a7e..efc5d6b 100644
--- a/drivers/media/radio/si4713/si4713.c
+++ b/drivers/media/radio/si4713/si4713.c
@@ -1450,7 +1450,7 @@ static int si4713_probe(struct i2c_client *client,
 	struct v4l2_ctrl_handler *hdl;
 	int rval, i;
 
-	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
+	sdev = devm_kzalloc(&client->dev, sizeof(*sdev), GFP_KERNEL);
 	if (!sdev) {
 		dev_err(&client->dev, "Failed to alloc video device.\n");
 		rval = -ENOMEM;
@@ -1467,7 +1467,7 @@ static int si4713_probe(struct i2c_client *client,
 	} else {
 		rval = PTR_ERR(sdev->gpio_reset);
 		dev_err(&client->dev, "Failed to request gpio: %d\n", rval);
-		goto free_sdev;
+		goto exit;
 	}
 
 	sdev->vdd = devm_regulator_get_optional(&client->dev, "vdd");
@@ -1615,8 +1615,6 @@ free_irq:
 		free_irq(client->irq, sdev);
 free_ctrls:
 	v4l2_ctrl_handler_free(hdl);
-free_sdev:
-	kfree(sdev);
 exit:
 	return rval;
 }
@@ -1635,7 +1633,6 @@ static int si4713_remove(struct i2c_client *client)
 
 	v4l2_device_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
-	kfree(sdev);
 
 	return 0;
 }
-- 
2.1.1

[RFCv2 4/8] [media] si4713: use managed irq request

[Sebastian Reichel]

Introduce the usage of managed irq request to
simplify the code slightly.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/media/radio/si4713/si4713.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
index efc5d6b..ebec16d 100644
--- a/drivers/media/radio/si4713/si4713.c
+++ b/drivers/media/radio/si4713/si4713.c
@@ -1590,7 +1590,7 @@ static int si4713_probe(struct i2c_client *client,
 	sdev->sd.ctrl_handler = hdl;
 
 	if (client->irq) {
-		rval = request_irq(client->irq,
+		rval = devm_request_irq(&client->dev, client->irq,
 			si4713_handler, IRQF_TRIGGER_FALLING,
 			client->name, sdev);
 		if (rval < 0) {
@@ -1605,14 +1605,11 @@ static int si4713_probe(struct i2c_client *client,
 	rval = si4713_initialize(sdev);
 	if (rval < 0) {
 		v4l2_err(&sdev->sd, "Failed to probe device information.\n");
-		goto free_irq;
+		goto free_ctrls;
 	}
 
 	return 0;
 
-free_irq:
-	if (client->irq)
-		free_irq(client->irq, sdev);
 free_ctrls:
 	v4l2_ctrl_handler_free(hdl);
 exit:
@@ -1628,9 +1625,6 @@ static int si4713_remove(struct i2c_client *client)
 	if (sdev->power_state)
 		si4713_set_power_state(sdev, POWER_DOWN);
 
-	if (client->irq > 0)
-		free_irq(client->irq, sdev);
-
 	v4l2_device_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 
-- 
2.1.1

[RFCv2 5/8] [media] si4713: add device tree support

[Sebastian Reichel]

Add device tree support by changing the device registration order.
In the device tree the si4713 node is a normal I2C device, which
will be probed as such. Thus the V4L device must be probed from
the I2C device and not the other way around.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/media/radio/si4713/radio-platform-si4713.c | 28 ++++--------------
 drivers/media/radio/si4713/si4713.c                | 34 ++++++++++++++++++++--
 drivers/media/radio/si4713/si4713.h                |  6 ++++
 include/media/radio-si4713.h                       | 30 -------------------
 include/media/si4713.h                             |  1 +
 5 files changed, 45 insertions(+), 54 deletions(-)
 delete mode 100644 include/media/radio-si4713.h

diff --git a/drivers/media/radio/si4713/radio-platform-si4713.c b/drivers/media/radio/si4713/radio-platform-si4713.c
index a47502a..2de5439 100644
--- a/drivers/media/radio/si4713/radio-platform-si4713.c
+++ b/drivers/media/radio/si4713/radio-platform-si4713.c
@@ -34,7 +34,7 @@
 #include <media/v4l2-fh.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-event.h>
-#include <media/radio-si4713.h>
+#include "si4713.h"
 
 /* module parameters */
 static int radio_nr = -1;	/* radio device minor (-1 ==> auto assign) */
@@ -153,7 +153,6 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 {
 	struct radio_si4713_platform_data *pdata = pdev->dev.platform_data;
 	struct radio_si4713_device *rsdev;
-	struct i2c_adapter *adapter;
 	struct v4l2_subdev *sd;
 	int rval = 0;
 
@@ -177,20 +176,11 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 		goto exit;
 	}
 
-	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;
-	}
-
-	sd = v4l2_i2c_new_subdev_board(&rsdev->v4l2_dev, adapter,
-				       pdata->subdev_board_info, NULL);
-	if (!sd) {
+	sd = i2c_get_clientdata(pdata->subdev);
+	rval = v4l2_device_register_subdev(&rsdev->v4l2_dev, sd);
+	if (rval) {
 		dev_err(&pdev->dev, "Cannot get v4l2 subdevice\n");
-		rval = -ENODEV;
-		goto put_adapter;
+		goto unregister_v4l2_dev;
 	}
 
 	rsdev->radio_dev = radio_si4713_vdev_template;
@@ -202,14 +192,12 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 	if (video_register_device(&rsdev->radio_dev, VFL_TYPE_RADIO, radio_nr)) {
 		dev_err(&pdev->dev, "Could not register video device.\n");
 		rval = -EIO;
-		goto put_adapter;
+		goto unregister_v4l2_dev;
 	}
 	dev_info(&pdev->dev, "New device successfully probed\n");
 
 	goto exit;
 
-put_adapter:
-	i2c_put_adapter(adapter);
 unregister_v4l2_dev:
 	v4l2_device_unregister(&rsdev->v4l2_dev);
 exit:
@@ -220,14 +208,10 @@ exit:
 static int radio_si4713_pdriver_remove(struct platform_device *pdev)
 {
 	struct v4l2_device *v4l2_dev = platform_get_drvdata(pdev);
-	struct v4l2_subdev *sd = list_entry(v4l2_dev->subdevs.next,
-					    struct v4l2_subdev, list);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct radio_si4713_device *rsdev;
 
 	rsdev = container_of(v4l2_dev, struct radio_si4713_device, v4l2_dev);
 	video_unregister_device(&rsdev->radio_dev);
-	i2c_put_adapter(client->adapter);
 	v4l2_device_unregister(&rsdev->v4l2_dev);
 
 	return 0;
diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
index ebec16d..94fe3c6 100644
--- a/drivers/media/radio/si4713/si4713.c
+++ b/drivers/media/radio/si4713/si4713.c
@@ -1446,9 +1446,13 @@ 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;
 	struct v4l2_ctrl_handler *hdl;
-	int rval, i;
+	struct si4713_platform_data *pdata = client->dev.platform_data;
+	struct device_node *np = client->dev.of_node;
+	int rval;
+
+	struct radio_si4713_platform_data si4713_pdev_pdata;
+	struct platform_device *si4713_pdev;
 
 	sdev = devm_kzalloc(&client->dev, sizeof(*sdev), GFP_KERNEL);
 	if (!sdev) {
@@ -1608,8 +1612,31 @@ static int si4713_probe(struct i2c_client *client,
 		goto free_ctrls;
 	}
 
+	if ((pdata && pdata->is_platform_device) || np) {
+		si4713_pdev = platform_device_alloc("radio-si4713", -1);
+		if (!si4713_pdev)
+			goto put_main_pdev;
+
+		si4713_pdev_pdata.subdev = client;
+		rval = platform_device_add_data(si4713_pdev, &si4713_pdev_pdata,
+						sizeof(si4713_pdev_pdata));
+		if (rval)
+			goto put_main_pdev;
+
+		rval = platform_device_add(si4713_pdev);
+		if (rval)
+			goto put_main_pdev;
+
+		sdev->pd = si4713_pdev;
+	} else {
+		sdev->pd = NULL;
+	}
+
 	return 0;
 
+put_main_pdev:
+	platform_device_put(si4713_pdev);
+	v4l2_device_unregister_subdev(&sdev->sd);
 free_ctrls:
 	v4l2_ctrl_handler_free(hdl);
 exit:
@@ -1622,6 +1649,9 @@ static int si4713_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct si4713_device *sdev = to_si4713_device(sd);
 
+	if (sdev->pd)
+		platform_device_unregister(sdev->pd);
+
 	if (sdev->power_state)
 		si4713_set_power_state(sdev, POWER_DOWN);
 
diff --git a/drivers/media/radio/si4713/si4713.h b/drivers/media/radio/si4713/si4713.h
index 7c2479f..8a376e1 100644
--- a/drivers/media/radio/si4713/si4713.h
+++ b/drivers/media/radio/si4713/si4713.h
@@ -15,6 +15,7 @@
 #ifndef SI4713_I2C_H
 #define SI4713_I2C_H
 
+#include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/gpio/consumer.h>
 #include <media/v4l2-subdev.h>
@@ -238,6 +239,7 @@ struct si4713_device {
 	struct regulator *vdd;
 	struct regulator *vio;
 	struct gpio_desc *gpio_reset;
+	struct platform_device *pd;
 	u32 power_state;
 	u32 rds_enabled;
 	u32 frequency;
@@ -245,4 +247,8 @@ struct si4713_device {
 	u32 stereo;
 	u32 tune_rnl;
 };
+
+struct radio_si4713_platform_data {
+	struct i2c_client *subdev;
+};
 #endif /* ifndef SI4713_I2C_H */
diff --git a/include/media/radio-si4713.h b/include/media/radio-si4713.h
deleted file mode 100644
index f6aae29..0000000
--- a/include/media/radio-si4713.h
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- * include/media/radio-si4713.h
- *
- * Board related data definitions for Si4713 radio transmitter chip.
- *
- * Copyright (c) 2009 Nokia Corporation
- * Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
- *
- */
-
-#ifndef RADIO_SI4713_H
-#define RADIO_SI4713_H
-
-#include <linux/i2c.h>
-
-#define SI4713_NAME "radio-si4713"
-
-/*
- * Platform dependent definition
- */
-struct radio_si4713_platform_data {
-	int i2c_bus;
-	struct i2c_board_info *subdev_board_info;
-};
-
-#endif /* ifndef RADIO_SI4713_H*/
diff --git a/include/media/si4713.h b/include/media/si4713.h
index f98a0a7..343b8fb5 100644
--- a/include/media/si4713.h
+++ b/include/media/si4713.h
@@ -26,6 +26,7 @@ struct si4713_platform_data {
 	const char * const *supply_names;
 	unsigned supplies;
 	int gpio_reset; /* < 0 if not used */
+	bool is_platform_device;
 };
 
 /*
-- 
2.1.1

[RFCv2 6/8] [media] si4713: add DT binding documentation

[Sebastian Reichel]

This patch adds the DT bindings documentation for Silicon Labs Si4713 FM
radio transmitter.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 Documentation/devicetree/bindings/media/si4713.txt | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/si4713.txt

diff --git a/Documentation/devicetree/bindings/media/si4713.txt b/Documentation/devicetree/bindings/media/si4713.txt
new file mode 100644
index 0000000..5ee5552
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/si4713.txt
@@ -0,0 +1,30 @@
+* Silicon Labs FM Radio transmitter
+
+The Silicon Labs Si4713 is an FM radio transmitter with receive power scan
+supporting 76-108 MHz. It includes an RDS encoder and has both, a stereo-analog
+and a digital interface, which supports I2S, left-justified and a custom
+DSP-mode format. It is programmable through an I2C interface.
+
+Required Properties:
+- compatible: Should contain "silabs,si4713"
+- reg: the I2C address of the device
+
+Optional Properties:
+- interrupts-extended: Interrupt specifier for the chips interrupt
+- reset-gpios: GPIO specifier for the chips reset line
+- vdd-supply: phandle for Vdd regulator
+- vio-supply: phandle for Vio regulator
+
+Example:
+
+&i2c2 {
+        fmtx: si4713@63 {
+                compatible = "silabs,si4713";
+                reg = <0x63>;
+
+                interrupts-extended = <&gpio2 21 IRQ_TYPE_EDGE_FALLING>; /* 53 */
+                reset-gpios = <&gpio6 3 GPIO_ACTIVE_HIGH>; /* 163 */
+                vio-supply = <&vio>;
+                vdd-supply = <&vaux1>;
+        };
+};
-- 
2.1.1

[RFCv2 7/8] ARM: OMAP2: RX-51: update si4713 platform data

[Sebastian Reichel]

This updates platform data related to Si4713, which
has been updated to be compatible with DT interface.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 arch/arm/mach-omap2/board-rx51-peripherals.c | 69 +++++++++++++---------------
 1 file changed, 31 insertions(+), 38 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index ddfc8df..ec2e410 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -23,6 +23,7 @@
 #include <linux/regulator/machine.h>
 #include <linux/gpio.h>
 #include <linux/gpio_keys.h>
+#include <linux/gpio/machine.h>
 #include <linux/mmc/host.h>
 #include <linux/power/isp1704_charger.h>
 #include <linux/platform_data/spi-omap2-mcspi.h>
@@ -38,7 +39,6 @@
 
 #include <sound/tlv320aic3x.h>
 #include <sound/tpa6130a2-plat.h>
-#include <media/radio-si4713.h>
 #include <media/si4713.h>
 #include <linux/platform_data/leds-lp55xx.h>
 
@@ -760,46 +760,17 @@ static struct regulator_init_data rx51_vintdig = {
 	},
 };
 
-static const char * const si4713_supply_names[] = {
-	"vio",
-	"vdd",
-};
-
-static struct si4713_platform_data rx51_si4713_i2c_data __initdata_or_module = {
-	.supplies	= ARRAY_SIZE(si4713_supply_names),
-	.supply_names	= si4713_supply_names,
-	.gpio_reset	= RX51_FMTX_RESET_GPIO,
-};
-
-static struct i2c_board_info rx51_si4713_board_info __initdata_or_module = {
-	I2C_BOARD_INFO("si4713", SI4713_I2C_ADDR_BUSEN_HIGH),
-	.platform_data	= &rx51_si4713_i2c_data,
-};
-
-static struct radio_si4713_platform_data rx51_si4713_data __initdata_or_module = {
-	.i2c_bus	= 2,
-	.subdev_board_info = &rx51_si4713_board_info,
-};
-
-static struct platform_device rx51_si4713_dev __initdata_or_module = {
-	.name	= "radio-si4713",
-	.id	= -1,
-	.dev	= {
-		.platform_data	= &rx51_si4713_data,
+static struct gpiod_lookup_table rx51_fmtx_gpios_table = {
+	.dev_id = "2-0063",
+	.table = {
+		GPIO_LOOKUP("gpio.6", 3, "reset", GPIO_ACTIVE_HIGH), /* 163 */
+		{ },
 	},
 };
 
-static __init void rx51_init_si4713(void)
+static __init void rx51_gpio_init(void)
 {
-	int err;
-
-	err = gpio_request_one(RX51_FMTX_IRQ, GPIOF_DIR_IN, "si4713 irq");
-	if (err) {
-		printk(KERN_ERR "Cannot request si4713 irq gpio. %d\n", err);
-		return;
-	}
-	rx51_si4713_board_info.irq = gpio_to_irq(RX51_FMTX_IRQ);
-	platform_device_register(&rx51_si4713_dev);
+	gpiod_add_lookup_table(&rx51_fmtx_gpios_table);
 }
 
 static int rx51_twlgpio_setup(struct device *dev, unsigned gpio, unsigned n)
@@ -1029,7 +1000,17 @@ static struct aic3x_pdata rx51_aic3x_data2 = {
 	.gpio_reset = 60,
 };
 
+static struct si4713_platform_data rx51_si4713_platform_data = {
+	.is_platform_device = true
+};
+
 static struct i2c_board_info __initdata rx51_peripherals_i2c_board_info_2[] = {
+#if IS_ENABLED(CONFIG_I2C_SI4713) && IS_ENABLED(CONFIG_PLATFORM_SI4713)
+	{
+		I2C_BOARD_INFO("si4713", 0x63),
+		.platform_data = &rx51_si4713_platform_data,
+	},
+#endif
 	{
 		I2C_BOARD_INFO("tlv320aic3x", 0x18),
 		.platform_data = &rx51_aic3x_data,
@@ -1070,6 +1051,10 @@ static struct i2c_board_info __initdata rx51_peripherals_i2c_board_info_3[] = {
 
 static int __init rx51_i2c_init(void)
 {
+#if IS_ENABLED(CONFIG_I2C_SI4713) && IS_ENABLED(CONFIG_PLATFORM_SI4713)
+	int err;
+#endif
+
 	if ((system_rev >= SYSTEM_REV_S_USES_VAUX3 && system_rev < 0x100) ||
 	    system_rev >= SYSTEM_REV_B_USES_VAUX3) {
 		rx51_twldata.vaux3 = &rx51_vaux3_mmc;
@@ -1087,6 +1072,14 @@ static int __init rx51_i2c_init(void)
 	rx51_twldata.vdac->constraints.name = "VDAC";
 
 	omap_pmic_init(1, 2200, "twl5030", 7 + OMAP_INTC_START, &rx51_twldata);
+#if IS_ENABLED(CONFIG_I2C_SI4713) && IS_ENABLED(CONFIG_PLATFORM_SI4713)
+	err = gpio_request_one(RX51_FMTX_IRQ, GPIOF_DIR_IN, "si4713 irq");
+	if (err) {
+		printk(KERN_ERR "Cannot request si4713 irq gpio. %d\n", err);
+		return err;
+	}
+	rx51_peripherals_i2c_board_info_2[0].irq = gpio_to_irq(RX51_FMTX_IRQ);
+#endif
 	omap_register_i2c_bus(2, 100, rx51_peripherals_i2c_board_info_2,
 			      ARRAY_SIZE(rx51_peripherals_i2c_board_info_2));
 #if defined(CONFIG_SENSORS_LIS3_I2C) || defined(CONFIG_SENSORS_LIS3_I2C_MODULE)
@@ -1300,6 +1293,7 @@ static void __init rx51_init_omap3_rom_rng(void)
 
 void __init rx51_peripherals_init(void)
 {
+	rx51_gpio_init();
 	rx51_i2c_init();
 	regulator_has_full_constraints();
 	gpmc_onenand_init(board_onenand_data);
@@ -1307,7 +1301,6 @@ void __init rx51_peripherals_init(void)
 	rx51_add_gpio_keys();
 	rx51_init_wl1251();
 	rx51_init_tsc2005();
-	rx51_init_si4713();
 	rx51_init_lirc();
 	spi_register_board_info(rx51_peripherals_spi_board_info,
 				ARRAY_SIZE(rx51_peripherals_spi_board_info));
-- 
2.1.1

[RFCv2 8/8] [media] si4713: cleanup platform data

[Sebastian Reichel]

Remove unreferenced members from the platform
data's structure.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 include/media/si4713.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/media/si4713.h b/include/media/si4713.h
index 343b8fb5..be4f58e 100644
--- a/include/media/si4713.h
+++ b/include/media/si4713.h
@@ -23,9 +23,6 @@
  * Platform dependent definition
  */
 struct si4713_platform_data {
-	const char * const *supply_names;
-	unsigned supplies;
-	int gpio_reset; /* < 0 if not used */
 	bool is_platform_device;
 };
 
-- 
2.1.1

Re: [RFCv2 5/8] [media] si4713: add device tree support

[Sakari Ailus]

Hi Sebastian,

Nice set of patches! Thanks! :-)

On Tue, Oct 21, 2014 at 05:07:04PM +0200, Sebastian Reichel wrote:
> Add device tree support by changing the device registration order.
> In the device tree the si4713 node is a normal I2C device, which
> will be probed as such. Thus the V4L device must be probed from
> the I2C device and not the other way around.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  drivers/media/radio/si4713/radio-platform-si4713.c | 28 ++++--------------
>  drivers/media/radio/si4713/si4713.c                | 34 ++++++++++++++++++++--
>  drivers/media/radio/si4713/si4713.h                |  6 ++++
>  include/media/radio-si4713.h                       | 30 -------------------
>  include/media/si4713.h                             |  1 +
>  5 files changed, 45 insertions(+), 54 deletions(-)
>  delete mode 100644 include/media/radio-si4713.h
> 
> diff --git a/drivers/media/radio/si4713/radio-platform-si4713.c b/drivers/media/radio/si4713/radio-platform-si4713.c
> index a47502a..2de5439 100644
> --- a/drivers/media/radio/si4713/radio-platform-si4713.c
> +++ b/drivers/media/radio/si4713/radio-platform-si4713.c
> @@ -34,7 +34,7 @@
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-event.h>
> -#include <media/radio-si4713.h>
> +#include "si4713.h"
>  
>  /* module parameters */
>  static int radio_nr = -1;	/* radio device minor (-1 ==> auto assign) */
> @@ -153,7 +153,6 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  {
>  	struct radio_si4713_platform_data *pdata = pdev->dev.platform_data;
>  	struct radio_si4713_device *rsdev;
> -	struct i2c_adapter *adapter;
>  	struct v4l2_subdev *sd;
>  	int rval = 0;
>  
> @@ -177,20 +176,11 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  		goto exit;
>  	}
>  
> -	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;
> -	}
> -
> -	sd = v4l2_i2c_new_subdev_board(&rsdev->v4l2_dev, adapter,
> -				       pdata->subdev_board_info, NULL);
> -	if (!sd) {
> +	sd = i2c_get_clientdata(pdata->subdev);
> +	rval = v4l2_device_register_subdev(&rsdev->v4l2_dev, sd);
> +	if (rval) {
>  		dev_err(&pdev->dev, "Cannot get v4l2 subdevice\n");
> -		rval = -ENODEV;
> -		goto put_adapter;
> +		goto unregister_v4l2_dev;
>  	}
>  
>  	rsdev->radio_dev = radio_si4713_vdev_template;
> @@ -202,14 +192,12 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
>  	if (video_register_device(&rsdev->radio_dev, VFL_TYPE_RADIO, radio_nr)) {
>  		dev_err(&pdev->dev, "Could not register video device.\n");
>  		rval = -EIO;
> -		goto put_adapter;
> +		goto unregister_v4l2_dev;
>  	}
>  	dev_info(&pdev->dev, "New device successfully probed\n");
>  
>  	goto exit;
>  
> -put_adapter:
> -	i2c_put_adapter(adapter);
>  unregister_v4l2_dev:
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  exit:
> @@ -220,14 +208,10 @@ exit:
>  static int radio_si4713_pdriver_remove(struct platform_device *pdev)
>  {
>  	struct v4l2_device *v4l2_dev = platform_get_drvdata(pdev);
> -	struct v4l2_subdev *sd = list_entry(v4l2_dev->subdevs.next,
> -					    struct v4l2_subdev, list);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct radio_si4713_device *rsdev;
>  
>  	rsdev = container_of(v4l2_dev, struct radio_si4713_device, v4l2_dev);
>  	video_unregister_device(&rsdev->radio_dev);
> -	i2c_put_adapter(client->adapter);
>  	v4l2_device_unregister(&rsdev->v4l2_dev);
>  
>  	return 0;
> diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
> index ebec16d..94fe3c6 100644
> --- a/drivers/media/radio/si4713/si4713.c
> +++ b/drivers/media/radio/si4713/si4713.c
> @@ -1446,9 +1446,13 @@ 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;
>  	struct v4l2_ctrl_handler *hdl;
> -	int rval, i;
> +	struct si4713_platform_data *pdata = client->dev.platform_data;
> +	struct device_node *np = client->dev.of_node;
> +	int rval;
> +

Why empty line here?

It's not a bad practice to declare short temporary variables etc. as last.

> +	struct radio_si4713_platform_data si4713_pdev_pdata;
> +	struct platform_device *si4713_pdev;
>  
>  	sdev = devm_kzalloc(&client->dev, sizeof(*sdev), GFP_KERNEL);
>  	if (!sdev) {
> @@ -1608,8 +1612,31 @@ static int si4713_probe(struct i2c_client *client,
>  		goto free_ctrls;
>  	}
>  
> +	if ((pdata && pdata->is_platform_device) || np) {
> +		si4713_pdev = platform_device_alloc("radio-si4713", -1);

You could declare si4713_pdev here since you're not using it elsewhere.

> +		if (!si4713_pdev)
> +			goto put_main_pdev;
> +
> +		si4713_pdev_pdata.subdev = client;
> +		rval = platform_device_add_data(si4713_pdev, &si4713_pdev_pdata,
> +						sizeof(si4713_pdev_pdata));
> +		if (rval)
> +			goto put_main_pdev;
> +
> +		rval = platform_device_add(si4713_pdev);
> +		if (rval)
> +			goto put_main_pdev;
> +
> +		sdev->pd = si4713_pdev;
> +	} else {
> +		sdev->pd = NULL;

sdev->pd is NULL already here. You could simply return in if () and
proceed to create the platform device if need be.

Speaking of which --- I wonder if there are other than historical
reasons to create the platform device. I guess that's out of the scope
of the set anyway.

> +	}
> +
>  	return 0;
>  
> +put_main_pdev:
> +	platform_device_put(si4713_pdev);
> +	v4l2_device_unregister_subdev(&sdev->sd);
>  free_ctrls:
>  	v4l2_ctrl_handler_free(hdl);
>  exit:
> @@ -1622,6 +1649,9 @@ static int si4713_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  	struct si4713_device *sdev = to_si4713_device(sd);
>  
> +	if (sdev->pd)
> +		platform_device_unregister(sdev->pd);

platform_device_unregister() may be safely called with NULL argument.

> +
>  	if (sdev->power_state)
>  		si4713_set_power_state(sdev, POWER_DOWN);
>  
> diff --git a/drivers/media/radio/si4713/si4713.h b/drivers/media/radio/si4713/si4713.h
> index 7c2479f..8a376e1 100644
> --- a/drivers/media/radio/si4713/si4713.h
> +++ b/drivers/media/radio/si4713/si4713.h
> @@ -15,6 +15,7 @@
>  #ifndef SI4713_I2C_H
>  #define SI4713_I2C_H
>  
> +#include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/gpio/consumer.h>
>  #include <media/v4l2-subdev.h>
> @@ -238,6 +239,7 @@ struct si4713_device {
>  	struct regulator *vdd;
>  	struct regulator *vio;
>  	struct gpio_desc *gpio_reset;
> +	struct platform_device *pd;
>  	u32 power_state;
>  	u32 rds_enabled;
>  	u32 frequency;
> @@ -245,4 +247,8 @@ struct si4713_device {
>  	u32 stereo;
>  	u32 tune_rnl;
>  };
> +
> +struct radio_si4713_platform_data {
> +	struct i2c_client *subdev;
> +};
>  #endif /* ifndef SI4713_I2C_H */
> diff --git a/include/media/radio-si4713.h b/include/media/radio-si4713.h
> deleted file mode 100644
> index f6aae29..0000000
> --- a/include/media/radio-si4713.h
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -/*
> - * include/media/radio-si4713.h
> - *
> - * Board related data definitions for Si4713 radio transmitter chip.
> - *
> - * Copyright (c) 2009 Nokia Corporation
> - * Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
> - *
> - */
> -
> -#ifndef RADIO_SI4713_H
> -#define RADIO_SI4713_H
> -
> -#include <linux/i2c.h>
> -
> -#define SI4713_NAME "radio-si4713"
> -
> -/*
> - * Platform dependent definition
> - */
> -struct radio_si4713_platform_data {
> -	int i2c_bus;
> -	struct i2c_board_info *subdev_board_info;
> -};
> -
> -#endif /* ifndef RADIO_SI4713_H*/
> diff --git a/include/media/si4713.h b/include/media/si4713.h
> index f98a0a7..343b8fb5 100644
> --- a/include/media/si4713.h
> +++ b/include/media/si4713.h
> @@ -26,6 +26,7 @@ struct si4713_platform_data {
>  	const char * const *supply_names;
>  	unsigned supplies;
>  	int gpio_reset; /* < 0 if not used */
> +	bool is_platform_device;
>  };
>  
>  /*

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFCv2 0/8] [media] si4713 DT binding

[Hans Verkuil]

On 10/21/14 17:06, Sebastian Reichel wrote:
> Hi,
> 
> This is the RFCv2 patchset adding DT support to the si4713
> radio transmitter i2c driver. The changes can be summarized
> as follows:
> 
>  * Move regulator information back into the driver. The
>    regulators needed are documented in the chip and have
>    nothing to do with boarddata. Instead devm_regulator_get_optional
>    is used and errors are handled quite loosely now. Maybe the USB
>    driver should provide dummy regulators.
>  * GPIO handling is updated to gpiod consumer interface, resulting
>    in a driver cleanup and easy DT handling
>  * The driver is updated to use managed resources wherever possible
> 
> So much about the nice stuff. But there is also
> 
>  * Instantiation of the platform device from the i2c (sub-)device. Since DT
>    is not supposed to contain linuxisms the device is a simple i2c node
>    resulting in the i2c probe function being called. Thus registering the main
>    v4l device must happen from there.
> 
> Tested:
>  * Compilation on torvalds/linux.git:master (based on 52d589a)
>  * Booting in DT mode
>  * Some simply driver queries using v4l2-ctl
> 
> Not tested:
>  * The USB driver, since I do not own the USB dongle

I will test this on Monday for the USB device. It looks good, but I need
to verify that it doesn't break the USB driver.

Regards,

	Hans

>  * The legacy platform code (only DT boot has been tested).
>    (The legacy platform code is supposed to removed in the near future anyways)
> 
> Changes since RFCv1 (requested by Hans Verkuil):
>  - splitted the patchset into more patches
>  - replaced dev_info with dev_dbg for missing regulators
>  - check for ENOSYS value from devm_gpiod_get (disabled GPIOLIB)
> 
> -- Sebastian
> 
> Sebastian Reichel (8):
>   [media] si4713: switch to devm regulator API
>   [media] si4713: switch reset gpio to devm_gpiod API
>   [media] si4713: use managed memory allocation
>   [media] si4713: use managed irq request
>   [media] si4713: add device tree support
>   [media] si4713: add DT binding documentation
>   ARM: OMAP2: RX-51: update si4713 platform data
>   [media] si4713: cleanup platform data
> 
>  Documentation/devicetree/bindings/media/si4713.txt |  30 ++++
>  arch/arm/mach-omap2/board-rx51-peripherals.c       |  69 ++++-----
>  drivers/media/radio/si4713/radio-platform-si4713.c |  28 +---
>  drivers/media/radio/si4713/si4713.c                | 167 +++++++++++++--------
>  drivers/media/radio/si4713/si4713.h                |  15 +-
>  include/media/radio-si4713.h                       |  30 ----
>  include/media/si4713.h                             |   4 +-
>  7 files changed, 186 insertions(+), 157 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/si4713.txt
>  delete mode 100644 include/media/radio-si4713.h
> 

Re: [RFCv2 0/8] [media] si4713 DT binding

[Hans Verkuil]

Hi Sebastian,

I've tested the whole 8-part patch series with my si4713 USB dev board, and it
is working fine.

I've accepted patches 1-4. The others need to be reposted since patch 5 had a
change request.

Regards,

	Hans

On 11/07/2014 01:49 PM, Hans Verkuil wrote:
> On 10/21/14 17:06, Sebastian Reichel wrote:
>> Hi,
>>
>> This is the RFCv2 patchset adding DT support to the si4713
>> radio transmitter i2c driver. The changes can be summarized
>> as follows:
>>
>>  * Move regulator information back into the driver. The
>>    regulators needed are documented in the chip and have
>>    nothing to do with boarddata. Instead devm_regulator_get_optional
>>    is used and errors are handled quite loosely now. Maybe the USB
>>    driver should provide dummy regulators.
>>  * GPIO handling is updated to gpiod consumer interface, resulting
>>    in a driver cleanup and easy DT handling
>>  * The driver is updated to use managed resources wherever possible
>>
>> So much about the nice stuff. But there is also
>>
>>  * Instantiation of the platform device from the i2c (sub-)device. Since DT
>>    is not supposed to contain linuxisms the device is a simple i2c node
>>    resulting in the i2c probe function being called. Thus registering the main
>>    v4l device must happen from there.
>>
>> Tested:
>>  * Compilation on torvalds/linux.git:master (based on 52d589a)
>>  * Booting in DT mode
>>  * Some simply driver queries using v4l2-ctl
>>
>> Not tested:
>>  * The USB driver, since I do not own the USB dongle
> 
> I will test this on Monday for the USB device. It looks good, but I need
> to verify that it doesn't break the USB driver.
> 
> Regards,
> 
> 	Hans
> 
>>  * The legacy platform code (only DT boot has been tested).
>>    (The legacy platform code is supposed to removed in the near future anyways)
>>
>> Changes since RFCv1 (requested by Hans Verkuil):
>>  - splitted the patchset into more patches
>>  - replaced dev_info with dev_dbg for missing regulators
>>  - check for ENOSYS value from devm_gpiod_get (disabled GPIOLIB)
>>
>> -- Sebastian
>>
>> Sebastian Reichel (8):
>>   [media] si4713: switch to devm regulator API
>>   [media] si4713: switch reset gpio to devm_gpiod API
>>   [media] si4713: use managed memory allocation
>>   [media] si4713: use managed irq request
>>   [media] si4713: add device tree support
>>   [media] si4713: add DT binding documentation
>>   ARM: OMAP2: RX-51: update si4713 platform data
>>   [media] si4713: cleanup platform data
>>
>>  Documentation/devicetree/bindings/media/si4713.txt |  30 ++++
>>  arch/arm/mach-omap2/board-rx51-peripherals.c       |  69 ++++-----
>>  drivers/media/radio/si4713/radio-platform-si4713.c |  28 +---
>>  drivers/media/radio/si4713/si4713.c                | 167 +++++++++++++--------
>>  drivers/media/radio/si4713/si4713.h                |  15 +-
>>  include/media/radio-si4713.h                       |  30 ----
>>  include/media/si4713.h                             |   4 +-
>>  7 files changed, 186 insertions(+), 157 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/media/si4713.txt
>>  delete mode 100644 include/media/radio-si4713.h
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFCv2 5/8] [media] si4713: add device tree support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

Hi Sakari,

On Tue, Nov 04, 2014 at 11:47:14PM +0200, Sakari Ailus wrote:
> Nice set of patches! Thanks! :-)

Thanks :)

> > [...]
> >  	struct si4713_device *sdev;
> > -	struct si4713_platform_data *pdata = client->dev.platform_data;
> >  	struct v4l2_ctrl_handler *hdl;
> > -	int rval, i;
> > +	struct si4713_platform_data *pdata = client->dev.platform_data;
> > +	struct device_node *np = client->dev.of_node;
> > +	int rval;
> > +
> 
> Why empty line here?
> 
> It's not a bad practice to declare short temporary variables etc. as last.

Fixed in PATCHv3.

> > +	struct radio_si4713_platform_data si4713_pdev_pdata;
> > +	struct platform_device *si4713_pdev;
> >  
> >  	sdev = devm_kzalloc(&client->dev, sizeof(*sdev), GFP_KERNEL);
> >  	if (!sdev) {
> > @@ -1608,8 +1612,31 @@ static int si4713_probe(struct i2c_client *client,
> >  		goto free_ctrls;
> >  	}
> >  
> > +	if ((pdata && pdata->is_platform_device) || np) {
> > +		si4713_pdev = platform_device_alloc("radio-si4713", -1);
> 
> You could declare si4713_pdev here since you're not using it elsewhere.

It has been used in the put_main_pdev jump label at the bottom
outside of the scope and all access will happen out of the scope
after the refactoring you suggested below.

> > +		if (!si4713_pdev)
> > +			goto put_main_pdev;
> > +
> > +		si4713_pdev_pdata.subdev = client;
> > +		rval = platform_device_add_data(si4713_pdev, &si4713_pdev_pdata,
> > +						sizeof(si4713_pdev_pdata));
> > +		if (rval)
> > +			goto put_main_pdev;
> > +
> > +		rval = platform_device_add(si4713_pdev);
> > +		if (rval)
> > +			goto put_main_pdev;
> > +
> > +		sdev->pd = si4713_pdev;
> > +	} else {
> > +		sdev->pd = NULL;
> 
> sdev->pd is NULL already here. You could simply return in if () and
> proceed to create the platform device if need be.

Right. I simplified the code accordingly in PATCHv3.

> Speaking of which --- I wonder if there are other than historical
> reasons to create the platform device. I guess that's out of the scope
> of the set anyway.

I think this was done, so that the usb device can export its own
control functions.

> > [...]
> >
> > +	if (sdev->pd)
> > +		platform_device_unregister(sdev->pd);
> 
> platform_device_unregister() may be safely called with NULL argument.

Ok. Changed in PATCHv3.

> > [...]

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFCv2 1/8] [media] si4713: switch to devm regulator API

[Mauro Carvalho Chehab]

Em Tue, 21 Oct 2014 17:07:00 +0200
Sebastian Reichel <sre@kernel.org> escreveu:

> This switches back to the normal regulator API (but use
> managed variant) in preparation for device tree support.

This patch broke compilation. Please be sure that none of the patches in
the series would break it, as otherwise git bisect would be broken.

Thanks,
Mauro

drivers/media/radio/si4713/si4713.c: In function 'si4713_powerup':
drivers/media/radio/si4713/si4713.c:369:10: error: 'struct si4713_device' has no member named 'supplies'
 
          ^
drivers/media/radio/si4713/si4713.c:370:35: error: 'struct si4713_device' has no member named 'supplies'
  if (sdev->vdd) {
                                   ^
drivers/media/radio/si4713/si4713.c:370:51: error: 'struct si4713_device' has no member named 'supply_data'
  if (sdev->vdd) {
                                                   ^
drivers/media/radio/si4713/si4713.c:402:10: error: 'struct si4713_device' has no member named 'supplies'
   v4l2_dbg(1, debug, &sdev->sd, "Device in power up mode\n");
          ^
drivers/media/radio/si4713/si4713.c:403:36: error: 'struct si4713_device' has no member named 'supplies'
   sdev->power_state = POWER_ON;
                                    ^
drivers/media/radio/si4713/si4713.c:403:52: error: 'struct si4713_device' has no member named 'supply_data'
   sdev->power_state = POWER_ON;
                                                    ^
drivers/media/radio/si4713/si4713.c: In function 'si4713_powerdown':
drivers/media/radio/si4713/si4713.c:435:11: error: 'struct si4713_device' has no member named 'supplies'
  int err;
           ^
drivers/media/radio/si4713/si4713.c:436:37: error: 'struct si4713_device' has no member named 'supplies'
  u8 resp[SI4713_PWDN_NRESP];
                                     ^
drivers/media/radio/si4713/si4713.c:437:16: error: 'struct si4713_device' has no member named 'supply_data'
 
                ^
drivers/media/radio/si4713/si4713.c: In function 'si4713_probe':
drivers/media/radio/si4713/si4713.c:1444:7: error: 'struct si4713_device' has no member named 'supplies'
 /* si4713_probe - probe for the device */
       ^
drivers/media/radio/si4713/si4713.c:1447:22: error: 'struct si4713_device' has no member named 'supplies'
 {
                      ^
drivers/media/radio/si4713/si4713.c:1448:7: error: 'struct si4713_device' has no member named 'supply_data'
  struct si4713_device *sdev;
       ^
drivers/media/radio/si4713/si4713.c:1450:46: error: 'struct si4713_device' has no member named 'supplies'
  struct v4l2_ctrl_handler *hdl;
                                              ^
drivers/media/radio/si4713/si4713.c:1451:11: error: 'struct si4713_device' has no member named 'supply_data'
  int rval, i;
           ^
drivers/media/radio/si4713/si4713.c:1583:26: error: 'struct si4713_device' has no member named 'supplies'
 
                          ^
drivers/media/radio/si4713/si4713.c:1583:42: error: 'struct si4713_device' has no member named 'supply_data'
 
                                          ^
drivers/media/radio/si4713/si4713.c: In function 'si4713_remove':
drivers/media/radio/si4713/si4713.c:1607:26: error: 'struct si4713_device' has no member named 'supplies'
   goto free_irq;
                          ^
drivers/media/radio/si4713/si4713.c:1607:42: error: 'struct si4713_device' has no member named 'supply_data'
   goto free_irq;

Re: [RFCv2 1/8] [media] si4713: switch to devm regulator API

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

Hi Mauro,

On Tue, Nov 11, 2014 at 09:07:10AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 21 Oct 2014 17:07:00 +0200
> Sebastian Reichel <sre@kernel.org> escreveu:
> 
> > This switches back to the normal regulator API (but use
> > managed variant) in preparation for device tree support.
> 
> This patch broke compilation. Please be sure that none of the patches in
> the series would break it, as otherwise git bisect would be broken.
>
> [...]

mh, the errors seem to be from the old code (without the patch
applied to drivers/media/radio/si4713/si4713.c) and the inlined code
fragment displayed by the compiler seems to be the new code (with
the patch applied to drivers/media/radio/si4713/si4713.c).

Possible reasons I can think of:

 * You are using some kind of object cache, which assumed it could
   link the previously compiled si4713.o
 * You started the kernel compilation before merging the patch and
   the commit was only half applied when the compilation reached
   the si4713 driver.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFCv2 1/8] [media] si4713: switch to devm regulator API

[Hans Verkuil]

Hi Mauro,

On 11/11/2014 12:07 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 21 Oct 2014 17:07:00 +0200
> Sebastian Reichel <sre@kernel.org> escreveu:
> 
>> This switches back to the normal regulator API (but use
>> managed variant) in preparation for device tree support.
> 
> This patch broke compilation. Please be sure that none of the patches in
> the series would break it, as otherwise git bisect would be broken.

Weird, as reported by Sebastian, it works for me.

However, after applying this patch I get these new warnings:

  CC      drivers/media/radio/si4713/radio-usb-si4713.o
drivers/media/radio/si4713/si4713.c: In function ‘si4713_probe’:
drivers/media/radio/si4713/si4713.c:1617:1: warning: label ‘free_gpio’ defined but not used [-Wunused-label]
 free_gpio:
 ^
drivers/media/radio/si4713/si4713.c:1451:12: warning: unused variable ‘i’ [-Wunused-variable]
  int rval, i;
            ^

So it's probably not a good idea to merge this patch anyway until this is fixed.

Sebastian, can you fix these warnings and repost?

Thanks!

	Hans

> 
> Thanks,
> Mauro
> 
> drivers/media/radio/si4713/si4713.c: In function 'si4713_powerup':
> drivers/media/radio/si4713/si4713.c:369:10: error: 'struct si4713_device' has no member named 'supplies'
>  
>           ^
> drivers/media/radio/si4713/si4713.c:370:35: error: 'struct si4713_device' has no member named 'supplies'
>   if (sdev->vdd) {
>                                    ^
> drivers/media/radio/si4713/si4713.c:370:51: error: 'struct si4713_device' has no member named 'supply_data'
>   if (sdev->vdd) {
>                                                    ^
> drivers/media/radio/si4713/si4713.c:402:10: error: 'struct si4713_device' has no member named 'supplies'
>    v4l2_dbg(1, debug, &sdev->sd, "Device in power up mode\n");
>           ^
> drivers/media/radio/si4713/si4713.c:403:36: error: 'struct si4713_device' has no member named 'supplies'
>    sdev->power_state = POWER_ON;
>                                     ^
> drivers/media/radio/si4713/si4713.c:403:52: error: 'struct si4713_device' has no member named 'supply_data'
>    sdev->power_state = POWER_ON;
>                                                     ^
> drivers/media/radio/si4713/si4713.c: In function 'si4713_powerdown':
> drivers/media/radio/si4713/si4713.c:435:11: error: 'struct si4713_device' has no member named 'supplies'
>   int err;
>            ^
> drivers/media/radio/si4713/si4713.c:436:37: error: 'struct si4713_device' has no member named 'supplies'
>   u8 resp[SI4713_PWDN_NRESP];
>                                      ^
> drivers/media/radio/si4713/si4713.c:437:16: error: 'struct si4713_device' has no member named 'supply_data'
>  
>                 ^
> drivers/media/radio/si4713/si4713.c: In function 'si4713_probe':
> drivers/media/radio/si4713/si4713.c:1444:7: error: 'struct si4713_device' has no member named 'supplies'
>  /* si4713_probe - probe for the device */
>        ^
> drivers/media/radio/si4713/si4713.c:1447:22: error: 'struct si4713_device' has no member named 'supplies'
>  {
>                       ^
> drivers/media/radio/si4713/si4713.c:1448:7: error: 'struct si4713_device' has no member named 'supply_data'
>   struct si4713_device *sdev;
>        ^
> drivers/media/radio/si4713/si4713.c:1450:46: error: 'struct si4713_device' has no member named 'supplies'
>   struct v4l2_ctrl_handler *hdl;
>                                               ^
> drivers/media/radio/si4713/si4713.c:1451:11: error: 'struct si4713_device' has no member named 'supply_data'
>   int rval, i;
>            ^
> drivers/media/radio/si4713/si4713.c:1583:26: error: 'struct si4713_device' has no member named 'supplies'
>  
>                           ^
> drivers/media/radio/si4713/si4713.c:1583:42: error: 'struct si4713_device' has no member named 'supply_data'
>  
>                                           ^
> drivers/media/radio/si4713/si4713.c: In function 'si4713_remove':
> drivers/media/radio/si4713/si4713.c:1607:26: error: 'struct si4713_device' has no member named 'supplies'
>    goto free_irq;
>                           ^
> drivers/media/radio/si4713/si4713.c:1607:42: error: 'struct si4713_device' has no member named 'supply_data'
>    goto free_irq;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFCv2 1/8] [media] si4713: switch to devm regulator API

[Mauro Carvalho Chehab]

Em Tue, 11 Nov 2014 18:59:31 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> Hi Mauro,
> 
> On 11/11/2014 12:07 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 21 Oct 2014 17:07:00 +0200
> > Sebastian Reichel <sre@kernel.org> escreveu:
> > 
> >> This switches back to the normal regulator API (but use
> >> managed variant) in preparation for device tree support.
> > 
> > This patch broke compilation. Please be sure that none of the patches in
> > the series would break it, as otherwise git bisect would be broken.
> 
> Weird, as reported by Sebastian, it works for me.

Weird. Not sure what happened here.

> 
> However, after applying this patch I get these new warnings:
> 
>   CC      drivers/media/radio/si4713/radio-usb-si4713.o
> drivers/media/radio/si4713/si4713.c: In function ‘si4713_probe’:
> drivers/media/radio/si4713/si4713.c:1617:1: warning: label ‘free_gpio’ defined but not used [-Wunused-label]
>  free_gpio:
>  ^
> drivers/media/radio/si4713/si4713.c:1451:12: warning: unused variable ‘i’ [-Wunused-variable]
>   int rval, i;
>             ^
> 
> So it's probably not a good idea to merge this patch anyway until this is fixed.

Agreed. Also, v3 of this series apparently came after the pull request.

Regards,
Mauro


> Sebastian, can you fix these warnings and repost?
> 
> Thanks!
> 
> 	Hans
> 
> > 
> > Thanks,
> > Mauro
> > 
> > drivers/media/radio/si4713/si4713.c: In function 'si4713_powerup':
> > drivers/media/radio/si4713/si4713.c:369:10: error: 'struct si4713_device' has no member named 'supplies'
> >  
> >           ^
> > drivers/media/radio/si4713/si4713.c:370:35: error: 'struct si4713_device' has no member named 'supplies'
> >   if (sdev->vdd) {
> >                                    ^
> > drivers/media/radio/si4713/si4713.c:370:51: error: 'struct si4713_device' has no member named 'supply_data'
> >   if (sdev->vdd) {
> >                                                    ^
> > drivers/media/radio/si4713/si4713.c:402:10: error: 'struct si4713_device' has no member named 'supplies'
> >    v4l2_dbg(1, debug, &sdev->sd, "Device in power up mode\n");
> >           ^
> > drivers/media/radio/si4713/si4713.c:403:36: error: 'struct si4713_device' has no member named 'supplies'
> >    sdev->power_state = POWER_ON;
> >                                     ^
> > drivers/media/radio/si4713/si4713.c:403:52: error: 'struct si4713_device' has no member named 'supply_data'
> >    sdev->power_state = POWER_ON;
> >                                                     ^
> > drivers/media/radio/si4713/si4713.c: In function 'si4713_powerdown':
> > drivers/media/radio/si4713/si4713.c:435:11: error: 'struct si4713_device' has no member named 'supplies'
> >   int err;
> >            ^
> > drivers/media/radio/si4713/si4713.c:436:37: error: 'struct si4713_device' has no member named 'supplies'
> >   u8 resp[SI4713_PWDN_NRESP];
> >                                      ^
> > drivers/media/radio/si4713/si4713.c:437:16: error: 'struct si4713_device' has no member named 'supply_data'
> >  
> >                 ^
> > drivers/media/radio/si4713/si4713.c: In function 'si4713_probe':
> > drivers/media/radio/si4713/si4713.c:1444:7: error: 'struct si4713_device' has no member named 'supplies'
> >  /* si4713_probe - probe for the device */
> >        ^
> > drivers/media/radio/si4713/si4713.c:1447:22: error: 'struct si4713_device' has no member named 'supplies'
> >  {
> >                       ^
> > drivers/media/radio/si4713/si4713.c:1448:7: error: 'struct si4713_device' has no member named 'supply_data'
> >   struct si4713_device *sdev;
> >        ^
> > drivers/media/radio/si4713/si4713.c:1450:46: error: 'struct si4713_device' has no member named 'supplies'
> >   struct v4l2_ctrl_handler *hdl;
> >                                               ^
> > drivers/media/radio/si4713/si4713.c:1451:11: error: 'struct si4713_device' has no member named 'supply_data'
> >   int rval, i;
> >            ^
> > drivers/media/radio/si4713/si4713.c:1583:26: error: 'struct si4713_device' has no member named 'supplies'
> >  
> >                           ^
> > drivers/media/radio/si4713/si4713.c:1583:42: error: 'struct si4713_device' has no member named 'supply_data'
> >  
> >                                           ^
> > drivers/media/radio/si4713/si4713.c: In function 'si4713_remove':
> > drivers/media/radio/si4713/si4713.c:1607:26: error: 'struct si4713_device' has no member named 'supplies'
> >    goto free_irq;
> >                           ^
> > drivers/media/radio/si4713/si4713.c:1607:42: error: 'struct si4713_device' has no member named 'supply_data'
> >    goto free_irq;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
>