Hi Ivaylo,
On Sat, Jun 04, 2016 at 10:54:58PM +0300, Ivaylo Dimitrov wrote:
> Hi,
>
> On 26.05.2016 00:45, Sakari Ailus wrote:
> >Hi Ivaylo,
> >
> >I've got some comments here but I haven't reviewed everything yet. What's
> >missing is
> >
> >- the user space interface for selecting the sensor configuration "mode",
> >
> >- passing information on the sensor configuration to the user space.
> >
> >I'll try to take a look at those some time in the near future.
> >
>
> ok
>
> >
> >I very much appreciate your work towards finally upstreaming this! :-)
> >
> >On Tue, May 03, 2016 at 05:50:04PM +0300, Ivaylo Dimitrov wrote:
> >>The sensor is found in Nokia N900 main camera
> >>
> >>Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@xxxxxxxxx>
> >>---
> >> .../bindings/media/i2c/toshiba,et8ek8.txt | 53 +
> >> drivers/media/i2c/Kconfig | 1 +
> >> drivers/media/i2c/Makefile | 1 +
> >> drivers/media/i2c/et8ek8/Kconfig | 6 +
> >> drivers/media/i2c/et8ek8/Makefile | 2 +
> >> drivers/media/i2c/et8ek8/et8ek8_driver.c | 1711 ++++++++++++++++++++
> >> drivers/media/i2c/et8ek8/et8ek8_mode.c | 591 +++++++
> >> drivers/media/i2c/et8ek8/et8ek8_reg.h | 100 ++
> >> include/uapi/linux/v4l2-controls.h | 5 +
> >> 9 files changed, 2470 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> >> create mode 100644 drivers/media/i2c/et8ek8/Kconfig
> >> create mode 100644 drivers/media/i2c/et8ek8/Makefile
> >> create mode 100644 drivers/media/i2c/et8ek8/et8ek8_driver.c
> >> create mode 100644 drivers/media/i2c/et8ek8/et8ek8_mode.c
> >> create mode 100644 drivers/media/i2c/et8ek8/et8ek8_reg.h
> >>
> >>diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> >>new file mode 100644
> >>index 0000000..55f712c
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> >>@@ -0,0 +1,53 @@
> >>+Toshiba et8ek8 5MP sensor
> >>+
> >>+Toshiba et8ek8 5MP sensor is an image sensor found in Nokia N900 device
> >>+
> >>+More detailed documentation can be found in
> >>+Documentation/devicetree/bindings/media/video-interfaces.txt .
> >>+
> >>+
> >>+Mandatory properties
> >>+--------------------
> >>+
> >>+- compatible: "toshiba,et8ek8"
> >>+- reg: I2C address (0x3e, or an alternative address)
> >>+- vana-supply: Analogue voltage supply (VANA), typically 2,8 volts (sensor
> >>+ dependent).
> >
> >As these are bindings for a particular sensor, 2,8 volts it is.
> >
> >The sensor has also a digital voltage supply but it might be controlled by
> >the same GPIO which controls the CCP2 switch. Ugly stuff. Perhaps we could
> >just omit that here.
> >
>
> ok
>
> >>+- clocks: External clock to the sensor
> >>+- clock-frequency: Frequency of the external clock to the sensor
> >>+
> >>+
> >>+Optional properties
> >>+-------------------
> >>+
> >>+- reset-gpios: XSHUTDOWN GPIO
> >
> >I guess this should be mandatory.
> >
>
> yeah. Also, I will change xxx-lanes to optional
>
> >>+
> >>+
> >>+Endpoint node mandatory properties
> >>+----------------------------------
> >>+
> >>+- clock-lanes: <0>
> >>+- data-lanes: <1..n>
> >>+- remote-endpoint: A phandle to the bus receiver's endpoint node.
> >>+
> >>+
> >>+Example
> >>+-------
> >>+
> >>+&i2c3 {
> >>+ clock-frequency = <400000>;
> >>+
> >>+ cam1: camera@3e {
> >>+ compatible = "toshiba,et8ek8";
> >>+ reg = <0x3e>;
> >>+ vana-supply = <&vaux4>;
> >>+ clocks = <&isp 0>;
> >>+ clock-frequency = <9600000>;
> >>+ reset-gpio = <&gpio4 6 GPIO_ACTIVE_HIGH>; /* 102 */
> >>+ port {
> >>+ csi_cam1: endpoint {
> >>+ remote-endpoint = <&csi_out1>;
> >>+ };
> >>+ };
> >>+ };
> >>+};
> >
> >Please split the DT documentation from the driver.
> >
>
> Split it how? Send as series [patch 1] - driver, [patch 2] - doc?
Yes, please. As the ad5820, for instance.
>
> >I remember having discussed showing the module in DT with Sebastian but I
> >couldn't find the patches anywhere. We currently consider the lens and
> >sensor entirely separate, the module has not been shown in software as
> >there's been nothing to control it.
> >
>
> Not sure what am I supposed to do with that comment :)
Not necessarily anything. But I'd like to continue the discussion on the
topic. :-)
>
> >Sebastian: do you still have those patches around somewhere?
> >
> >>diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> >>index 993dc50..e964787 100644
> >>--- a/drivers/media/i2c/Kconfig
> >>+++ b/drivers/media/i2c/Kconfig
> >>@@ -629,6 +629,7 @@ config VIDEO_S5K5BAF
> >> camera sensor with an embedded SoC image signal processor.
> >>
> >> source "drivers/media/i2c/smiapp/Kconfig"
> >>+source "drivers/media/i2c/et8ek8/Kconfig"
> >>
> >> config VIDEO_S5C73M3
> >> tristate "Samsung S5C73M3 sensor support"
> >>diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> >>index 94f2c99..907b180 100644
> >>--- a/drivers/media/i2c/Makefile
> >>+++ b/drivers/media/i2c/Makefile
> >>@@ -2,6 +2,7 @@ msp3400-objs := msp3400-driver.o msp3400-kthreads.o
> >> obj-$(CONFIG_VIDEO_MSP3400) += msp3400.o
> >>
> >> obj-$(CONFIG_VIDEO_SMIAPP) += smiapp/
> >>+obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
> >> obj-$(CONFIG_VIDEO_CX25840) += cx25840/
> >> obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/
> >> obj-y += soc_camera/
> >>diff --git a/drivers/media/i2c/et8ek8/Kconfig b/drivers/media/i2c/et8ek8/Kconfig
> >>new file mode 100644
> >>index 0000000..1439936
> >>--- /dev/null
> >>+++ b/drivers/media/i2c/et8ek8/Kconfig
> >>@@ -0,0 +1,6 @@
> >>+config VIDEO_ET8EK8
> >>+ tristate "ET8EK8 camera sensor support"
> >>+ depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> >>+ ---help---
> >>+ This is a driver for the Toshiba ET8EK8 5 MP camera sensor.
> >>+ It is used for example in Nokia N900 (RX-51).
> >>diff --git a/drivers/media/i2c/et8ek8/Makefile b/drivers/media/i2c/et8ek8/Makefile
> >>new file mode 100644
> >>index 0000000..66d1b7d
> >>--- /dev/null
> >>+++ b/drivers/media/i2c/et8ek8/Makefile
> >>@@ -0,0 +1,2 @@
> >>+et8ek8-objs += et8ek8_mode.o et8ek8_driver.o
> >>+obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8.o
> >>diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> >>new file mode 100644
> >>index 0000000..1eaef78
> >>--- /dev/null
> >>+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> >>@@ -0,0 +1,1711 @@
> >>+/*
> >>+ * et8ek8_driver.c
> >>+ *
> >>+ * Copyright (C) 2008 Nokia Corporation
> >>+ *
> >>+ * Contact: Sakari Ailus <sakari.ailus@xxxxxx>
> >>+ * Tuukka Toivonen <tuukka.o.toivonen@xxxxxxxxx>
> >
> >tuukkat76@xxxxxxxxx
> >
>
> ok
>
> >>+ *
> >>+ * Based on code from Toni Leinonen <toni.leinonen@xxxxxxxxxx>.
> >>+ *
> >>+ * This driver is based on the Micron MT9T012 camera imager driver
> >>+ * (C) Texas Instruments.
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or
> >>+ * modify it under the terms of the GNU General Public License
> >>+ * version 2 as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful, but
> >>+ * WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >>+ * General Public License for more details.
> >>+ */
> >>+
> >>+#include <linux/clk.h>
> >>+#include <linux/delay.h>
> >>+#include <linux/i2c.h>
> >>+#include <linux/kernel.h>
> >>+#include <linux/module.h>
> >>+#include <linux/mutex.h>
> >>+#include <linux/gpio/consumer.h>
> >
> >Alphabetical order, please.
> >
>
> ok
>
> >>+#include <linux/regulator/consumer.h>
> >>+#include <linux/slab.h>
> >>+#include <linux/sort.h>
> >>+#include <linux/version.h>
> >
> >Is linux/version.h needed?
> >
>
> no, will remove it
>
> >>+#include <linux/v4l2-mediabus.h>
> >>+
> >>+#include <media/media-entity.h>
> >>+#include <media/v4l2-ctrls.h>
> >>+#include <media/v4l2-device.h>
> >>+#include <media/v4l2-subdev.h>
> >>+
> >>+#include "et8ek8_reg.h"
> >>+
> >>+#define ET8EK8_NAME "et8ek8"
> >>+#define ET8EK8_PRIV_MEM_SIZE 128
> >>+
> >>+#define ET8EK8_CID_USER_FRAME_WIDTH (V4L2_CID_USER_ET8EK8_BASE + 1)
> >>+#define ET8EK8_CID_USER_FRAME_HEIGHT (V4L2_CID_USER_ET8EK8_BASE + 2)
> >>+#define ET8EK8_CID_USER_VISIBLE_WIDTH (V4L2_CID_USER_ET8EK8_BASE + 3)
> >>+#define ET8EK8_CID_USER_VISIBLE_HEIGHT (V4L2_CID_USER_ET8EK8_BASE + 4)
> >>+#define ET8EK8_CID_USER_SENSITIVITY (V4L2_CID_USER_ET8EK8_BASE + 5)
> >
> >If you have custom controls,
> >
>
> hmm?
Yes, this is apparently left unfinished and it's a bit hard to grasp what
was the real meaning behind that comment. I agree.
What I meant to say that the controls would be better to be defined in a
header file. However, the interface to access the information shouldn't be
controls.
In this case, I believe the information is already provided to the user:
VIDIOC_SUBDEV_S_FMT is used to set the output format, and as none of the
modes use cropping.
In order to resolve this, I suggest dropping these controls.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
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
