Hi Mark,

Thanks for the review, sorry for late response.

On 09/20/2013 07:06 PM, Mark Rutland wrote:

On Fri, Sep 06, 2013 at 11:31:06AM +0100, Andrzej Hajda wrote:

quoted

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 <redacted>
Signed-off-by: Andrzej Hajda <redacted>
Signed-off-by: Kyungmin Park <redacted>
---
Hi,

This is the 8th iteration of the patch.
I have applied suggestions from Laurent, Sylwester and Mark, thanks.
One exeception, I have left static struct v4l2_rect s5k5baf_cis_rect
not const due to fact its address is passed to function which could
modify its arguments, of course it never modifies s5k5baf_cis_rect.

Regards
Andrzej

v8
- improved description of data-lanes binding,
- added algorithm caching,
- added comments to functions,
- video bus type checking moved to probe,
- clk_get/put moved to probe,
- moved streaming checking under mutex,
- use proper functions for endian conversion,
- probe returns -EPROBE_DEFER in case it cannot get clock,
- v4l2_async_unregister_subdev is called on remove,
- cosmetic changes

v7
- changed description of 'clock-frequency' DT property

v6
- endpoint node presence is now optional,
- added asynchronous subdev registration support and clock
  handling,
- use named gpios in DT bindings

v5
- removed hflip/vflip device tree properties

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  |   58 +
 MAINTAINERS                                        |    7 +
 drivers/media/i2c/Kconfig                          |    7 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/s5k5baf.c                        | 2050 ++++++++++++++++++++
 5 files changed, 2123 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 0000000..7704a1e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt

@@ -0,0 +1,58 @@
+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);
+- stbyn-gpios    : GPIO connected to STDBYN pin;
+- rstn-gpios     : GPIO connected to RSTN pin;
+- clocks         : the sensor's master clock specifier (from the common
+                   clock bindings);
+- clock-names    : must be "mclk";

I'd reword this slightly:

- clocks: clock-specifiers (per the common clock bindings) for the
  clocks described in clock-names

OK

- clock-names: should include "mclk" for the sensor's master clock

IMHO it suggests there could be more than one clock, is it OK?

quoted

+
+Optional properties:
+
+- 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 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) specifies MIPI CSI-2 data lanes as covered in
+  video-interfaces.txt. This sensor doesn't support data lane remapping
+  and physical lane indexes in subsequent elements of the array should
+  have consecutive values.

Do these need to start at 1, or may they have any initial value?

After re-checking I have found some inconsistency in the specs,
regarding lanes.
Final conclusion is that sensor supports only one lane without re-mapping.
I would then change description to:

- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
  video-interfaces.txt. If present it should be <1> - the device supports only one data lane
  without re-mapping.

quoted

+
+Example:
+
+s5k5bafx@2d {
+       compatible = "samsung,s5k5baf";
+       reg = <0x2d>;
+       vdda-supply = <&cam_io_en_reg>;
+       vddreg-supply = <&vt_core_15v_reg>;
+       vddio-supply = <&vtcam_reg>;
+       stbyn-gpios = <&gpl2 0 1>;
+       rstn-gpios = <&gpl2 1 1>;
+       clock-names = "mclk";
+       clocks = <&clock_cam 0>;
+       clock-frequency = <24000000>;
+
+       port {
+               s5k5bafx_ep: endpoint {
+                       remote-endpoint = <&csis1_ep>;
+                       data-lanes = <1>;
+               };
+       };
+};

Otherwise, I think the binding looks fine.

I took a quick skim over the driver and I have a few other comments.

[...]

quoted

+enum s5k5baf_gpio_id {
+       STBY,
+       RST,
+       GPIO_NUM,

I'd expect this to be NUM_GPIOS or MAX_GPIOS, as GPIO_NUM sounds like
the index for a GPIO, rather than how many GPIOs there are.

OK

quoted

+};
+
+#define PAD_CIS 0
+#define PAD_OUT 1
+#define CIS_PAD_NUM 1
+#define ISP_PAD_NUM 2

Similarly here, I think NUM_*S or MAX_*S is preferable.

