Re: [PATCHv2] media: i2c/adp1653: devicetree support for adp1653
Hi! > Thanks for the patch! A few comments below. > > On Wed, Dec 24, 2014 at 11:34:34PM +0100, Pavel Machek wrote: > > > > We are moving to device tree support on OMAP3, but that currently > > breaks ADP1653 driver. This adds device tree support, plus required > > documentation. > > > > Signed-off-by: Pavel Machek > > > > --- > > > > Changed -microsec to -us, as requested by devicetree people. > > > > Fixed checkpatch issues. > > > > diff --git a/Documentation/devicetree/bindings/leds/common.txt > > b/Documentation/devicetree/bindings/leds/common.txt > > index 2d88816..2c6c7c5 100644 > > --- a/Documentation/devicetree/bindings/leds/common.txt > > +++ b/Documentation/devicetree/bindings/leds/common.txt > > @@ -14,6 +14,15 @@ Optional properties for child nodes: > > "ide-disk" - LED indicates disk activity > > "timer" - LED flashes at a fixed, configurable rate > > > > +- max-microamp : maximum intensity in microamperes of the LED > > +(torch LED for flash devices) > > s/torch LED/torch mode/ > > > +- flash-max-microamp : maximum intensity in microamperes of the > > + flash LED; it is mandatory if the LED should > > + support the flash mode > > +- flash-timeout-microsec : timeout in microseconds after which the flash > > + LED is turned off > > These should go to a different patch. Actually these both should not be in this patch in the first place. > > + - reg: I2C slave address > > + > > + - gpios: References to the GPIO that controls the power for the chip. > > + > > +There are two led outputs available - flash and indicator. One led is > > +represented by one child node, nodes need to be named "flash" and > > "indicator". > > 80 characters per line. Count them. It is. > > + > > +Required properties of the LED child node: > > +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt > > + > > +Required properties of the flash LED child node: > > + > > +- flash-max-microamp : see > > Documentation/devicetree/bindings/leds/common.txt > > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt > > + > > +Example: > > + > > +adp1653: led-controller@30 { > > +compatible = "adi,adp1653"; > > + reg = <0x30>; > > +gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ > > Please use tabs for indentation above (and below). Ok. > > --- a/arch/arm/boot/dts/omap3-n900.dts > > +++ b/arch/arm/boot/dts/omap3-n900.dts > > @@ -553,6 +558,22 @@ > > > > ti,usb-charger-detection = <&isp1704>; > > }; > > + > > + adp1653: led-controller@30 { > > + compatible = "adi,adp1653"; > > + reg = <0x30>; > > + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ > > + > > + flash { > > + flash-timeout-us = <50>; > > + flash-max-microamp = <32>; > > + max-microamp = <5>; > > + }; > > + > > + indicator { > > + max-microamp = <17500>; > > + }; > > + }; > > This should go to a separate patch as well. How many patches do I need to do for one trivial change? :-(. > > + if (flash->platform_data->power) > > + ret = flash->platform_data->power(&flash->subdev, on); > > if () { > } else { > } Ok. > > + else { > > + gpio_set_value(flash->platform_data->power_gpio, on); > > Shouldn't you add this to the platform data struct? I don't see what you mean. > power_gpio is actually a poor name for this, as is the "power" callback. > This is really "EN" gpio in the spec, I'd call it perhaps just "gpio", or > "enable_gpio". Feel free to clean that that up in followup patch. > > + if (on) { > > + /* Some delay is apparently required. */ > > + udelay(20); > > The driver should always handle the delay, platform data or not. This > reminds me --- is there a need to retain the support for platform data? I > don't think it's being used anywhere. I'm fine with both keeping and > removing it. Lets do that in followup patch, if needed. > > + child = of_get_child_by_name(node, "indicator"); > > + if (!child) > > + return -EINVAL; > > Do you require an indicator to be connected? I think it shouldn't be > mandatory, at least the driver should work without it, even if it > exposes > the control (making that conditional would be a subject for another patch, > but that doesn't need to be done now). Another patch, if someone needs it, yes. > > + if (of_property_read_u32(child, "max-microamp", &val)) > > + return -EINVAL; > > + pd->max_indicator_intensity = val; > > + > > + if (!of_find_property(node, "gpios", NULL)) { > > + dev_err(&client->dev, "No gpio node\n"); > > + return -EINVAL; > > + } > > + > > + gpio = of_get_gpio_flags(node, 0, &flags); > > You could assign t
Re: [PATCHv2] media: i2c/adp1653: devicetree support for adp1653
Hi Pavel, Thanks for the patch! A few comments below. On Wed, Dec 24, 2014 at 11:34:34PM +0100, Pavel Machek wrote: > > We are moving to device tree support on OMAP3, but that currently > breaks ADP1653 driver. This adds device tree support, plus required > documentation. > > Signed-off-by: Pavel Machek > > --- > > Changed -microsec to -us, as requested by devicetree people. > > Fixed checkpatch issues. > > diff --git a/Documentation/devicetree/bindings/leds/common.txt > b/Documentation/devicetree/bindings/leds/common.txt > index 2d88816..2c6c7c5 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -14,6 +14,15 @@ Optional properties for child nodes: > "ide-disk" - LED indicates disk activity > "timer" - LED flashes at a fixed, configurable rate > > +- max-microamp : maximum intensity in microamperes of the LED > + (torch LED for flash devices) s/torch LED/torch mode/ > +- flash-max-microamp : maximum intensity in microamperes of the > + flash LED; it is mandatory if the LED should > +support the flash mode > +- flash-timeout-microsec : timeout in microseconds after which the flash > + LED is turned off These should go to a different patch. > + > + > Examples: > > system-status { > @@ -21,3 +30,10 @@ system-status { > linux,default-trigger = "heartbeat"; > ... > }; > + > +camera-flash { > + label = "Flash"; > + max-microamp = <5>; > + flash-max-microamp = <32>; > + flash-timeout-microsec = <50>; > +} > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt > b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > new file mode 100644 > index 000..3c7065f > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > @@ -0,0 +1,38 @@ > +* Analog Devices ADP1653 flash LED driver > + > +Required Properties: > + > + - compatible: Must contain one of the following > +- "adi,adp1653" I doubt whether there are going to be more chips supported with the driver. There hasn't been since the driver was written not I'm aware of one now. > + - reg: I2C slave address > + > + - gpios: References to the GPIO that controls the power for the chip. > + > +There are two led outputs available - flash and indicator. One led is > +represented by one child node, nodes need to be named "flash" and > "indicator". 80 characters per line. > + > +Required properties of the LED child node: > +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt > + > +Required properties of the flash LED child node: > + > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt > + > +Example: > + > +adp1653: led-controller@30 { > +compatible = "adi,adp1653"; > + reg = <0x30>; > +gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ Please use tabs for indentation above (and below). > + > + flash { > +flash-timeout-us = <50>; > +flash-max-microamp = <32>; > +max-microamp = <5>; > + }; > +indicator { > +max-microamp = <17500>; > + }; > +}; > diff --git a/arch/arm/boot/dts/omap3-n900.dts > b/arch/arm/boot/dts/omap3-n900.dts > index bc82a12..d04e7cc 100644 > --- a/arch/arm/boot/dts/omap3-n900.dts > +++ b/arch/arm/boot/dts/omap3-n900.dts > @@ -553,6 +558,22 @@ > > ti,usb-charger-detection = <&isp1704>; > }; > + > + adp1653: led-controller@30 { > + compatible = "adi,adp1653"; > + reg = <0x30>; > + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ > + > + flash { > + flash-timeout-us = <50>; > + flash-max-microamp = <32>; > + max-microamp = <5>; > + }; > + > + indicator { > + max-microamp = <17500>; > + }; > + }; This should go to a separate patch as well. > }; > > &i2c3 { > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > index 873fe19..78341d0 100644 > --- a/drivers/media/i2c/adp1653.c > +++ b/drivers/media/i2c/adp1653.c > @@ -8,6 +8,7 @@ > * Contributors: > * Sakari Ailus > * Tuukka Toivonen > + * Pavel Machek > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -34,9 +35,12 @@ > #include > #include > #include > +#include > #include > #include > > +#include Please arrange along with the rest of headers. > + > #define TIMEOUT_MAX 82 > #define TIMEOUT_STEP 54600 > #define TIMEOU
Re: [PATCHv2] media: i2c/adp1653: devicetree support for adp1653
On Fri, Dec 26, 2014 at 2:33 PM, Pavel Machek wrote: > Hi! > >> > We are moving to device tree support on OMAP3, but that currently >> > breaks ADP1653 driver. This adds device tree support, plus required >> > documentation. >> > >> > Signed-off-by: Pavel Machek >> > >> > --- >> > >> > Changed -microsec to -us, as requested by devicetree people. >> > >> > Fixed checkpatch issues. >> > >> > diff --git a/Documentation/devicetree/bindings/leds/common.txt >> > b/Documentation/devicetree/bindings/leds/common.txt >> > index 2d88816..2c6c7c5 100644 >> > --- a/Documentation/devicetree/bindings/leds/common.txt >> > +++ b/Documentation/devicetree/bindings/leds/common.txt >> > @@ -14,6 +14,15 @@ Optional properties for child nodes: >> > "ide-disk" - LED indicates disk activity >> > "timer" - LED flashes at a fixed, configurable rate >> > >> > +- max-microamp : maximum intensity in microamperes of the LED >> > +(torch LED for flash devices) >> > +- flash-max-microamp : maximum intensity in microamperes of the >> > + flash LED; it is mandatory if the LED should >> > + support the flash mode >> > +- flash-timeout-microsec : timeout in microseconds after which the flash >> > + LED is turned off >> >> Doesn't all this go in your flash led binding patch? > > No, I should not have included this part. > >> > +Example: >> > + >> > +adp1653: led-controller@30 { >> > +compatible = "adi,adp1653"; >> > + reg = <0x30>; >> > +gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ >> > + >> > + flash { >> > +flash-timeout-us = <50>; >> > +flash-max-microamp = <32>; >> > +max-microamp = <5>; >> > + }; >> > +indicator { >> >> These are different LEDs or different modes? > > flash & indicator are different LEDs. One is white, one is red. Flash > can be used as a flash and as a torch. > >> > +max-microamp = <17500>; >> >> This is a bit inconsistent. The binding says this is for flash LEDs >> torch mode, but I see no reason why it can't be common. Can you update >> the binding doc to be clear here. > > By inconsisnent, you mean you want patch below? Yes. >> Also, aren't you missing label properties? > > label is optional, and as my driver does not yet use it, I forgot > about it. Based on your node names, there are obviously user defined functions for them already. So they should have labels whether or not the driver uses them. Rob > Signed-off-by: Pavel Machek > > index 2c6c7c5..92d4dac 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -15,7 +15,6 @@ Optional properties for child nodes: > "timer" - LED flashes at a fixed, configurable rate > > - max-microamp : maximum intensity in microamperes of the LED > -(torch LED for flash devices) > - flash-max-microamp : maximum intensity in microamperes of the > flash LED; it is mandatory if the LED should >support the flash mode > > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] media: i2c/adp1653: devicetree support for adp1653
Hi! > > We are moving to device tree support on OMAP3, but that currently > > breaks ADP1653 driver. This adds device tree support, plus required > > documentation. > > > > Signed-off-by: Pavel Machek > > > > --- > > > > Changed -microsec to -us, as requested by devicetree people. > > > > Fixed checkpatch issues. > > > > diff --git a/Documentation/devicetree/bindings/leds/common.txt > > b/Documentation/devicetree/bindings/leds/common.txt > > index 2d88816..2c6c7c5 100644 > > --- a/Documentation/devicetree/bindings/leds/common.txt > > +++ b/Documentation/devicetree/bindings/leds/common.txt > > @@ -14,6 +14,15 @@ Optional properties for child nodes: > > "ide-disk" - LED indicates disk activity > > "timer" - LED flashes at a fixed, configurable rate > > > > +- max-microamp : maximum intensity in microamperes of the LED > > +(torch LED for flash devices) > > +- flash-max-microamp : maximum intensity in microamperes of the > > + flash LED; it is mandatory if the LED should > > + support the flash mode > > +- flash-timeout-microsec : timeout in microseconds after which the flash > > + LED is turned off > > Doesn't all this go in your flash led binding patch? No, I should not have included this part. > > +Example: > > + > > +adp1653: led-controller@30 { > > +compatible = "adi,adp1653"; > > + reg = <0x30>; > > +gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ > > + > > + flash { > > +flash-timeout-us = <50>; > > +flash-max-microamp = <32>; > > +max-microamp = <5>; > > + }; > > +indicator { > > These are different LEDs or different modes? flash & indicator are different LEDs. One is white, one is red. Flash can be used as a flash and as a torch. > > +max-microamp = <17500>; > > This is a bit inconsistent. The binding says this is for flash LEDs > torch mode, but I see no reason why it can't be common. Can you update > the binding doc to be clear here. By inconsisnent, you mean you want patch below? > Also, aren't you missing label properties? label is optional, and as my driver does not yet use it, I forgot about it. Signed-off-by: Pavel Machek index 2c6c7c5..92d4dac 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -15,7 +15,6 @@ Optional properties for child nodes: "timer" - LED flashes at a fixed, configurable rate - max-microamp : maximum intensity in microamperes of the LED -(torch LED for flash devices) - flash-max-microamp : maximum intensity in microamperes of the flash LED; it is mandatory if the LED should support the flash mode -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] media: i2c/adp1653: devicetree support for adp1653
On Wed, Dec 24, 2014 at 4:34 PM, Pavel Machek wrote: > > We are moving to device tree support on OMAP3, but that currently > breaks ADP1653 driver. This adds device tree support, plus required > documentation. > > Signed-off-by: Pavel Machek > > --- > > Changed -microsec to -us, as requested by devicetree people. > > Fixed checkpatch issues. > > diff --git a/Documentation/devicetree/bindings/leds/common.txt > b/Documentation/devicetree/bindings/leds/common.txt > index 2d88816..2c6c7c5 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -14,6 +14,15 @@ Optional properties for child nodes: > "ide-disk" - LED indicates disk activity > "timer" - LED flashes at a fixed, configurable rate > > +- max-microamp : maximum intensity in microamperes of the LED > +(torch LED for flash devices) > +- flash-max-microamp : maximum intensity in microamperes of the > + flash LED; it is mandatory if the LED should > + support the flash mode > +- flash-timeout-microsec : timeout in microseconds after which the flash > + LED is turned off Doesn't all this go in your flash led binding patch? > + > + > Examples: > > system-status { > @@ -21,3 +30,10 @@ system-status { > linux,default-trigger = "heartbeat"; > ... > }; > + > +camera-flash { > + label = "Flash"; > + max-microamp = <5>; > + flash-max-microamp = <32>; > + flash-timeout-microsec = <50>; > +} > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt > b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > new file mode 100644 > index 000..3c7065f > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > @@ -0,0 +1,38 @@ > +* Analog Devices ADP1653 flash LED driver > + > +Required Properties: > + > + - compatible: Must contain one of the following > +- "adi,adp1653" > + > + - reg: I2C slave address > + > + - gpios: References to the GPIO that controls the power for the chip. > + > +There are two led outputs available - flash and indicator. One led is > +represented by one child node, nodes need to be named "flash" and > "indicator". > + > +Required properties of the LED child node: > +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt > + > +Required properties of the flash LED child node: > + > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt > + > +Example: > + > +adp1653: led-controller@30 { > +compatible = "adi,adp1653"; > + reg = <0x30>; > +gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ > + > + flash { > +flash-timeout-us = <50>; > +flash-max-microamp = <32>; > +max-microamp = <5>; > + }; > +indicator { These are different LEDs or different modes? > +max-microamp = <17500>; This is a bit inconsistent. The binding says this is for flash LEDs torch mode, but I see no reason why it can't be common. Can you update the binding doc to be clear here. Also, aren't you missing label properties? Rob -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] media: i2c/adp1653: devicetree support for adp1653
We are moving to device tree support on OMAP3, but that currently breaks ADP1653 driver. This adds device tree support, plus required documentation. Signed-off-by: Pavel Machek --- Changed -microsec to -us, as requested by devicetree people. Fixed checkpatch issues. diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 2d88816..2c6c7c5 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -14,6 +14,15 @@ Optional properties for child nodes: "ide-disk" - LED indicates disk activity "timer" - LED flashes at a fixed, configurable rate +- max-microamp : maximum intensity in microamperes of the LED +(torch LED for flash devices) +- flash-max-microamp : maximum intensity in microamperes of the + flash LED; it is mandatory if the LED should + support the flash mode +- flash-timeout-microsec : timeout in microseconds after which the flash + LED is turned off + + Examples: system-status { @@ -21,3 +30,10 @@ system-status { linux,default-trigger = "heartbeat"; ... }; + +camera-flash { + label = "Flash"; + max-microamp = <5>; + flash-max-microamp = <32>; + flash-timeout-microsec = <50>; +} diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt new file mode 100644 index 000..3c7065f --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt @@ -0,0 +1,38 @@ +* Analog Devices ADP1653 flash LED driver + +Required Properties: + + - compatible: Must contain one of the following +- "adi,adp1653" + + - reg: I2C slave address + + - gpios: References to the GPIO that controls the power for the chip. + +There are two led outputs available - flash and indicator. One led is +represented by one child node, nodes need to be named "flash" and "indicator". + +Required properties of the LED child node: +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt + +Required properties of the flash LED child node: + +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt + +Example: + +adp1653: led-controller@30 { +compatible = "adi,adp1653"; + reg = <0x30>; +gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ + + flash { +flash-timeout-us = <50>; +flash-max-microamp = <32>; +max-microamp = <5>; + }; +indicator { +max-microamp = <17500>; + }; +}; diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index bc82a12..d04e7cc 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -553,6 +558,22 @@ ti,usb-charger-detection = <&isp1704>; }; + + adp1653: led-controller@30 { + compatible = "adi,adp1653"; + reg = <0x30>; + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ + + flash { + flash-timeout-us = <50>; + flash-max-microamp = <32>; + max-microamp = <5>; + }; + + indicator { + max-microamp = <17500>; + }; + }; }; &i2c3 { diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c index 873fe19..78341d0 100644 --- a/drivers/media/i2c/adp1653.c +++ b/drivers/media/i2c/adp1653.c @@ -8,6 +8,7 @@ * Contributors: * Sakari Ailus * Tuukka Toivonen + * Pavel Machek * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -34,9 +35,12 @@ #include #include #include +#include #include #include +#include + #define TIMEOUT_MAX82 #define TIMEOUT_STEP 54600 #define TIMEOUT_MIN(TIMEOUT_MAX - ADP1653_REG_CONFIG_TMR_SET_MAX \ @@ -306,9 +318,18 @@ adp1653_init_device(struct adp1653_flash *flash) static int __adp1653_set_power(struct adp1653_flash *flash, int on) { - int ret; + int ret = 0; + + if (flash->platform_data->power) + ret = flash->platform_data->power(&flash->subdev, on); + else { + gpio_set_value(flash->platform_data->power_gpio, on); + if (on) { + /* Some delay is apparently required. */ + udelay(20); + } + } - ret = flash->platform_data->power(&flash->subdev, on); if (ret < 0) return ret; @@ -316,8 +337,13