Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On 08/20/2013 12:57 AM, Stephen Warren wrote: > On 08/19/2013 04:53 PM, Tomasz Figa wrote: >> On Monday 19 of August 2013 16:30:45 Stephen Warren wrote: >>> On 08/19/2013 11:25 AM, Sylwester Nawrocki wrote: On 08/19/2013 03:25 PM, Pawel Moll wrote: > On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: >> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt >> @@ -0,0 +1,51 @@ >> +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC >> ISP >> +- >> + >> +Required properties: >> + >> +- compatible : "samsung,s5k5baf"; >> +- reg: I2C slave address of the sensor; >> +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); >> +- vddreg-supply : regulator input power supply 1.8V (1.7V >> to 1.9V) +or 2.8V (2.6V to 3.0); >> +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) >> +or 2.8V (2.5V to 3.1V); >> +- gpios : GPIOs connected to STDBYN and RSTN pins, >> +in order: STBYN, RSTN; > > You probably want to use the "[-]gpios" convention here (see > Documentation/devicetree/bindings/gpio/gpio.txt), so something like > stbyn-gpios and rstn-gpios. Unless using multiple named properties is really preferred over a single "gpios" property I would like to keep the single property containing a list of GPIOs. ... >>> >>> Yes, a separate property for each type of GPIO is typical. Multiple >>> entries in the same property are allowed if they're used for the same >>> purpose/type, whereas here they're clearly different things. Yes, that's a good argument. Those GPIOs are pretty unrelated. >>> Inconsistent with (some) other properties, admittedly... It might depend on which properties we consider together. >> I'm not really convinced about the superiority of named gpio properties >> over a single gpios property with multiple entries in this case. I'd say >> it's more just a matter of preference. >> >> See the clock or interrupt bindings. They all specify all the clocks and >> interrupts in single property, without any differentiation based on their >> purposes. Also keep in mind that original GPIO bindings used only a single >> "gpios" property and was only extended to allow named ones. > > Well, it's not so much about what's best, but just being consistent with > what's already there. OK, thanks a lot for clarification. We'll rework this to use separate named properties. -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On 08/19/2013 04:53 PM, Tomasz Figa wrote: > On Monday 19 of August 2013 16:30:45 Stephen Warren wrote: >> On 08/19/2013 11:25 AM, Sylwester Nawrocki wrote: >>> On 08/19/2013 03:25 PM, Pawel Moll wrote: On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: > +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt > @@ -0,0 +1,51 @@ > +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC > ISP > +- > + > +Required properties: > + > +- compatible : "samsung,s5k5baf"; > +- reg: I2C slave address of the sensor; > +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); > +- vddreg-supply : regulator input power supply 1.8V (1.7V > to 1.9V) +or 2.8V (2.6V to 3.0); > +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) > +or 2.8V (2.5V to 3.1V); > +- gpios : GPIOs connected to STDBYN and RSTN pins, > +in order: STBYN, RSTN; You probably want to use the "[-]gpios" convention here (see Documentation/devicetree/bindings/gpio/gpio.txt), so something like stbyn-gpios and rstn-gpios. >>> >>> Unless using multiple named properties is really preferred over a >>> single "gpios" property I would like to keep the single property >>> containing a list of GPIOs. ... >> >> Yes, a separate property for each type of GPIO is typical. Multiple >> entries in the same property are allowed if they're used for the same >> purpose/type, whereas here they're clearly different things. >> Inconsistent with (some) other properties, admittedly... > > I'm not really convinced about the superiority of named gpio properties > over a single gpios property with multiple entries in this case. I'd say > it's more just a matter of preference. > > See the clock or interrupt bindings. They all specify all the clocks and > interrupts in single property, without any differentiation based on their > purposes. Also keep in mind that original GPIO bindings used only a single > "gpios" property and was only extended to allow named ones. Well, it's not so much about what's best, but just being consistent with what's already there. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On Monday 19 of August 2013 16:30:45 Stephen Warren wrote: > On 08/19/2013 11:25 AM, Sylwester Nawrocki wrote: > > On 08/19/2013 03:25 PM, Pawel Moll wrote: > >> On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: > >>> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt > >>> @@ -0,0 +1,51 @@ > >>> +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC > >>> ISP > >>> +- > >>> + > >>> +Required properties: > >>> + > >>> +- compatible : "samsung,s5k5baf"; > >>> +- reg: I2C slave address of the sensor; > >>> +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); > >>> +- vddreg-supply : regulator input power supply 1.8V (1.7V > >>> to 1.9V) +or 2.8V (2.6V to 3.0); > >>> +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) > >>> +or 2.8V (2.5V to 3.1V); > >>> +- gpios : GPIOs connected to STDBYN and RSTN pins, > >>> +in order: STBYN, RSTN; > >> > >> You probably want to use the "[-]gpios" convention here (see > >> Documentation/devicetree/bindings/gpio/gpio.txt), so something like > >> stbyn-gpios and rstn-gpios. > > > > Unless using multiple named properties is really preferred over a > > single "gpios" property I would like to keep the single property > > containing a list of GPIOs. ... > > Yes, a separate property for each type of GPIO is typical. Multiple > entries in the same property are allowed if they're used for the same > purpose/type, whereas here they're clearly different things. > Inconsistent with (some) other properties, admittedly... I'm not really convinced about the superiority of named gpio properties over a single gpios property with multiple entries in this case. I'd say it's more just a matter of preference. See the clock or interrupt bindings. They all specify all the clocks and interrupts in single property, without any differentiation based on their purposes. Also keep in mind that original GPIO bindings used only a single "gpios" property and was only extended to allow named ones. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On 08/19/2013 11:25 AM, Sylwester Nawrocki wrote: > On 08/19/2013 03:25 PM, Pawel Moll wrote: >> On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: >>> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt >>> @@ -0,0 +1,51 @@ >>> +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP >>> +- >>> + >>> +Required properties: >>> + >>> +- compatible : "samsung,s5k5baf"; >>> +- reg: I2C slave address of the sensor; >>> +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); >>> +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V) >>> +or 2.8V (2.6V to 3.0); >>> +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) >>> +or 2.8V (2.5V to 3.1V); >>> +- gpios : GPIOs connected to STDBYN and RSTN pins, >>> +in order: STBYN, RSTN; >> >> You probably want to use the "[-]gpios" convention here (see >> Documentation/devicetree/bindings/gpio/gpio.txt), so something like >> stbyn-gpios and rstn-gpios. > > Unless using multiple named properties is really preferred over a single > "gpios" property I would like to keep the single property containing > a list of GPIOs. ... Yes, a separate property for each type of GPIO is typical. Multiple entries in the same property are allowed if they're used for the same purpose/type, whereas here they're clearly different things. Inconsistent with (some) other properties, admittedly... -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On 08/19/2013 03:39 PM, Mark Rutland wrote: On Mon, Aug 19, 2013 at 02:18:27PM +0100, Andrzej Hajda wrote: Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP. The driver exposes the sensor as two V4L2 subdevices: - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format, no controls. - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200, pre/post ISP cropping, downscaling via selection API, controls. Signed-off-by: Sylwester Nawrocki Signed-off-by: Andrzej Hajda Signed-off-by: Kyungmin Park --- v5 - removed non-standard samsung hflip/vflip device tree bindings v4 - GPL changed to GPLv2, - bitfields replaced by u8, - cosmetic changes, - corrected s_stream flow, - gpio pins are no longer exported, - added I2C addresses to subdev names, - CIS subdev registration postponed after succesfull HW initialization, - added enums for pads, - selections are initialized only during probe, - default resolution changed to 1600x1200, - state->error pattern removed from few other functions, - entity link creation moved to registered callback. [...] --- .../devicetree/bindings/media/samsung-s5k5baf.txt | 51 + MAINTAINERS|7 + drivers/media/i2c/Kconfig |7 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/s5k5baf.c| 1980 5 files changed, 2046 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode 100644 drivers/media/i2c/s5k5baf.c diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt new file mode 100644 index 000..b1f2fde --- /dev/null +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt @@ -0,0 +1,51 @@ +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP +- + +Required properties: + +- compatible : "samsung,s5k5baf"; +- reg: I2C slave address of the sensor; +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V) +or 2.8V (2.6V to 3.0); +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) +or 2.8V (2.5V to 3.1V); +- gpios : GPIOs connected to STDBYN and RSTN pins, +in order: STBYN, RSTN; +- clock-frequency : master clock frequency in Hz; Why is this necessary? Could you not just require having a clocks property? You could then get equivalent functionality to the clock-frequency property by having a fixed-clock node if you don't ahve a real clock specifier to wire up. Oops, looks like we didn't consolidate all the changes that were present in v4 [1]. clock, clock-names should be required properties and clock-frequency should be optional. Yes, fixed clock could be used when, e.g. the sensor feeds its master clock from a separate external oscillator. The clock-frequency property is there to _set_ a board specific master clock frequency of the sensor at the driver. I hope it doesn't fall into category "doesn't describe hardware", because the optimal frequency often needs to be specified per board and some common denominator value or range might not work well, leading to video signal distortions, etc. + +Optional properties: + +- clocks : contains the sensor's master clock specifier; +- clock-names: contains "mclk" entry; + +The device node should contain one 'port' child node with one child 'endpoint' +node, according to the bindings defined in Documentation/devicetree/bindings/ +media/video-interfaces.txt. The following are properties specific to those nodes. + +endpoint node +- + +- data-lanes : (optional) an array specifying active physical MIPI-CSI2 + data output lanes and their mapping to logical lanes; the + array's content is unused, only its length is meaningful; Is that a property of the driver, or does the design of the hardware mean that this can never encode useful information? This sensor doesn't support the data lanes re-routing at the MIPI CSI-2 transmitter [2]. The data/clock lanes just appear on fixed physical pins, thus there is nothing that could be done with data in the array. The number of entries determines how many lanes are wired between the transmitter and the receiver and this is configurable for that particular device in range <1, 2> - it can transmit data on either 1 or 2 lanes. Presumably an important detail missing here is that this is the common property from video-interfaces.txt and what we mention here is only some device-specific constraints. What does the length of the property imply? The description of this property should really be as in v4 of the patch: "- data-lanes : (optional) spec
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On 08/19/2013 03:25 PM, Pawel Moll wrote: > On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: >> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt >> @@ -0,0 +1,51 @@ >> +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP >> +- >> + >> +Required properties: >> + >> +- compatible : "samsung,s5k5baf"; >> +- reg: I2C slave address of the sensor; >> +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); >> +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V) >> +or 2.8V (2.6V to 3.0); >> +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) >> +or 2.8V (2.5V to 3.1V); >> +- gpios : GPIOs connected to STDBYN and RSTN pins, >> +in order: STBYN, RSTN; > > You probably want to use the "[-]gpios" convention here (see > Documentation/devicetree/bindings/gpio/gpio.txt), so something like > stbyn-gpios and rstn-gpios. Unless using multiple named properties is really preferred over a single "gpios" property I would like to keep the single property containing a list of GPIOs. Each list entry has clearly defined meaning and it seems simpler to me to reference the GPIOs by index, rather than by name. This also saves a few bytes in dtb and there is no need to store the list of names in the driver. Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On Mon, Aug 19, 2013 at 02:18:27PM +0100, Andrzej Hajda wrote:
> Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor
> with embedded SoC ISP.
> The driver exposes the sensor as two V4L2 subdevices:
> - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
> no controls.
> - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
> pre/post ISP cropping, downscaling via selection API, controls.
>
> Signed-off-by: Sylwester Nawrocki
> Signed-off-by: Andrzej Hajda
> Signed-off-by: Kyungmin Park
> ---
> v5
> - removed non-standard samsung hflip/vflip device tree bindings
>
> v4
> - GPL changed to GPLv2,
> - bitfields replaced by u8,
> - cosmetic changes,
> - corrected s_stream flow,
> - gpio pins are no longer exported,
> - added I2C addresses to subdev names,
> - CIS subdev registration postponed after
> succesfull HW initialization,
> - added enums for pads,
> - selections are initialized only during probe,
> - default resolution changed to 1600x1200,
> - state->error pattern removed from few other functions,
> - entity link creation moved to registered callback.
>
> v3:
> - narrowed state->error usage to i2c and power errors,
> - private gain controls replaced by red/blue balance user controls,
> - added checks to devicetree gpio node parsing
>
> v2:
> - lower-cased driver name,
> - removed underscore from regulator names,
> - removed platform data code,
> - v4l controls grouped in anonymous structs,
> - added s5k5baf_clear_error function,
> - private controls definitions moved to uapi header file,
> - added v4l2-controls.h reservation for private controls,
> - corrected subdev registered/unregistered code,
> - .log_status sudbev op set to v4l2 helper,
> - moved entity link creation to probe routines,
> - added cleanup on error to probe function.
> ---
> .../devicetree/bindings/media/samsung-s5k5baf.txt | 51 +
> MAINTAINERS|7 +
> drivers/media/i2c/Kconfig |7 +
> drivers/media/i2c/Makefile |1 +
> drivers/media/i2c/s5k5baf.c| 1980
>
> 5 files changed, 2046 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> create mode 100644 drivers/media/i2c/s5k5baf.c
>
> diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> new file mode 100644
> index 000..b1f2fde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> @@ -0,0 +1,51 @@
> +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP
> +-
> +
> +Required properties:
> +
> +- compatible : "samsung,s5k5baf";
> +- reg: I2C slave address of the sensor;
> +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V);
> +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V)
> +or 2.8V (2.6V to 3.0);
> +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V)
> +or 2.8V (2.5V to 3.1V);
> +- gpios : GPIOs connected to STDBYN and RSTN pins,
> +in order: STBYN, RSTN;
> +- clock-frequency : master clock frequency in Hz;
Why is this necessary? Could you not just require having a clocks
property? You could then get equivalent functionality to the
clock-frequency property by having a fixed-clock node if you don't ahve
a real clock specifier to wire up.
> +
> +Optional properties:
> +
> +- clocks : contains the sensor's master clock specifier;
> +- clock-names: contains "mclk" entry;
> +
> +The device node should contain one 'port' child node with one child
> 'endpoint'
> +node, according to the bindings defined in Documentation/devicetree/bindings/
> +media/video-interfaces.txt. The following are properties specific to those
> nodes.
> +
> +endpoint node
> +-
> +
> +- data-lanes : (optional) an array specifying active physical MIPI-CSI2
> + data output lanes and their mapping to logical lanes; the
> + array's content is unused, only its length is meaningful;
Is that a property of the driver, or does the design of the hardware
mean that this can never encode useful information?
What does the length of the property imply?
> +
> +Example:
> +
> +s5k5bafx@2d {
> + compatible = "samsung,s5k5baf";
> + reg = <0x2d>;
> + vdda-supply = <&cam_io_en_reg>;
> + vddreg-supply = <&vt_core_15v_reg>;
> + vddio-supply = <&vtcam_reg>;
> + gpios = <&gpl2 0 1>,
> + <&gpl2 1 1>;
> + clock-frequency = <2400>;
> +
> + port {
> + s5k5bafx_ep: endpoint {
> + remote-endpoint = <&csis1_ep>;
> + data-lanes = <1>;
> + };
> + };
> +};
Thanks,
Mark.
--
To un
Re: [PATCH RFC v5] s5k5baf: add camera sensor driver
On Mon, 2013-08-19 at 14:18 +0100, Andrzej Hajda wrote: > +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt > @@ -0,0 +1,51 @@ > +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP > +- > + > +Required properties: > + > +- compatible : "samsung,s5k5baf"; > +- reg: I2C slave address of the sensor; > +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V); > +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V) > +or 2.8V (2.6V to 3.0); > +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) > +or 2.8V (2.5V to 3.1V); > +- gpios : GPIOs connected to STDBYN and RSTN pins, > +in order: STBYN, RSTN; You probably want to use the "[-]gpios" convention here (see Documentation/devicetree/bindings/gpio/gpio.txt), so something like stbyn-gpios and rstn-gpios. Pawel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
