Re: [PATCH v8] s5k5baf: add camera sensor driver

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html