Hi Sylwester, Will make the changes you suggested. Will re-spin this entire series with some more minor fixes after more rigorous testing. Regards Arun On Fri, Sep 6, 2013 at 1:32 AM, Sylwester Nawrocki <sylvester.nawrocki@xxxxxxxxx> wrote: > On 08/21/2013 08:34 AM, Arun Kumar K wrote: >> >> This patch adds subdev driver for Samsung S5K4E5 raw image sensor. >> Like s5k6a3, it is also another fimc-is firmware controlled >> sensor. This minimal sensor driver doesn't do any I2C communications >> as its done by ISP firmware. It can be updated if needed to a >> regular sensor driver by adding the I2C communication. >> >> Signed-off-by: Arun Kumar K<arun.kk@xxxxxxxxxxx> >> Reviewed-by: Sylwester Nawrocki<s.nawrocki@xxxxxxxxxxx> >> --- >> .../devicetree/bindings/media/i2c/s5k4e5.txt | 43 +++ >> drivers/media/i2c/Kconfig | 8 + >> drivers/media/i2c/Makefile | 1 + >> drivers/media/i2c/s5k4e5.c | 361 >> ++++++++++++++++++++ >> 4 files changed, 413 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/media/i2c/s5k4e5.txt >> create mode 100644 drivers/media/i2c/s5k4e5.c >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/s5k4e5.txt >> b/Documentation/devicetree/bindings/media/i2c/s5k4e5.txt >> new file mode 100644 >> index 0000000..5af462c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/s5k4e5.txt >> @@ -0,0 +1,43 @@ >> +* Samsung S5K4E5 Raw Image Sensor >> + >> +S5K4E5 is a raw image sensor with maximum resolution of 2560x1920 >> +pixels. Data transfer is carried out via MIPI CSI-2 port and controls >> +via I2C bus. >> + >> +Required Properties: >> +- compatible : must be "samsung,s5k4e5" >> +- reg : I2C device address >> +- gpios : reset gpio pin > > > I guess this should be "reset-gpios". How about changing description to: > > - reset-gpios : specifier of a GPIO connected to the RESET pin; > > > ? >> >> +- clocks : clock specifier for the clock-names property > > > Perhaps something along the lines of: > > - clocks : "should contain the sensor's MCLK clock specifier, from > the common clock bindings" > > ? >> >> +- clock-names : must contain "mclk" entry > > > Is name of the clock input pin really MCLK ? > > >> +- svdda-supply : core voltage supply >> +- svddio-supply : I/O voltage supply >> + >> +Optional Properties: >> +- clock-frequency : operating frequency for the sensor >> + default value will be taken if not provided. > > > How about clarifying it a bit, as Stephen suggested, e.g.: > > - clock-frequency : the frequency at which the "mclk" clock should be > configured to operate, in Hz; if this property is not > specified default 24 MHz value will be used. > > >> +The device node should be added to respective control bus controller >> +(e.g. I2C0) nodes and linked to the csis port node, using the common >> +video interfaces bindings, defined in video-interfaces.txt. > > > This seems misplaced, S5K4E5 image sensor has nothingto do with csis nodes. > How about something like this instead: > > "The common video interfaces bindings (see video-interfaces.txt) should be > used to specify link to the image data receiver. The S5K6A3(YX) device > node should contain one 'port' child node with an 'endpoint' subnode. > > Following properties are valid for the endpoint node: > ..." > > >> +Example: >> + >> + i2c-isp@13130000 { >> + s5k4e5@20 { >> + compatible = "samsung,s5k4e5"; >> + reg =<0x20>; >> + gpios =<&gpx1 2 1>; >> + clock-frequency =<24000000>; >> + clocks =<&clock 129>; >> + clock-names = "mclk"; >> + svdda-supply =<...>; >> + svddio-supply =<...>; >> + port { >> + is_s5k4e5_ep: endpoint { >> + data-lanes =<1 2 3 4>; >> + remote-endpoint =<&csis0_ep>; >> + }; >> + }; >> + }; >> + }; > > > -- > Thanks, > Sylwester -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html