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: >> 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 <s.nawrocki@xxxxxxxxxxx> >> Signed-off-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx> >> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> >> --- >> 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? > >> + >> +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. >> + >> +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. > > [...] > >> +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 > >> +}; >> + >> +#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 > > [...] > >> +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 :) > >> +/* 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html