| Message ID | 1373713959-31066-1-git-send-email-prabhakar.csengg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Prabhakar, On 07/13/2013 01:12 PM, Prabhakar Lad wrote: > From: "Lad, Prabhakar"<prabhakar.csengg@gmail.com> > > add OF support for the adv7343 driver. > > Signed-off-by: Lad, Prabhakar<prabhakar.csengg@gmail.com> > --- > 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 <sylvester.nawrocki@gmail.com> wrote: > Hi Prabhakar, > > > On 07/13/2013 01:12 PM, Prabhakar Lad wrote: >> >> From: "Lad, Prabhakar"<prabhakar.csengg@gmail.com> >> >> add OF support for the adv7343 driver. >> >> Signed-off-by: Lad, Prabhakar<prabhakar.csengg@gmail.com> >> --- >> 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
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", },