From: Lad, Prabhakar <[email protected]>
add OF support for the adv7343 driver.
Signed-off-by: Lad, Prabhakar <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Sylwester Nawrocki <[email protected]>
Cc: Sakari Ailus <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Rob Landley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
.../devicetree/bindings/media/i2c/adv7343.txt | 69 ++++++++++++++++++
drivers/media/i2c/adv7343.c | 75 +++++++++++++++++++-
2 files changed, 142 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/i2c/adv7343.txt
diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
new file mode 100644
index 0000000..8426f8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
@@ -0,0 +1,69 @@
+* Analog Devices adv7343 video encoder
+
+The ADV7343 are high speed, digital-to-analog video encoders in a 64-lead LQFP
+package. Six high speed, 3.3 V, 11-bit video DACs provide support for composite
+(CVBS), S-Video (Y-C), and component (YPrPb/RGB) analog outputs in standard
+definition (SD), enhanced definition (ED), or high definition (HD) video
+formats.
+
+The ADV7343 have a 24-bit pixel input port that can be configured in a variety
+of ways. SD video formats are supported over an SDR interface, and ED/HD video
+formats are supported over SDR and DDR interfaces. Pixel data can be supplied
+in either the YCrCb or RGB color spaces.
+
+Required Properties :
+- compatible: Must be "ad,adv7343-encoder"
+
+Optional Properties :
+- ad-adv7343-power-mode-sleep-mode: on enable the current consumption is
+ reduced to micro ampere level. All DACs and
+ the internal PLL circuit are disabled.
+- ad-adv7343-power-mode-pll-ctrl: PLL and oversampling control. This control
+ allows internal PLL 1 circuit to be powered
+ down and the oversampling to beswitched off.
+- ad-adv7343-power-mode-dac-1: power on/off DAC 1.
+- ad-adv7343-power-mode-dac-2: power on/off DAC 2.
+- ad-adv7343-power-mode-dac-3: power on/off DAC 3.
+- ad-adv7343-power-mode-dac-4: power on/off DAC 4.
+- ad-adv7343-power-mode-dac-5: power on/off DAC 5.
+- ad-adv7343-power-mode-dac-6: power on/off DAC 6.
+- ad-adv7343-sd-config-dac-out-1: Configure SD DAC Output 1.
+- ad-adv7343-sd-config-dac-out-2: Configure SD DAC Output 2.
+
+Example:
+
+i2c0@1c22000 {
+ ...
+ ...
+
+ adv7343@2a {
+ compatible = "ad,adv7343-encoder";
+ reg = <0x2a>;
+
+ port {
+ adv7343_1: endpoint {
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-sleep-mode;
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-pll-ctrl;
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-dac-1;
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-dac-2;
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-dac-3;
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-dac-4;
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-dac-5;
+ /* Active high (Defaults to false) */
+ ad-adv7343-power-mode-dac-6;
+ /* Active high (Defaults to false) */
+ ad-adv7343-sd-config-dac-out-1;
+ /* Active high (Defaults to false) */
+ ad-adv7343-sd-config-dac-out-2 = <0>;
+ };
+ };
+ };
+ ...
+};
diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
index 469e262..eb12d1a 100644
--- a/drivers/media/i2c/adv7343.c
+++ b/drivers/media/i2c/adv7343.c
@@ -25,12 +25,14 @@
#include <linux/module.h>
#include <linux/videodev2.h>
#include <linux/uaccess.h>
+#include <linux/of_device.h>
#include <media/adv7343.h>
#include <media/v4l2-async.h>
#include <media/v4l2-device.h>
#include <media/v4l2-chip-ident.h>
#include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
#include "adv7343_regs.h"
@@ -409,6 +411,75 @@ static int adv7343_initialize(struct v4l2_subdev *sd)
return err;
}
+#if defined(CONFIG_OF)
+static const struct of_device_id adv7343_of_match[] = {
+ {.compatible = "ad,adv7343-encoder", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, adv7343_of_match);
+
+static void adv7343_get_pdata(struct i2c_client *client,
+ struct adv7343_state *decoder)
+{
+ if (!client->dev.platform_data && client->dev.of_node) {
+ struct device_node *np;
+ struct adv7343_platform_data *pdata;
+
+ np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+ if (!np)
+ return;
+
+ pdata = devm_kzalloc(&client->dev,
+ sizeof(struct adv7343_platform_data),
+ GFP_KERNEL);
+ if (!pdata) {
+ pr_warn("adv7343 failed allocate memeory\n");
+ return;
+ }
+
+ pdata->mode_config.sleep_mode =
+ of_property_read_bool(np, "ad-adv7343-power-mode-sleep-mode");
+
+ pdata->mode_config.pll_control =
+ of_property_read_bool(np, "ad-adv7343-power-mode-pll-ctrl");
+
+ pdata->mode_config.dac_1 =
+ of_property_read_bool(np, "ad-adv7343-power-mode-dac-1");
+
+ pdata->mode_config.dac_2 =
+ of_property_read_bool(np, "ad-adv7343-power-mode-dac-2");
+
+ pdata->mode_config.dac_3 =
+ of_property_read_bool(np, "ad-adv7343-power-mode-dac-3");
+
+ pdata->mode_config.dac_4 =
+ of_property_read_bool(np, "ad-adv7343-power-mode-dac-4");
+
+ pdata->mode_config.dac_5 =
+ of_property_read_bool(np, "ad-adv7343-power-mode-dac-5");
+
+ pdata->mode_config.dac_6 =
+ of_property_read_bool(np, "ad-adv7343-power-mode-dac-6");
+
+ pdata->sd_config.sd_dac_out1 =
+ of_property_read_bool(np, "ad-adv7343-sd-config-dac-out-1");
+
+ pdata->sd_config.sd_dac_out2 =
+ of_property_read_bool(np, "ad-adv7343-sd-config-dac-out-2");
+
+ decoder->pdata = pdata;
+ }
+}
+#else
+#define adv7343_of_match NULL
+
+static void adv7343_get_pdata(struct i2c_client *client,
+ struct adv7343_state *decoder)
+{
+ decoder->pdata = client->dev.platform_data;
+}
+#endif
+
static int adv7343_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -426,8 +497,7 @@ static int adv7343_probe(struct i2c_client *client,
if (state == NULL)
return -ENOMEM;
- /* Copy board specific information here */
- state->pdata = client->dev.platform_data;
+ adv7343_get_pdata(client, state);
state->reg00 = 0x80;
state->reg01 = 0x00;
@@ -496,6 +566,7 @@ MODULE_DEVICE_TABLE(i2c, adv7343_id);
static struct i2c_driver adv7343_driver = {
.driver = {
+ .of_match_table = adv7343_of_match,
.owner = THIS_MODULE,
.name = "adv7343",
},
--
1.7.4.1
Hi Prabhakar,
Thank you for the patch.
On Friday 26 April 2013 18:48:06 Prabhakar Lad wrote:
> From: Lad, Prabhakar <[email protected]>
>
> add OF support for the adv7343 driver.
>
> Signed-off-by: Lad, Prabhakar <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Guennadi Liakhovetski <[email protected]>
> Cc: Sylwester Nawrocki <[email protected]>
> Cc: Sakari Ailus <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Rob Landley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> .../devicetree/bindings/media/i2c/adv7343.txt | 69 +++++++++++++++++
> drivers/media/i2c/adv7343.c | 75 ++++++++++++++++-
> 2 files changed, 142 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/adv7343.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt
> b/Documentation/devicetree/bindings/media/i2c/adv7343.txt new file mode
> 100644
> index 0000000..8426f8d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
> @@ -0,0 +1,69 @@
> +* Analog Devices adv7343 video encoder
> +
> +The ADV7343 are high speed, digital-to-analog video encoders in a 64-lead
> LQFP +package. Six high speed, 3.3 V, 11-bit video DACs provide support for
> composite +(CVBS), S-Video (Y-C), and component (YPrPb/RGB) analog outputs
> in standard +definition (SD), enhanced definition (ED), or high definition
> (HD) video +formats.
> +
> +The ADV7343 have a 24-bit pixel input port that can be configured in a
> variety +of ways. SD video formats are supported over an SDR interface, and
> ED/HD video +formats are supported over SDR and DDR interfaces. Pixel data
> can be supplied +in either the YCrCb or RGB color spaces.
> +
> +Required Properties :
> +- compatible: Must be "ad,adv7343-encoder"
> +
> +Optional Properties :
> +- ad-adv7343-power-mode-sleep-mode: on enable the current consumption is
> + reduced to micro ampere level. All DACs
> + and the internal PLL circuit are
> + disabled.
> +- ad-adv7343-power-mode-pll-ctrl: PLL and oversampling control. This
> + control allows internal PLL 1 circuit to
> + be powered down and the oversampling to
> + be switched off.
> +
> +- ad-adv7343-power-mode-dac-1: power on/off DAC 1.
> +- ad-adv7343-power-mode-dac-2: power on/off DAC 2.
> +- ad-adv7343-power-mode-dac-3: power on/off DAC 3.
> +- ad-adv7343-power-mode-dac-4: power on/off DAC 4.
> +- ad-adv7343-power-mode-dac-5: power on/off DAC 5.
> +- ad-adv7343-power-mode-dac-6: power on/off DAC 6.
> +- ad-adv7343-sd-config-dac-out-1: Configure SD DAC Output 1.
> +- ad-adv7343-sd-config-dac-out-2: Configure SD DAC Output 2.
s/ad-/ad,/
Do all those properties really need to be specified at the endpoint level
instead of the device node level ?
I'll let Hans comment on the individual properties, he knows more than I do
about DACs.
> +Example:
> +
> +i2c0@1c22000 {
> + ...
> + ...
> +
> + adv7343@2a {
> + compatible = "ad,adv7343-encoder";
> + reg = <0x2a>;
> +
> + port {
> + adv7343_1: endpoint {
> + /* Active high (Defaults to false) */
Active high ?
> + ad-adv7343-power-mode-sleep-mode;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-pll-ctrl;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-1;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-2;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-3;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-4;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-5;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-6;
> + /* Active high (Defaults to false) */
> + ad-adv7343-sd-config-dac-out-1;
> + /* Active high (Defaults to false) */
> + ad-adv7343-sd-config-dac-out-2 = <0>;
> + };
> + };
> + };
> + ...
> +};
> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
> index 469e262..eb12d1a 100644
> --- a/drivers/media/i2c/adv7343.c
> +++ b/drivers/media/i2c/adv7343.c
> @@ -25,12 +25,14 @@
> #include <linux/module.h>
> #include <linux/videodev2.h>
> #include <linux/uaccess.h>
> +#include <linux/of_device.h>
>
> #include <media/adv7343.h>
> #include <media/v4l2-async.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-chip-ident.h>
> #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-of.h>
>
> #include "adv7343_regs.h"
>
> @@ -409,6 +411,75 @@ static int adv7343_initialize(struct v4l2_subdev *sd)
> return err;
> }
>
> +#if defined(CONFIG_OF)
> +static const struct of_device_id adv7343_of_match[] = {
> + {.compatible = "ad,adv7343-encoder", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, adv7343_of_match);
> +
> +static void adv7343_get_pdata(struct i2c_client *client,
> + struct adv7343_state *decoder)
> +{
> + if (!client->dev.platform_data && client->dev.of_node) {
> + struct device_node *np;
> + struct adv7343_platform_data *pdata;
> +
> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return;
> +
> + pdata = devm_kzalloc(&client->dev,
> + sizeof(struct adv7343_platform_data),
> + GFP_KERNEL);
> + if (!pdata) {
> + pr_warn("adv7343 failed allocate memeory\n");
> + return;
> + }
> +
> + pdata->mode_config.sleep_mode =
> + of_property_read_bool(np, "ad-adv7343-power-mode-sleep-mode");
> +
> + pdata->mode_config.pll_control =
> + of_property_read_bool(np, "ad-adv7343-power-mode-pll-ctrl");
> +
> + pdata->mode_config.dac_1 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-1");
> +
> + pdata->mode_config.dac_2 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-2");
> +
> + pdata->mode_config.dac_3 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-3");
> +
> + pdata->mode_config.dac_4 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-4");
> +
> + pdata->mode_config.dac_5 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-5");
> +
> + pdata->mode_config.dac_6 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-6");
> +
> + pdata->sd_config.sd_dac_out1 =
> + of_property_read_bool(np, "ad-adv7343-sd-config-dac-out-1");
> +
> + pdata->sd_config.sd_dac_out2 =
> + of_property_read_bool(np, "ad-adv7343-sd-config-dac-out-2");
> +
> + decoder->pdata = pdata;
> + }
> +}
> +#else
> +#define adv7343_of_match NULL
> +
> +static void adv7343_get_pdata(struct i2c_client *client,
> + struct adv7343_state *decoder)
> +{
> + decoder->pdata = client->dev.platform_data;
> +}
> +#endif
> +
> static int adv7343_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -426,8 +497,7 @@ static int adv7343_probe(struct i2c_client *client,
> if (state == NULL)
> return -ENOMEM;
>
> - /* Copy board specific information here */
> - state->pdata = client->dev.platform_data;
> + adv7343_get_pdata(client, state);
>
> state->reg00 = 0x80;
> state->reg01 = 0x00;
> @@ -496,6 +566,7 @@ MODULE_DEVICE_TABLE(i2c, adv7343_id);
>
> static struct i2c_driver adv7343_driver = {
> .driver = {
> + .of_match_table = adv7343_of_match,
> .owner = THIS_MODULE,
> .name = "adv7343",
> },
--
Regards,
Laurent Pinchart
Hi,
On 04/26/2013 03:18 PM, Prabhakar Lad wrote:
> From: Lad, Prabhakar <[email protected]>
>
> add OF support for the adv7343 driver.
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
> @@ -0,0 +1,69 @@
> +* Analog Devices adv7343 video encoder
> +
> +The ADV7343 are high speed, digital-to-analog video encoders in a 64-lead LQFP
> +package. Six high speed, 3.3 V, 11-bit video DACs provide support for composite
> +(CVBS), S-Video (Y-C), and component (YPrPb/RGB) analog outputs in standard
> +definition (SD), enhanced definition (ED), or high definition (HD) video
> +formats.
> +
> +The ADV7343 have a 24-bit pixel input port that can be configured in a variety
> +of ways. SD video formats are supported over an SDR interface, and ED/HD video
> +formats are supported over SDR and DDR interfaces. Pixel data can be supplied
> +in either the YCrCb or RGB color spaces.
> +
> +Required Properties :
> +- compatible: Must be "ad,adv7343-encoder"
As Laurent pointed out, "-encoder" is probably not necessary, since
there is nothing else in the ADV7343 chip than the video encoder ?
> +Optional Properties :
> +- ad-adv7343-power-mode-sleep-mode: on enable the current consumption is
> + reduced to micro ampere level. All DACs and
> + the internal PLL circuit are disabled.
Why this needs to be specified in the device tree ? How will the hardware
be switched over to normal state if this property is set ?
Couldn't it be a default state by the driver ? And how is it related to
ad-adv7343-power-mode-dac-? properties ?
As pointed out earlier, vendor name in the property names should be separated
with commas, rather than dashes.
> +- ad-adv7343-power-mode-pll-ctrl: PLL and oversampling control. This control
> + allows internal PLL 1 circuit to be powered
> + down and the oversampling to beswitched off.
> +- ad-adv7343-power-mode-dac-1: power on/off DAC 1.
> +- ad-adv7343-power-mode-dac-2: power on/off DAC 2.
> +- ad-adv7343-power-mode-dac-3: power on/off DAC 3.
> +- ad-adv7343-power-mode-dac-4: power on/off DAC 4.
> +- ad-adv7343-power-mode-dac-5: power on/off DAC 5.
> +- ad-adv7343-power-mode-dac-6: power on/off DAC 6.
Is this somehow related to actual wiring on a PCB ? It's also not really
explicit what value corresponds to which state.
> +- ad-adv7343-sd-config-dac-out-1: Configure SD DAC Output 1.
> +- ad-adv7343-sd-config-dac-out-2: Configure SD DAC Output 2.
What are valid values and their meaning ?
> +Example:
> +
> +i2c0@1c22000 {
> + adv7343@2a {
> + compatible = "ad,adv7343-encoder";
> + reg = <0x2a>;
> +
> + port {
> + adv7343_1: endpoint {
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-sleep-mode;
> + /* Active high (Defaults to false) */
Isn't it obvious that if property is not listed it will default to false ?
> + ad-adv7343-power-mode-pll-ctrl;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-1;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-2;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-3;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-4;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-5;
> + /* Active high (Defaults to false) */
> + ad-adv7343-power-mode-dac-6;
> + /* Active high (Defaults to false) */
> + ad-adv7343-sd-config-dac-out-1;
> + /* Active high (Defaults to false) */
> + ad-adv7343-sd-config-dac-out-2 = <0>;
> + };
> + };
> + };
> + ...
> +};
> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
> index 469e262..eb12d1a 100644
> +static void adv7343_get_pdata(struct i2c_client *client,
> + struct adv7343_state *decoder)
> +{
> + if (!client->dev.platform_data && client->dev.of_node) {
> + struct device_node *np;
> + struct adv7343_platform_data *pdata;
> +
> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return;
> +
> + pdata = devm_kzalloc(&client->dev,
> + sizeof(struct adv7343_platform_data),
> + GFP_KERNEL);
> + if (!pdata) {
> + pr_warn("adv7343 failed allocate memeory\n");
Note that (devm_)k*alloc() functions already log any errors. If this function
would be returning pointer to platform data this error message would not be
needed for sure.
> + return;
> + }
> +
> + pdata->mode_config.sleep_mode =
> + of_property_read_bool(np, "ad-adv7343-power-mode-sleep-mode");
> +
> + pdata->mode_config.pll_control =
> + of_property_read_bool(np, "ad-adv7343-power-mode-pll-ctrl");
> +
> + pdata->mode_config.dac_1 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-1");
> +
> + pdata->mode_config.dac_2 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-2");
> +
> + pdata->mode_config.dac_3 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-3");
> +
> + pdata->mode_config.dac_4 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-4");
> +
> + pdata->mode_config.dac_5 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-5");
> +
> + pdata->mode_config.dac_6 =
> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-6");
Looks like you transformed the platform data structure directly into device
tree properties, which in most cases is a wrong approach. I wonder how these
properties are related to actual hardware architecture and if there are no
more hardware specific properties from which these DAC power modes could be
derived.
If you need to always configure all DACs, wouldn't an array property be a
better option ?
> + pdata->sd_config.sd_dac_out1 =
> + of_property_read_bool(np, "ad-adv7343-sd-config-dac-out-1");
> +
> + pdata->sd_config.sd_dac_out2 =
> + of_property_read_bool(np, "ad-adv7343-sd-config-dac-out-2");
> +
> + decoder->pdata = pdata;
> + }
> +}
Thanks,
Sylwester
Hi Laurent,
Thanks for the review.
On Mon, Apr 29, 2013 at 7:32 PM, Laurent Pinchart
<[email protected]> wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Friday 26 April 2013 18:48:06 Prabhakar Lad wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> add OF support for the adv7343 driver.
>>
>> Signed-off-by: Lad, Prabhakar <[email protected]>
>> Cc: Hans Verkuil <[email protected]>
>> Cc: Laurent Pinchart <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> Cc: Guennadi Liakhovetski <[email protected]>
>> Cc: Sylwester Nawrocki <[email protected]>
>> Cc: Sakari Ailus <[email protected]>
>> Cc: Grant Likely <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Rob Landley <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> .../devicetree/bindings/media/i2c/adv7343.txt | 69 +++++++++++++++++
>> drivers/media/i2c/adv7343.c | 75 ++++++++++++++++-
>> 2 files changed, 142 insertions(+), 2 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/media/i2c/adv7343.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>> b/Documentation/devicetree/bindings/media/i2c/adv7343.txt new file mode
>> 100644
>> index 0000000..8426f8d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>> @@ -0,0 +1,69 @@
>> +* Analog Devices adv7343 video encoder
>> +
>> +The ADV7343 are high speed, digital-to-analog video encoders in a 64-lead
>> LQFP +package. Six high speed, 3.3 V, 11-bit video DACs provide support for
>> composite +(CVBS), S-Video (Y-C), and component (YPrPb/RGB) analog outputs
>> in standard +definition (SD), enhanced definition (ED), or high definition
>> (HD) video +formats.
>> +
>> +The ADV7343 have a 24-bit pixel input port that can be configured in a
>> variety +of ways. SD video formats are supported over an SDR interface, and
>> ED/HD video +formats are supported over SDR and DDR interfaces. Pixel data
>> can be supplied +in either the YCrCb or RGB color spaces.
>> +
>> +Required Properties :
>> +- compatible: Must be "ad,adv7343-encoder"
>> +
>> +Optional Properties :
>> +- ad-adv7343-power-mode-sleep-mode: on enable the current consumption is
>> + reduced to micro ampere level. All DACs
>> + and the internal PLL circuit are
>> + disabled.
>> +- ad-adv7343-power-mode-pll-ctrl: PLL and oversampling control. This
>> + control allows internal PLL 1 circuit to
>> + be powered down and the oversampling to
>> + be switched off.
>> +
>> +- ad-adv7343-power-mode-dac-1: power on/off DAC 1.
>> +- ad-adv7343-power-mode-dac-2: power on/off DAC 2.
>> +- ad-adv7343-power-mode-dac-3: power on/off DAC 3.
>> +- ad-adv7343-power-mode-dac-4: power on/off DAC 4.
>> +- ad-adv7343-power-mode-dac-5: power on/off DAC 5.
>> +- ad-adv7343-power-mode-dac-6: power on/off DAC 6.
>> +- ad-adv7343-sd-config-dac-out-1: Configure SD DAC Output 1.
>> +- ad-adv7343-sd-config-dac-out-2: Configure SD DAC Output 2.
>
> s/ad-/ad,/
>
OK
> Do all those properties really need to be specified at the endpoint level
> instead of the device node level ?
>
Yes.
> I'll let Hans comment on the individual properties, he knows more than I do
> about DACs.
>
>> +Example:
>> +
>> +i2c0@1c22000 {
>> + ...
>> + ...
>> +
>> + adv7343@2a {
>> + compatible = "ad,adv7343-encoder";
>> + reg = <0x2a>;
>> +
>> + port {
>> + adv7343_1: endpoint {
>> + /* Active high (Defaults to false) */
>
> Active high ?
>
:-) will fix it.
Regards,
--Prabhakar Lad
Hi Sylwester,
Thanks for the review.
On Mon, Apr 29, 2013 at 10:21 PM, Sylwester Nawrocki
<[email protected]> wrote:
> Hi,
>
> On 04/26/2013 03:18 PM, Prabhakar Lad wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> add OF support for the adv7343 driver.
>
>> +++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>> @@ -0,0 +1,69 @@
>> +* Analog Devices adv7343 video encoder
>> +
>> +The ADV7343 are high speed, digital-to-analog video encoders in a 64-lead LQFP
>> +package. Six high speed, 3.3 V, 11-bit video DACs provide support for composite
>> +(CVBS), S-Video (Y-C), and component (YPrPb/RGB) analog outputs in standard
>> +definition (SD), enhanced definition (ED), or high definition (HD) video
>> +formats.
>> +
>> +The ADV7343 have a 24-bit pixel input port that can be configured in a variety
>> +of ways. SD video formats are supported over an SDR interface, and ED/HD video
>> +formats are supported over SDR and DDR interfaces. Pixel data can be supplied
>> +in either the YCrCb or RGB color spaces.
>> +
>> +Required Properties :
>> +- compatible: Must be "ad,adv7343-encoder"
>
> As Laurent pointed out, "-encoder" is probably not necessary, since
> there is nothing else in the ADV7343 chip than the video encoder ?
>
OK agreed upon.
>> +Optional Properties :
>> +- ad-adv7343-power-mode-sleep-mode: on enable the current consumption is
>> + reduced to micro ampere level. All DACs and
>> + the internal PLL circuit are disabled.
>
> Why this needs to be specified in the device tree ? How will the hardware
> be switched over to normal state if this property is set ?
> Couldn't it be a default state by the driver ? And how is it related to
> ad-adv7343-power-mode-dac-? properties ?
>
well this is the entire register "power mode", hmm as of now there isnt any way
to get back to normal state, this needs to be implemented as part of
suspend/resume
callbacks. Its not related to dac properties.
> As pointed out earlier, vendor name in the property names should be separated
> with commas, rather than dashes.
>
OK
>> +- ad-adv7343-power-mode-pll-ctrl: PLL and oversampling control. This control
>> + allows internal PLL 1 circuit to be powered
>> + down and the oversampling to beswitched off.
>
>> +- ad-adv7343-power-mode-dac-1: power on/off DAC 1.
>> +- ad-adv7343-power-mode-dac-2: power on/off DAC 2.
>> +- ad-adv7343-power-mode-dac-3: power on/off DAC 3.
>> +- ad-adv7343-power-mode-dac-4: power on/off DAC 4.
>> +- ad-adv7343-power-mode-dac-5: power on/off DAC 5.
>> +- ad-adv7343-power-mode-dac-6: power on/off DAC 6.
>
> Is this somehow related to actual wiring on a PCB ? It's also not really
> explicit what value corresponds to which state.
>
No not related to the wiring on PCB. 0 corresponds to OFF state and 1
corresponds
to ON state.
>> +- ad-adv7343-sd-config-dac-out-1: Configure SD DAC Output 1.
>> +- ad-adv7343-sd-config-dac-out-2: Configure SD DAC Output 2.
>
> What are valid values and their meaning ?
>
>> +Example:
>> +
>> +i2c0@1c22000 {
>
>> + adv7343@2a {
>> + compatible = "ad,adv7343-encoder";
>> + reg = <0x2a>;
>> +
>> + port {
>> + adv7343_1: endpoint {
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-power-mode-sleep-mode;
>> + /* Active high (Defaults to false) */
>
> Isn't it obvious that if property is not listed it will default to false ?
>
Yes
>> + ad-adv7343-power-mode-pll-ctrl;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-power-mode-dac-1;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-power-mode-dac-2;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-power-mode-dac-3;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-power-mode-dac-4;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-power-mode-dac-5;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-power-mode-dac-6;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-sd-config-dac-out-1;
>> + /* Active high (Defaults to false) */
>> + ad-adv7343-sd-config-dac-out-2 = <0>;
>> + };
>> + };
>> + };
>> + ...
>> +};
>> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
>> index 469e262..eb12d1a 100644
>
>> +static void adv7343_get_pdata(struct i2c_client *client,
>> + struct adv7343_state *decoder)
>> +{
>> + if (!client->dev.platform_data && client->dev.of_node) {
>> + struct device_node *np;
>> + struct adv7343_platform_data *pdata;
>> +
>> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
>> + if (!np)
>> + return;
>> +
>> + pdata = devm_kzalloc(&client->dev,
>> + sizeof(struct adv7343_platform_data),
>> + GFP_KERNEL);
>> + if (!pdata) {
>> + pr_warn("adv7343 failed allocate memeory\n");
>
> Note that (devm_)k*alloc() functions already log any errors. If this function
> would be returning pointer to platform data this error message would not be
> needed for sure.
>
OK
>> + return;
>> + }
>> +
>> + pdata->mode_config.sleep_mode =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-sleep-mode");
>> +
>> + pdata->mode_config.pll_control =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-pll-ctrl");
>> +
>> + pdata->mode_config.dac_1 =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-1");
>> +
>> + pdata->mode_config.dac_2 =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-2");
>> +
>> + pdata->mode_config.dac_3 =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-3");
>> +
>> + pdata->mode_config.dac_4 =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-4");
>> +
>> + pdata->mode_config.dac_5 =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-5");
>> +
>> + pdata->mode_config.dac_6 =
>> + of_property_read_bool(np, "ad-adv7343-power-mode-dac-6");
>
> Looks like you transformed the platform data structure directly into device
> tree properties, which in most cases is a wrong approach. I wonder how these
> properties are related to actual hardware architecture and if there are no
> more hardware specific properties from which these DAC power modes could be
> derived.
>
yes the platform data is transformed into the device properties.
> If you need to always configure all DACs, wouldn't an array property be a
> better option ?
>
yes that?s a good idea I'll have array instead.
Regards,
--Prabhakar Lad