Hi Andrzej,
On 04/16/2013 03:38 PM, 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: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
It's worth to note that this driver currently doesn't use the asynchronous
sub-device probing API [1] but the intention is to switch to it once it's
settled.
[1] http://www.spinics.net/lists/linux-sh/msg18565.html
A few more comments below...
.../devicetree/bindings/media/samsung-s5k5baf.txt | 50 +
MAINTAINERS | 8 +
drivers/media/i2c/Kconfig | 7 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/s5k5baf.c | 1962 ++++++++++++++++++++
include/media/s5k5baf.h | 48 +
6 files changed, 2076 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
create mode 100644 drivers/media/i2c/s5k5baf.c
create mode 100644 include/media/s5k5baf.h
diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
new file mode 100644
index 0000000..1099c1d
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
@@ -0,0 +1,50 @@
+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);
+- vdd_reg-supply : regulator input power 1.8V (1.7V to 1.9V)
s/power/power supply ?
Underscores should be avoided in the regulator supply names AFAIK, so
s/vdd_reg/vddreg
+ or 2.8V (2.6V to 3.0);
+- vddio-supply : I/O supply 1.8V (1.65V to 1.95V)
s/supply/power supply ?
+ or 2.8V (2.5V to 3.1V);
+- gpios : standby and reset gpios;
You are not clear about the order, how about
- gpios : GPIOs connected to STDBYN and RSTN pins, in order: STBYN, RSTN;
?
+- clock-frequency : master clock frequency in Hz;
+
+Optional properties:
+
+- samsung,hflip : horizontal image flip
+- samsung,vflip : vertical image flip
Optional clock properties are missing:
clocks : contains the sensor's master clock specifier;
clock-names : contains "mclk" entry;
MCLK happens to be the clock input pin name in the datasheet.
+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) an array specifying active physical MIPI-CSI2
+ data output lanes and their mapping to logical lanes; the
+ array's content is unused, only its length is meaningful;
+
+Example:
+
+s5k5bafx@2d {
+ compatible = "samsung,s5k5baf";
+ reg = <0x2d>;
+ vdda-supply = <&cam_io_en_reg>;
+ vdd_reg-supply = <&vt_core_15v_reg>;
+ vddio-supply = <&vtcam_reg>;
+ gpios = <&gpl2 0 1>,
+ <&gpl2 1 1>;
+ clock-frequency = <24000000>;
+
+ port {
+ s5k5bafx_ep: endpoint {
+ remote-endpoint = <&csis1_ep>;
+ data-lanes = <1>;
+ };
+ };
+};
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9e7ce8b..e487f7d 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -553,6 +553,13 @@ config VIDEO_S5K4ECGX
This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
camera sensor with an embedded SoC image signal processor.
+config VIDEO_S5K5BAF
+ tristate "Samsung S5K5BAF sensor support"
+ depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+ ---help---
+ This is a V4L2 sensor-level driver for Samsung S5K5BAF 2M
+ camera sensor with an embedded SoC image signal processor.
+
source "drivers/media/i2c/smiapp/Kconfig"
config VIDEO_S5C73M3
diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
new file mode 100644
index 0000000..0bae2d3
--- /dev/null
+++ b/drivers/media/i2c/s5k5baf.c
@@ -0,0 +1,1962 @@
+/*
+ * Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor
+ * with embedded SoC ISP.
+ *
+ * Copyright (C) 2013, Samsung Electronics Co., Ltd.
+ * Andrzej Hajda <a.hajda@xxxxxxxxxxx>
+ *
+ * Based on S5K6AA driver authored by Sylwester Nawrocki
+ * Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
I would rephrase it to:
+ * Based on S5K6AA driver authored by Sylwester Nawrocki
+ * Copyright (C) 2013, Samsung Electronics Co., Ltd.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+static const char * const s5k5baf_supply_names[] = {
+ "vdda", /* Analog power supply 2.8V (2.6V to 3.0V) */
+ "vdd_reg", /* Regulator input power 1.8V (1.7V to 1.9V)
"vddreg"
+ or 2.8V (2.6V to 3.0) */
+ "vddio", /* I/O supply 1.8V (1.65V to 1.95V)
+ or 2.8V (2.5V to 3.1V) */
+};
+#define S5K5BAF_NUM_SUPPLIES ARRAY_SIZE(s5k5baf_supply_names)
+struct s5k5baf_ctrls {
+ struct v4l2_ctrl_handler handler;
+ /* Auto / manual white balance cluster */
+ struct v4l2_ctrl *awb;
+ struct v4l2_ctrl *gain_red;
+ struct v4l2_ctrl *gain_blue;
+ struct v4l2_ctrl *gain_green;
+ /* Mirror cluster */
+ struct v4l2_ctrl *hflip;
+ struct v4l2_ctrl *vflip;
+ /* Auto exposure / manual exposure and gain cluster */
+ struct v4l2_ctrl *auto_exp;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *gain;
Let's put each cluster in separate anonymous struct, so e.g.
+ struct { /* Auto exposure / manual exposure and gain cluster */
+ struct v4l2_ctrl *auto_exp;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *gain;
+ };
+};
+
+/*
+ * V4L2 subdev controls
+ */
+static int s5k5baf_log_status(struct v4l2_subdev *sd)
+{
+ v4l2_ctrl_handler_log_status(sd->ctrl_handler, sd->name);
+ return 0;
+}
This function is not needed, you could use now v4l2_ctrl_subdev_log_status()
directly.
+#define V4L2_CID_RED_GAIN (V4L2_CTRL_CLASS_CAMERA | 0x1001)
+#define V4L2_CID_GREEN_GAIN (V4L2_CTRL_CLASS_CAMERA | 0x1002)
+#define V4L2_CID_BLUE_GAIN (V4L2_CTRL_CLASS_CAMERA | 0x1003)
A range should be reserved for the private controls in
include/uapi/linux/v4l2-controls.h and used as a base here, so control
IDs of various drivers are not overlapping.
+/*
+ * V4L2 subdev internal operations
+ */
+static void s5k5baf_check_fw_revision(struct s5k5baf *state)
+{
+ u16 api_ver = 0, fw_rev = 0, s_id = 0;
+
+ api_ver = s5k5baf_read(state, REG_FW_APIVER);
+ fw_rev = s5k5baf_read(state, REG_FW_REVISION) & 0xff;
+ s_id = s5k5baf_read(state, REG_FW_SENSOR_ID);
+ if (state->error)
+ return;
+
+ v4l2_info(&state->sd, "FW API=%#x, revision=%#x sensor_id=%#x\n",
+ api_ver, fw_rev, s_id);
+
+ if (api_ver == S5K5BAF_FW_APIVER)
+ return;
+
+ v4l2_err(&state->sd, "FW API version not supported\n");
+ state->error = -ENODEV;
When we initially discussed it in private my intention was to use
'state->error' mainly to ease handling of multiple I2C write calls.
I have a feeling that the code would be easier to follow if it would
be returning here the error value explicitly.
+}
+
+static int s5k5baf_registered(struct v4l2_subdev *sd)
+{
+ struct s5k5baf *state = to_s5k5baf(sd);
+ int ret;
+
+ ret = v4l2_device_register_subdev(sd->v4l2_dev, &state->cis_sd);
+ if (ret) {
+ v4l2_err(sd, "failed to register subdev %s\n",
+ state->cis_sd.name);
+ return ret;
+ }
+
+ ret = media_entity_create_link(&state->cis_sd.entity,
+ 0, &state->sd.entity, 0,
+ MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+
+ mutex_lock(&state->lock);
+
+ s5k5baf_power_on(state);
+ s5k5baf_hw_init(state);
+ s5k5baf_check_fw_revision(state);
+ s5k5baf_power_off(state);
+
+ ret = state->error;
+ state->error = 0;
Probably makes sense to create a helper function that would get and
clear state->error, e.g. s5k5baf_error_get_clear() ?
+
+ mutex_unlock(&state->lock);
+
+ return ret;
+}
+
+static const struct v4l2_subdev_ops s5k5baf_cis_subdev_ops = {
+ .pad = &s5k5baf_cis_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops s5k5baf_cis_subdev_internal_ops = {
+ .open = s5k5baf_open,
+};
+
+static const struct v4l2_subdev_internal_ops s5k5baf_subdev_internal_ops = {
+ .registered = s5k5baf_registered,
The 'unregistered' callback is missing, it should undo anything what's
done in s5k5baf_registered() or s5k5baf_registered() should be protected
against multiple calls.
+ .open = s5k5baf_open,
+};
+
+static const struct v4l2_subdev_core_ops s5k5baf_core_ops = {
+ .s_power = s5k5baf_set_power,
+ .log_status = s5k5baf_log_status,
.log_status = v4l2_ctrl_subdev_log_status,
+};
+
+static const struct v4l2_subdev_ops s5k5baf_subdev_ops = {
+ .core = &s5k5baf_core_ops,
+ .pad = &s5k5baf_pad_ops,
+ .video = &s5k5baf_video_ops,
+};
+
+static void s5k5baf_configure_gpios(struct s5k5baf *state)
+{
+ static const char const *name[] = { "S5K5BAF_STBY", "S5K5BAF_RST" };
+ struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
+ struct s5k5baf_gpio *g = state->pdata.gpios;
+ int ret, i;
+
+ if (state->error)
+ return;
+
+ for (i = 0; i < GPIO_NUM; ++i) {
+ int flags = GPIOF_EXPORT | GPIOF_DIR_OUT;
+ if (g[i].level)
+ flags |= GPIOF_INIT_HIGH;
+ ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, name[i]);
+ if (ret) {
+ v4l2_err(c, "failed to request gpio %s\n", name[i]);
+ state->error = ret;
+ return;
+ }
+ }
+ return;
Not needed, but anyway would be better to change the return value type to 'int'.
+}
+
+static void s5k5baf_get_platform_data(struct s5k5baf *state, struct device *dev)
+{
+ struct device_node *node = dev->of_node;
+ struct s5k5baf_platform_data *pd;
+ struct device_node *node_ep;
+ struct v4l2_of_endpoint ep;
+ enum of_gpio_flags of_flags;
+
+ if (state->error)
+ return;
+
+ if (!node) {
+ pd = dev->platform_data;
+ if (!pd) {
+ dev_err(dev, "No platform data\n");
+ state->error = -EINVAL;
+ }
+ state->pdata = *pd;
+ return;
+ }
+
+ pd = &state->pdata;
+
+ of_property_read_u32(node, "clock-frequency", &pd->mclk_frequency);
+ pd->hflip = of_property_read_bool(node, "samsung,hflip");
+ pd->vflip = of_property_read_bool(node, "samsung,vflip");
+ pd->gpios[STBY].gpio = of_get_gpio_flags(node, STBY, &of_flags);
+ pd->gpios[STBY].level = !(of_flags & OF_GPIO_ACTIVE_LOW);
+ pd->gpios[RST].gpio = of_get_gpio_flags(node, RST, &of_flags);
+ pd->gpios[RST].level = !(of_flags & OF_GPIO_ACTIVE_LOW);
+
+ node_ep = v4l2_of_get_next_endpoint(node, NULL);
+ if (!node_ep) {
+ dev_err(dev, "no endpoint defined\n");
+ state->error = -EINVAL;
+ return;
+ }
+ v4l2_of_parse_endpoint(node_ep, &ep);
+ of_node_put(node_ep);
+ pd->bus_type = ep.bus_type;
+ if (pd->bus_type == V4L2_MBUS_CSI2)
+ pd->nlanes = ep.bus.mipi_csi2.num_data_lanes;
+}
+
+static void s5k5baf_configure_subdevs(struct s5k5baf *state,
+ struct i2c_client *c)
+{
+ struct v4l2_subdev *sd;
+
+ if (state->error)
+ return;
+
+ sd = &state->cis_sd;
+ v4l2_subdev_init(sd, &s5k5baf_cis_subdev_ops);
+ sd->owner = c->driver->driver.owner;
+ v4l2_set_subdevdata(sd, state);
+ strlcpy(sd->name, "S5K5BAF-CIS", sizeof(sd->name));
+
+ sd->internal_ops = &s5k5baf_cis_subdev_internal_ops;
+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ state->cis_pad.flags = MEDIA_PAD_FL_SOURCE;
+ sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
+ state->error = media_entity_init(&sd->entity, 1, &state->cis_pad, 0);
+ if (state->error)
+ goto err;
+
+ sd = &state->sd;
+ v4l2_i2c_subdev_init(sd, c, &s5k5baf_subdev_ops);
+ strlcpy(sd->name, "S5K5BAF-ISP", sizeof(sd->name));
+
+ sd->internal_ops = &s5k5baf_subdev_internal_ops;
+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ state->pads[0].flags = MEDIA_PAD_FL_SINK;
+ state->pads[1].flags = MEDIA_PAD_FL_SOURCE;
+ sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV;
+ state->error = media_entity_init(&sd->entity, 2, state->pads, 0);
+ if (!state->error)
+ return;
+
+ media_entity_cleanup(&state->cis_sd.entity);
+err:
+ dev_err(&c->dev, "cannot init media entity %s\n", sd->name);
Not sure if this error log is needed. Might be better to log this
upon return from this function, so 'goto' can be avoided.
+ return state->error;
+}
+
+static int s5k5baf_remove(struct i2c_client *c)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(c);
+ struct s5k5baf *state = to_s5k5baf(sd);
+
+ v4l2_device_unregister_subdev(sd);
+ v4l2_ctrl_handler_free(sd->ctrl_handler);
+ media_entity_cleanup(&sd->entity);
+
+ sd = &state->cis_sd;
+ v4l2_device_unregister_subdev(sd);
+ media_entity_cleanup(&sd->entity);
+
+ return 0;
+}
+
+static const struct i2c_device_id s5k5baf_id[] = {
+ { DRIVER_NAME, 0 },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, s5k5baf_id);
+
+static const struct of_device_id s5k5baf_of_match[] = {
+ { .compatible = "samsung," DRIVER_NAME },
Hmm, it looks a bit obfuscated to me, how about writing whole compatible
string explicitly ?
+ { }
+};
+MODULE_DEVICE_TABLE(of, s5k5baf_of_match);
+
+static struct i2c_driver s5k5baf_i2c_driver = {
+ .driver = {
+ .of_match_table = s5k5baf_of_match,
+ .name = DRIVER_NAME
+ },
+ .probe = s5k5baf_probe,
+ .remove = s5k5baf_remove,
+ .id_table = s5k5baf_id,
+};
+
+module_i2c_driver(s5k5baf_i2c_driver);
+
+MODULE_DESCRIPTION("Samsung S5K5BAF(X) UXGA camera driver");
+MODULE_AUTHOR("Andrzej Hajda <a.hajda@xxxxxxxxxxx>");
+MODULE_LICENSE("GPL");
diff --git a/include/media/s5k5baf.h b/include/media/s5k5baf.h
new file mode 100644
index 0000000..957e708
--- /dev/null
+++ b/include/media/s5k5baf.h
@@ -0,0 +1,48 @@
+/*
+ * S5K5BAF camera sensor driver header
+ *
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef S5K5BAF_H
+#define S5K5BAF_H
+
+#include <media/v4l2-mediabus.h>
+
+/**
+ * struct s5k5baf_gpio - data structure describing a GPIO
+ * @gpio: GPIO number
+ * @level: indicates active state of the @gpio
+ */
+struct s5k5baf_gpio {
+ int gpio;
+ int level;
+};
+
+/**
+ * struct s5k5baf_platform_data - s5k5baf driver platform data
+ * @set_power: an additional callback to the board code, called
+ * after enabling the regulators and before switching
+ * the sensor off
+ * @mclk_frequency: sensor's master clock frequency in Hz
+ * @gpios: GPIOs driving pins: STBY, RESET
+ * @nlanes: maximum number of MIPI-CSI lanes used
+ * @hflip: default horizontal image flip value, non zero to enable
+ * @vflip: default vertical image flip value, non zero to enable
+ */
+
+struct s5k5baf_platform_data {
+ u32 mclk_frequency;
+ struct s5k5baf_gpio gpios[2];
+ enum v4l2_mbus_type bus_type;
+ u8 nlanes;
+ u8 hflip:1;
+ u8 vflip:1;
+};
+
+#endif /* S5K5BAF_H */
Since we are not going to be using platform_data, I think this whole
header file could be removed for now. Let's not add a dead code. If it
happens someone ever needs platform_data I think it can be added then.
Thanks,
Sylwester
