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 linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
