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
- clock-names: should include "mclk" for the sensor's master clock
> +
> +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?
> +
> +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.
> +};
> +
> +#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.
[...]
> +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).
> +/* 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