OK

[...]

quoted

+static void s5k5baf_hw_patch(struct s5k5baf *state)
+{
+       static const u16 nseq_patch[] = {
+               NSEQ(0x1668,
+               0xb5fe, 0x0007, 0x683c, 0x687e, 0x1da5, 0x88a0, 0x2800, 0xd00b,
+               0x88a8, 0x2800, 0xd008, 0x8820, 0x8829, 0x4288, 0xd301, 0x1a40,
+               0xe000, 0x1a08, 0x9001, 0xe001, 0x2019, 0x9001, 0x4916, 0x466b,
+               0x8a48, 0x8118, 0x8a88, 0x8158, 0x4814, 0x8940, 0x0040, 0x2103,
+               0xf000, 0xf826, 0x88a1, 0x4288, 0xd908, 0x8828, 0x8030, 0x8868,
+               0x8070, 0x88a8, 0x6038, 0xbcfe, 0xbc08, 0x4718, 0x88a9, 0x4288,
+               0xd906, 0x8820, 0x8030, 0x8860, 0x8070, 0x88a0, 0x6038, 0xe7f2,
+               0x9801, 0xa902, 0xf000, 0xf812, 0x0033, 0x0029, 0x9a02, 0x0020,
+               0xf000, 0xf814, 0x6038, 0xe7e6, 0x1a28, 0x7000, 0x0d64, 0x7000,
+               0x4778, 0x46c0, 0xf004, 0xe51f, 0xa464, 0x0000, 0x4778, 0x46c0,
+               0xc000, 0xe59f, 0xff1c, 0xe12f, 0x6009, 0x0000, 0x4778, 0x46c0,
+               0xc000, 0xe59f, 0xff1c, 0xe12f, 0x622f, 0x0000),

[... snipping another ~50 lines of binary blob ...]

Could you please describe what this is precisely?

This looks like a firmware blob, and I don't think this should be
embedded in the driver (see Documentation/firmware_class/README).

It would be nice to have some description of the format of the other
binary dumps below if possible (even if by reference to a manual).

The problem is that I do not have detailed specification, these blobs were
taken from internal/android drivers. But in fact I could move all three
blobs to 'firmware'
file, it will look better :)

quoted

+/* set custom color correction matrices for various illuminations */
+static void s5k5baf_hw_set_ccm(struct s5k5baf *state)
+{
+       static const u16 nseq_cfg[] = {
+               NSEQ(REG_PTR_CCM_HORIZON,
+               REG_ARR_CCM(0), PAGE_IF_SW,
+               REG_ARR_CCM(1), PAGE_IF_SW,
+               REG_ARR_CCM(2), PAGE_IF_SW,
+               REG_ARR_CCM(3), PAGE_IF_SW,
+               REG_ARR_CCM(4), PAGE_IF_SW,
+               REG_ARR_CCM(5), PAGE_IF_SW),
+               NSEQ(REG_PTR_CCM_OUTDOOR,
+               REG_ARR_CCM(6), PAGE_IF_SW),
+               NSEQ(REG_ARR_CCM(0),
+               /* horizon */
+               0x010d, 0xffa7, 0xfff5, 0x003b, 0x00ef, 0xff38,
+               0xfe42, 0x0270, 0xff71, 0xfeed, 0x0198, 0x0198,
+               0xff95, 0xffa3, 0x0260, 0x00ec, 0xff33, 0x00f4,
+               /* incandescent */
+               0x010d, 0xffa7, 0xfff5, 0x003b, 0x00ef, 0xff38,
+               0xfe42, 0x0270, 0xff71, 0xfeed, 0x0198, 0x0198,
+               0xff95, 0xffa3, 0x0260, 0x00ec, 0xff33, 0x00f4,
+               /* warm white */
+               0x01ea, 0xffb9, 0xffdb, 0x0127, 0x0109, 0xff3c,
+               0xff2b, 0x021b, 0xff48, 0xff03, 0x0207, 0x0113,
+               0xffca, 0xff93, 0x016f, 0x0164, 0xff55, 0x0163,
+               /* cool white */
+               0x01ea, 0xffb9, 0xffdb, 0x0127, 0x0109, 0xff3c,
+               0xff2b, 0x021b, 0xff48, 0xff03, 0x0207, 0x0113,
+               0xffca, 0xff93, 0x016f, 0x0164, 0xff55, 0x0163,
+               /* daylight 5000K */
+               0x0194, 0xffad, 0xfffe, 0x00c5, 0x0103, 0xff5d,
+               0xfee3, 0x01ae, 0xff27, 0xff18, 0x018f, 0x00c8,
+               0xffe8, 0xffaa, 0x01c8, 0x0132, 0xff3e, 0x0100,
+               /* daylight 6500K */
+               0x0194, 0xffad, 0xfffe, 0x00c5, 0x0103, 0xff5d,
+               0xfee3, 0x01ae, 0xff27, 0xff18, 0x018f, 0x00c8,
+               0xffe8, 0xffaa, 0x01c8, 0x0132, 0xff3e, 0x0100,
+               /* outdoor */
+               0x01cc, 0xffc3, 0x0009, 0x00a2, 0x0106, 0xff3f,
+               0xfed8, 0x01fe, 0xff08, 0xfec7, 0x00f5, 0x0119,
+               0xffdf, 0x0024, 0x01a8, 0x0170, 0xffad, 0x011b),
+               0
+       };
+       s5k5baf_write_nseq(state, nseq_cfg);
+}
+
+/* CIS sensor tuning, based on undocumented android driver code */
+static void s5k5baf_hw_set_cis(struct s5k5baf *state)
+{
+       static const u16 nseq_cfg[] = {
+               NSEQ(0xc202, 0x0700),
+               NSEQ(0xf260, 0x0001),
+               NSEQ(0xf414, 0x0030),
+               NSEQ(0xc204, 0x0100),
+               NSEQ(0xf402, 0x0092, 0x007f),
+               NSEQ(0xf700, 0x0040),
+               NSEQ(0xf708,
+               0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
+               0x0040, 0x0040, 0x0040, 0x0040, 0x0040,
+               0x0001, 0x0015, 0x0001, 0x0040),
+               NSEQ(0xf48a, 0x0048),
+               NSEQ(0xf10a, 0x008b),
+               NSEQ(0xf900, 0x0067),
+               NSEQ(0xf406, 0x0092, 0x007f, 0x0003, 0x0003, 0x0003),
+               NSEQ(0xf442, 0x0000, 0x0000),
+               NSEQ(0xf448, 0x0000),
+               NSEQ(0xf456, 0x0001, 0x0010, 0x0000),
+               NSEQ(0xf41a, 0x00ff, 0x0003, 0x0030),
+               NSEQ(0xf410, 0x0001, 0x0000),
+               NSEQ(0xf416, 0x0001),
+               NSEQ(0xf424, 0x0000),
+               NSEQ(0xf422, 0x0000),
+               NSEQ(0xf41e, 0x0000),
+               NSEQ(0xf428, 0x0000, 0x0000, 0x0000),
+               NSEQ(0xf430, 0x0000, 0x0000, 0x0008, 0x0005, 0x000f, 0x0001,
+               0x0040, 0x0040, 0x0010),
+               NSEQ(0xf4d6, 0x0090, 0x0000),
+               NSEQ(0xf47c, 0x000c, 0x0000),
+               NSEQ(0xf49a, 0x0008, 0x0000),
+               NSEQ(0xf4a2, 0x0008, 0x0000),
+               NSEQ(0xf4b2, 0x0013, 0x0000, 0x0013, 0x0000),
+               NSEQ(0xf4aa, 0x009b, 0x00fb, 0x009b, 0x00fb),
+               0
+       };
+
+       s5k5baf_i2c_write(state, REG_CMDWR_PAGE, PAGE_IF_HW);
+       s5k5baf_write_nseq(state, nseq_cfg);
+       s5k5baf_i2c_write(state, REG_CMDWR_PAGE, PAGE_IF_SW);
+}

Cheers,
Mark.

--
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