[PATCH] media: i2c: adv7343: fix the DT binding properties

[PATCH] media: i2c: adv7343: fix the DT binding properties

[Prabhakar Lad]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

This patch fixes the DT binding properties of adv7343 decoder.
The pdata which was being read from the DT property, is removed
as this can done internally in the driver using cable detection
register.

This patch also removes the pdata of ADV7343 which was passed from
DA850 machine.

Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
 .../devicetree/bindings/media/i2c/adv7343.txt      |   33 ++++----
 arch/arm/mach-davinci/board-da850-evm.c            |   10 ---
 drivers/media/i2c/adv7343.c                        |   86 ++++++--------------
 drivers/media/i2c/adv7343_regs.h                   |    8 +-
 include/media/adv7343.h                            |   40 ---------
 5 files changed, 42 insertions(+), 135 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
index 5653bc2..5d0e7e4 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7343.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
@@ -8,39 +8,34 @@ formats.
 
 Required Properties :
 - compatible: Must be "adi,adv7343"
+- reg: I2C device address.
+- vddio-supply: I/O voltage supply.
+- vddcore-supply: core voltage supply.
+- vaa-supply: Analog power supply.
+- pvdd-supply: PLL power supply.
 
 Optional Properties :
-- adi,power-mode-sleep-mode: on enable the current consumption is reduced to
-			      micro ampere level. All DACs and the internal PLL
-			      circuit are disabled.
-- adi,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: array configuring the power on/off DAC's 1..6,
-			      0 = OFF and 1 = ON, Default value when this
-			      property is not specified is <0 0 0 0 0 0>.
-- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF
-				 and 1 = ON, Default value when this property is
-				 not specified is <0 0>.
+- vref-supply: Voltage reference output.
+
+For further reading on port node refer to
+Documentation/devicetree/bindings/media/video-interfaces.txt.
 
 Example:
 
 i2c0@1c22000 {
 	...
 	...
-
 	adv7343@2a {
 		compatible = "adi,adv7343";
 		reg = <0x2a>;
+		vddio-supply = <&regulator1>;
+		vddcore-supply = <&regulator2>;
+		vaa-supply = <&regulator3>;
+		pvdd-supply = <&regulator4>;
 
 		port {
 			adv7343_1: endpoint {
-					adi,power-mode-sleep-mode;
-					adi,power-mode-pll-ctrl;
-					/* Use DAC1..3, DAC6 */
-					adi,dac-enable = <1 1 1 0 0 1>;
-					/* Use SD DAC output 1 */
-					adi,sd-dac-enable = <1 0>;
+				remote-endpoint = <&vpif0_1>;
 			};
 		};
 	};
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index dd1fb24..5c72c8a 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1243,21 +1243,11 @@ static struct vpif_capture_config da850_vpif_capture_config = {
 
 /* VPIF display configuration */
 
-static struct adv7343_platform_data adv7343_pdata = {
-	.mode_config = {
-		.dac = { 1, 1, 1 },
-	},
-	.sd_config = {
-		.sd_dac_out = { 1 },
-	},
-};
-
 static struct vpif_subdev_info da850_vpif_subdev[] = {
 	{
 		.name = "adv7343",
 		.board_info = {
 			I2C_BOARD_INFO("adv7343", 0x2a),
-			.platform_data = &adv7343_pdata,
 		},
 	},
 };
diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
index aeb56c5..97aa8e5 100644
--- a/drivers/media/i2c/adv7343.c
+++ b/drivers/media/i2c/adv7343.c
@@ -44,7 +44,6 @@ MODULE_PARM_DESC(debug, "Debug level 0-1");
 struct adv7343_state {
 	struct v4l2_subdev sd;
 	struct v4l2_ctrl_handler hdl;
-	const struct adv7343_platform_data *pdata;
 	u8 reg00;
 	u8 reg01;
 	u8 reg02;
@@ -72,6 +71,13 @@ static inline int adv7343_write(struct v4l2_subdev *sd, u8 reg, u8 value)
 	return i2c_smbus_write_byte_data(client, reg, value);
 }
 
+static int adv7343_read(struct v4l2_subdev *sd, u8 reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
 static const u8 adv7343_init_reg_val[] = {
 	ADV7343_SOFT_RESET, ADV7343_SOFT_RESET_DEFAULT,
 	ADV7343_POWER_MODE_REG, ADV7343_POWER_MODE_REG_DEFAULT,
@@ -204,6 +210,7 @@ setstd_exit:
 static int adv7343_setoutput(struct v4l2_subdev *sd, u32 output_type)
 {
 	struct adv7343_state *state = to_state(sd);
+	unsigned char cable_detect;
 	unsigned char val;
 	int err = 0;
 
@@ -217,23 +224,8 @@ static int adv7343_setoutput(struct v4l2_subdev *sd, u32 output_type)
 	/* Enable Appropriate DAC */
 	val = state->reg00 & 0x03;
 
-	/* configure default configuration */
-	if (!state->pdata)
-		if (output_type == ADV7343_COMPOSITE_ID)
-			val |= ADV7343_COMPOSITE_POWER_VALUE;
-		else if (output_type == ADV7343_COMPONENT_ID)
-			val |= ADV7343_COMPONENT_POWER_VALUE;
-		else
-			val |= ADV7343_SVIDEO_POWER_VALUE;
-	else
-		val = state->pdata->mode_config.sleep_mode << 0 |
-		      state->pdata->mode_config.pll_control << 1 |
-		      state->pdata->mode_config.dac[2] << 2 |
-		      state->pdata->mode_config.dac[1] << 3 |
-		      state->pdata->mode_config.dac[0] << 4 |
-		      state->pdata->mode_config.dac[5] << 5 |
-		      state->pdata->mode_config.dac[4] << 6 |
-		      state->pdata->mode_config.dac[3] << 7;
+	/* Enable all the DAC's DAC1..DAC6 */
+	val |= ADV7343_POWER_VALUE;
 
 	err = adv7343_write(sd, ADV7343_POWER_MODE_REG, val);
 	if (err < 0)
@@ -252,15 +244,14 @@ static int adv7343_setoutput(struct v4l2_subdev *sd, u32 output_type)
 	/* configure SD DAC Output 2 and SD DAC Output 1 bit to zero */
 	val = state->reg82 & (SD_DAC_1_DI & SD_DAC_2_DI);
 
-	if (state->pdata && state->pdata->sd_config.sd_dac_out[0])
-		val = val | (state->pdata->sd_config.sd_dac_out[0] << 1);
-	else if (state->pdata && !state->pdata->sd_config.sd_dac_out[0])
-		val = val & ~(state->pdata->sd_config.sd_dac_out[0] << 1);
+	cable_detect = adv7343_read(sd, ADV7343_CABLE_DETECTION);
+	/* enable SD DAC output 1 */
+	if (!(cable_detect & (1 << 0)))
+		val = val | 0x2;
 
-	if (state->pdata && state->pdata->sd_config.sd_dac_out[1])
-		val = val | (state->pdata->sd_config.sd_dac_out[1] << 2);
-	else if (state->pdata && !state->pdata->sd_config.sd_dac_out[1])
-		val = val & ~(state->pdata->sd_config.sd_dac_out[1] << 2);
+	/* enable SD DAC output 2 */
+	if (!(cable_detect & (1 << 1)))
+		val = val | 0x4;
 
 	err = adv7343_write(sd, ADV7343_SD_MODE_REG2, val);
 	if (err < 0)
@@ -268,6 +259,12 @@ static int adv7343_setoutput(struct v4l2_subdev *sd, u32 output_type)
 
 	state->reg82 = val;
 
+	/* enable unconnected DAC auto power down */
+	err = adv7343_write(sd, ADV7343_CABLE_DETECTION,
+			    cable_detect | ADV7343_AUTO_POWER_DOWN_ENABLE);
+	if (err < 0)
+		goto setoutput_exit;
+
 	/* configure ED/HD Color DAC Swap and ED/HD RGB Input Enable bit to
 	 * zero */
 	val = state->reg35 & (HD_RGB_INPUT_DI & HD_DAC_SWAP_DI);
@@ -400,40 +397,6 @@ 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(*pdata), GFP_KERNEL);
-	if (!pdata)
-		goto done;
-
-	pdata->mode_config.sleep_mode =
-			of_property_read_bool(np, "adi,power-mode-sleep-mode");
-
-	pdata->mode_config.pll_control =
-			of_property_read_bool(np, "adi,power-mode-pll-ctrl");
-
-	of_property_read_u32_array(np, "adi,dac-enable",
-				   pdata->mode_config.dac, 6);
-
-	of_property_read_u32_array(np, "adi,sd-dac-enable",
-				   pdata->sd_config.sd_dac_out, 2);
-
-done:
-	of_node_put(np);
-	return pdata;
-}
-
 static int adv7343_probe(struct i2c_client *client,
 				const struct i2c_device_id *id)
 {
@@ -451,9 +414,6 @@ static int adv7343_probe(struct i2c_client *client,
 	if (state == NULL)
 		return -ENOMEM;
 
-	/* Copy board specific information here */
-	state->pdata = adv7343_get_pdata(client);
-
 	state->reg00	= 0x80;
 	state->reg01	= 0x00;
 	state->reg02	= 0x20;
diff --git a/drivers/media/i2c/adv7343_regs.h b/drivers/media/i2c/adv7343_regs.h
index 4466067..94d9722 100644
--- a/drivers/media/i2c/adv7343_regs.h
+++ b/drivers/media/i2c/adv7343_regs.h
@@ -29,6 +29,8 @@ struct adv7343_std_info {
 
 #define ADV7343_DAC2_OUTPUT_LEVEL	(0x0b)
 
+#define ADV7343_CABLE_DETECTION		0x10
+
 #define ADV7343_SOFT_RESET		(0x17)
 
 #define ADV7343_HD_MODE_REG1		(0x30)
@@ -72,9 +74,7 @@ struct adv7343_std_info {
 #define ADV7343_HD_MODE_REG7_DEFAULT		(0x00)
 #define ADV7343_SD_MODE_REG8_DEFAULT		(0x00)
 #define ADV7343_SOFT_RESET_DEFAULT		(0x02)
-#define ADV7343_COMPOSITE_POWER_VALUE		(0x80)
-#define ADV7343_COMPONENT_POWER_VALUE		(0x1C)
-#define ADV7343_SVIDEO_POWER_VALUE		(0x60)
+#define ADV7343_POWER_VALUE			0xfc
 #define ADV7343_SD_HUE_REG_DEFAULT		(127)
 #define ADV7343_SD_BRIGHTNESS_WSS_DEFAULT	(0x03)
 
@@ -178,4 +178,6 @@ struct adv7343_std_info {
 #define ADV7343_GAIN_MIN	(-64)
 #define ADV7343_GAIN_DEF	(0)
 
+#define ADV7343_AUTO_POWER_DOWN_ENABLE	0x10
+
 #endif
diff --git a/include/media/adv7343.h b/include/media/adv7343.h
index e4142b1..d6f8a4e 100644
--- a/include/media/adv7343.h
+++ b/include/media/adv7343.h
@@ -20,44 +20,4 @@
 #define ADV7343_COMPONENT_ID	(1)
 #define ADV7343_SVIDEO_ID	(2)
 
-/**
- * adv7343_power_mode - power mode configuration.
- * @sleep_mode: on enable the current consumption is reduced to micro ampere
- *		level. All DACs and the internal PLL circuit are disabled.
- *		Registers can be read from and written in sleep mode.
- * @pll_control: PLL and oversampling control. This control allows internal
- *		 PLL 1 circuit to be powered down and the oversampling to be
- *		 switched off.
- * @dac: array to configure power on/off DAC's 1..6
- *
- * Power mode register (Register 0x0), for more info refer REGISTER MAP ACCESS
- * section of datasheet[1], table 17 page no 30.
- *
- * [1] http://www.analog.com/static/imported-files/data_sheets/ADV7342_7343.pdf
- */
-struct adv7343_power_mode {
-	bool sleep_mode;
-	bool pll_control;
-	u32 dac[6];
-};
-
-/**
- * struct adv7343_sd_config - SD Only Output Configuration.
- * @sd_dac_out: array configuring SD DAC Outputs 1 and 2
- */
-struct adv7343_sd_config {
-	/* SD only Output Configuration */
-	u32 sd_dac_out[2];
-};
-
-/**
- * struct adv7343_platform_data - Platform data values and access functions.
- * @mode_config: Configuration for power mode.
- * @sd_config: SD Only Configuration.
- */
-struct adv7343_platform_data {
-	struct adv7343_power_mode mode_config;
-	struct adv7343_sd_config sd_config;
-};
-
 #endif				/* End of #ifndef ADV7343_H */
-- 
1.7.9.5

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Stephen Warren]

On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> This patch fixes the DT binding properties of adv7343 decoder.
> The pdata which was being read from the DT property, is removed
> as this can done internally in the driver using cable detection
> register.
> 
> This patch also removes the pdata of ADV7343 which was passed from
> DA850 machine.

> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt

>  Required Properties :
>  - compatible: Must be "adi,adv7343"
> +- reg: I2C device address.
> +- vddio-supply: I/O voltage supply.
> +- vddcore-supply: core voltage supply.
> +- vaa-supply: Analog power supply.
> +- pvdd-supply: PLL power supply.

Old DTs won't contain those properties. This breaks the DT ABI if those
properties are required. Is that acceptable?

If it is, I think we should document that older versions of the binding
didn't require those properties, so they may in fact be missing.

I note that this patch doesn't actually update the driver to
regulator_get() anything. Shouldn't it?

>  Optional Properties :
> -- adi,power-mode-sleep-mode: on enable the current consumption is reduced to
> -			      micro ampere level. All DACs and the internal PLL
> -			      circuit are disabled.
> -- adi,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: array configuring the power on/off DAC's 1..6,
> -			      0 = OFF and 1 = ON, Default value when this
> -			      property is not specified is <0 0 0 0 0 0>.
> -- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF
> -				 and 1 = ON, Default value when this property is
> -				 not specified is <0 0>.

At a very quick glance, it's not really clear why those properties are
being removed. They seem like HW configuration, so might be fine to put
into DT. What replaces these?

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Prabhakar Lad]

Hi Stephen,

This patch should have been marked as RFC.

Thanks for the review.

On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> This patch fixes the DT binding properties of adv7343 decoder.
>> The pdata which was being read from the DT property, is removed
>> as this can done internally in the driver using cable detection
>> register.
>>
>> This patch also removes the pdata of ADV7343 which was passed from
>> DA850 machine.
>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>
>>  Required Properties :
>>  - compatible: Must be "adi,adv7343"
>> +- reg: I2C device address.
>> +- vddio-supply: I/O voltage supply.
>> +- vddcore-supply: core voltage supply.
>> +- vaa-supply: Analog power supply.
>> +- pvdd-supply: PLL power supply.
>
> Old DTs won't contain those properties. This breaks the DT ABI if those
> properties are required. Is that acceptable?
>
As of now adv7343 via DT binding is not enabled in any platforms
so this wont break any DT ABI.

> If it is, I think we should document that older versions of the binding
> didn't require those properties, so they may in fact be missing.
>
> I note that this patch doesn't actually update the driver to
> regulator_get() anything. Shouldn't it?
>
As of now the driver isn’t enabling/accepting the regulators,
so should I add those in DT properties or not ?

>>  Optional Properties :
>> -- adi,power-mode-sleep-mode: on enable the current consumption is reduced to
>> -                           micro ampere level. All DACs and the internal PLL
>> -                           circuit are disabled.
>> -- adi,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: array configuring the power on/off DAC's 1..6,
>> -                           0 = OFF and 1 = ON, Default value when this
>> -                           property is not specified is <0 0 0 0 0 0>.
>> -- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF
>> -                              and 1 = ON, Default value when this property is
>> -                              not specified is <0 0>.
>
> At a very quick glance, it's not really clear why those properties are
> being removed. They seem like HW configuration, so might be fine to put
> into DT. What replaces these?

Yes these were HW configuration but, its now internally handled in
the driver.  The 'ad,adv7343-power-mode-dac' property which enabled the
DAC's 1..6 , so now in the driver by default all the DAC's are enabled by
default and enable unconnected DAC auto power down. Similarly
'ad,adv7343-sd-config-dac-out' property enabled SD DAC's 1..2 but
now is enabled by reading the CABLE DETECT register which tells
the status of DAC1/2.

Regards,
--Prabhakar

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Stephen Warren]

On 09/13/2013 11:23 PM, Prabhakar Lad wrote:
> Hi Stephen,
> 
> This patch should have been marked as RFC.
> 
> Thanks for the review.
> 
> On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren <swarren-3lzwWm7+WeqIh2JisoIxyg@public.gmane.orgg> wrote:
>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>> From: "Lad, Prabhakar" <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>
>>> This patch fixes the DT binding properties of adv7343 decoder.
>>> The pdata which was being read from the DT property, is removed
>>> as this can done internally in the driver using cable detection
>>> register.
>>>
>>> This patch also removes the pdata of ADV7343 which was passed from
>>> DA850 machine.
>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>>
>>>  Required Properties :
>>>  - compatible: Must be "adi,adv7343"
>>> +- reg: I2C device address.
>>> +- vddio-supply: I/O voltage supply.
>>> +- vddcore-supply: core voltage supply.
>>> +- vaa-supply: Analog power supply.
>>> +- pvdd-supply: PLL power supply.
>>
>> Old DTs won't contain those properties. This breaks the DT ABI if those
>> properties are required. Is that acceptable?
>
> As of now adv7343 via DT binding is not enabled in any platforms
> so this wont break any DT ABI.

Well, if the binding has already been written, it technically already is
an ABI. Perhaps the binding can be fixed if it isn't in use yet, but
this is definitely not the correct approach to DT.

>> If it is, I think we should document that older versions of the binding
>> didn't require those properties, so they may in fact be missing.
>>
>> I note that this patch doesn't actually update the driver to
>> regulator_get() anything. Shouldn't it?
>
> As of now the driver isn’t enabling/accepting the regulators,
> so should I add those in DT properties or not ?

The binding should describe the HW, not what the driver does/doesn't yet
do. I wrote the above because it looked like the driver was broken, not
to encourage you to remove properties from the binding. How does the
driver work if it doesn't enable the required regulators though, I
wonder? I suppose the boards this driver has been tested on all must
used fixed (non-SW-controlled) regulators.

>>>  Optional Properties :
>>> -- adi,power-mode-sleep-mode: on enable the current consumption is reduced to
>>> -                           micro ampere level. All DACs and the internal PLL
>>> -                           circuit are disabled.
>>> -- adi,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: array configuring the power on/off DAC's 1..6,
>>> -                           0 = OFF and 1 = ON, Default value when this
>>> -                           property is not specified is <0 0 0 0 0 0>.
>>> -- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF
>>> -                              and 1 = ON, Default value when this property is
>>> -                              not specified is <0 0>.
>>
>> At a very quick glance, it's not really clear why those properties are
>> being removed. They seem like HW configuration, so might be fine to put
>> into DT. What replaces these?
> 
> Yes these were HW configuration but, its now internally handled in
> the driver.  The 'ad,adv7343-power-mode-dac' property which enabled the
> DAC's 1..6 , so now in the driver by default all the DAC's are enabled by
> default and enable unconnected DAC auto power down. Similarly
> 'ad,adv7343-sd-config-dac-out' property enabled SD DAC's 1..2 but
> now is enabled by reading the CABLE DETECT register which tells
> the status of DAC1/2.

OK, that's probably fine for the two properties you mentioned (you
didn't describe two of them...). Some more discussion on why SW doesn't
need these options might be useful in the patch description. Note that
the discussion should be written for software in general (i.e. any OS's
driver), and not for Linux's specific driver, since DT is not tied to
any one OS.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Prabhakar Lad]

Hi Stephen,

On Mon, Sep 16, 2013 at 9:54 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 09/13/2013 11:23 PM, Prabhakar Lad wrote:
>> Hi Stephen,
>>
>> This patch should have been marked as RFC.
>>
>> Thanks for the review.
>>
>> On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>>> From: "Lad, Prabhakar" <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>
>>>> This patch fixes the DT binding properties of adv7343 decoder.
>>>> The pdata which was being read from the DT property, is removed
>>>> as this can done internally in the driver using cable detection
>>>> register.
>>>>
>>>> This patch also removes the pdata of ADV7343 which was passed from
>>>> DA850 machine.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>>>
>>>>  Required Properties :
>>>>  - compatible: Must be "adi,adv7343"
>>>> +- reg: I2C device address.
>>>> +- vddio-supply: I/O voltage supply.
>>>> +- vddcore-supply: core voltage supply.
>>>> +- vaa-supply: Analog power supply.
>>>> +- pvdd-supply: PLL power supply.
>>>
>>> Old DTs won't contain those properties. This breaks the DT ABI if those
>>> properties are required. Is that acceptable?
>>
>> As of now adv7343 via DT binding is not enabled in any platforms
>> so this wont break any DT ABI.
>
> Well, if the binding has already been written, it technically already is
> an ABI. Perhaps the binding can be fixed if it isn't in use yet, but
> this is definitely not the correct approach to DT.
>
Ok

>>> If it is, I think we should document that older versions of the binding
>>> didn't require those properties, so they may in fact be missing.
>>>
>>> I note that this patch doesn't actually update the driver to
>>> regulator_get() anything. Shouldn't it?
>>
>> As of now the driver isn’t enabling/accepting the regulators,
>> so should I add those in DT properties or not ?
>
> The binding should describe the HW, not what the driver does/doesn't yet
> do. I wrote the above because it looked like the driver was broken, not
> to encourage you to remove properties from the binding.
OK

> How does the
> driver work if it doesn't enable the required regulators though, I
> wonder? I suppose the boards this driver has been tested on all must
> used fixed (non-SW-controlled) regulators.
>
on all the boards on which this decoder is connected the power to it
is provided by static circuit and not by regulators, So for this how would
you suggest to add the DT nodes for regulators ?

>>>>  Optional Properties :
>>>> -- adi,power-mode-sleep-mode: on enable the current consumption is reduced to
>>>> -                           micro ampere level. All DACs and the internal PLL
>>>> -                           circuit are disabled.
>>>> -- adi,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: array configuring the power on/off DAC's 1..6,
>>>> -                           0 = OFF and 1 = ON, Default value when this
>>>> -                           property is not specified is <0 0 0 0 0 0>.
>>>> -- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF
>>>> -                              and 1 = ON, Default value when this property is
>>>> -                              not specified is <0 0>.
>>>
>>> At a very quick glance, it's not really clear why those properties are
>>> being removed. They seem like HW configuration, so might be fine to put
>>> into DT. What replaces these?
>>
>> Yes these were HW configuration but, its now internally handled in
>> the driver.  The 'ad,adv7343-power-mode-dac' property which enabled the
>> DAC's 1..6 , so now in the driver by default all the DAC's are enabled by
>> default and enable unconnected DAC auto power down. Similarly
>> 'ad,adv7343-sd-config-dac-out' property enabled SD DAC's 1..2 but
>> now is enabled by reading the CABLE DETECT register which tells
>> the status of DAC1/2.
>
> OK, that's probably fine for the two properties you mentioned (you
> didn't describe two of them...). Some more discussion on why SW doesn't
> need these options might be useful in the patch description. Note that
> the discussion should be written for software in general (i.e. any OS's
> driver), and not for Linux's specific driver, since DT is not tied to
> any one OS.
OK will update the description.

Regards,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Sylwester Nawrocki]

On 09/19/2013 06:06 PM, Prabhakar Lad wrote:
> On Mon, Sep 16, 2013 at 9:54 PM, Stephen Warren<swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>  wrote:
>> On 09/13/2013 11:23 PM, Prabhakar Lad wrote:
>>> On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren<swarren@wwwdotorg.org>  wrote:
>>>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>>>> From: "Lad, Prabhakar"<prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>
>>>>> This patch fixes the DT binding properties of adv7343 decoder.
>>>>> The pdata which was being read from the DT property, is removed
>>>>> as this can done internally in the driver using cable detection
>>>>> register.
>>>>>
>>>>> This patch also removes the pdata of ADV7343 which was passed from
>>>>> DA850 machine.
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>>>>
>>>>>   Required Properties :
>>>>>   - compatible: Must be "adi,adv7343"
>>>>> +- reg: I2C device address.
>>>>> +- vddio-supply: I/O voltage supply.
>>>>> +- vddcore-supply: core voltage supply.
>>>>> +- vaa-supply: Analog power supply.
>>>>> +- pvdd-supply: PLL power supply.
>>>>
>>>> Old DTs won't contain those properties. This breaks the DT ABI if those
>>>> properties are required. Is that acceptable?
>>>
>>> As of now adv7343 via DT binding is not enabled in any platforms
>>> so this wont break any DT ABI.
>>
>> Well, if the binding has already been written, it technically already is
>> an ABI. Perhaps the binding can be fixed if it isn't in use yet, but
>> this is definitely not the correct approach to DT.

The binding got merged for 3.12-rc1 and the intention of this patch was
to correct that binding. There we some issues like mismatch between names
of properties documented and used in the driver.

After Mark's suggestion Prabhakar removed some properties and the platform
data usage altogether. IMHO there should be only minimal changes in that
"fixup" patch, i.e. no platform data usage should be removed. Perhaps it
is fine since that's just code removal. I guess it is better to do this
sort of cleanup for the next kernel release.

Also I believe the argument of backward compatibility shouldn't really be
considered here. The $subject patch is supposed to correct the binding
before it becomes and ABI.

>>>> If it is, I think we should document that older versions of the binding
>>>> didn't require those properties, so they may in fact be missing.
>>>>
>>>> I note that this patch doesn't actually update the driver to
>>>> regulator_get() anything. Shouldn't it?
>>>
>>> As of now the driver isn’t enabling/accepting the regulators,
>>> so should I add those in DT properties or not ?
>>
>> The binding should describe the HW, not what the driver does/doesn't yet
>> do. I wrote the above because it looked like the driver was broken, not
>> to encourage you to remove properties from the binding.
> OK
>
>> How does the
>> driver work if it doesn't enable the required regulators though, I
>> wonder? I suppose the boards this driver has been tested on all must
>> used fixed (non-SW-controlled) regulators.
>>
> on all the boards on which this decoder is connected the power to it
> is provided by static circuit and not by regulators, So for this how would
> you suggest to add the DT nodes for regulators ?

I believe the regulator DT properties should be made optional. Since some
(actually all upstream) boards don't bother with software controlled
regulators. We might have specified them and have defined relevant fixed
regulator(s) in DT. But I doubt it is sensible, given that it may never
happen in practice the regulators are required to be controlled by software
through the regulators API. Such devices can often be put in a low power
mode by a write to one of the registers, where their supply current is at
uA level. Looking at the datasheet ADV7343 has SLEEP_MODE in which its
typical current consumption is 5 uA.

That said the chip could be supplied from shared voltage regulators and
the driver would then have to properly request and enable the regulators.

Anyway I'm inclined to make the regulator properties optional.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Prabhakar Lad]

Hi Sylwester,

On Fri, Sep 20, 2013 at 1:19 AM, Sylwester Nawrocki
<sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 09/19/2013 06:06 PM, Prabhakar Lad wrote:
>>
>> On Mon, Sep 16, 2013 at 9:54 PM, Stephen Warren<swarren-3lzwWm7+WeqIh2JisoIxyg@public.gmane.orgg>
>> wrote:
>>>
>>> On 09/13/2013 11:23 PM, Prabhakar Lad wrote:
>>>>
>>>> On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren<swarren@wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>>>>>
>>>>>> From: "Lad, Prabhakar"<prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>
>>>>>> This patch fixes the DT binding properties of adv7343 decoder.
>>>>>> The pdata which was being read from the DT property, is removed
>>>>>> as this can done internally in the driver using cable detection
>>>>>> register.
>>>>>>
>>>>>> This patch also removes the pdata of ADV7343 which was passed from
>>>>>> DA850 machine.
>>>>>
>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>>>>>> b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>>>>>
>>>>>
>>>>>>   Required Properties :
>>>>>>   - compatible: Must be "adi,adv7343"
>>>>>> +- reg: I2C device address.
>>>>>> +- vddio-supply: I/O voltage supply.
>>>>>> +- vddcore-supply: core voltage supply.
>>>>>> +- vaa-supply: Analog power supply.
>>>>>> +- pvdd-supply: PLL power supply.
>>>>>
>>>>>
>>>>> Old DTs won't contain those properties. This breaks the DT ABI if those
>>>>> properties are required. Is that acceptable?
>>>>
>>>>
>>>> As of now adv7343 via DT binding is not enabled in any platforms
>>>> so this wont break any DT ABI.
>>>
>>>
>>> Well, if the binding has already been written, it technically already is
>>> an ABI. Perhaps the binding can be fixed if it isn't in use yet, but
>>> this is definitely not the correct approach to DT.
>
>
> The binding got merged for 3.12-rc1 and the intention of this patch was
> to correct that binding. There we some issues like mismatch between names
> of properties documented and used in the driver.
>
> After Mark's suggestion Prabhakar removed some properties and the platform
> data usage altogether. IMHO there should be only minimal changes in that
> "fixup" patch, i.e. no platform data usage should be removed. Perhaps it
> is fine since that's just code removal. I guess it is better to do this
> sort of cleanup for the next kernel release.
>
OK I will, just send out a fix up patch which fixes the mismatch between
names for the rc-cycle, and later send out a patch which removes the
platform data usage for next release with proper DT bindings.

> Also I believe the argument of backward compatibility shouldn't really be
> considered here. The $subject patch is supposed to correct the binding
> before it becomes and ABI.
>
>
>>>>> If it is, I think we should document that older versions of the binding
>>>>> didn't require those properties, so they may in fact be missing.
>>>>>
>>>>> I note that this patch doesn't actually update the driver to
>>>>> regulator_get() anything. Shouldn't it?
>>>>
>>>>
>>>> As of now the driver isn’t enabling/accepting the regulators,
>>>> so should I add those in DT properties or not ?
>>>
>>>
>>> The binding should describe the HW, not what the driver does/doesn't yet
>>> do. I wrote the above because it looked like the driver was broken, not
>>> to encourage you to remove properties from the binding.
>>
>> OK
>>
>>> How does the
>>> driver work if it doesn't enable the required regulators though, I
>>> wonder? I suppose the boards this driver has been tested on all must
>>> used fixed (non-SW-controlled) regulators.
>>>
>> on all the boards on which this decoder is connected the power to it
>> is provided by static circuit and not by regulators, So for this how would
>> you suggest to add the DT nodes for regulators ?
>
>
> I believe the regulator DT properties should be made optional. Since some
> (actually all upstream) boards don't bother with software controlled
> regulators. We might have specified them and have defined relevant fixed
> regulator(s) in DT. But I doubt it is sensible, given that it may never
> happen in practice the regulators are required to be controlled by software
> through the regulators API. Such devices can often be put in a low power
> mode by a write to one of the registers, where their supply current is at
> uA level. Looking at the datasheet ADV7343 has SLEEP_MODE in which its
> typical current consumption is 5 uA.
>
> That said the chip could be supplied from shared voltage regulators and
> the driver would then have to properly request and enable the regulators.
>
> Anyway I'm inclined to make the regulator properties optional.
>
I'm OK with making regulator properties as optional, But still it would
change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
pins are required properties (but still making them as optional) :-(

I think there might several devices where this situation may arise so
just thinking of a alternative solution.

say we have property 'software-regulator' which takes true/false(0/1)
If set to true we make the regulators as required property or else we
assume it is handled and ignore it ?


Thanks,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Sylwester Nawrocki]

Hi Prabhakar,

On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> OK I will, just send out a fix up patch which fixes the mismatch between
> names for the rc-cycle, and later send out a patch which removes the
> platform data usage for next release with proper DT bindings.

I think the binding need to be fully corrected now, I just meant to not
touch the board file, i.e. leave non-dt support unchanged.

> I'm OK with making regulator properties as optional, But still it would
> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> pins are required properties (but still making them as optional) :-(
> 
> I think there might several devices where this situation may arise so
> just thinking of a alternative solution.
> 
> say we have property 'software-regulator' which takes true/false(0/1)
> If set to true we make the regulators as required property or else we
> assume it is handled and ignore it ?

I don't think this is a good idea. You would have to add a similar platform 
data flag for non-dt, it doesn't sound right. I can see two options here: 

1. Make the regulator properties mandatory and, e.g. define a fixed 
   voltage GPIO regulator in DT with an empty 'gpio' property. Then
   pass a phandle to that regulator in the adv7343 *-supply properties.
   For non-dt similarly a fixed voltage regulator(s) and voltage 
   supplies  would need to be defined in the board files.

2. Make the properties optional and use (devm_)regulator_get_optional()
   calls in the driver (a recently added function). I must admit I don't 
   fully understand description of this function, it currently looks 
   pretty much same as (devm_)regulator_get(). Thus the driver would 
   need to be handling regulator supplies only when non ERR_PTR() is 
   returned from regulator_get_optional() and otherwise assume a non 
   critical error. There is already quite a few example occurrences of 
   regulator_get_optional() usage.

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Prabhakar Lad]

Hi Sylwester,

On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Hi Prabhakar,
>
> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>> OK I will, just send out a fix up patch which fixes the mismatch between
>> names for the rc-cycle, and later send out a patch which removes the
>> platform data usage for next release with proper DT bindings.
>
> I think the binding need to be fully corrected now, I just meant to not
> touch the board file, i.e. leave non-dt support unchanged.
>
Ok

>> I'm OK with making regulator properties as optional, But still it would
>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>> pins are required properties (but still making them as optional) :-(
>>
>> I think there might several devices where this situation may arise so
>> just thinking of a alternative solution.
>>
>> say we have property 'software-regulator' which takes true/false(0/1)
>> If set to true we make the regulators as required property or else we
>> assume it is handled and ignore it ?
>
> I don't think this is a good idea. You would have to add a similar platform
> data flag for non-dt, it doesn't sound right. I can see two options here:
>
> 1. Make the regulator properties mandatory and, e.g. define a fixed
>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>    pass a phandle to that regulator in the adv7343 *-supply properties.
>    For non-dt similarly a fixed voltage regulator(s) and voltage
>    supplies  would need to be defined in the board files.
>
> 2. Make the properties optional and use (devm_)regulator_get_optional()
>    calls in the driver (a recently added function). I must admit I don't
>    fully understand description of this function, it currently looks
>    pretty much same as (devm_)regulator_get(). Thus the driver would
>    need to be handling regulator supplies only when non ERR_PTR() is
>    returned from regulator_get_optional() and otherwise assume a non
>    critical error. There is already quite a few example occurrences of
>    regulator_get_optional() usage.
>
Thanks for pointing it I'll choose option 2 and post the patch.

Regards,
--Prabhakar Lad

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Laurent Pinchart]

On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
> > On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> >> OK I will, just send out a fix up patch which fixes the mismatch between
> >> names for the rc-cycle, and later send out a patch which removes the
> >> platform data usage for next release with proper DT bindings.
> > 
> > I think the binding need to be fully corrected now, I just meant to not
> > touch the board file, i.e. leave non-dt support unchanged.
> 
> Ok
> 
> >> I'm OK with making regulator properties as optional, But still it would
> >> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> >> pins are required properties (but still making them as optional) :-(
> >> 
> >> I think there might several devices where this situation may arise so
> >> just thinking of a alternative solution.
> >> 
> >> say we have property 'software-regulator' which takes true/false(0/1)
> >> If set to true we make the regulators as required property or else we
> >> assume it is handled and ignore it ?
> > 
> > I don't think this is a good idea. You would have to add a similar
> > platform data flag for non-dt, it doesn't sound right. I can see two
> > options here:
> > 
> > 1. Make the regulator properties mandatory and, e.g. define a fixed
> >    voltage GPIO regulator in DT with an empty 'gpio' property. Then
> >    pass a phandle to that regulator in the adv7343 *-supply properties.
> >    For non-dt similarly a fixed voltage regulator(s) and voltage
> >    supplies  would need to be defined in the board files.
> > 
> > 2. Make the properties optional and use (devm_)regulator_get_optional()
> >    calls in the driver (a recently added function). I must admit I don't
> >    fully understand description of this function, it currently looks
> >    pretty much same as (devm_)regulator_get(). Thus the driver would
> >    need to be handling regulator supplies only when non ERR_PTR() is
> >    returned from regulator_get_optional() and otherwise assume a non
> >    critical error. There is already quite a few example occurrences of
> >    regulator_get_optional() usage.
> 
> Thanks for pointing it I'll choose option 2 and post the patch.

Isn't regulator_get_optional() intended for devices that can have supplies 
unconnected in normal use ? The ADV7343 supplies are mandatory from a hardware 
point of view, so I think we should use regulator_get(). Otherwise the driver 
won't be able to tell the difference between a regulator that isn't present 
yet (for instance because the regulator device/driver hasn't been probed yet), 
which should result in deferred probing, and an always-on regulator that has 
been left out.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Stephen Warren]

On 09/23/2013 05:50 AM, Laurent Pinchart wrote:
> On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
>> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
>>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>>>> OK I will, just send out a fix up patch which fixes the mismatch between
>>>> names for the rc-cycle, and later send out a patch which removes the
>>>> platform data usage for next release with proper DT bindings.
>>>
>>> I think the binding need to be fully corrected now, I just meant to not
>>> touch the board file, i.e. leave non-dt support unchanged.
>>
>> Ok
>>
>>>> I'm OK with making regulator properties as optional, But still it would
>>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>>>> pins are required properties (but still making them as optional) :-(
>>>>
>>>> I think there might several devices where this situation may arise so
>>>> just thinking of a alternative solution.
>>>>
>>>> say we have property 'software-regulator' which takes true/false(0/1)
>>>> If set to true we make the regulators as required property or else we
>>>> assume it is handled and ignore it ?
>>>
>>> I don't think this is a good idea. You would have to add a similar
>>> platform data flag for non-dt, it doesn't sound right. I can see two
>>> options here:
>>>
>>> 1. Make the regulator properties mandatory and, e.g. define a fixed
>>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>>>    pass a phandle to that regulator in the adv7343 *-supply properties.
>>>    For non-dt similarly a fixed voltage regulator(s) and voltage
>>>    supplies  would need to be defined in the board files.
>>>
>>> 2. Make the properties optional and use (devm_)regulator_get_optional()
>>>    calls in the driver (a recently added function). I must admit I don't
>>>    fully understand description of this function, it currently looks
>>>    pretty much same as (devm_)regulator_get(). Thus the driver would
>>>    need to be handling regulator supplies only when non ERR_PTR() is
>>>    returned from regulator_get_optional() and otherwise assume a non
>>>    critical error. There is already quite a few example occurrences of
>>>    regulator_get_optional() usage.
>>
>> Thanks for pointing it I'll choose option 2 and post the patch.
> 
> Isn't regulator_get_optional() intended for devices that can have supplies 
> unconnected in normal use ?

I believe so, yes.

> The ADV7343 supplies are mandatory from a hardware 
> point of view, so I think we should use regulator_get(). Otherwise the driver 
> won't be able to tell the difference between a regulator that isn't present 
> yet (for instance because the regulator device/driver hasn't been probed yet), 
> which should result in deferred probing, and an always-on regulator that has 
> been left out.

So I think you want to make the supply properties mandatory in DT (since
some form of supply is mandatory in HW), yet make the driver support
broken DTs which don't have those properties, by error-checking the
return value from regulator_get(). You might want to put a note into DT
saying that a previous version of the binding didn't require those
supply properties, so they may be missing from older DTs.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Prabhakar Lad]

On Tue, Sep 24, 2013 at 3:03 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 09/23/2013 05:50 AM, Laurent Pinchart wrote:
>> On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
>>> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
>>>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>>>>> OK I will, just send out a fix up patch which fixes the mismatch between
>>>>> names for the rc-cycle, and later send out a patch which removes the
>>>>> platform data usage for next release with proper DT bindings.
>>>>
>>>> I think the binding need to be fully corrected now, I just meant to not
>>>> touch the board file, i.e. leave non-dt support unchanged.
>>>
>>> Ok
>>>
>>>>> I'm OK with making regulator properties as optional, But still it would
>>>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>>>>> pins are required properties (but still making them as optional) :-(
>>>>>
>>>>> I think there might several devices where this situation may arise so
>>>>> just thinking of a alternative solution.
>>>>>
>>>>> say we have property 'software-regulator' which takes true/false(0/1)
>>>>> If set to true we make the regulators as required property or else we
>>>>> assume it is handled and ignore it ?
>>>>
>>>> I don't think this is a good idea. You would have to add a similar
>>>> platform data flag for non-dt, it doesn't sound right. I can see two
>>>> options here:
>>>>
>>>> 1. Make the regulator properties mandatory and, e.g. define a fixed
>>>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>>>>    pass a phandle to that regulator in the adv7343 *-supply properties.
>>>>    For non-dt similarly a fixed voltage regulator(s) and voltage
>>>>    supplies  would need to be defined in the board files.
>>>>
>>>> 2. Make the properties optional and use (devm_)regulator_get_optional()
>>>>    calls in the driver (a recently added function). I must admit I don't
>>>>    fully understand description of this function, it currently looks
>>>>    pretty much same as (devm_)regulator_get(). Thus the driver would
>>>>    need to be handling regulator supplies only when non ERR_PTR() is
>>>>    returned from regulator_get_optional() and otherwise assume a non
>>>>    critical error. There is already quite a few example occurrences of
>>>>    regulator_get_optional() usage.
>>>
>>> Thanks for pointing it I'll choose option 2 and post the patch.
>>
>> Isn't regulator_get_optional() intended for devices that can have supplies
>> unconnected in normal use ?
>
> I believe so, yes.
>
Agreed

>> The ADV7343 supplies are mandatory from a hardware
>> point of view, so I think we should use regulator_get(). Otherwise the driver
>> won't be able to tell the difference between a regulator that isn't present
>> yet (for instance because the regulator device/driver hasn't been probed yet),
>> which should result in deferred probing, and an always-on regulator that has
>> been left out.
>
> So I think you want to make the supply properties mandatory in DT (since
> some form of supply is mandatory in HW), yet make the driver support
> broken DTs which don't have those properties, by error-checking the
> return value from regulator_get(). You might want to put a note into DT
> saying that a previous version of the binding didn't require those
> supply properties, so they may be missing from older DTs.

OK, I'll fix it and repost the patch.

Regards,
--Prabhakar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Laurent Pinchart]

On Monday 23 September 2013 15:33:10 Stephen Warren wrote:
> On 09/23/2013 05:50 AM, Laurent Pinchart wrote:
> > On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
> >> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
> >>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> >>>> OK I will, just send out a fix up patch which fixes the mismatch
> >>>> between
> >>>> names for the rc-cycle, and later send out a patch which removes the
> >>>> platform data usage for next release with proper DT bindings.
> >>> 
> >>> I think the binding need to be fully corrected now, I just meant to not
> >>> touch the board file, i.e. leave non-dt support unchanged.
> >> 
> >> Ok
> >> 
> >>>> I'm OK with making regulator properties as optional, But still it would
> >>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> >>>> pins are required properties (but still making them as optional) :-(
> >>>> 
> >>>> I think there might several devices where this situation may arise so
> >>>> just thinking of a alternative solution.
> >>>> 
> >>>> say we have property 'software-regulator' which takes true/false(0/1)
> >>>> If set to true we make the regulators as required property or else we
> >>>> assume it is handled and ignore it ?
> >>> 
> >>> I don't think this is a good idea. You would have to add a similar
> >>> platform data flag for non-dt, it doesn't sound right. I can see two
> >>> options here:
> >>> 
> >>> 1. Make the regulator properties mandatory and, e.g. define a fixed
> >>> 
> >>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
> >>>    pass a phandle to that regulator in the adv7343 *-supply properties.
> >>>    For non-dt similarly a fixed voltage regulator(s) and voltage
> >>>    supplies  would need to be defined in the board files.
> >>> 
> >>> 2. Make the properties optional and use (devm_)regulator_get_optional()
> >>> 
> >>>    calls in the driver (a recently added function). I must admit I don't
> >>>    fully understand description of this function, it currently looks
> >>>    pretty much same as (devm_)regulator_get(). Thus the driver would
> >>>    need to be handling regulator supplies only when non ERR_PTR() is
> >>>    returned from regulator_get_optional() and otherwise assume a non
> >>>    critical error. There is already quite a few example occurrences of
> >>>    regulator_get_optional() usage.
> >> 
> >> Thanks for pointing it I'll choose option 2 and post the patch.
> > 
> > Isn't regulator_get_optional() intended for devices that can have supplies
> > unconnected in normal use ?
> 
> I believe so, yes.
> 
> > The ADV7343 supplies are mandatory from a hardware
> > point of view, so I think we should use regulator_get(). Otherwise the
> > driver won't be able to tell the difference between a regulator that
> > isn't present yet (for instance because the regulator device/driver
> > hasn't been probed yet), which should result in deferred probing, and an
> > always-on regulator that has been left out.
> 
> So I think you want to make the supply properties mandatory in DT (since
> some form of supply is mandatory in HW), yet make the driver support
> broken DTs which don't have those properties, by error-checking the
> return value from regulator_get(). You might want to put a note into DT
> saying that a previous version of the binding didn't require those
> supply properties, so they may be missing from older DTs.

Are there such devices in the wild ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Mark Brown]

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

On Tue, Sep 24, 2013 at 11:44:57AM +0200, Laurent Pinchart wrote:
> On Monday 23 September 2013 15:33:10 Stephen Warren wrote:

> > So I think you want to make the supply properties mandatory in DT (since
> > some form of supply is mandatory in HW), yet make the driver support
> > broken DTs which don't have those properties, by error-checking the
> > return value from regulator_get(). You might want to put a note into DT
> > saying that a previous version of the binding didn't require those
> > supply properties, so they may be missing from older DTs.

> Are there such devices in the wild ?

From v3.12 on the kernel will stub in a dummy supply if one isn't found
when using DT but I wouldn't mention that in the bindings since it's
very much fixing up broken DTs rather than a good idea.

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

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 619 bytes --]

On Mon, Sep 23, 2013 at 01:50:51PM +0200, Laurent Pinchart wrote:

> Isn't regulator_get_optional() intended for devices that can have supplies 
> unconnected in normal use ? The ADV7343 supplies are mandatory from a hardware 
> point of view, so I think we should use regulator_get(). Otherwise the driver 
> won't be able to tell the difference between a regulator that isn't present 
> yet (for instance because the regulator device/driver hasn't been probed yet), 
> which should result in deferred probing, and an always-on regulator that has 
> been left out.

Yes, everything you say here is correct.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Prabhakar Lad]

Hi All,

On Mon, Sep 23, 2013 at 8:18 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> Hi Sylwester,
>
> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> Hi Prabhakar,
>>
>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>>> OK I will, just send out a fix up patch which fixes the mismatch between
>>> names for the rc-cycle, and later send out a patch which removes the
>>> platform data usage for next release with proper DT bindings.
>>
>> I think the binding need to be fully corrected now, I just meant to not
>> touch the board file, i.e. leave non-dt support unchanged.
>>
> Ok
>
>>> I'm OK with making regulator properties as optional, But still it would
>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>>> pins are required properties (but still making them as optional) :-(
>>>
>>> I think there might several devices where this situation may arise so
>>> just thinking of a alternative solution.
>>>
>>> say we have property 'software-regulator' which takes true/false(0/1)
>>> If set to true we make the regulators as required property or else we
>>> assume it is handled and ignore it ?
>>
>> I don't think this is a good idea. You would have to add a similar platform
>> data flag for non-dt, it doesn't sound right. I can see two options here:
>>
>> 1. Make the regulator properties mandatory and, e.g. define a fixed
>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>>    pass a phandle to that regulator in the adv7343 *-supply properties.
>>    For non-dt similarly a fixed voltage regulator(s) and voltage
>>    supplies  would need to be defined in the board files.
>>
>> 2. Make the properties optional and use (devm_)regulator_get_optional()
>>    calls in the driver (a recently added function). I must admit I don't
>>    fully understand description of this function, it currently looks
>>    pretty much same as (devm_)regulator_get(). Thus the driver would
>>    need to be handling regulator supplies only when non ERR_PTR() is
>>    returned from regulator_get_optional() and otherwise assume a non
>>    critical error. There is already quite a few example occurrences of
>>    regulator_get_optional() usage.
>>
The same question arises in case of the clock, The adv7343 encoder has
two input clocks CLKIN_A and CLKIN_B. I case of da850 EVM the
clock source to adv7343 encoder is fixed source which is enabled
by default so none of the bridge nor the adv7343 driver cares of the
clock to enable/disable.
So in this case should I be registering  (v4l2_clk_register() /
v4l2_clk_unregister())
a dummy clock in the bridge driver or in the board file ?

Regards,
--Prabhakar Lad

Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

[Laurent Pinchart]

Hi Prabhakar,

On Monday 30 September 2013 18:57:01 Prabhakar Lad wrote:
> On Mon, Sep 23, 2013 at 8:18 AM, Prabhakar Lad wrote:
> > On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
> >> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> >>> OK I will, just send out a fix up patch which fixes the mismatch between
> >>> names for the rc-cycle, and later send out a patch which removes the
> >>> platform data usage for next release with proper DT bindings.
> >> 
> >> I think the binding need to be fully corrected now, I just meant to not
> >> touch the board file, i.e. leave non-dt support unchanged.
> > 
> > Ok
> > 
> >>> I'm OK with making regulator properties as optional, But still it would
> >>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> >>> pins are required properties (but still making them as optional) :-(
> >>> 
> >>> I think there might several devices where this situation may arise so
> >>> just thinking of a alternative solution.
> >>> 
> >>> say we have property 'software-regulator' which takes true/false(0/1)
> >>> If set to true we make the regulators as required property or else we
> >>> assume it is handled and ignore it ?
> >> 
> >> I don't think this is a good idea. You would have to add a similar
> >> platform data flag for non-dt, it doesn't sound right. I can see two
> >> options here:
> >> 
> >> 1. Make the regulator properties mandatory and, e.g. define a fixed
> >>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
> >>    pass a phandle to that regulator in the adv7343 *-supply properties.
> >>    For non-dt similarly a fixed voltage regulator(s) and voltage
> >>    supplies  would need to be defined in the board files.
> >> 
> >> 2. Make the properties optional and use (devm_)regulator_get_optional()
> >>    calls in the driver (a recently added function). I must admit I don't
> >>    fully understand description of this function, it currently looks
> >>    pretty much same as (devm_)regulator_get(). Thus the driver would
> >>    need to be handling regulator supplies only when non ERR_PTR() is
> >>    returned from regulator_get_optional() and otherwise assume a non
> >>    critical error. There is already quite a few example occurrences of
> >>    regulator_get_optional() usage.
> 
> The same question arises in case of the clock, The adv7343 encoder has two
> input clocks CLKIN_A and CLKIN_B. I case of da850 EVM the clock source to
> adv7343 encoder is fixed source which is enabled by default so none of the
> bridge nor the adv7343 driver cares of the clock to enable/disable. So in
> this case should I be registering  (v4l2_clk_register() /
> v4l2_clk_unregister()) a dummy clock in the bridge driver or in the board
> file ?

The fixed clock (which is a real clock, not a dummy clock) should be 
registered in board file, preferably using the common clock framework if 
that's available on your platform.

-- 
Regards,

Laurent Pinchart