From: "Lad, Prabhakar" <[email protected]>
add OF support for the adv7343 driver.
Signed-off-by: Lad, Prabhakar <[email protected]>
---
Changes for v2:
1: Fixed naming of properties.
.../devicetree/bindings/media/i2c/adv7343.txt | 54 ++++++++++++++++
drivers/media/i2c/adv7343.c | 65 +++++++++++++++++++-
2 files changed, 118 insertions(+), 1 deletion(-)
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..1d2e854
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
@@ -0,0 +1,54 @@
+* 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.
+
+Required Properties :
+- compatible: Must be "ad,adv7343"
+
+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, 0 = OFF and 1 = ON.
+- ad,adv7343-power-mode-dac-2: power on/off DAC 2, 0 = OFF and 1 = ON.
+- ad,adv7343-power-mode-dac-3: power on/off DAC 3, 0 = OFF and 1 = ON.
+- ad,adv7343-power-mode-dac-4: power on/off DAC 4, 0 = OFF and 1 = ON.
+- ad,adv7343-power-mode-dac-5: power on/off DAC 5, 0 = OFF and 1 = ON.
+- ad,adv7343-power-mode-dac-6: power on/off DAC 6, 0 = OFF and 1 = ON.
+- 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";
+ reg = <0x2a>;
+
+ port {
+ adv7343_1: endpoint {
+ ad,adv7343-power-mode-sleep-mode;
+ ad,adv7343-power-mode-pll-ctrl;
+ ad,adv7343-power-mode-dac-1;
+ ad,adv7343-power-mode-dac-2;
+ ad,adv7343-power-mode-dac-3;
+ ad,adv7343-power-mode-dac-4;
+ ad,adv7343-power-mode-dac-5;
+ ad,adv7343-power-mode-dac-6;
+ ad,adv7343-sd-config-dac-out-1;
+ ad,adv7343-sd-config-dac-out-2;
+ };
+ };
+ };
+ ...
+};
diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
index 7606218..22ee6f4 100644
--- a/drivers/media/i2c/adv7343.c
+++ b/drivers/media/i2c/adv7343.c
@@ -29,6 +29,7 @@
#include <media/adv7343.h>
#include <media/v4l2-device.h>
#include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
#include "adv7343_regs.h"
@@ -398,6 +399,59 @@ static int adv7343_initialize(struct v4l2_subdev *sd)
return err;
}
+static struct adv7343_platform_data *
+adv7343_get_pdata(struct i2c_client *client)
+{
+ struct adv7343_platform_data *pdata;
+ struct device_node *np;
+
+ if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
+ return client->dev.platform_data;
+
+ np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+ if (!np)
+ return NULL;
+
+ pdata = devm_kzalloc(&client->dev, sizeof(struct adv7343_platform_data),
+ GFP_KERNEL);
+ if (!pdata)
+ goto done;
+
+ 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");
+
+done:
+ of_node_put(np);
+ return pdata;
+}
+
static int adv7343_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -416,7 +470,7 @@ static int adv7343_probe(struct i2c_client *client,
return -ENOMEM;
/* Copy board specific information here */
- state->pdata = client->dev.platform_data;
+ state->pdata = adv7343_get_pdata(client);
state->reg00 = 0x80;
state->reg01 = 0x00;
@@ -476,8 +530,17 @@ static const struct i2c_device_id adv7343_id[] = {
MODULE_DEVICE_TABLE(i2c, adv7343_id);
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id adv7343_of_match[] = {
+ {.compatible = "ad,adv7343", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, adv7343_of_match);
+#endif
+
static struct i2c_driver adv7343_driver = {
.driver = {
+ .of_match_table = of_match_ptr(adv7343_of_match),
.owner = THIS_MODULE,
.name = "adv7343",
},
--
1.7.9.5
Hi Prabhakar,
On 07/13/2013 01:12 PM, Prabhakar Lad wrote:
> From: "Lad, Prabhakar"<[email protected]>
>
> add OF support for the adv7343 driver.
>
> Signed-off-by: Lad, Prabhakar<[email protected]>
> ---
> Changes for v2:
> 1: Fixed naming of properties.
>
> .../devicetree/bindings/media/i2c/adv7343.txt | 54 ++++++++++++++++
> drivers/media/i2c/adv7343.c | 65 +++++++++++++++++++-
> 2 files changed, 118 insertions(+), 1 deletion(-)
> 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..1d2e854
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
> @@ -0,0 +1,54 @@
> +* 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.
> +
> +Required Properties :
> +- compatible: Must be "ad,adv7343"
Please have a look at Documentation/devicetree/bindings/vendor-prefixes.txt.
'ad' is a vendor prefix reserved for "Avionic Design GmbH".
For "Analog Devices, Inc." 'adi' should be used.
If I would have to draft a new DT binding proposal checklist checking
vendor-prefixes.txt would certainly be one of the first steps.
> +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, 0 = OFF and 1 = ON.
> +- ad,adv7343-power-mode-dac-2: power on/off DAC 2, 0 = OFF and 1 = ON.
> +- ad,adv7343-power-mode-dac-3: power on/off DAC 3, 0 = OFF and 1 = ON.
> +- ad,adv7343-power-mode-dac-4: power on/off DAC 4, 0 = OFF and 1 = ON.
> +- ad,adv7343-power-mode-dac-5: power on/off DAC 5, 0 = OFF and 1 = ON.
> +- ad,adv7343-power-mode-dac-6: power on/off DAC 6, 0 = OFF and 1 = ON.
> +- ad,adv7343-sd-config-dac-out-1: Configure SD DAC Output 1.
> +- ad,adv7343-sd-config-dac-out-2: Configure SD DAC Output 2.
All these properties look more like hardware configuration, rather than
hardware description. So at first sight I would say none of these properties
is suitable for the device tree.
sleep mode and pll ctrl should likely only have default values in the
driver.
sleep-mode disables all DAC, while power-mode-dac-? does power on/off
(enables / disables?) individual DACs. How those properties interact,
what's
going on here exactly ? :)
That said, how about only leaving the properties indicating which DACs
(including SD DACs) should be enabled ? E.g.
adi,dac-enable - an array indicating which DACs are enabled, in order
DAC1...DAC6, 1 to enable DAC, 0 to disable.
adi,sd-dac-enable - an array indicating which SD DACs are enabled, in order
DAC1...DAC2, 1 to enable SD DAC, 0 to disable.
Please note you don't need ",adv7343-" prefix in each single property
for that device.
> +Example:
> +
> +i2c0@1c22000 {
> + ...
> + ...
> +
> + adv7343@2a {
> + compatible = "ad,adv7343";
> + reg =<0x2a>;
> +
> + port {
> + adv7343_1: endpoint {
> + ad,adv7343-power-mode-sleep-mode;
> + ad,adv7343-power-mode-pll-ctrl;
> + ad,adv7343-power-mode-dac-1;
> + ad,adv7343-power-mode-dac-2;
> + ad,adv7343-power-mode-dac-3;
> + ad,adv7343-power-mode-dac-4;
> + ad,adv7343-power-mode-dac-5;
> + ad,adv7343-power-mode-dac-6;
Then this would have become:
adi,dac-enable = <1 1 1 1 1 1>;
But I would put some disabled DACs in the example as well:
/* Use DAC1..3, DAC6 */
adi,dac-enable = <1 1 1 0 0 1>;
> + ad,adv7343-sd-config-dac-out-1;
> + ad,adv7343-sd-config-dac-out-2;
And this:
adi,sd-dac-enable = <1 1>;
> + };
> + };
> + };
> + ...
> +};
> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
> index 7606218..22ee6f4 100644
> --- a/drivers/media/i2c/adv7343.c
> +++ b/drivers/media/i2c/adv7343.c
> @@ -29,6 +29,7 @@
> #include<media/adv7343.h>
> #include<media/v4l2-device.h>
> #include<media/v4l2-ctrls.h>
> +#include<media/v4l2-of.h>
>
> #include "adv7343_regs.h"
>
> @@ -398,6 +399,59 @@ static int adv7343_initialize(struct v4l2_subdev *sd)
> return err;
> }
>
> +static struct adv7343_platform_data *
> +adv7343_get_pdata(struct i2c_client *client)
> +{
> + struct adv7343_platform_data *pdata;
> + struct device_node *np;
> +
> + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> + return client->dev.platform_data;
> +
> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return NULL;
> +
> + pdata = devm_kzalloc(&client->dev, sizeof(struct adv7343_platform_data),
> + GFP_KERNEL);
> + if (!pdata)
> + goto done;
> +
> + 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");
This doesn't look very impressive. IMHO changing
pdata->mode_config.{dac,sd_dac}
to an array type could simplify this code a little. But you could as
well use
of_property_read_u32_array() and assign each element to corresponding
mode_config.(sd_)dac field.
> +done:
> + of_node_put(np);
> + return pdata;
> +}
> +
--
Thanks,
Sylwester
Hi Sylwester,
Thanks for the review.
On Mon, Jul 15, 2013 at 2:03 AM, Sylwester Nawrocki
<[email protected]> wrote:
> Hi Prabhakar,
>
>
> On 07/13/2013 01:12 PM, Prabhakar Lad wrote:
>>
>> From: "Lad, Prabhakar"<[email protected]>
>>
>> add OF support for the adv7343 driver.
>>
>> Signed-off-by: Lad, Prabhakar<[email protected]>
>> ---
>> Changes for v2:
>> 1: Fixed naming of properties.
>>
>> .../devicetree/bindings/media/i2c/adv7343.txt | 54
>> ++++++++++++++++
>> drivers/media/i2c/adv7343.c | 65
>> +++++++++++++++++++-
>> 2 files changed, 118 insertions(+), 1 deletion(-)
>> 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..1d2e854
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>> @@ -0,0 +1,54 @@
>> +* 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.
>> +
>> +Required Properties :
>> +- compatible: Must be "ad,adv7343"
>
>
> Please have a look at Documentation/devicetree/bindings/vendor-prefixes.txt.
> 'ad' is a vendor prefix reserved for "Avionic Design GmbH".
> For "Analog Devices, Inc." 'adi' should be used.
>
> If I would have to draft a new DT binding proposal checklist checking
> vendor-prefixes.txt would certainly be one of the first steps.
>
That makes it easier I wasn?t aware of this file thanks :)
>
>> +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, 0 = OFF and 1 = ON.
>> +- ad,adv7343-power-mode-dac-2: power on/off DAC 2, 0 = OFF and 1 = ON.
>> +- ad,adv7343-power-mode-dac-3: power on/off DAC 3, 0 = OFF and 1 = ON.
>> +- ad,adv7343-power-mode-dac-4: power on/off DAC 4, 0 = OFF and 1 = ON.
>> +- ad,adv7343-power-mode-dac-5: power on/off DAC 5, 0 = OFF and 1 = ON.
>> +- ad,adv7343-power-mode-dac-6: power on/off DAC 6, 0 = OFF and 1 = ON.
>> +- ad,adv7343-sd-config-dac-out-1: Configure SD DAC Output 1.
>> +- ad,adv7343-sd-config-dac-out-2: Configure SD DAC Output 2.
>
>
> All these properties look more like hardware configuration, rather than
> hardware description. So at first sight I would say none of these properties
> is suitable for the device tree.
>
> sleep mode and pll ctrl should likely only have default values in the
> driver.
> sleep-mode disables all DAC, while power-mode-dac-? does power on/off
> (enables / disables?) individual DACs. How those properties interact, what's
> going on here exactly ? :)
>
Agreed the device tree is meant to describe the hardware, basically the the need
had arrived since the adv7343 is present on DM6467 EVM and also on the
OMAP-L138 EVM, the configuration of this two register's differs on these
two evm's mentioned, because of which this is now being passed as part of
the device tree.
> That said, how about only leaving the properties indicating which DACs
> (including SD DACs) should be enabled ? E.g.
>
> adi,dac-enable - an array indicating which DACs are enabled, in order
> DAC1...DAC6, 1 to enable DAC, 0 to disable.
>
OK
> adi,sd-dac-enable - an array indicating which SD DACs are enabled, in order
> DAC1...DAC2, 1 to enable SD DAC, 0 to disable.
>
OK
> Please note you don't need ",adv7343-" prefix in each single property
> for that device.
>
Agreed.
>
>> +Example:
>> +
>> +i2c0@1c22000 {
>> + ...
>> + ...
>> +
>> + adv7343@2a {
>> + compatible = "ad,adv7343";
>> + reg =<0x2a>;
>> +
>> + port {
>> + adv7343_1: endpoint {
>> + ad,adv7343-power-mode-sleep-mode;
>> + ad,adv7343-power-mode-pll-ctrl;
>> + ad,adv7343-power-mode-dac-1;
>> + ad,adv7343-power-mode-dac-2;
>> + ad,adv7343-power-mode-dac-3;
>> + ad,adv7343-power-mode-dac-4;
>> + ad,adv7343-power-mode-dac-5;
>> + ad,adv7343-power-mode-dac-6;
>
>
> Then this would have become:
> adi,dac-enable = <1 1 1 1 1 1>;
>
> But I would put some disabled DACs in the example as well:
>
OK will do that.
> /* Use DAC1..3, DAC6 */
> adi,dac-enable = <1 1 1 0 0 1>;
>
>> + ad,adv7343-sd-config-dac-out-1;
>> + ad,adv7343-sd-config-dac-out-2;
>
>
> And this:
> adi,sd-dac-enable = <1 1>;
>
Ok.
>
>> + };
>> + };
>> + };
>> + ...
>> +};
[snip]
>> + 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");
>
>
> This doesn't look very impressive. IMHO changing
> pdata->mode_config.{dac,sd_dac}
> to an array type could simplify this code a little. But you could as well
> use
> of_property_read_u32_array() and assign each element to corresponding
> mode_config.(sd_)dac field.
>
OK.
Regards,
--Prabhakar Lad