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