lkml.org 
[lkml]   [2013]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2] media: i2c: adv7343: add OF support
    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

    \
     
     \ /
      Last update: 2013-07-14 22:41    [W:3.034 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site