Re: [PATCH v6 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 7/21/2016 17:13, Hans Verkuil wrote: On 07/21/2016 10:14 AM, Songjun Wu wrote: Add driver for the Image Sensor Controller. It manages incoming data from a parallel based CMOS/CCD sensor. It has an internal image processor, also integrates a triple channel direct memory access controller master interface. Signed-off-by: Songjun Wu --- Changes in v6: None Changes in v5: - Modify the macro definition and the related code. Changes in v4: - Modify the isc clock code since the dt is changed. Changes in v3: - Add pm runtime feature. - Modify the isc clock code since the dt is changed. Changes in v2: - Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API" in Kconfig file. - Correct typos and coding style according to Laurent's remarks - Delete the loop while in 'isc_clk_enable' function. - Replace 'hsync_active', 'vsync_active' and 'pclk_sample' with 'pfe_cfg0' in struct isc_subdev_entity. - Add the code to support VIDIOC_CREATE_BUFS in 'isc_queue_setup' function. - Invoke isc_config to configure register in 'isc_start_streaming' function. - Add the struct completion 'comp' to synchronize with the frame end interrupt in 'isc_stop_streaming' function. - Check the return value of the clk_prepare_enable in 'isc_open' function. - Set the default format in 'isc_open' function. - Add an exit condition in the loop while in 'isc_config'. - Delete the hardware setup operation in 'isc_set_format'. - Refuse format modification during streaming in 'isc_s_fmt_vid_cap' function. - Invoke v4l2_subdev_alloc_pad_config to allocate and initialize the pad config in 'isc_async_complete' function. - Remove the '.owner = THIS_MODULE,' in atmel_isc_driver. - Replace the module_platform_driver_probe() with module_platform_driver(). drivers/media/platform/Kconfig|1 + drivers/media/platform/Makefile |2 + drivers/media/platform/atmel/Kconfig |9 + drivers/media/platform/atmel/Makefile |1 + drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++ drivers/media/platform/atmel/atmel-isc.c | 1554 + 6 files changed, 1732 insertions(+) create mode 100644 drivers/media/platform/atmel/Kconfig create mode 100644 drivers/media/platform/atmel/Makefile create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h create mode 100644 drivers/media/platform/atmel/atmel-isc.c diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index f25344b..b23db17 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -111,6 +111,7 @@ source "drivers/media/platform/s5p-tv/Kconfig" source "drivers/media/platform/am437x/Kconfig" source "drivers/media/platform/xilinx/Kconfig" source "drivers/media/platform/rcar-vin/Kconfig" +source "drivers/media/platform/atmel/Kconfig" config VIDEO_TI_CAL tristate "TI CAL (Camera Adaptation Layer) driver" diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile index 21771c1..37b6c75 100644 --- a/drivers/media/platform/Makefile +++ b/drivers/media/platform/Makefile @@ -58,6 +58,8 @@ obj-$(CONFIG_VIDEO_XILINX)+= xilinx/ obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin/ +obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel/ + ccflags-y += -I$(srctree)/drivers/media/i2c obj-$(CONFIG_VIDEO_MEDIATEK_VPU) += mtk-vpu/ diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig new file mode 100644 index 000..867dca2 --- /dev/null +++ b/drivers/media/platform/atmel/Kconfig @@ -0,0 +1,9 @@ +config VIDEO_ATMEL_ISC + tristate "ATMEL Image Sensor Controller (ISC) support" + depends on VIDEO_V4L2 && COMMON_CLK && VIDEO_V4L2_SUBDEV_API && HAS_DMA + depends on ARCH_AT91 || COMPILE_TEST + select VIDEOBUF2_DMA_CONTIG + select REGMAP_MMIO + help + This module makes the ATMEL Image Sensor Controller available + as a v4l2 device. \ No newline at end of file diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile new file mode 100644 index 000..9d7c999 --- /dev/null +++ b/drivers/media/platform/atmel/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h new file mode 100644 index 000..00c4497 --- /dev/null +++ b/drivers/media/platform/atmel/atmel-isc-regs.h @@ -0,0 +1,165 @@ +#ifndef __ATMEL_ISC_REGS_H +#define __ATMEL_ISC_REGS_H + +#include + +/* ISC Control Enable Register 0 */ +#define ISC_CTRLEN 0x + +/* ISC Control Disable Register 0 */ +#define ISC_CTRLDIS 0x0004 + +/* ISC Control Status Register 0 */ +#define ISC_CTRLSR 0x0008 + +#define ISC_CTRL_CAPTURE BIT(0) +#define ISC_CTRL_UPPRO BIT(1) +#define ISC_CTRL_HISREQBIT(2) +#define ISC_CTRL_HISCLRBIT(3) + +/* ISC Parallel Front E
Re: [PATCH v6 0/2] [media] atmel-isc: add driver for Atmel ISC
On 7/21/2016 16:41, Hans Verkuil wrote: On 07/21/2016 10:14 AM, Songjun Wu wrote: The Image Sensor Controller driver includes two parts. 1) Driver code to implement the ISC function. 2) Device tree binding documentation, it describes how to add the ISC in device tree. Test result with v4l-utils 1.10.1 Please compile from the v4l-utils repository. The version you used here is out-of-date. I continually add new tests, so always compile the latest version from the repo. OK, I will compile the latest version from the v4l-utils repository. Thank you. Regards, Hans Driver Info: Driver name : atmel_isc Card type : Atmel Image Sensor Controller Bus info : platform:atmel_isc f0008000.isc Driver version: 4.7.0 Capabilities : 0x8421 Video Capture Streaming Extended Pix Format Device Capabilities Device Caps : 0x0421 Video Capture Streaming Extended Pix Format Compliance test for device /dev/video0 (not using libv4l2): Required ioctls: test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second video open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Test input 0: Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK Test input 0: Stream using all formats: test MMAP for Format BA81, Frame Size 640x480: Stride 640, Field None: OK test MMAP for Format YUYV, Frame Size 640x480: Stride 1280, Field None: OK Total: 44, Succeeded: 44, Failed: 0, Warnings: 0 Changes in v6: - Add "iscck" and "gck" to clock-names. Changes in v5: - Modify the macro definition and the related code. - Add clock-output-names. Changes in v4: - Modify the isc clock code since the dt is changed. - Remove the isc clock nodes. Changes in v3: - Add pm runtime feature. - Modify the isc clock code since the dt is changed. - Remove the 'atmel,sensor-preferred'. - Modify the isc clock node according to the Rob's remarks. Changes in v2: - Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API" in Kconfig file. - Correct typos and coding style according to Laurent's remarks - Delete the loop while in 'isc_clk_enable' function. - Replace 'hsync_active', 'vsync_active' and 'pclk_sample' with 'pfe_cfg0' in struct isc_subdev_entity. - Add the code to support VIDIOC_CREATE_BUFS in 'isc_queue_setup' function. - Invoke isc_config to configure register in 'isc_start_streaming' function. - Add the struct completion 'comp' to syn
Re: [PATCH v7 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
On 7/30/2016 05:44, Rob Herring wrote: On Fri, Jul 29, 2016 at 03:54:08PM +0800, Songjun Wu wrote: DT binding documentation for ISC driver. Signed-off-by: Songjun Wu --- Changes in v7: None Changes in v6: - Add "iscck" and "gck" to clock-names. Changes in v5: - Add clock-output-names. Changes in v4: - Remove the isc clock nodes. Changes in v3: - Remove the 'atmel,sensor-preferred'. - Modify the isc clock node according to the Rob's remarks. Changes in v2: - Remove the unit address of the endpoint. - Add the unit address to the clock node. - Avoid using underscores in node names. - Drop the "0x" in the unit address of the i2c node. - Modify the description of 'atmel,sensor-preferred'. - Add the description for the ISC internal clock. .../devicetree/bindings/media/atmel-isc.txt| 65 ++ 1 file changed, 65 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt Please add acks when posting new versions. Rob Hi Rob, Thank you for your reminder. Should I Add 'Acked-by: Rob Herring ' behind 'Signed-off-by: Songjun Wu '? Should I resend this patch?
Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 8/8/2016 17:37, Hans Verkuil wrote:
On 08/03/2016 10:08 AM, Songjun Wu wrote:
Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.
Signed-off-by: Songjun Wu
---
Changes in v8:
- Power on the sensor on the first open in function
'isc_open'.
- Power off the sensor on the last release in function
'isc_release'.
- Remove the switch of the pipeline.
Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.
Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.
Changes in v4:
- Modify the isc clock code since the dt is changed.
Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.
Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
in Kconfig file.
- Correct typos and coding style according to Laurent's remarks
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
'isc_queue_setup' function.
- Invoke isc_config to configure register in
'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
module_platform_driver().
drivers/media/platform/Kconfig|1 +
drivers/media/platform/Makefile |2 +
drivers/media/platform/atmel/Kconfig |9 +
drivers/media/platform/atmel/Makefile |1 +
drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++
drivers/media/platform/atmel/atmel-isc.c | 1503 +
6 files changed, 1681 insertions(+)
create mode 100644 drivers/media/platform/atmel/Kconfig
create mode 100644 drivers/media/platform/atmel/Makefile
create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
create mode 100644 drivers/media/platform/atmel/atmel-isc.c
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
+static int isc_set_default_fmt(struct isc_device *isc)
+{
+ u32 index = isc->num_user_formats - 1;
Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.
Accept, thank you.
The raw format is listed on the beginning, in order to select non-raw
format, the last format is picked. But it's not very important, it's
better to use the beginning of the format list.
+ struct v4l2_format f = {
+ .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+ .fmt.pix = {
+ .width = VGA_WIDTH,
+ .height = VGA_HEIGHT,
+ .field = V4L2_FIELD_NONE,
+ .pixelformat= isc->user_formats[index]->fourcc,
+ },
+ };
+
+ return isc_set_fmt(isc, &f);
+}
+
+static int isc_open(struct file *file)
+{
+ struct isc_device *isc = video_drvdata(file);
+ struct v4l2_subdev *sd = isc->current_subdev->sd;
+ int ret;
+
+ if (mutex_lock_interruptible(&isc->lock))
+ return -ERESTARTSYS;
+
+ ret = v4l2_fh_open(file);
+ if (ret < 0)
+ goto unlock;
+
+ if (!v4l2_fh_is_singular_file(file))
+ goto unlock;
+
+ ret = v4l2_subdev_call(sd, core, s_power, 1);
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ goto unlock;
+
+ ret = isc_set_default_fmt(isc);
This doesn't belong here, this needs to be done in isc_async_complete().
Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.
Only on the first open, the default format will be set. If it's done in
isc_async_complete(), there is a prob
Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 8/8/2016 17:56, Hans Verkuil wrote:
On 08/08/2016 11:37 AM, Hans Verkuil wrote:
On 08/03/2016 10:08 AM, Songjun Wu wrote:
Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.
Signed-off-by: Songjun Wu
---
Changes in v8:
- Power on the sensor on the first open in function
'isc_open'.
- Power off the sensor on the last release in function
'isc_release'.
- Remove the switch of the pipeline.
Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.
Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.
Changes in v4:
- Modify the isc clock code since the dt is changed.
Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.
Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
in Kconfig file.
- Correct typos and coding style according to Laurent's remarks
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
'isc_queue_setup' function.
- Invoke isc_config to configure register in
'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
module_platform_driver().
drivers/media/platform/Kconfig|1 +
drivers/media/platform/Makefile |2 +
drivers/media/platform/atmel/Kconfig |9 +
drivers/media/platform/atmel/Makefile |1 +
drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++
drivers/media/platform/atmel/atmel-isc.c | 1503 +
6 files changed, 1681 insertions(+)
create mode 100644 drivers/media/platform/atmel/Kconfig
create mode 100644 drivers/media/platform/atmel/Makefile
create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
create mode 100644 drivers/media/platform/atmel/atmel-isc.c
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
+static int isc_set_default_fmt(struct isc_device *isc)
+{
+ u32 index = isc->num_user_formats - 1;
Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.
+ struct v4l2_format f = {
+ .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+ .fmt.pix = {
+ .width = VGA_WIDTH,
+ .height = VGA_HEIGHT,
+ .field = V4L2_FIELD_NONE,
+ .pixelformat= isc->user_formats[index]->fourcc,
+ },
+ };
+
+ return isc_set_fmt(isc, &f);
+}
+
+static int isc_open(struct file *file)
+{
+ struct isc_device *isc = video_drvdata(file);
+ struct v4l2_subdev *sd = isc->current_subdev->sd;
+ int ret;
+
+ if (mutex_lock_interruptible(&isc->lock))
+ return -ERESTARTSYS;
+
+ ret = v4l2_fh_open(file);
+ if (ret < 0)
+ goto unlock;
+
+ if (!v4l2_fh_is_singular_file(file))
+ goto unlock;
+
+ ret = v4l2_subdev_call(sd, core, s_power, 1);
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ goto unlock;
+
+ ret = isc_set_default_fmt(isc);
This doesn't belong here, this needs to be done in isc_async_complete().
Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.
Actually, you do need to set the format here since here is where you turn on
the sensor power, but it should be the current format, not the default format.
And in isc_set_default_fmt I recommend that you call the try fmt of the subdev
in order to let the subdev adjust the p
Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code
On 9/3/2015 19:37, Mark Brown wrote:
On Tue, Sep 01, 2015 at 01:41:40PM +0800, Songjun Wu wrote:
+static const char * const mono_text[] = {
+ "stereo", "mono"
+};
+
+static SOC_ENUM_SINGLE_DECL(classd_mono_enum,
+ CLASSD_INTPMR, CLASSD_INTPMR_MONO_SHIFT,
+ mono_text);
This looks like it should be a simple Switch control called something
like "Stereo Switch" or "Mono Switch".
Thank you, the code will be modified as you suggest.
+static const char * const deemp_text[] = {
+ "disabled", "enabled"
+};
+
+static SOC_ENUM_SINGLE_DECL(classd_deemp_enum,
+ CLASSD_INTPMR, CLASSD_INTPMR_DEEMP_SHIFT,
+ deemp_text);
Similarly this looks like it should be "Deemph Switch".
Yes, it also will be modified.
+static const char * const eqcfg_bass_text[] = {
+ "-12 dB", "-6 dB", "0 dB", "+6 dB", "+12 dB"
+};
+static const unsigned int eqcfg_bass_value[] = {
+ CLASSD_INTPMR_EQCFG_B_CUT_12,
+ CLASSD_INTPMR_EQCFG_B_CUT_6, CLASSD_INTPMR_EQCFG_FLAT,
+ CLASSD_INTPMR_EQCFG_B_BOOST_6, CLASSD_INTPMR_EQCFG_B_BOOST_12
+};
This should be a Volume control with TLV information, as should the
following few controls.
The Volume control with TLV information is not suitable for this case.
Bass, Medium, and treble are mutually exclusive.
So I think the SOC_ENUM control is suitable for this case.
The register layout is not very good,
The register is defined as below.
• EQCFG: Equalization Selection
Value Name Description
0 FLAT Flat Response
1 BBOOST12 Bass boost +12 dB
2 BBOOST6Bass boost +6 dB
3 BCUT12 Bass cut -12 dB
4 BCUT6 Bass cut -6 dB
5 MBOOST3Medium boost +3 dB
6 MBOOST8Medium boost +8 dB
7 MCUT3 Medium cut -3 dB
8 MCUT8 Medium cut -8 dB
9 TBOOST12 Treble boost +12 dB
10TBOOST6Treble boost +6 dB
11TCUT12 Treble cut -12 dB
12TCUT6 Treble cut -6 dB
+static const struct snd_kcontrol_new atmel_classd_snd_controls[] = {
+SOC_SINGLE_TLV("Left Volume", CLASSD_INTPMR,
+ CLASSD_INTPMR_ATTL_SHIFT, 78, 1, classd_digital_tlv),
+
+SOC_SINGLE_TLV("Right Volume", CLASSD_INTPMR,
+ CLASSD_INTPMR_ATTR_SHIFT, 78, 1, classd_digital_tlv),
This should be a single stereo control rather than separate left and
right controls.
Since the classD IP defines two register fields to control left volume
and right volume respectively, I think it's better to provide two
controls to user.
+static const char * const pwm_type[] = {
+ "single-ended", "differential"
+};
The normal style for ALSA controls is to capitalise strings so
"Single ended" and "Differential".
Accept. It will be modified.
+ if (pdata->non_overlap_enable) {
+ val |= (CLASSD_MR_NON_OVERLAP_EN
+ << CLASSD_MR_NON_OVERLAP_SHIFT);
+
+ mask |= CLASSD_MR_NOVR_VAL_MASK;
+ switch (pdata->non_overlap_time) {
+ case 5:
+ val |= (CLASSD_MR_NOVR_VAL_5NS
+ << CLASSD_MR_NOVR_VAL_SHIFT);
+ break;
+ case 10:
+ val |= (CLASSD_MR_NOVR_VAL_10NS
+ << CLASSD_MR_NOVR_VAL_SHIFT);
+ break;
+ case 15:
+ val |= (CLASSD_MR_NOVR_VAL_15NS
+ << CLASSD_MR_NOVR_VAL_SHIFT);
+ break;
+ case 20:
+ val |= (CLASSD_MR_NOVR_VAL_20NS
+ << CLASSD_MR_NOVR_VAL_SHIFT);
+ break;
+ default:
+ val |= (CLASSD_MR_NOVR_VAL_10NS
+ << CLASSD_MR_NOVR_VAL_SHIFT);
+ break;
+ }
I'd expect at least a warning if the user trys to specify an invalid
value (if they didn't specify any value then I'd expect the DT parsing
function to assign the default value).
Accept. A warning will be added if the user trys to sepcify an invalid
value. This function is optional, So if the user did not specify any
value, this function will be disabled.
+static struct regmap *atmel_classd_codec_get_remap(struct device *dev)
+{
+ struct atmel_classd *dd = dev_get_drvdata(dev);
+
+ return dd->regmap;
+}
Can you just use dev_get_regmap()?
Thank you for your reminder, it's better to use dev_get_regmap().
The code will be modified.
+static int atmel_classd_codec_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *codec_dai)
+{
+ struct atmel_classd *dd = snd_soc_dai_get_drvdata(codec_dai);
+
+ clk_prepare_enable(dd->aclk);
+ clk_prepare_enable(dd->gclk);
Should check for errors here.
Accept.
+ dev_info(dev,
+ "Atmel Class D Amplifier (CLASSD) device at 0
Re: [PATCH 2/2] ASoC: atmel-classd: DT binding for Class D audio amplifier driver
On 9/3/2015 19:43, Mark Brown wrote:
On Tue, Sep 01, 2015 at 01:41:41PM +0800, Songjun Wu wrote:
+classd: classd@fc048000 {
+ compatible = "atmel,sama5d2-classd";
+ reg = <0xfc048000 0x100>;
+ interrupts = <59 IRQ_TYPE_LEVEL_HIGH 7>;
+ dmas = <&dma0
+ (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
+ | AT91_XDMAC_DT_PERID(47))>;
+ dma-names = "tx";
+ clocks = <&classd_clk>, <&classd_gclk>, <&audio_pll_pmc>;
+ clock-names = "pclk", "gclk", "aclk";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_classd_default>;
+ atmel,pwm-type = "diff";
+ atmel,non-overlap-time = <10>;
+};
+Example:
+sound {
+ compatible = "atmel,asoc-classd";
+
+ atmel,model = "classd @ SAMA5D2-Xplained";
+ atmel,audio-platform = <&classd>;
+ atmel,audio-cpu-dai-name = "fc048000.classd";
+ atmel,audio-codec = <&classd>;
+};
Why is this a separate DT node? It seems that this IP is entirely self
contained so I'm not clear why we need a separate node for the card, the
card is usually a separate node because it ties together multiple
different devices in the system but that's not the case here.
The classD can finish the audio function without other devices.
But I want to reuse the code in ASoC, leave many things(like creating
PCM, DMA operations) to ASoC, then the driver can only focus on how to
configure classD.
The classD IP is divided to tree parts logically, platform, CPU dai,
and codec, and these parts are registered to ASoC.
This separate DT node is needed in ASoC, ties these tree parts in ClassD.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ASoC: atmel-classd: DT binding for Class D audio amplifier driver
On 9/8/2015 00:25, Mark Brown wrote: On Sun, Sep 06, 2015 at 05:44:30PM +0800, Wu, Songjun wrote: On 9/3/2015 19:43, Mark Brown wrote: Why is this a separate DT node? It seems that this IP is entirely self contained so I'm not clear why we need a separate node for the card, the card is usually a separate node because it ties together multiple different devices in the system but that's not the case here. The classD can finish the audio function without other devices. But I want to reuse the code in ASoC, leave many things(like creating PCM, DMA operations) to ASoC, then the driver can only focus on how to configure classD. The classD IP is divided to tree parts logically, platform, CPU dai, and codec, and these parts are registered to ASoC. This separate DT node is needed in ASoC, ties these tree parts in ClassD. Sure, there's no problem at all having that structure in software but it should be possible to do this without having to represent this structure in DT. It should be possible to register the card at the same time as the rest of the components rather than needing the separate device in the DT. Do you mean using a single entry in the DT for the whole classD system and instantiate ASoC components from it. For now, there are two entry, they could be combined to one entry. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code
On 9/8/2015 00:23, Mark Brown wrote:
On Sun, Sep 06, 2015 at 05:44:21PM +0800, Wu, Songjun wrote:
On 9/3/2015 19:37, Mark Brown wrote:
On Tue, Sep 01, 2015 at 01:41:40PM +0800, Songjun Wu wrote:
+static const char * const eqcfg_bass_text[] = {
+ "-12 dB", "-6 dB", "0 dB", "+6 dB", "+12 dB"
+};
+static const unsigned int eqcfg_bass_value[] = {
+ CLASSD_INTPMR_EQCFG_B_CUT_12,
+ CLASSD_INTPMR_EQCFG_B_CUT_6, CLASSD_INTPMR_EQCFG_FLAT,
+ CLASSD_INTPMR_EQCFG_B_BOOST_6, CLASSD_INTPMR_EQCFG_B_BOOST_12
+};
This should be a Volume control with TLV information, as should the
following few controls.
The Volume control with TLV information is not suitable for this case.
Bass, Medium, and treble are mutually exclusive.
So I think the SOC_ENUM control is suitable for this case.
The register layout is not very good,
The register is defined as below.
• EQCFG: Equalization Selection
Value Name Description
0 FLAT Flat Response
1 BBOOST12 Bass boost +12 dB
2 BBOOST6Bass boost +6 dB
3 BCUT12 Bass cut -12 dB
4 BCUT6 Bass cut -6 dB
5 MBOOST3Medium boost +3 dB
6 MBOOST8Medium boost +8 dB
7 MCUT3 Medium cut -3 dB
8 MCUT8 Medium cut -8 dB
9 TBOOST12 Treble boost +12 dB
10TBOOST6Treble boost +6 dB
11TCUT12 Treble cut -12 dB
12TCUT6 Treble cut -6 dB
OK, so that's not actually what the code was doing - it had separate
enums for bass, mid and treble. If you make this a single enum with all
the above options in it that seems like the best way of handling things.
A single enum seems not very friendly to user, there are tree EQs, bass,
medium and treble.
So I create tree enum controls to control three EQs.
The 'get' function is replaced by 'classd_get_eq_enum', if user operates
one of the tree EQ controls, the other two EQs will show 0 dB.
+static const struct snd_kcontrol_new atmel_classd_snd_controls[] = {
+SOC_SINGLE_TLV("Left Volume", CLASSD_INTPMR,
+ CLASSD_INTPMR_ATTL_SHIFT, 78, 1, classd_digital_tlv),
+
+SOC_SINGLE_TLV("Right Volume", CLASSD_INTPMR,
+ CLASSD_INTPMR_ATTR_SHIFT, 78, 1, classd_digital_tlv),
This should be a single stereo control rather than separate left and
right controls.
Since the classD IP defines two register fields to control left volume and
right volume respectively, I think it's better to provide two controls to
user.
No, this is really common, we combine them in Linux to present a
consistent interface to userspace.
I think carefully, your suggestion is reasonable.
The code will be modified, combine the left and right to a single stereo
control.
Thank you.
+ dev_info(dev,
+ "Atmel Class D Amplifier (CLASSD) device at 0x%p (irq %d)\n",
+ io_base, dd->irq);
This is a bit noisy and not really based on interaction with the
hardware... dev_dbg() seems better.
This information will occur only once when linux kernel starts.
It shows the classD is loaded to linux kernel.
I think it's better to provide more information to user.
This stuff all adds up and since it'll go out on the console by default
it both makes things more noisy and slows down boot - printing on the
serial port isn't free. If we want to have this sort of information we
printed we should really do it in the driver core so it appears
consistently for all devices rather than having individual code in each
driver.
Accept, the code will be modified to dev_dbg().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code
On 9/8/2015 20:23, Mark Brown wrote: On Tue, Sep 08, 2015 at 05:36:01PM +0800, Wu, Songjun wrote: On 9/8/2015 00:23, Mark Brown wrote: OK, so that's not actually what the code was doing - it had separate enums for bass, mid and treble. If you make this a single enum with all the above options in it that seems like the best way of handling things. A single enum seems not very friendly to user, there are tree EQs, bass, medium and treble. So I create tree enum controls to control three EQs. The 'get' function is replaced by 'classd_get_eq_enum', if user operates one of the tree EQ controls, the other two EQs will show 0 dB. If you want to have three controls you need to write code so that the user can only change one of them from 0dB at once, returning an error otherwise. That was why it looked like they were three separate controls. If user operates two or tree controls at the same time, for my understanding, these operations are serial actually in kernel, not parallel, and the last operation will be effective. I only write the function 'classd_get_eq_enum' to get the enumeration value, if user changes one of controls, the other controls will get 0dB. Is my understanding correct? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ASoC: atmel-classd: DT binding for Class D audio amplifier driver
On 9/8/2015 20:23, Mark Brown wrote: On Tue, Sep 08, 2015 at 05:36:13PM +0800, Wu, Songjun wrote: On 9/8/2015 00:25, Mark Brown wrote: Sure, there's no problem at all having that structure in software but it should be possible to do this without having to represent this structure in DT. It should be possible to register the card at the same time as the rest of the components rather than needing the separate device in the DT. Do you mean using a single entry in the DT for the whole classD system and instantiate ASoC components from it. For now, there are two entry, they could be combined to one entry. Yes, exactly. Accept. the two entries will be combined to one entry. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 15/18] serial: intel: Support more platform
On 8/3/2018 1:57 PM, Greg Kroah-Hartman wrote: On Fri, Aug 03, 2018 at 11:02:34AM +0800, Songjun Wu wrote: Support more platform. Signed-off-by: Songjun Wu --- Your changelog text makes no sense, sorry. Thanks for your comment. I will describe it more clearly.
Re: [PATCH v2 14/18] serial: intel: Add CCF support
On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote: On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote: Previous implementation uses platform-dependent API to get the clock. Those functions are not available for other SoC which uses the same IP. The CCF (Common Clock Framework) have an abstraction based APIs for clock. In future, the platform specific code will be removed when the legacy soc use CCF as well. Change to use CCF APIs to get clock and rate. So that different SoCs can use the same driver. Signed-off-by: Songjun Wu --- Changes in v2: None drivers/tty/serial/lantiq.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c index 36479d66fb7c..35518ab3a80d 100644 --- a/drivers/tty/serial/lantiq.c +++ b/drivers/tty/serial/lantiq.c @@ -26,7 +26,9 @@ #include #include +#ifdef CONFIG_LANTIQ #include +#endif That is never how you do this in Linux, you know better. Please go and get this patchset reviewed and signed-off-by from other internal Intel kernel developers before resending it next time. It is their job to find and fix your basic errors like this, not ours. Thank you for your comment. Actually, we have discussed this issue internally. We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit message in "[PATCH v2 08/18] serial: intel: Get serial id from dts". Please refer the commit message below. "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC macro is defined in lantiq_soc.h. lantiq_soc.h is in arch path for legacy product support. arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h If "#ifdef preprocessor" is changed to "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled, code using LTQ_EARLY_ASC is compiled. Compilation will fail for no LTQ_EARLY_ASC defined.
Re: [PATCH v2 14/18] serial: intel: Add CCF support
On 8/5/2018 5:03 AM, Arnd Bergmann wrote:
On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
wrote:
On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.
1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.
I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file. Checkpatch might have even
warned you about it, right?
So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel. You are not unique here.
The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.
Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.
>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as
clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?
Arnd
We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.
To support CCF in legacy product is another topic, is not included in
this patch.
The implementation is as following：
#ifdef CONFIG_LANTIQ
#include
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0
static inline struct clk *clk_get_fpi(void)
{
return NULL;
}
#endif
Re: [PATCH v2 15/18] serial: intel: Support more platform
On 8/5/2018 4:37 PM, Christoph Hellwig wrote: The subject line also seems odd, your are changing deps on the lantiq driver, not some (nonexistent) intel serial driver. Your suggestion is reasonable, it will be changed to "serial: lantiq".
Re: [PATCH v2 14/18] serial: intel: Add CCF support
On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:
Hi Songjun,
On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun wrote:
On 8/5/2018 5:03 AM, Arnd Bergmann wrote:
On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
wrote:
On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.
1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.
I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file. Checkpatch might have even
warned you about it, right?
So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel. You are not unique here.
The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.
Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.
>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as
clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?
Arnd
We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.
To support CCF in legacy product is another topic, is not included in
this patch.
The implementation is as following：
#ifdef CONFIG_LANTIQ
#include
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0
static inline struct clk *clk_get_fpi(void)
{
return NULL;
}
#endif
Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?
Gr{oetje,eeting}s,
Geert
The reason we add a new head file is also for two macros(LTQ_EARLY_ASC
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two
macro for new product.
Re: [PATCH v2 08/18] serial: intel: Get serial id from dts
On 8/3/2018 1:43 PM, Greg Kroah-Hartman wrote: On Fri, Aug 03, 2018 at 11:02:27AM +0800, Songjun Wu wrote: Get serial id from dts. "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC macro is defined in lantiq_soc.h. lantiq_soc.h is in arch path for legacy product support. arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h If "#ifdef preprocessor" is changed to "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled, code using LTQ_EARLY_ASC is compiled. Compilation will fail for no LTQ_EARLY_ASC defined. Signed-off-by: Songjun Wu --- Changes in v2: None drivers/tty/serial/lantiq.c | 19 +++ 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c index 044128277248..836ca51460f2 100644 --- a/drivers/tty/serial/lantiq.c +++ b/drivers/tty/serial/lantiq.c @@ -6,6 +6,7 @@ * Copyright (C) 2007 Felix Fietkau * Copyright (C) 2007 John Crispin * Copyright (C) 2010 Thomas Langer, + * Copyright (C) 2018 Intel Corporation. Your changes here do not warrent the addition of a copyright line, don't you agree? If not, please get a signed-off-by from your corporate lawyer who does this this is warrented when you resend this patch. thanks, greg k-h Thanks. The copyright line will be removed when we resend this patch.
Re: [PATCH v2 14/18] serial: intel: Add CCF support
On 8/6/2018 5:29 PM, Geert Uytterhoeven wrote:
Hi Songjun,
On Mon, Aug 6, 2018 at 10:58 AM Wu, Songjun wrote:
On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:
On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun wrote:
On 8/5/2018 5:03 AM, Arnd Bergmann wrote:
On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
wrote:
On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.
1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.
I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file. Checkpatch might have even
warned you about it, right?
So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel. You are not unique here.
The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.
Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.
>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as
clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?
Arnd
We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.
To support CCF in legacy product is another topic, is not included in
this patch.
The implementation is as following：
#ifdef CONFIG_LANTIQ
#include
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0
static inline struct clk *clk_get_fpi(void)
{
return NULL;
}
#endif
Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?
The reason we add a new head file is also for two macros(LTQ_EARLY_ASC
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two
macro for new product.
No you don't. The line number should not be obtained by comparing the
resource address with a hardcoded base address.
This is the previous code. Now the line number is obtained from dts.
We keep this code for the compatibility.
Referring to the conditional-compilation part in coding-style,
We add a header file to avoid using “#ifdef” in C file.
Perhaps the override of port->line should just be removed, as IIRC, the serial
core has already filled in that field with the (next available) line number?
Gr{oetje,eeting}s,
Geert
Re: [PATCH v2 08/18] serial: intel: Get serial id from dts
On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:
Hi Songjun,
On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu wrote:
Get serial id from dts.
"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.
arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.
Signed-off-by: Songjun Wu
Thanks for your patch!
@@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
return -ENODEV;
}
- /* check if this is the console port */
- if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
- line = 1;
+ /* get serial id */
+ line = of_alias_get_id(node, "serial");
+ if (line < 0) {
+#ifdef CONFIG_LANTIQ
+ if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
+ line = 0;
+ else
+ line = 1;
+#else
+ dev_err(&pdev->dev, "failed to get alias id, errno %d\n", line);
+ return line;
Please note that not providing a fallback here makes life harder when using
DT overlays.
See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances") for background info.
Thanks for your comment.
The logic in commit 7678f4c20fa7670f is not suitable here.
We need to know which serial instance is used for console.
We cannot use dynamic serial instance here.
Re: [PATCH v2 08/18] serial: intel: Get serial id from dts
On 8/8/2018 4:33 PM, Geert Uytterhoeven wrote:
Hi Songjun,
On Wed, Aug 8, 2018 at 6:05 AM Wu, Songjun wrote:
On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:
On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu wrote:
Get serial id from dts.
"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.
arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.
Signed-off-by: Songjun Wu
Thanks for your patch!
@@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
return -ENODEV;
}
- /* check if this is the console port */
- if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
- line = 1;
+ /* get serial id */
+ line = of_alias_get_id(node, "serial");
+ if (line < 0) {
+#ifdef CONFIG_LANTIQ
+ if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
+ line = 0;
+ else
+ line = 1;
+#else
+ dev_err(&pdev->dev, "failed to get alias id, errno %d\n", line);
+ return line;
Please note that not providing a fallback here makes life harder when using
DT overlays.
See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances") for background info.
Thanks for your comment.
The logic in commit 7678f4c20fa7670f is not suitable here.
We need to know which serial instance is used for console.
We cannot use dynamic serial instance here.
Why does the driver need to use which serial instance is used for the console?
Hardcoding that is not an option, as the board DTS may specify the console using
chosen/stdout-path.
In legacy platform in open source, it only defined asc1 in dts.
There's no asc0 in legacy dts.While in the new platform, asc0
is defined in dts. There's no asc1 in new platform dts.
To avoid hard code in driver, alias serial0 is used to unified
driver code. Actually only one serial is supported in SoC.
aliases {
serial0 = &asc0;
};
chosen {
bootargs = "earlycon clk_ignore_unused";
stdout-path = "serial0";
};
Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()
On 6/14/2018 6:07 PM, Arnd Bergmann wrote: On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu wrote: Previous implementation uses platform-dependent functions ltq_w32()/ltq_r32() to access registers. Those functions are not available for other SoC which uses the same IP. Change to OS provided readl()/writel() and readb()/writeb(), so that different SoCs can use the same driver. Signed-off-by: Songjun Wu Are there any big-endian machines using this driver? The original definition of ltq_r32() uses non-byteswapping __raw_readl() etc, which suggests that the registers might be wired up in a way that matches the CPU endianess (this is usally a bad idea in hardware design, but nothing we can influence in the OS). When you change it to readl(), that will breaks all machines that rely on the old behavior on big-endian kernels. Arnd It will not break existing big-endian SoC as SWAP_IO_SPACE is disabled. Disable SWAP_IO_SPACE will not impact ltq_r32 as it uses non-byte swapping __raw_readl() and it makes readl work in big-endian kernel too. The old Lantiq platform enable SWAP_IO_SPACE to support PCI as it's a little-endian bus plus PCI TX/RX swap enable impacted both data and control path. Alternatively PCI device driver has to do endian swap, It is better to let PCI device driver to do endian swap instead because SWAP_IO_SPACE is global wide macro. Once we set it, other peripheral such as USB has to change its register access as well
Re: [PATCH 1/7] MIPS: dts: Add aliases node for lantiq danube serial
On 6/14/2018 6:03 PM, Arnd Bergmann wrote:
On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu wrote:
Previous implementation uses a hard-coded register value to check if
the current serial entity is the console entity.
Now the lantiq serial driver uses the aliases for the index of the
serial port.
The lantiq danube serial dts are updated with aliases to support this.
Signed-off-by: Songjun Wu
---
arch/mips/boot/dts/lantiq/danube.dtsi | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi
b/arch/mips/boot/dts/lantiq/danube.dtsi
index 2dd950181f8a..7a9e15da6bd0 100644
--- a/arch/mips/boot/dts/lantiq/danube.dtsi
+++ b/arch/mips/boot/dts/lantiq/danube.dtsi
@@ -4,6 +4,10 @@
#size-cells = <1>;
compatible = "lantiq,xway", "lantiq,danube";
+ aliases {
+ serial0 = &asc1;
+ };
+
You generally want the aliases to be part of the board specific file,
not every board numbers their serial ports in the same way.
Arnd
In this chip only asc1 can be used as console, so serial0 is defined in
chip specific file.
Re: [PATCH 1/7] MIPS: dts: Add aliases node for lantiq danube serial
On 6/18/2018 6:59 PM, Arnd Bergmann wrote:
On Mon, Jun 18, 2018 at 11:42 AM, Wu, Songjun
wrote:
On 6/14/2018 6:03 PM, Arnd Bergmann wrote:
On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu
wrote:
Previous implementation uses a hard-coded register value to check if
the current serial entity is the console entity.
Now the lantiq serial driver uses the aliases for the index of the
serial port.
The lantiq danube serial dts are updated with aliases to support this.
Signed-off-by: Songjun Wu
---
arch/mips/boot/dts/lantiq/danube.dtsi | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi
b/arch/mips/boot/dts/lantiq/danube.dtsi
index 2dd950181f8a..7a9e15da6bd0 100644
--- a/arch/mips/boot/dts/lantiq/danube.dtsi
+++ b/arch/mips/boot/dts/lantiq/danube.dtsi
@@ -4,6 +4,10 @@
#size-cells = <1>;
compatible = "lantiq,xway", "lantiq,danube";
+ aliases {
+ serial0 = &asc1;
+ };
+
You generally want the aliases to be part of the board specific file,
not every board numbers their serial ports in the same way.
In this chip only asc1 can be used as console, so serial0 is defined in
chip specific file.
This was a more general comment about 'aliases' being board specific
in principle (though we've had exceptions in the past). Even if there
is only one uart on the chip, I'd recommend following the same
conventions as the other chips that have more than one uart.
Arnd
Accept, 'aliases' will be move to the board specific file.
Re: [PATCH 7/7] tty: serial: lantiq: Add CCF support
On 6/13/2018 6:39 AM, Rob Herring wrote:
On Tue, Jun 12, 2018 at 01:40:34PM +0800, Songjun Wu wrote:
Previous implementation uses platform-dependent API to get the clock.
Those functions are not available for other SoC which uses the same IP.
The CCF (Common Clock Framework) have an abstraction based APIs
for clock.
Change to use CCF APIs to get clock and rate.
So that different SoCs can use the same driver.
Clocks and clock-names are updated in device tree binding.
Signed-off-by: Songjun Wu
---
.../devicetree/bindings/serial/lantiq_asc.txt | 15 +++
Please split bindings to separate patch.
Thanks.
It will be split to two separate patches, one for bindings, the other
for code.
drivers/tty/serial/Kconfig | 2 +-
drivers/tty/serial/lantiq.c| 101 +
3 files changed, 98 insertions(+), 20 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/lantiq_asc.txt
b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
index 3acbd309ab9d..608f0c87a4af 100644
--- a/Documentation/devicetree/bindings/serial/lantiq_asc.txt
+++ b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
@@ -6,6 +6,10 @@ Required properties:
- interrupts: the 3 (tx rx err) interrupt numbers. The interrupt specifier
depends on the interrupt-parent interrupt controller.
+Optional properties:
+- clocks: Should contain frequency clock and gate clock
+- clock-names: Should be "freq" and "asc"
+
Example:
asc1: serial@e100c00 {
@@ -14,3 +18,14 @@ asc1: serial@e100c00 {
interrupt-parent = <&icu0>;
interrupts = <112 113 114>;
};
+
+asc0: serial@60 {
+ compatible = "lantiq,asc";
+ reg = <0x60 0x10>;
1MB of address space? That wastes a lot of virtual space on 32-bit
systems. Just make the size the actual used range.
The size of address space will be updated to the actual used range.
+ interrupt-parent = <&gic>;
+ interrupts = ,
+ ,
+ ;
+ clocks = <&pll0aclk SSX4_CLK>, <&clkgate1 GATE_URT_CLK>;
+ clock-names = "freq", "asc";
+};
Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()
On 6/12/2018 4:13 PM, Andy Shevchenko wrote:
On Tue, Jun 12, 2018 at 8:40 AM, Songjun Wu wrote:
Previous implementation uses platform-dependent functions
ltq_w32()/ltq_r32() to access registers. Those functions are not
available for other SoC which uses the same IP.
Change to OS provided readl()/writel() and readb()/writeb(), so
that different SoCs can use the same driver.
This patch consists 2 or even 3 ones:
- whitespace shuffling (indentation fixes, blank lines), I dunno if
it's needed at all
- some new registers / bits
- actual switch to readl() / writel()
Please, split.
It will be split to four patches, coding style, new registers,
readl()/writel() and asc_update_bits.
+#define asc_w32_mask(clear, set, reg) \
+ ({ typeof(reg) reg_ = (reg);\
+ writel((readl(reg_) & ~(clear)) | (set), reg_); })
This would be better as a static inline helper, and name is completely
misleading, it doesn't mask the register bits, it _updates_ them.
It will be changed to asc_update_bits.
Re: [PATCH 00/14] serial: langtiq: Add CCF suppport
On 10/16/2018 5:58 AM, Paul Burton wrote: Hi Songjun, On Mon, Sep 24, 2018 at 06:27:49PM +0800, Songjun Wu wrote: This patch series is for adding common clock framework support for langtiq serial driver, mainly includes: s/langtiq/lantiq/ ... Thanks, it will be fixed. 1) Add common clock framework support. 2) Modify the dts file according to the DT conventions. 3) Replace the platform dependent functions with kernel functions Songjun Wu (14): MIPS: dts: Change upper case to lower case MIPS: dts: Add aliases node for lantiq danube serial serial: lantiq: Get serial id from dts serial: lantiq: Change ltq_w32_mask to asc_update_bits MIPS: lantiq: Unselect SWAP_IO_SPACE when LANTIQ is selected serial: lantiq: Use readl/writel instead of ltq_r32/ltq_w32 serial: lantiq: Rename fpiclk to freqclk serial: lantiq: Replace clk_enable/clk_disable with clk generic API serial: lantiq: Add CCF support serial: lantiq: Reorder the head files include: Add lantiq.h in include/linux/ serial: lantiq: Replace lantiq_soc.h with lantiq.h serial: lantiq: Change init_lqasc to static declaration dt-bindings: serial: lantiq: Add optional properties for CCF It appears that you only copied me on patches 1, 2 & 5. I've applied patch 1 to mips-next for 4.20, but I have no clue whether your other patches were deemed acceptable by serial or DT maintainers & I have no context for the changes being made, so I can neither apply nor ack patches 2 & 5. Please copy me on the whole series next time. Thanks, Paul Thanks. I will resend the patches and cc all the patches to you.
Re: [PATCH] USB: gadget: udc: atmel: fix possible oops when unloading module
在 12/24/2014 00:24, Felipe Balbi 写道:
On Mon, Dec 22, 2014 at 05:26:14PM +0800, Songjun Wu wrote:
When unloading the module, the urb request will be dequeued
and the completion routine will be excuted.
If no urb packet, the urb request will not be added to the endpoint queue
and the completion routine pointer in urb request is NULL.
Accessing to the NULL function pointer will cause the oops issue.
Add the code to check the urb request is in the endpoint queue or not.
If the urb request is not in the endpoint queue, a negative error code
will be returned.
have you triggered the NULL pointer oops ? Care to add it to the commit
log.
Executing the 'insmod g_hid.ko', then executing the 'rmmod g_hid.ko',
the NULL pointer oops will be triggered.
Also, which commit is this fixing ? Does this need to be backported ?
When was the bug introduced ?
Signed-off-by: Songjun Wu
---
drivers/usb/gadget/udc/atmel_usba_udc.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c
b/drivers/usb/gadget/udc/atmel_usba_udc.c
index ce88237..48629cc 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -828,7 +828,7 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct
usb_request *_req)
{
struct usba_ep *ep = to_usba_ep(_ep);
struct usba_udc *udc = ep->udc;
- struct usba_request *req = to_usba_req(_req);
+ struct usba_request *req;
unsigned long flags;
u32 status;
@@ -837,6 +837,16 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct
usb_request *_req)
spin_lock_irqsave(&udc->lock, flags);
+ list_for_each_entry(req, &ep->queue, queue) {
+ if (&req->req == _req)
+ break;
+ }
+
+ if (&req->req != _req) {
+ spin_unlock_irqrestore(&udc->lock, flags);
+ return -EINVAL;
+ }
+
if (req->using_dma) {
/*
* If this request is currently being transferred,
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
看看这样回复可以吗？ Re: [PATCH] USB: gadget: udc: atmel: fix possible oops when unloading module
On 12/26/2014 23:27, Felipe Balbi wrote: Hi, On Wed, Dec 24, 2014 at 09:14:53AM +0800, Wu, Songjun wrote: 在 12/24/2014 00:24, Felipe Balbi 写道: On Mon, Dec 22, 2014 at 05:26:14PM +0800, Songjun Wu wrote: When unloading the module, the urb request will be dequeued and the completion routine will be excuted. If no urb packet, the urb request will not be added to the endpoint queue and the completion routine pointer in urb request is NULL. Accessing to the NULL function pointer will cause the oops issue. Add the code to check the urb request is in the endpoint queue or not. If the urb request is not in the endpoint queue, a negative error code will be returned. have you triggered the NULL pointer oops ? Care to add it to the commit log. Executing the 'insmod g_hid.ko', then executing the 'rmmod g_hid.ko', the NULL pointer oops will be triggered. what about all my other queries below and what about adding the oops dump to commit log ? Also, which commit is this fixing ? Does this need to be backported ? When was the bug introduced ? Fixes: commit a81ca786d049f3ac4b6062bfb426b468a5e02a3b This bug was introduced since the file atmel_usba_udc.c was initialized. The oops dump log is shown in the following. # insmod g_hid.ko g_hid gadget: HID Gadget, version: 2010/03/16 g_hid gadget: g_hid ready # rmmod g_hid.ko Unable to handle kernel NULL pointer dereference at virtual address pgd = dedf [] *pgd=3ede5831, *pte=, *ppte= Internal error: Oops: 8007 [#1] ARM Modules linked in: g_hid(-) usb_f_hid libcomposite CPU: 0 PID: 923 Comm: rmmod Not tainted 3.18.0+ #2 Hardware name: Atmel SAMA5 (Device Tree) task: df6b1100 ti: dedf6000 task.ti: dedf6000 PC is at 0x0 LR is at usb_gadget_giveback_request+0xc/0x10 pc : [<>]lr : []psr: 6093 sp : dedf7eb0 ip : df572634 fp : r10: r9 : df52e210 r8 : 6013 r7 : df6a9858 r6 : df52e210 r5 : df6a9858 r4 : df572600 r3 : r2 : ff98 r1 : df572600 r0 : df6a9868 Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c53c7d Table: 3edf0059 DAC: 0015 Process rmmod (pid: 923, stack limit = 0xdedf6230) Stack: (0xdedf7eb0 to 0xdedf8000) 7ea0: c02adbbc df572580 deced608 7ec0: df572600 df6a9868 df572634 c02aed3c df577c00 c01b8608 df6be27c 7ee0: 00200200 00100100 bf0162f4 c000e544 dedf6000 bf010c00 7f00: bf0162cc bf00159c df572980 df52e218 0001 df5729b8 bf0031d0 7f20: bf003234 df52e400 df52e400 a013 0081 c02acee0 c02ad570 7f40: bf0160b0 bf016290 bf0160c0 bf0167a0 c0056748 0022 69685f67 7f60: b6ff0064 0001 b6ff3000 dedf6000 bebaaa4c c008370c 7f80: 00100871 c0081078 0022 df5f31e8 b6ff4518 0001c588 69685f67 7fa0: b6ff0064 c000e3c0 0001c588 69685f67 bebaab78 0880 bebaab78 0880 7fc0: 0001c588 69685f67 b6ff0064 0081 bebaae14 009f 7fe0: bebaab70 bebaab60 0001c464 b6f69ba0 6010 bebaab78 3fffd821 3fffdc21 [] (usb_gadget_giveback_request) from [] (request_complete+0x64/0x88) [] (request_complete) from [] (usba_ep_dequeue+0x70/0x128) [] (usba_ep_dequeue) from [] (hidg_unbind+0x50/0x7c [usb_f_hid]) [] (hidg_unbind [usb_f_hid]) from [] (remove_config.isra.6+0x98/0x9c [libcomposite]) [] (remove_config.isra.6 [libcomposite]) from [] (__composite_unbind+0x34/0x98 [libcomposite]) [] (__composite_unbind [libcomposite]) from [] (usb_gadget_remove_driver+0x50/0x78) [] (usb_gadget_remove_driver) from [] (usb_gadget_unregister_driver+0x64/0x94) [] (usb_gadget_unregister_driver) from [] (hidg_cleanup+0x10/0x34 [g_hid]) [] (hidg_cleanup [g_hid]) from [] (SyS_delete_module+0x118/0x19c) [] (SyS_delete_module) from [] (ret_fast_syscall+0x0/0x30) Code: bad PC value ---[ end trace dd1fcf365005ba79 ]--- Segmentation fault # -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: gadget: udc: atmel: fix possible oops when unloading module
On 12/26/2014 23:27, Felipe Balbi wrote: Hi, On Wed, Dec 24, 2014 at 09:14:53AM +0800, Wu, Songjun wrote: 在 12/24/2014 00:24, Felipe Balbi 写道: On Mon, Dec 22, 2014 at 05:26:14PM +0800, Songjun Wu wrote: When unloading the module, the urb request will be dequeued and the completion routine will be excuted. If no urb packet, the urb request will not be added to the endpoint queue and the completion routine pointer in urb request is NULL. Accessing to the NULL function pointer will cause the oops issue. Add the code to check the urb request is in the endpoint queue or not. If the urb request is not in the endpoint queue, a negative error code will be returned. have you triggered the NULL pointer oops ? Care to add it to the commit log. Executing the 'insmod g_hid.ko', then executing the 'rmmod g_hid.ko', the NULL pointer oops will be triggered. what about all my other queries below and what about adding the oops dump to commit log ? Also, which commit is this fixing ? Does this need to be backported ? When was the bug introduced ? Fixes: 914a3f3b3754 (USB: add atmel_usba_udc driver) Cc: sta...@vger.kernel.org # always been there... This bug was introduced since the file atmel_usba_udc.c was initialized. The oops dump log is shown in the following. # insmod libcomposite.ko d usb_f_hid.ko insmod g_hid.ko # insmod usb_f_hid.ko # insmod g_hid.ko g_hid gadget: HID Gadget, version: 2010/03/16 g_hid gadget: g_hid ready # rmmod g_hid.ko Unable to handle kernel NULL pointer dereference at virtual address pgd = dedf [] *pgd=3ede5831, *pte=, *ppte= Internal error: Oops: 8007 [#1] ARM Modules linked in: g_hid(-) usb_f_hid libcomposite CPU: 0 PID: 923 Comm: rmmod Not tainted 3.18.0+ #2 Hardware name: Atmel SAMA5 (Device Tree) task: df6b1100 ti: dedf6000 task.ti: dedf6000 PC is at 0x0 LR is at usb_gadget_giveback_request+0xc/0x10 pc : [<>]lr : []psr: 6093 sp : dedf7eb0 ip : df572634 fp : r10: r9 : df52e210 r8 : 6013 r7 : df6a9858 r6 : df52e210 r5 : df6a9858 r4 : df572600 r3 : r2 : ff98 r1 : df572600 r0 : df6a9868 Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c53c7d Table: 3edf0059 DAC: 0015 Process rmmod (pid: 923, stack limit = 0xdedf6230) Stack: (0xdedf7eb0 to 0xdedf8000) 7ea0: c02adbbc df572580 deced608 7ec0: df572600 df6a9868 df572634 c02aed3c df577c00 c01b8608 df6be27c 7ee0: 00200200 00100100 bf0162f4 c000e544 dedf6000 bf010c00 7f00: bf0162cc bf00159c df572980 df52e218 0001 df5729b8 bf0031d0 7f20: bf003234 df52e400 df52e400 a013 0081 c02acee0 c02ad570 7f40: bf0160b0 bf016290 bf0160c0 bf0167a0 c0056748 0022 69685f67 7f60: b6ff0064 0001 b6ff3000 dedf6000 bebaaa4c c008370c 7f80: 00100871 c0081078 0022 df5f31e8 b6ff4518 0001c588 69685f67 7fa0: b6ff0064 c000e3c0 0001c588 69685f67 bebaab78 0880 bebaab78 0880 7fc0: 0001c588 69685f67 b6ff0064 0081 bebaae14 009f 7fe0: bebaab70 bebaab60 0001c464 b6f69ba0 6010 bebaab78 3fffd821 3fffdc21 [] (usb_gadget_giveback_request) from [] (request_complete+0x64/0x88) [] (request_complete) from [] (usba_ep_dequeue+0x70/0x128) [] (usba_ep_dequeue) from [] (hidg_unbind+0x50/0x7c [usb_f_hid]) [] (hidg_unbind [usb_f_hid]) from [] (remove_config.isra.6+0x98/0x9c [libcomposite]) [] (remove_config.isra.6 [libcomposite]) from [] (__composite_unbind+0x34/0x98 [libcomposite]) [] (__composite_unbind [libcomposite]) from [] (usb_gadget_remove_driver+0x50/0x78) [] (usb_gadget_remove_driver) from [] (usb_gadget_unregister_driver+0x64/0x94) [] (usb_gadget_unregister_driver) from [] (hidg_cleanup+0x10/0x34 [g_hid]) [] (hidg_cleanup [g_hid]) from [] (SyS_delete_module+0x118/0x19c) [] (SyS_delete_module) from [] (ret_fast_syscall+0x0/0x30) Code: bad PC value ---[ end trace dd1fcf365005ba79 ]--- Segmentation fault # -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: gadget: udc: atmel: fix possible oops when unloading module
On 12/26/2014 23:27, Felipe Balbi wrote: Hi, On Wed, Dec 24, 2014 at 09:14:53AM +0800, Wu, Songjun wrote: 在 12/24/2014 00:24, Felipe Balbi 写道: On Mon, Dec 22, 2014 at 05:26:14PM +0800, Songjun Wu wrote: When unloading the module, the urb request will be dequeued and the completion routine will be excuted. If no urb packet, the urb request will not be added to the endpoint queue and the completion routine pointer in urb request is NULL. Accessing to the NULL function pointer will cause the oops issue. Add the code to check the urb request is in the endpoint queue or not. If the urb request is not in the endpoint queue, a negative error code will be returned. have you triggered the NULL pointer oops ? Care to add it to the commit log. Executing the 'insmod g_hid.ko', then executing the 'rmmod g_hid.ko', the NULL pointer oops will be triggered. what about all my other queries below and what about adding the oops dump to commit log ? Also, which commit is this fixing ? Does this need to be backported ? When was the bug introduced ? Fixes: 914a3f3b3754 (USB: add atmel_usba_udc driver) Cc: sta...@vger.kernel.org # always been there... This bug was introduced since the file 'atmel_usba_udc.c' was initialized. The oops dump log is shown in the following. # insmod g_hid.ko g_hid gadget: HID Gadget, version: 2010/03/16 g_hid gadget: g_hid ready # rmmod g_hid.ko Unable to handle kernel NULL pointer dereference at virtual address pgd = dedf [] *pgd=3ede5831, *pte=, *ppte= Internal error: Oops: 8007 [#1] ARM Modules linked in: g_hid(-) usb_f_hid libcomposite CPU: 0 PID: 923 Comm: rmmod Not tainted 3.18.0+ #2 Hardware name: Atmel SAMA5 (Device Tree) task: df6b1100 ti: dedf6000 task.ti: dedf6000 PC is at 0x0 LR is at usb_gadget_giveback_request+0xc/0x10 pc : [<>]lr : []psr: 6093 sp : dedf7eb0 ip : df572634 fp : r10: r9 : df52e210 r8 : 6013 r7 : df6a9858 r6 : df52e210 r5 : df6a9858 r4 : df572600 r3 : r2 : ff98 r1 : df572600 r0 : df6a9868 Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c53c7d Table: 3edf0059 DAC: 0015 Process rmmod (pid: 923, stack limit = 0xdedf6230) Stack: (0xdedf7eb0 to 0xdedf8000) 7ea0: c02adbbc df572580 deced608 7ec0: df572600 df6a9868 df572634 c02aed3c df577c00 c01b8608 df6be27c 7ee0: 00200200 00100100 bf0162f4 c000e544 dedf6000 bf010c00 7f00: bf0162cc bf00159c df572980 df52e218 0001 df5729b8 bf0031d0 7f20: bf003234 df52e400 df52e400 a013 0081 c02acee0 c02ad570 7f40: bf0160b0 bf016290 bf0160c0 bf0167a0 c0056748 0022 69685f67 7f60: b6ff0064 0001 b6ff3000 dedf6000 bebaaa4c c008370c 7f80: 00100871 c0081078 0022 df5f31e8 b6ff4518 0001c588 69685f67 7fa0: b6ff0064 c000e3c0 0001c588 69685f67 bebaab78 0880 bebaab78 0880 7fc0: 0001c588 69685f67 b6ff0064 0081 bebaae14 009f 7fe0: bebaab70 bebaab60 0001c464 b6f69ba0 6010 bebaab78 3fffd821 3fffdc21 [] (usb_gadget_giveback_request) from [] (request_complete+0x64/0x88) [] (request_complete) from [] (usba_ep_dequeue+0x70/0x128) [] (usba_ep_dequeue) from [] (hidg_unbind+0x50/0x7c [usb_f_hid]) [] (hidg_unbind [usb_f_hid]) from [] (remove_config.isra.6+0x98/0x9c [libcomposite]) [] (remove_config.isra.6 [libcomposite]) from [] (__composite_unbind+0x34/0x98 [libcomposite]) [] (__composite_unbind [libcomposite]) from [] (usb_gadget_remove_driver+0x50/0x78) [] (usb_gadget_remove_driver) from [] (usb_gadget_unregister_driver+0x64/0x94) [] (usb_gadget_unregister_driver) from [] (hidg_cleanup+0x10/0x34 [g_hid]) [] (hidg_cleanup [g_hid]) from [] (SyS_delete_module+0x118/0x19c) [] (SyS_delete_module) from [] (ret_fast_syscall+0x0/0x30) Code: bad PC value ---[ end trace dd1fcf365005ba79 ]--- Segmentation fault # -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code
The option 'CONFIG_COMMON_CLK=y' is needed to add to '.config'. But I do not validate, '.config' will be generated automatically and overwritten when it is changed. On 4/14/2016 00:01, kbuild test robot wrote: Hi Songjun, [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v4.6-rc3 next-20160413] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Songjun-Wu/atmel-isc-add-driver-for-Atmel-ISC/20160413-155337 base: git://linuxtv.org/media_tree.git master config: powerpc-allyesconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones prefixed by >>): from include/linux/of.h:21, from drivers/media/platform/atmel/atmel-isc.c:27: drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_enable': include/linux/kernel.h:824:48: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of' #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of macro 'to_isc_clk' struct isc_clk *isc_clk = to_isc_clk(hw); ^ include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk') const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of' #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of macro 'to_isc_clk' struct isc_clk *isc_clk = to_isc_clk(hw); ^ drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_disable': include/linux/kernel.h:824:48: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of' #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of macro 'to_isc_clk' struct isc_clk *isc_clk = to_isc_clk(hw); ^ include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk') const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of' #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of macro 'to_isc_clk' struct isc_clk *isc_clk = to_isc_clk(hw); ^ drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_is_enabled': include/linux/kernel.h:824:48: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types] const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of' #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of macro 'to_isc_clk' struct isc_clk *isc_clk = to_isc_clk(hw); ^ include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk') const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of' #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of macro 'to_isc_clk' struct isc_clk *isc_clk = to_isc_clk(hw); ^ drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_recalc_rate': include/linux/kernel.h:824:48: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-ty
Re: [PATCH v2 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
Hi Rob,
Thank you for your comments.
On 5/19/2016 07:05, Rob Herring wrote:
On Wed, May 18, 2016 at 03:46:29PM +0800, Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of "atmel,sensor-preferred".
- Add the description for the ISC internal clock.
.../devicetree/bindings/media/atmel-isc.txt| 100 +
1 file changed, 100 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..9e65395
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,100 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties for ISC:
+- compatible
+ Must be "atmel,sama5d2-isc"
+- reg
+ Physical base address and length of the registers set for the device;
+- interrupts
+ Should contain IRQ line for the ISC;
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock", "ispck".
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
+- clk-in-isc
+ ISC internal clock node, it includes two clock nodes,
+ isc-ispck and isc-mck.
And you need to describe each of these nodes and the properties they
have. But more on this below...
Should I move the description of clock node below to here?
+- atmel,sensor-preferred
+ ISC may convert the raw format to the specified format when the sensor
+ outputs the raw format, and the sensor may output the specified format
+ directly. If sensor is preferred to output the specified format
+ directly, the value should be 1 (1-preferred, 0-not).
+ The default value is 1.
I don't think this belongs in DT. It sounds more like a configuration
and userspace decision. It is still not clear what you mean by
"specified format".
Accept, I will remove this.
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Required properties for the ISC internal clocks:
+- #address-cells
+ should be 1 (reg is used to encode clk id).
+- #size-cells
+ should be 0 (reg is used to encode clk id).
+- name
+ device tree node describing a ISC internal clock.
+ * #clock-cells: from common clock binding; should be set to 0.
+ * reg: clock id, there are two values,
+ <0> is ISP clock, <1> is master clock.
+ * clocks: shall be the isc internal clock source phandles.
+ e.g. clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&isc_ispck>;
+ clock-names = "hclock", "ispck";
+ atmel,sensor-preferred = <1>;
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_0: endpoint {
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
+ };
+ };
+
+ clk-in-isc {
Is this really separate from the rest of the isc block. It would be
much more simple to define all the input clocks at the parent and then
reference these 2 clocks with <&isc 0> and <&isc 1>.
Yes, it's separate from the rest of isc block.
It will register two clocks, one is used in ISC internal processor, the
other will provide the clock to the sensor outside.
I think it's better that the parent clock is included in node
'clk-in-isc' node. I will try to optimize this node.
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_ispck: isc-ispck@0 {
These would need a compatible string otherwise.
+ #clock-cells = <0>;
+ reg = <0>;
Are 0 and 1 meaningful in terms of the hardware description?
Yes, It's meaningful.
The register field is different, 0 and 1 can distinguish the different
clock.
+ clocks = <&isc_clk>, <&iscck>;
+ };
+
+ isc_mck: isc-mck@1 {
+ #clock-cells = <0>;
+ reg = <1>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ };
+ };
+};
Re: [PATCH 1/2] ASoC: atmel-pdmic: add the Pulse Density Modulation Interface Controller
On 12/16/2015 20:27, Mark Brown wrote:
On Mon, Dec 14, 2015 at 04:15:39PM +0800, Songjun Wu wrote:
Add driver for the Pulse Density Modulation Interface
Controller. It comes with digitallly controlled gain,
a High-Pass and a SINCC filter.
This looks basically OK but there's a *lot* of weird coding style issues
in here. It's really all that, nothing too serious that I noticed -
I've pointed out some of the patterns below not every individual issue.
+ for (i = 0; i < ARRAY_SIZE(mic_gain_table); i++) {
+ if ((mic_gain_table[i].dgain == dgain_val)
+ && (mic_gain_table[i].scale == scale_val))
+ ucontrol->value.integer.value[0] = i;
+ }
This indentation is really weird, why is the && aligned with the if?
Accept.
+ snd_soc_update_bits(codec, PDMIC_DSPR1,
+ PDMIC_DSPR1_OFFSET_MASK,
+ (u32)(dd->pdata->mic_offset << PDMIC_DSPR1_OFFSET_SHIFT));
These are weird too, I'd expect the second line to be part of the first.
+static struct regmap *atmel_pdmic_codec_get_remap(struct device *dev)
+{
+ return dev_get_regmap(dev, NULL);
+}
This is (or should be) the default in the core.
You are right. The core has initialized the regmap in function
snd_soc_component_add_unlocked.
This function can be removed.
Thank you.
+ if ((fs < rate_min) || (fs > rate_max)) {
+ dev_err(codec->dev,
+ "sample rate is %dHz, min rate is %dHz, max rate is %dHz\n",
+ fs, rate_min, rate_max);
This too, alignment after the (.
Accept.
+ if (bits == 16)
+ dspr0_val = (PDMIC_DSPR0_SIZE_16_BITS
+ << PDMIC_DSPR0_SIZE_SHIFT);
+ else if (bits == 32)
+ dspr0_val = (PDMIC_DSPR0_SIZE_32_BITS
+ << PDMIC_DSPR0_SIZE_SHIFT);
+ else
+ return -EINVAL;
This looks like it should be a switch statement.
Accept.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code
On 9/9/2015 17:52, Mark Brown wrote: On Wed, Sep 09, 2015 at 11:16:08AM +0800, Wu, Songjun wrote: On 9/8/2015 20:23, Mark Brown wrote: If you want to have three controls you need to write code so that the user can only change one of them from 0dB at once, returning an error otherwise. That was why it looked like they were three separate controls. If user operates two or tree controls at the same time, for my understanding, these operations are serial actually in kernel, not parallel, and the last operation will be effective. I only write the function 'classd_get_eq_enum' to get the enumeration value, if user changes one of controls, the other controls will get 0dB. Is my understanding correct? Yes, that's what's going to end up happening but it's not how controls are expected to behave - applications will expect changing one control to leave others unaffected so it's better to return an error rather than change the other control. If application change non EQ controls, the others will be unaffected. But the classD IP can only supports one EQ control at once, these three EQ controls point to the same register field, if application set a different EQ control, the error occurs, there will be many errors, it's not very reasonable to application. The best way I think is if application set one EQ control, the other EQ controls will change to 0dB, it's also consistent with fact. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code
On 9/11/2015 18:34, Mark Brown wrote: On Thu, Sep 10, 2015 at 10:31:04AM +0800, Wu, Songjun wrote: On 9/9/2015 17:52, Mark Brown wrote: Yes, that's what's going to end up happening but it's not how controls are expected to behave - applications will expect changing one control to leave others unaffected so it's better to return an error rather than change the other control. If application change non EQ controls, the others will be unaffected. But the classD IP can only supports one EQ control at once, these three EQ controls point to the same register field, if application set a different EQ control, the error occurs, there will be many errors, it's not very reasonable to application. The best way I think is if application set one EQ control, the other EQ controls will change to 0dB, it's also consistent with fact. There's no really good solutions here - this is why my initial suggestion was to have a single enumerated control. You are right, your suggestion is reasonable, to have a single enumerated control. The second version will be made and sent soon. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ASoC: atmel_ssc_dai: distinguish the different SSC
Any comments on this patch?
On 1/13/2016 23:08, Nicolas Ferre wrote:
Le 13/01/2016 07:35, Songjun Wu a écrit :
Cpu_dai id always equals 0, can't distinguish the
different SSC. Use platform_device id to record
and distinguish the different SSC.
Signed-off-by: Songjun Wu
It seems okay:
Acked-by: Nicolas Ferre
---
drivers/misc/atmel-ssc.c|1 +
sound/soc/atmel/atmel_ssc_dai.c | 27 ++-
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/atmel-ssc.c b/drivers/misc/atmel-ssc.c
index e11a0bd..0516ecd 100644
--- a/drivers/misc/atmel-ssc.c
+++ b/drivers/misc/atmel-ssc.c
@@ -34,6 +34,7 @@ struct ssc_device *ssc_request(unsigned int ssc_num)
if (ssc->pdev->dev.of_node) {
if (of_alias_get_id(ssc->pdev->dev.of_node, "ssc")
== ssc_num) {
+ ssc->pdev->id = ssc_num;
ssc_valid = 1;
break;
}
diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index ba8def5..2768970 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -285,7 +285,8 @@ static int atmel_ssc_hw_rule_rate(struct snd_pcm_hw_params
*params,
static int atmel_ssc_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
- struct atmel_ssc_info *ssc_p = &ssc_info[dai->id];
+ struct platform_device *pdev = to_platform_device(dai->dev);
+ struct atmel_ssc_info *ssc_p = &ssc_info[pdev->id];
struct atmel_pcm_dma_params *dma_params;
int dir, dir_mask;
int ret;
@@ -346,7 +347,8 @@ static int atmel_ssc_startup(struct snd_pcm_substream
*substream,
static void atmel_ssc_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
- struct atmel_ssc_info *ssc_p = &ssc_info[dai->id];
+ struct platform_device *pdev = to_platform_device(dai->dev);
+ struct atmel_ssc_info *ssc_p = &ssc_info[pdev->id];
struct atmel_pcm_dma_params *dma_params;
int dir, dir_mask;
@@ -392,7 +394,8 @@ static void atmel_ssc_shutdown(struct snd_pcm_substream
*substream,
static int atmel_ssc_set_dai_fmt(struct snd_soc_dai *cpu_dai,
unsigned int fmt)
{
- struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id];
+ struct platform_device *pdev = to_platform_device(cpu_dai->dev);
+ struct atmel_ssc_info *ssc_p = &ssc_info[pdev->id];
ssc_p->daifmt = fmt;
return 0;
@@ -404,7 +407,8 @@ static int atmel_ssc_set_dai_fmt(struct snd_soc_dai
*cpu_dai,
static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
int div_id, int div)
{
- struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id];
+ struct platform_device *pdev = to_platform_device(cpu_dai->dev);
+ struct atmel_ssc_info *ssc_p = &ssc_info[pdev->id];
switch (div_id) {
case ATMEL_SSC_CMR_DIV:
@@ -445,7 +449,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream
*substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
{
- int id = dai->id;
+ struct platform_device *pdev = to_platform_device(dai->dev);
+ int id = pdev->id;
struct atmel_ssc_info *ssc_p = &ssc_info[id];
struct ssc_device *ssc = ssc_p->ssc;
struct atmel_pcm_dma_params *dma_params;
@@ -772,7 +777,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream
*substream,
static int atmel_ssc_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
- struct atmel_ssc_info *ssc_p = &ssc_info[dai->id];
+ struct platform_device *pdev = to_platform_device(dai->dev);
+ struct atmel_ssc_info *ssc_p = &ssc_info[pdev->id];
struct atmel_pcm_dma_params *dma_params;
int dir;
@@ -795,7 +801,8 @@ static int atmel_ssc_prepare(struct snd_pcm_substream
*substream,
static int atmel_ssc_trigger(struct snd_pcm_substream *substream,
int cmd, struct snd_soc_dai *dai)
{
- struct atmel_ssc_info *ssc_p = &ssc_info[dai->id];
+ struct platform_device *pdev = to_platform_device(dai->dev);
+ struct atmel_ssc_info *ssc_p = &ssc_info[pdev->id];
struct atmel_pcm_dma_params *dma_params;
int dir;
@@ -824,11 +831,12 @@ static int atmel_ssc_trigger(struct snd_pcm_substream
*substream,
static int atmel_ssc_suspend(struct snd_soc_dai *cpu_dai)
{
struct atmel_ssc_info *ssc_p;
+ struct platform_device *pdev = to_platform_device(cpu_dai->dev);
if (!cpu_dai->active)
return 0;
- ssc_p = &ssc_info[cpu_dai->id];
+ ssc_p = &ssc_info[pdev->id];
/* Save the status register before disabling transmit and receive */
ssc_p->ssc_state.ssc_sr = ssc_readl(ss
Re: [PATCH 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
On 4/14/2016 23:29, Rob Herring wrote:
On Wed, Apr 13, 2016 at 03:44:20PM +0800, Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
.../devicetree/bindings/media/atmel-isc.txt| 84 ++
1 file changed, 84 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..449f05f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,84 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties:
+- compatible
+ Must be "atmel,sama5d2-isc"
+- reg
+ Physical base address and length of the registers set for the device;
+- interrupts
+ Should contain IRQ line for the ISI;
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock", "ispck".
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
+- clk_in_isc
No underscores and this needs a better explanation.
Accept.
This is a node includes the isc internal clocks.
Maybe it should be 'List of ISC interal clocks'.
Do you think it's better?
Thank you.
+ ISC internal clock node, it includes the isc_ispck and isc_mck.
+ Please refer to clock-bindings.txt.
+- atmel,sensor-preferred
+ Sensor is preferred to process image (1-preferred, 0-not).
+ The default value is 1.
This seems a bit questionable.
ISC has an internal image processor, it can convert raw format to the
other format, like YUYV format. If user want to output YUYV format, ISC
driver has two choices, one is sensor output YUYV format, ISC does not
do any process, the other is sensor output raw format, ISC converts raw
format to YUYV format.
But how to choose? I set a option 'atmel,sensor-preferred' in dts to let
user decide.
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&isc_ispck>;
+ clock-names = "hclock", "ispck";
+ atmel,sensor-preferred = <1>;
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_0: endpoint@0 {
Don't need a unit address for a single endpoint.
Yes, it's a single endpoint.
But we can create more endpoints, but only one endpoint will be
effective. If you want to change the sensor, you can use the same dtb
file. It's convenient for user.
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
Are these documented? They are really properties of the sensor and
should be part of the sensor node.
Accept. It should be part of the sensor node.
Thank you.
+ };
+ };
+
+ clk_in_isc {
Completely missed this is a node from the above description. I should be
able to write or validate the example from the description.
I think perhaps you should just drop all this from the DT, just list the
input clocks, and make the ISC node the clock controller.
I think this node is needed. ISC will provide the clock to sensor, we
need create the corresponding clock node int DT file, then the sensor
will get this clock and set the clock rate in DT file
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_ispck: isc_ispck {
No underscores in node names.
Accept, thank you.
+ #clock-cells = <0>;
+ reg = <0>;
Drop or you need a unit address.
OK, I will set a unit address.
+ clocks = <&isc_clk>, <&iscck>;
+ };
+
+ isc_mck: isc_mck {
+ #clock-cells = <0>;
+ reg = <1>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ };
+ };
+};
+
+i2c1: i2c@fc028000 {
+ ov7740: camera@0x21 {
Drop "0x"
Accept, thank you.
+ compatible = "ovti,ov7740";
+ reg = <0x21>;
+
+ clocks = <&isc_mck>;
+ clock-names = "xvclk";
+ assigned-clocks = <&isc_mck>;
+ assigned-clock-rates = <2400>;
+
+ port {
+ ov7740_0: endpoint {
+ remote-endpoint = <&isc_0>;
+ };
+ };
+};
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body
Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 4/14/2016 22:14, Laurent Pinchart wrote: Hello Songjun, On Thursday 14 Apr 2016 13:44:27 Wu, Songjun wrote: The option 'CONFIG_COMMON_CLK=y' is needed to add to '.config'. But I do not validate, '.config' will be generated automatically and overwritten when it is changed. Your driver's Kconfig entry should then contain "depends on COMMON_CLK". Accept. Thank you. On 4/14/2016 00:01, kbuild test robot wrote: Hi Songjun, [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v4.6-rc3 next-20160413] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Songjun-Wu/atmel-isc-add-driver-> > for-Atmel-ISC/20160413-155337 base: git://linuxtv.org/media_tree.git master config: powerpc-allyesconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/p lain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones prefixed by >>): from include/linux/of.h:21, from drivers/media/platform/atmel/atmel-isc.c:27: drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_enable': include/linux/kernel.h:824:48: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]> const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of'> #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of macro 'to_isc_clk'> struct isc_clk *isc_clk = to_isc_clk(hw); ^ include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk') const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of'> #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of macro 'to_isc_clk'> struct isc_clk *isc_clk = to_isc_clk(hw); ^ drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_disable': include/linux/kernel.h:824:48: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]> const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of'> #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of macro 'to_isc_clk'> struct isc_clk *isc_clk = to_isc_clk(hw); ^ include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk') const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of'> #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of macro 'to_isc_clk'> struct isc_clk *isc_clk = to_isc_clk(hw); ^ drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_is_enabled': include/linux/kernel.h:824:48: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]> const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 'container_of'> #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw) ^ drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of macro 'to_isc_clk'> struct isc_clk *isc_clk = to_isc_clk(hw); ^ include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk&#x
Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 4/15/2016 00:21, Laurent Pinchart wrote: Hello Songjun, Thank you for the patch. On Wednesday 13 Apr 2016 15:44:19 Songjun Wu wrote: Add driver for the Image Sensor Controller. It manages incoming data from a parallel based CMOS/CCD sensor. It has an internal image processor, also integrates a triple channel direct memory access controller master interface. Signed-off-by: Songjun Wu --- drivers/media/platform/Kconfig|1 + drivers/media/platform/Makefile |2 + drivers/media/platform/atmel/Kconfig |9 + drivers/media/platform/atmel/Makefile |3 + drivers/media/platform/atmel/atmel-isc-regs.h | 280 + drivers/media/platform/atmel/atmel-isc.c | 1537 ++ 6 files changed, 1832 insertions(+) create mode 100644 drivers/media/platform/atmel/Kconfig create mode 100644 drivers/media/platform/atmel/Makefile create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h create mode 100644 drivers/media/platform/atmel/atmel-isc.c diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 201f5c2..1b50ed1 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -110,6 +110,7 @@ source "drivers/media/platform/exynos4-is/Kconfig" source "drivers/media/platform/s5p-tv/Kconfig" source "drivers/media/platform/am437x/Kconfig" source "drivers/media/platform/xilinx/Kconfig" +source "drivers/media/platform/atmel/Kconfig" config VIDEO_TI_CAL tristate "TI CAL (Camera Adaptation Layer) driver" diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile index bbb7bd1..ad8f471 100644 --- a/drivers/media/platform/Makefile +++ b/drivers/media/platform/Makefile @@ -55,4 +55,6 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE) += am437x/ obj-$(CONFIG_VIDEO_XILINX)+= xilinx/ +obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel/ + ccflags-y += -I$(srctree)/drivers/media/i2c diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig new file mode 100644 index 000..5ebc4a6 --- /dev/null +++ b/drivers/media/platform/atmel/Kconfig @@ -0,0 +1,9 @@ +config VIDEO_ATMEL_ISC + tristate "ATMEL Image Sensor Controller (ISC) support" + depends on VIDEO_V4L2 && HAS_DMA + depends on ARCH_AT91 || COMPILE_TEST As commented separately, you're missing "depends on COMMON_CLK". Accept, thank you. + select VIDEOBUF2_DMA_CONTIG + select REGMAP_MMIO + help + This module makes the ATMEL Image Sensor Controller available + as a v4l2 device. \ No newline at end of file diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile new file mode 100644 index 000..eb8cdbb --- /dev/null +++ b/drivers/media/platform/atmel/Makefile @@ -0,0 +1,3 @@ +# Makefile for ATMEL ISC driver The makefile isn't limited to the ISC driver, even if that's the only one currently located in the atmel directory. The atmel-isi driver should be placed here when it will move away from soc-camera. I would just write "Makefile for Atmel drivers", or even remove the comment completely. Accept. atmel-isi driver will be added into this folder later. I think "Makefile for Atmel drivers" is better. Thank you. +obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h new file mode 100644 index 000..8be9e4a --- /dev/null +++ b/drivers/media/platform/atmel/atmel-isc-regs.h @@ -0,0 +1,280 @@ + No need for a blank line here. Accept, thank you. +#ifndef __ATMEL_ISC_REGS_H +#define __ATMEL_ISC_REGS_H + +#include [snip] +/* ISC Clock Configuration Register */ +#define ISC_CLKCFG 0x0024 +#define ISC_CLKCFG_DIV_SHIFT(n) (n*16) As n can be an expression, you should enclose it in parentheses, ((n)*16). Same for tall the macros below. Accept, thank you. +#define ISC_CLKCFG_DIV_MASK(n) GENMASK((n*16 + 7), n*16) +#define ISC_CLKCFG_SEL_SHIFT(n) (n*16 + 8) +#define ISC_CLKCFG_SEL_MASK(n) GENMASK((n*17 + 8), (n*16 + 8)) + [snip] diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c new file mode 100644 index 000..4ffbfc9 --- /dev/null +++ b/drivers/media/platform/atmel/atmel-isc.c @@ -0,0 +1,1537 @@ +/* + * Atmel Image Sensor Controller (ISC) driver + * + * Copyright (C) 2016 Atmel + * + * Author: Songjun Wu + * + * This program is free software; you may redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * Sensor-->PFE-->WB-->CFA-->CC-->GAM-->CSC-->CBC-->SUB-->RLP-->DMA + * + * ISC video pipeline integrates the following submodules: + * PFE: Parallel Front End to sample the camera sensor input stream + * WB: Programmable white balance in the Bayer domain + * CFA: Col
Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 4/18/2016 15:24, Hans Verkuil wrote: On 04/13/2016 09:44 AM, Songjun Wu wrote: Add driver for the Image Sensor Controller. It manages incoming data from a parallel based CMOS/CCD sensor. It has an internal image processor, also integrates a triple channel direct memory access controller master interface. Signed-off-by: Songjun Wu Hi Songjun, Before this driver can be accepted it has to pass the v4l2-compliance test. The v4l2-compliance utility is here: git://linuxtv.org/v4l-utils.git Compile the utility straight from this repository so you're up to date. First fix any failures you get when running 'v4l2-compliance', then do the same when running 'v4l2-compliance -s' (now it is streaming as well) and finally when running 'v4l2-compliance -f' (streaming and testing all available formats). I would like to see the output of 'v4l2-compliance -f' as part of the cover letter of the next patch series. Looking at the code I see that it will definitely fail a few tests :-) Certainly the VIDIOC_CREATE_BUFS support in the queue_setup function is missing. Read the comments for queue_setup in videobuf2-core.h for more information. Thank you very much, I will pass the v4l2-compliance test before I send the version 2 patch. Regards, Hans
Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 4/15/2016 00:21, Laurent Pinchart wrote:
>+ return -EINVAL;
>+
>+ parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
>+ if (!parent_names)
>+ return -ENOMEM;
>+
>+ of_clk_parent_fill(np, parent_names, num_parents);
>+
>+ init.parent_names = parent_names;
>+ init.num_parents= num_parents;
>+ init.name = clk_name;
>+ init.ops= &isc_clk_ops;
>+ init.flags = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
>+
>+ isc_clk = &isc->isc_clks[id];
>+ isc_clk->hw.init = &init;
>+ isc_clk->regmap = regmap;
>+ isc_clk->lock= lock;
>+ isc_clk->id = id;
>+
>+ isc_clk->clk = clk_register(NULL, &isc_clk->hw);
>+ if (!IS_ERR(isc_clk->clk))
>+ of_clk_add_provider(np, of_clk_src_simple_get, isc_clk->clk);
>+ else {
>+ dev_err(isc->dev, "%s: clock register fail\n", clk_name);
>+ ret = PTR_ERR(isc_clk->clk);
>+ goto free_parent_names;
>+ }
>+
>+free_parent_names:
>+ kfree(parent_names);
>+ return ret;
>+}
>+
>+static int isc_clk_init(struct isc_device *isc)
>+{
>+ struct device_node *np = of_get_child_by_name(isc->dev->of_node,
>+ "clk_in_isc");
Do you really need the clk_in_isc DT node ? I would have assumed that the
clock topology inside the ISC is fixed, and that it would be enough to just
specify the three parent clocks in the ISC DT node and create the two internal
clocks in the driver without needing a DT description.
Hi Laurent,
I think more, and the clk_in_isc DT node should be needed. The clock
topology inside the ISC is fixed, but isc will provide the clock to
sensor, we need create the corresponding clock node int DT file, then
the sensor will get this clock and set the clock rate in DT file.
Re: [PATCH] atmel-isc: fix platform_no_drv_owner.cocci warnings
Accept.
Thank you for your comments.
On 4/13/2016 16:46, Julia Lawall wrote:
Remove .owner field if calls are used which set it automatically
Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
CC: Songjun Wu
Signed-off-by: Fengguang Wu
Signed-off-by: Julia Lawall
---
atmel-isc.c |1 -
1 file changed, 1 deletion(-)
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1524,7 +1524,6 @@ static struct platform_driver atmel_isc_
.remove = atmel_isc_remove,
.driver = {
.name = ATMEL_ISC_NAME,
- .owner = THIS_MODULE,
.of_match_table = of_match_ptr(atmel_isc_of_match),
},
};
Re: [PATCH] atmel-isc: fix compare_const_fl.cocci warnings
Accept. Thank you for your comments. On 4/13/2016 16:49, Julia Lawall wrote: Move constants to the right of binary operators. Generated by: scripts/coccinelle/misc/compare_const_fl.cocci CC: Songjun Wu Signed-off-by: Fengguang Wu Signed-off-by: Julia Lawall --- Up to you. Seems a tiny bit more readable to me not to have ISC_DCTRL and ISC_DCTRL_IE_IS right together. atmel-isc.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/media/platform/atmel/atmel-isc.c +++ b/drivers/media/platform/atmel/atmel-isc.c @@ -639,7 +639,7 @@ static inline void isc_start_dma(struct addr = vb2_dma_contig_plane_dma_addr(&frm->vb.vb2_buf, 0); - regmap_write(regmap, ISC_DCTRL, ISC_DCTRL_IE_IS | dview); + regmap_write(regmap, ISC_DCTRL, dview | ISC_DCTRL_IE_IS); regmap_write(regmap, ISC_DAD0, addr); regmap_update_bits(regmap, ISC_CTRLEN, ISC_CTRLEN_CAPTURE_MASK, ISC_CTRLEN_CAPTURE);
Re: [PATCH] ASoC: Atmel: ClassD: Set GCK's parent clock
On 11/21/2015 02:08, Mark Brown wrote:
On Thu, Nov 19, 2015 at 11:26:30AM +0800, Songjun Wu wrote:
Set GCK's parent clock as audio clock, make
sure the GCK's parent clock is audio clock.
+ ret = clk_set_parent(dd->gclk, dd->aclk);
+ if (ret) {
+ dev_err(dev, "failed to set GCK parent clock: %d\n", ret);
+ return ret;
+ }
Why are we doing this in the driver? This should be done by whatever
creates the clock tree, not by the driver that uses the clocks - that
way if some future SoC has a different clock tree the driver will
continue to work.
You are right. The GCK's parent clock should be assigned in device tree,
not in the driver. The DT binding for classD should be modified to set
the GCK's parent clock as audio clock.
Thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ASoC: atmel-classd: DT binding for Class D audio amplifier driver
On 9/8/2015 20:23, Mark Brown wrote: On Tue, Sep 08, 2015 at 05:36:13PM +0800, Wu, Songjun wrote: On 9/8/2015 00:25, Mark Brown wrote: Sure, there's no problem at all having that structure in software but it should be possible to do this without having to represent this structure in DT. It should be possible to register the card at the same time as the rest of the components rather than needing the separate device in the DT. Do you mean using a single entry in the DT for the whole classD system and instantiate ASoC components from it. For now, there are two entry, they could be combined to one entry. Yes, exactly. I try to use one entry, but there is a problem. It's about 'driver_data' in struct device. In function snd_soc_register_card, the parameter 'card' will be set to 'driver_data' by the code 'dev_set_drvdata(card->dev, card)'. Then some resources(eg. regmap, clock) also need be recorded by 'driver_data'. One entry could only has one 'driver_data'. I think the best way is to create two entries, like the current dts. What's your opinion? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] atmel-isc: add the isc pipeline function
Hi Hans,
Thank you for your comments.
On 1/9/2017 20:10, Hans Verkuil wrote:
+
+static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct isc_device *isc = container_of(ctrl->handler,
+struct isc_device, ctrls.handler);
+ struct isc_ctrls *ctrls = &isc->ctrls;
+
+ switch (ctrl->id) {
+ case V4L2_CID_BRIGHTNESS:
+ ctrls->brightness = ctrl->val & ISC_CBC_BRIGHT_MASK;
+ break;
+ case V4L2_CID_CONTRAST:
+ ctrls->contrast = (ctrl->val << 8) & ISC_CBC_CONTRAST_MASK;
As I understand it only bits 11-8 contain the contrast in the register?
Wouldn't '(ctrl->val & ISC_CBC_CONTRAST_MASK) << 8' be more readable?
Either that or the mask should be 0xf00, not 0xfff.
Actually, bits 12-0 contain the contrast, it is a fixed-point
number(signed 12 bits 1:3:8), ranges from -2048 to 2047. Then only the
integral part is output to be adjusted.
Maybe both the fractional part and integral part should be output.
Then the contrast control should be written as below.
v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -2048, 2047, 1, 1);
Do you have any good suggestion?
+ break;
+ case V4L2_CID_GAMMA:
+ ctrls->gamma_index = ctrl->val;
+ break;
+ case V4L2_CID_AUTO_WHITE_BALANCE:
+ ctrls->awb = ctrl->val;
+ if (ctrls->hist_stat != HIST_ENABLED) {
+ ctrls->r_gain = 0x1 << 9;
+ ctrls->b_gain = 0x1 << 9;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct v4l2_ctrl_ops isc_ctrl_ops = {
+ .s_ctrl = isc_s_ctrl,
+};
+
+static int isc_ctrl_init(struct isc_device *isc)
+{
+ const struct v4l2_ctrl_ops *ops = &isc_ctrl_ops;
+ struct isc_ctrls *ctrls = &isc->ctrls;
+ struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+ int ret;
+
+ ctrls->hist_stat = HIST_INIT;
+
+ ret = v4l2_ctrl_handler_init(hdl, 4);
+ if (ret < 0)
+ return ret;
+
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -1024, 1023, 1, 0);
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -8, 7, 1, 1);
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, GAMMA_MAX - 1, 1, 2);
Why is the maximum GAMMA_MAX - 1? I would assume that GAMMA_MAX is the maximum.
Looks weird. It's either a bug or it needs a comment.
+ v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+
+ v4l2_ctrl_handler_setup(hdl);
+
+ return 0;
+}
+
+
static int isc_async_bound(struct v4l2_async_notifier *notifier,
struct v4l2_subdev *subdev,
struct v4l2_async_subdev *asd)
@@ -1047,10 +1435,11 @@ static void isc_async_unbind(struct v4l2_async_notifier
*notifier,
{
struct isc_device *isc = container_of(notifier->v4l2_dev,
struct isc_device, v4l2_dev);
-
+ cancel_work_sync(&isc->awb_work);
video_unregister_device(&isc->video_dev);
if (isc->current_subdev->config)
v4l2_subdev_free_pad_config(isc->current_subdev->config);
+ v4l2_ctrl_handler_free(&isc->ctrls.handler);
}
static struct isc_format *find_format_by_code(unsigned int code, int *index)
@@ -1081,7 +1470,9 @@ static int isc_formats_init(struct isc_device *isc)
fmt = &isc_formats[0];
for (i = 0; i < ARRAY_SIZE(isc_formats); i++) {
- fmt->support = false;
+ fmt->isc_support = false;
+ fmt->sd_support = false;
+
fmt++;
}
@@ -1092,8 +1483,22 @@ static int isc_formats_init(struct isc_device *isc)
if (!fmt)
continue;
- fmt->support = true;
- num_fmts++;
+ fmt->sd_support = true;
+
+ if (i <= RAW_FMT_INDEX_END) {
+ for (j = ISC_FMT_INDEX_START;
+j <= ISC_FMT_INDEX_END; j++)
Just merge these two lines, easier to read.
Do you mean merge these two lines like 'for (j = ISC_FMT_INDEX_START; j
<= ISC_FMT_INDEX_END; j++)', but the line is over 80 characters
+ isc_formats[j].isc_support = true;
+
+ isc->raw_fmt = fmt;
+ }
Re: [PATCH v5 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
Hi Rob,
Thank you for your comments.
On 6/20/2016 21:25, Rob Herring wrote:
On Fri, Jun 17, 2016 at 04:57:14PM +0800, Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
Changes in v5:
- Add clock names.
Changes in v4:
- Remove the isc clock nodes.
Changes in v3:
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.
Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.
.../devicetree/bindings/media/atmel-isc.txt| 64 ++
1 file changed, 64 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..9558a77
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,64 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties for ISC:
+- compatible
+ Must be "atmel,sama5d2-isc".
+- reg
+ Physical base address and length of the registers set for the device.
+- interrupts
+ Should contain IRQ line for the ISC.
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock".
What about the 2 other clocks in the example?
The other clocks is optional, not required.
Do you have any suggestion?
+- #clock-cells
+ Should be 0.
+- clock-output-names
+ Should contain the name of the clock driving the sensor master clock.
State what the name is.
"isc-mck" will be added.
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ clock-names = "hclock", "iscck", "gck";
+ #clock-cells = <0>;
+ clock-output-names = "isc-mck";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_isc_base &pinctrl_isc_data_8bit &pinctrl_isc_data_9_10
&pinctrl_isc_data_11_12>;
+
+ port {
+ isc_0: endpoint {
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
+ };
+ };
+};
+
+i2c1: i2c@fc028000 {
+ ov7740: camera@21 {
+ compatible = "ovti,ov7740";
Indentation is still wrong here...
Sorry, my mistake.
It should be fixed.
+ reg = <0x21>;
+ clocks = <&isc>;
+ clock-names = "xvclk";
+ assigned-clocks = <&isc>;
+ assigned-clock-rates = <2400>;
+
+ port {
+ ov7740_0: endpoint {
+ remote-endpoint = <&isc_0>;
+ };
+ };
+};
--
2.7.4
Re: [PATCH v5 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
On 6/24/2016 15:35, Boris Brezillon wrote: On Fri, 24 Jun 2016 13:54:09 +0800 "Wu, Songjun" wrote: Hi Rob, Thank you for your comments. On 6/20/2016 21:25, Rob Herring wrote: On Fri, Jun 17, 2016 at 04:57:14PM +0800, Songjun Wu wrote: DT binding documentation for ISC driver. Signed-off-by: Songjun Wu --- Changes in v5: - Add clock names. Changes in v4: - Remove the isc clock nodes. Changes in v3: - Remove the 'atmel,sensor-preferred'. - Modify the isc clock node according to the Rob's remarks. Changes in v2: - Remove the unit address of the endpoint. - Add the unit address to the clock node. - Avoid using underscores in node names. - Drop the "0x" in the unit address of the i2c node. - Modify the description of 'atmel,sensor-preferred'. - Add the description for the ISC internal clock. .../devicetree/bindings/media/atmel-isc.txt| 64 ++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt new file mode 100644 index 000..9558a77 --- /dev/null +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt @@ -0,0 +1,64 @@ +Atmel Image Sensor Controller (ISC) +-- + +Required properties for ISC: +- compatible + Must be "atmel,sama5d2-isc". +- reg + Physical base address and length of the registers set for the device. +- interrupts + Should contain IRQ line for the ISC. +- clocks + List of clock specifiers, corresponding to entries in + the clock-names property; + Please refer to clock-bindings.txt. +- clock-names + Required elements: "hclock". What about the 2 other clocks in the example? The other clocks is optional, not required. Do you have any suggestion? Just add a second look at the sama5d2 and iscck gck are not optional. What we call optional clocks are clocks that are not necessarily required depending on the IP revision or external clocks that are board dependent. This is not the case here: iscck and gck are internal to the SoC, and are always available. Whether you want to use them or not is a different question, and should not be encoded in the clocks/clock-names properties. To sum-up, define those 3 clocks as required and document their names. Thank you for your suggestion.
Re: [PATCH v5 0/2] [media] atmel-isc: add driver for Atmel ISC
On 7/1/2016 20:20, Hans Verkuil wrote: Hi Songjun, First of all, please CC patch 2/2 to linux-media as well the next time you post this. I only see 1/2 on the mailinglist, and we need both. Secondly, before I can accept it you need to run the v4l2-compliance test first and I need to see the output of that test. The compliance test is here: https://git.linuxtv.org/v4l-utils.git. Always compile it from the repository so you know you are using the latest most up to date version. Since this driver supports multiple pixelformats you need to test with the -f option, which tests streaming for all pixelformats. Obviously, there shouldn't be any FAILs :-) I greatly simplifies the code review if I know it passes the compliance test. Hi Hans, You suggestion is very helpful to me. I will give the output of the compliance test in next version. Regards, Hans
Re: [PATCH] [media] atmel-isc: release the filehandle if it's not the only one.
Sorry, my mistake, the device should be able to opened multiple times.
It's a wrong patch.
On 11/1/2016 16:52, Hans Verkuil wrote:
On 01/11/16 09:08, Songjun Wu wrote:
Release the filehandle in 'isc_open' if it's not the only filehandle
opened for the associated video_device.
What's wrong with that? You should always be able to open the device
multiple times. v4l2-compliance will fail after this patch. I'm not sure
what you intended to do here, but this patch is wrong.
Regards,
Hans
Signed-off-by: Songjun Wu
---
drivers/media/platform/atmel/atmel-isc.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index 8e25d3f..5e08404 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -926,21 +926,21 @@ static int isc_open(struct file *file)
if (ret < 0)
goto unlock;
-if (!v4l2_fh_is_singular_file(file))
-goto unlock;
+ret = !v4l2_fh_is_singular_file(file);
+if (ret)
+goto fh_rel;
ret = v4l2_subdev_call(sd, core, s_power, 1);
-if (ret < 0 && ret != -ENOIOCTLCMD) {
-v4l2_fh_release(file);
-goto unlock;
-}
+if (ret < 0 && ret != -ENOIOCTLCMD)
+goto fh_rel;
ret = isc_set_fmt(isc, &isc->fmt);
-if (ret) {
+if (ret)
v4l2_subdev_call(sd, core, s_power, 0);
-v4l2_fh_release(file);
-}
+fh_rel:
+if (ret)
+v4l2_fh_release(file);
unlock:
mutex_unlock(&isc->lock);
return ret;
Re: [PATCH] kobject: set state_initialized to 0 in kobject_cleanup
On 11/1/2016 22:56, Greg KH wrote: On Tue, Nov 01, 2016 at 06:41:44PM +0800, Songjun Wu wrote: If state_initialized is not set to 0 when a kobject is released, a device is registered, unregistered, and registered again, the error below will occur. kobject (dec04bb0): tried to init an initialized object, something is seriously wrong. Yes, your code is wrong, don't try to change the kernel core to work around it :) That message is there for a reason, and this patch has been rejected many times in the past. kobjects can NOT ever be reused, and should never be static (but yes, there are lots of in-kernel users with static kobjects, they just never get reused...) What code is emitting this message? I'll be glad to help you fix it up if you can point me at it. Thank you very much. I will not use the static kobjects to fix the error. :) thanks, greg k-h
Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue
Hi Colin,
Thank you for your comment.
It is a bug, will be fixed in the next patch.
On 3/7/2017 22:30, Colin King wrote:
From: Colin Ian King
The are only HIST_ENTRIES worth of entries in hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry. Fix this by iterating by
the correct number of times.
Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
Signed-off-by: Colin Ian King
---
drivers/media/platform/atmel/atmel-isc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
*hist_count = 0;
- for (i = 0; i <= HIST_ENTRIES; i++)
+ for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
}
Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue
On 3/9/2017 19:50, Colin Ian King wrote:
On 09/03/17 11:49, walter harms wrote:
Am 09.03.2017 11:57, schrieb Hans Verkuil:
Hi Songjun,
On 08/03/17 03:25, Wu, Songjun wrote:
Hi Colin,
Thank you for your comment.
It is a bug, will be fixed in the next patch.
Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.
Regards,
Hans
perhaps he will make it a bit more readable, like:
*hist_count += i * (*hist_entry++);
*hist_count += hist_entry[i]*i;
As long as it gets fixed somehow, then I'm happy.
You suggestion is very good, I will modify it like this.
Thank you.
Colin
re,
wh
On 3/7/2017 22:30, Colin King wrote:
From: Colin Ian King
The are only HIST_ENTRIES worth of entries in hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry. Fix this by iterating by
the correct number of times.
Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
Signed-off-by: Colin Ian King
---
drivers/media/platform/atmel/atmel-isc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
*hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
}
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue
On 3/9/2017 18:57, Hans Verkuil wrote:
Hi Songjun,
On 08/03/17 03:25, Wu, Songjun wrote:
Hi Colin,
Thank you for your comment.
It is a bug, will be fixed in the next patch.
Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.
Hi Hans,
I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches,
based on the current master branch?
Regards,
Hans
On 3/7/2017 22:30, Colin King wrote:
From: Colin Ian King
The are only HIST_ENTRIES worth of entries in hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry. Fix this by iterating by
the correct number of times.
Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
Signed-off-by: Colin Ian King
---
drivers/media/platform/atmel/atmel-isc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
*hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
}
Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue
On 3/13/2017 17:25, Hans Verkuil wrote:
On 03/13/2017 06:53 AM, Wu, Songjun wrote:
On 3/9/2017 18:57, Hans Verkuil wrote:
Hi Songjun,
On 08/03/17 03:25, Wu, Songjun wrote:
Hi Colin,
Thank you for your comment.
It is a bug, will be fixed in the next patch.
Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.
Hi Hans,
I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches,
based on the current master branch?
Huh? Where do you see that this patch is merged? I don't see it in the
media_tree master
branch.
Hi Hans,
I see this patch on the master branch in media_tree.
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/atmel/atmel-isc.c
Regards,
Hans
Regards,
Hans
On 3/7/2017 22:30, Colin King wrote:
From: Colin Ian King
The are only HIST_ENTRIES worth of entries in hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry. Fix this by iterating by
the correct number of times.
Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
Signed-off-by: Colin Ian King
---
drivers/media/platform/atmel/atmel-isc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
*hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
}
Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 8/8/2016 17:56, Hans Verkuil wrote:
On 08/08/2016 11:37 AM, Hans Verkuil wrote:
On 08/03/2016 10:08 AM, Songjun Wu wrote:
Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.
Signed-off-by: Songjun Wu
---
Changes in v8:
- Power on the sensor on the first open in function
'isc_open'.
- Power off the sensor on the last release in function
'isc_release'.
- Remove the switch of the pipeline.
Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.
Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.
Changes in v4:
- Modify the isc clock code since the dt is changed.
Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.
Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
in Kconfig file.
- Correct typos and coding style according to Laurent's remarks
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
'isc_queue_setup' function.
- Invoke isc_config to configure register in
'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
module_platform_driver().
drivers/media/platform/Kconfig|1 +
drivers/media/platform/Makefile |2 +
drivers/media/platform/atmel/Kconfig |9 +
drivers/media/platform/atmel/Makefile |1 +
drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++
drivers/media/platform/atmel/atmel-isc.c | 1503 +
6 files changed, 1681 insertions(+)
create mode 100644 drivers/media/platform/atmel/Kconfig
create mode 100644 drivers/media/platform/atmel/Makefile
create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
create mode 100644 drivers/media/platform/atmel/atmel-isc.c
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
+static int isc_set_default_fmt(struct isc_device *isc)
+{
+ u32 index = isc->num_user_formats - 1;
Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.
+ struct v4l2_format f = {
+ .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+ .fmt.pix = {
+ .width = VGA_WIDTH,
+ .height = VGA_HEIGHT,
+ .field = V4L2_FIELD_NONE,
+ .pixelformat= isc->user_formats[index]->fourcc,
+ },
+ };
+
+ return isc_set_fmt(isc, &f);
+}
+
+static int isc_open(struct file *file)
+{
+ struct isc_device *isc = video_drvdata(file);
+ struct v4l2_subdev *sd = isc->current_subdev->sd;
+ int ret;
+
+ if (mutex_lock_interruptible(&isc->lock))
+ return -ERESTARTSYS;
+
+ ret = v4l2_fh_open(file);
+ if (ret < 0)
+ goto unlock;
+
+ if (!v4l2_fh_is_singular_file(file))
+ goto unlock;
+
+ ret = v4l2_subdev_call(sd, core, s_power, 1);
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ goto unlock;
+
+ ret = isc_set_default_fmt(isc);
This doesn't belong here, this needs to be done in isc_async_complete().
Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.
Actually, you do need to set the format here since here is where you turn on
the sensor power, but it should be the current format, not the default format.
And in isc_set_default_fmt I recommend that you call the try fmt of the subdev
in order to let the subdev adjust the p
Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 8/10/2016 15:12, Hans Verkuil wrote:
On 08/10/2016 07:36 AM, Wu, Songjun wrote:
On 8/8/2016 17:56, Hans Verkuil wrote:
On 08/08/2016 11:37 AM, Hans Verkuil wrote:
On 08/03/2016 10:08 AM, Songjun Wu wrote:
Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.
Signed-off-by: Songjun Wu
---
Changes in v8:
- Power on the sensor on the first open in function
'isc_open'.
- Power off the sensor on the last release in function
'isc_release'.
- Remove the switch of the pipeline.
Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.
Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.
Changes in v4:
- Modify the isc clock code since the dt is changed.
Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.
Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
in Kconfig file.
- Correct typos and coding style according to Laurent's remarks
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
'isc_queue_setup' function.
- Invoke isc_config to configure register in
'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
module_platform_driver().
drivers/media/platform/Kconfig|1 +
drivers/media/platform/Makefile |2 +
drivers/media/platform/atmel/Kconfig |9 +
drivers/media/platform/atmel/Makefile |1 +
drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++
drivers/media/platform/atmel/atmel-isc.c | 1503 +
6 files changed, 1681 insertions(+)
create mode 100644 drivers/media/platform/atmel/Kconfig
create mode 100644 drivers/media/platform/atmel/Makefile
create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
create mode 100644 drivers/media/platform/atmel/atmel-isc.c
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
+static int isc_set_default_fmt(struct isc_device *isc)
+{
+ u32 index = isc->num_user_formats - 1;
Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.
+ struct v4l2_format f = {
+ .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+ .fmt.pix = {
+ .width = VGA_WIDTH,
+ .height = VGA_HEIGHT,
+ .field = V4L2_FIELD_NONE,
+ .pixelformat= isc->user_formats[index]->fourcc,
+ },
+ };
+
+ return isc_set_fmt(isc, &f);
+}
+
+static int isc_open(struct file *file)
+{
+ struct isc_device *isc = video_drvdata(file);
+ struct v4l2_subdev *sd = isc->current_subdev->sd;
+ int ret;
+
+ if (mutex_lock_interruptible(&isc->lock))
+ return -ERESTARTSYS;
+
+ ret = v4l2_fh_open(file);
+ if (ret < 0)
+ goto unlock;
+
+ if (!v4l2_fh_is_singular_file(file))
+ goto unlock;
+
+ ret = v4l2_subdev_call(sd, core, s_power, 1);
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ goto unlock;
+
+ ret = isc_set_default_fmt(isc);
This doesn't belong here, this needs to be done in isc_async_complete().
Having the code here means that every time you open the device
Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 8/1/2016 17:47, Hans Verkuil wrote:
Hi Songjun,
Some more comments below. Except for one in the open/release functions
it's all small things.
On 07/29/2016 09:54 AM, Songjun Wu wrote:
Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.
Signed-off-by: Songjun Wu
---
Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.
Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.
Changes in v4:
- Modify the isc clock code since the dt is changed.
Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.
Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
in Kconfig file.
- Correct typos and coding style according to Laurent's remarks
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
'isc_queue_setup' function.
- Invoke isc_config to configure register in
'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
module_platform_driver().
drivers/media/platform/Kconfig|1 +
drivers/media/platform/Makefile |2 +
drivers/media/platform/atmel/Kconfig |9 +
drivers/media/platform/atmel/Makefile |1 +
drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++
drivers/media/platform/atmel/atmel-isc.c | 1611 +
6 files changed, 1789 insertions(+)
create mode 100644 drivers/media/platform/atmel/Kconfig
create mode 100644 drivers/media/platform/atmel/Makefile
create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
create mode 100644 drivers/media/platform/atmel/atmel-isc.c
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..f2ef664
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -0,0 +1,1611 @@
+static unsigned int sensor_preferred = 1;
+module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(sensor_preferred,
+"Sensor is preferred to output the specified format (1-on 0-off)
default 1");
I have no idea what this means. Can you elaborate? Why would you want to set
this to 0?
ISC can convert the raw format to the other format, e.g. YUYV.
If we want to output YUYV format, there are two choices, one is the
sensor output YUYV format, ISC bypass the data to the memory, the other
is the sensor output raw format, ISC convert raw format to YUYV.
So I provide a module parameter to user to select.
I prefer to select the sensor to output the specified format, then I set
this parameter to '1', not '0'.
+static inline bool sensor_is_preferred(const struct isc_format *isc_fmt)
+{
+ if ((sensor_preferred && isc_fmt->sd_support) ||
+ !isc_fmt->isc_support)
I'd just do:
return (sensor_preferred && isc_fmt->sd_support) ||
!isc_fmt->isc_support;
Accept, thank you.
+ return true;
+ else
+ return false;
+}
+
+static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
+ struct isc_format **current_fmt, u32 *code)
+{
+ struct isc_format *isc_fmt;
+ struct v4l2_pix_format *pixfmt = &f->fmt.pix;
+ struct v4l2_subdev_format format = {
+ .which = V4L2_SUBDEV_FORMAT_TRY,
+ };
+ u32 mbus_code;
+ int ret;
+
+ if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
+ isc_fmt = find_format_by_fourcc(isc, pixfmt->pixelformat);
+ if (!isc_fmt) {
+ v4l2_warn(&isc->v4l2_dev, "Format 0x%x not found\n",
+ pixfmt->pixelformat);
+ isc_fmt = isc->user_formats[isc->num_user_
Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code
On 8/2/2016 15:32, Hans Verkuil wrote: On 08/02/2016 08:20 AM, Wu, Songjun wrote: +static unsigned int sensor_preferred = 1; +module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(sensor_preferred, +"Sensor is preferred to output the specified format (1-on 0-off) default 1"); I have no idea what this means. Can you elaborate? Why would you want to set this to 0? ISC can convert the raw format to the other format, e.g. YUYV. If we want to output YUYV format, there are two choices, one is the sensor output YUYV format, ISC bypass the data to the memory, the other is the sensor output raw format, ISC convert raw format to YUYV. So I provide a module parameter to user to select. I prefer to select the sensor to output the specified format, then I set this parameter to '1', not '0'. Does this only apply to YUYV? The reason I am hesitant about this option is that I am not convinced you need it. The default (sensor preferred) makes sense and that's what other drivers do as well. Unless you know of a real use-case where you want to set this to 0, I would just drop this option. If there *is* a real use-case, then split off adding this module option into a separate patch so we can discuss it more without blocking getting this driver into mainline. I don't like the way this is done here. This does not only apply to YUYV, ISC IP defines some formats that can be converted from raw format. In some scenarios, ISC can extend the formats, e.g. if the sensor does not support YUYV, but raw format, the ISC can output YUYV. OK, I will remove the related code. After this driver is accepted, I will submit another patch to add this feature. Thank you. Regards, Hans
Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
On 6/9/2016 05:57, Boris Brezillon wrote:
On Tue, 7 Jun 2016 15:11:53 +0800
Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
Changes in v4:
- Remove the isc clock nodes.
Changes in v3:
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.
Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.
.../devicetree/bindings/media/atmel-isc.txt| 69 ++
1 file changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..3f83524
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,69 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties for ISC:
+- compatible
+ Must be "atmel,sama5d2-isc".
+- reg
+ Physical base address and length of the registers set for the device.
+- interrupts
+ Should contain IRQ line for the ISC.
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock".
+- #clock-cells
+ Should be 0.
+- clock-output-names
+ Should contain the name of the clock driving the sensor master clock.
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
+
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ clock-names = "hclock";
You have 3 clocks here and only one name. Are you sure this example is
actually working?
The isc_clk is mandatory, but the other two clocks are optional, so I
did not give their name. This example is tested.
Should I add the name for the other two clocks?
+ #clock-cells = <0>;
+ clock-output-names = "isc-mck";
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_0: endpoint {
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
+ };
+ };
+
+
+};
+
+i2c1: i2c@fc028000 {
+ ov7740: camera@21 {
+ compatible = "ovti,ov7740";
+ reg = <0x21>;
+
+ clocks = <&isc>;
+ clock-names = "xvclk";
+ assigned-clocks = <&isc>;
+ assigned-clock-rates = <2400>;
+
+ port {
+ ov7740_0: endpoint {
+ remote-endpoint = <&isc_0>;
+ };
+ };
+};
Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
On 6/12/2016 15:23, Boris Brezillon wrote:
On Sun, 12 Jun 2016 11:04:27 +0800
"Wu, Songjun" wrote:
On 6/9/2016 05:57, Boris Brezillon wrote:
On Tue, 7 Jun 2016 15:11:53 +0800
Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
Changes in v4:
- Remove the isc clock nodes.
Changes in v3:
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.
Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.
.../devicetree/bindings/media/atmel-isc.txt| 69 ++
1 file changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..3f83524
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,69 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties for ISC:
+- compatible
+ Must be "atmel,sama5d2-isc".
+- reg
+ Physical base address and length of the registers set for the device.
+- interrupts
+ Should contain IRQ line for the ISC.
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock".
+- #clock-cells
+ Should be 0.
+- clock-output-names
+ Should contain the name of the clock driving the sensor master clock.
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
+
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ clock-names = "hclock";
You have 3 clocks here and only one name. Are you sure this example is
actually working?
The isc_clk is mandatory, but the other two clocks are optional, so I
did not give their name. This example is tested.
Should I add the name for the other two clocks?
It only works here because you're using these clocks as the parents of
the ispck ismck clocks, but that's an implementation detail, and
consumer are usually referencing their clocks by name. So yes, I'd
recommend specifying names here, even if it's not strictly required by
your current implementation.
Accept. The clock names will be specified.
Thank you.
+ #clock-cells = <0>;
+ clock-output-names = "isc-mck";
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_0: endpoint {
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
+ };
+ };
+
+
+};
+
+i2c1: i2c@fc028000 {
+ ov7740: camera@21 {
+ compatible = "ovti,ov7740";
+ reg = <0x21>;
+
+ clocks = <&isc>;
+ clock-names = "xvclk";
+ assigned-clocks = <&isc>;
+ assigned-clock-rates = <2400>;
+
+ port {
+ ov7740_0: endpoint {
+ remote-endpoint = <&isc_0>;
+ };
+ };
+};
Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
On 6/9/2016 04:00, Rob Herring wrote:
On Tue, Jun 07, 2016 at 03:11:53PM +0800, Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
Changes in v4:
- Remove the isc clock nodes.
Changes in v3:
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.
Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.
.../devicetree/bindings/media/atmel-isc.txt| 69 ++
1 file changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
Almost there...
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..3f83524
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,69 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties for ISC:
+- compatible
+ Must be "atmel,sama5d2-isc".
+- reg
+ Physical base address and length of the registers set for the device.
+- interrupts
+ Should contain IRQ line for the ISC.
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock".
+- #clock-cells
+ Should be 0.
+- clock-output-names
+ Should contain the name of the clock driving the sensor master clock.
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
Required, but not in your example.
I will add it to the example.
+
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ clock-names = "hclock";
+ #clock-cells = <0>;
+ clock-output-names = "isc-mck";
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
These 2 you can drop.
Accept. Thank you.
+
+ isc_0: endpoint {
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
+ };
+ };
+
+
+};
+
+i2c1: i2c@fc028000 {
+ ov7740: camera@21 {
+ compatible = "ovti,ov7740";
Fix the indentation.
Accept. Thank you.
+ reg = <0x21>;
+
+ clocks = <&isc>;
+ clock-names = "xvclk";
+ assigned-clocks = <&isc>;
+ assigned-clock-rates = <2400>;
+
+ port {
+ ov7740_0: endpoint {
+ remote-endpoint = <&isc_0>;
+ };
+ };
+};
--
2.7.4
Re: [PATCH v3 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
Hi Rob,
Thank you for your comments.
On 6/3/2016 07:16, Rob Herring wrote:
On Tue, May 31, 2016 at 02:58:23PM +0800, Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
Changes in v3:
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.
Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.
.../devicetree/bindings/media/atmel-isc.txt| 88 ++
1 file changed, 88 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..2ae1d60
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,88 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties for ISC:
+- compatible
+ Must be "atmel,sama5d2-isc"
+- reg
+ Physical base address and length of the registers set for the device;
+- interrupts
+ Should contain IRQ line for the ISI;
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock", "ispck".
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
+- isc-ispck
+ The clock for the ISC digital pipeline.
+ - compatible
+ Must be "atmel,sama5d2-isc-ispck".
+ - clock-cells
+ From common clock binding; should be set to 0.
+ - clocks
+ The clock source phandles.
+- isc-mck
+ The clock for the image sensor.
+ - compatible
+ Must be "atmel,sama5d2-isc-mck".
+ - clock-cells
+ From common clock binding; should be set to 0.
+ - clocks
+ The clock source phandles.
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&isc_ispck>;
+ clock-names = "hclock", "ispck";
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_0: endpoint {
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
+ };
+ };
+
+ isc_ispck: isc-ispck@0 {
Drop the unit-address. You should only have one if you have a reg
property.
Accept. Thank you.
+ compatible = "atmel,sama5d2-isc-ispck";
+ #clock-cells = <0>;
+ clocks = <&isc_clk>, <&iscck>;
+ };
+
+ isc_mck: isc-mck@1 {
ditto.
I still think these should be implied by atmel,sama5d2-isc and not in
DT. The fact that you don't have any registers for them pretty much
indicates that. It is also strange that isc-ispck is used by isc. If
that is the only user, there is certainly no need to put it in DT.
The isc-ispck in only used by isc, it can be removed from DT.
Thank you.
+ compatible = "atmel,sama5d2-isc-mck";
+ #clock-cells = <0>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ };
+};
+
+i2c1: i2c@fc028000 {
+ ov7740: camera@21 {
+ compatible = "ovti,ov7740";
+ reg = <0x21>;
+
+ clocks = <&isc_mck>;
+ clock-names = "xvclk";
+ assigned-clocks = <&isc_mck>;
+ assigned-clock-rates = <2400>;
+
+ port {
+ ov7740_0: endpoint {
+ remote-endpoint = <&isc_0>;
+ };
+ };
+};
--
2.7.4
Re: [PATCH v3 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
On 6/3/2016 19:10, Boris Brezillon wrote:
On Thu, 2 Jun 2016 18:16:09 -0500
Rob Herring wrote:
On Tue, May 31, 2016 at 02:58:23PM +0800, Songjun Wu wrote:
DT binding documentation for ISC driver.
Signed-off-by: Songjun Wu
---
Changes in v3:
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.
Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.
.../devicetree/bindings/media/atmel-isc.txt| 88 ++
1 file changed, 88 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 000..2ae1d60
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,88 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties for ISC:
+- compatible
+ Must be "atmel,sama5d2-isc"
+- reg
+ Physical base address and length of the registers set for the device;
+- interrupts
+ Should contain IRQ line for the ISI;
+- clocks
+ List of clock specifiers, corresponding to entries in
+ the clock-names property;
+ Please refer to clock-bindings.txt.
+- clock-names
+ Required elements: "hclock", "ispck".
+- pinctrl-names, pinctrl-0
+ Please refer to pinctrl-bindings.txt.
+- isc-ispck
+ The clock for the ISC digital pipeline.
+ - compatible
+ Must be "atmel,sama5d2-isc-ispck".
+ - clock-cells
+ From common clock binding; should be set to 0.
+ - clocks
+ The clock source phandles.
+- isc-mck
+ The clock for the image sensor.
+ - compatible
+ Must be "atmel,sama5d2-isc-mck".
+ - clock-cells
+ From common clock binding; should be set to 0.
+ - clocks
+ The clock source phandles.
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+ compatible = "atmel,sama5d2-isc";
+ reg = <0xf0008000 0x4000>;
+ interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&isc_clk>, <&isc_ispck>;
+ clock-names = "hclock", "ispck";
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ isc_0: endpoint {
+ remote-endpoint = <&ov7740_0>;
+ hsync-active = <1>;
+ vsync-active = <0>;
+ pclk-sample = <1>;
+ };
+ };
+
+ isc_ispck: isc-ispck@0 {
Drop the unit-address. You should only have one if you have a reg
property.
+ compatible = "atmel,sama5d2-isc-ispck";
+ #clock-cells = <0>;
+ clocks = <&isc_clk>, <&iscck>;
+ };
+
+ isc_mck: isc-mck@1 {
ditto.
I still think these should be implied by atmel,sama5d2-isc and not in
DT. The fact that you don't have any registers for them pretty much
indicates that. It is also strange that isc-ispck is used by isc. If
that is the only user, there is certainly no need to put it in DT.
I had a look at the block diagram, and it seems you are partially right.
The only clock that is really exported by the ISC is isc_mck (isc_pck is
not).
So I'd suggest dropping these clk sub-nodes, putting the #clock-cells,
and clock-names properties in the isc node, and then referencing the
isc node in you i2c device.
isc: isc@f0008000 {
compatible = "atmel,sama5d2-isc";
reg = <0xf0008000 0x4000>;
interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
clocks = <&isc_clk>, <&isc_ispck>;
clock-names = "hclock", "ispck";
#clock-cells = <0>;
clock-output-names = "isc-mck";
};
i2c1: i2c@fc028000 {
/* ... */
ov7740: camera@21 {
/* ... */
clocks = <&isc>;
clock-names = "xvclk";
}
};
If I'm wrong and the ISC IP is really exposing several clks, you just
have to change #clock-cells to 1, and use clocks = <&isc X>; in the i2c
device node.
Rob, Songjun, would you agree on this representation?
Agree, I think it's a good solution.
Thank you for your comments.
+ compatible = "atmel,sama5d2-isc-mck";
+ #clock-cells = <0>;
+ clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+ };
+};
+
+i2c1: i2c@fc028000 {
+ ov7740: camera@21 {
+ compatible = "ovti,ov7740";
+ reg = <0x21>;
+
+
Re: [PATCH v2 0/2] ASoC: atmel-classd: add driver for Atmel CLASSD
On 9/26/2015 02:07, Mark Brown wrote: On Thu, Sep 24, 2015 at 01:41:25PM +0800, Songjun Wu wrote: Songjun Wu (2): ASoC: atmel-classd: add the Audio Class D Amplifier ASoC: atmel-classd: DT binding for Class D audio amplifier driver I don't seem to have the second patch with the binding documentation. Sorry, I use the patman to send the patch this time. And the patman will sent the patch automatically. I didn't check, and it missed you. I will forward to you at once. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH] ASoC: atmel-classd: fix odd_ptr_err.cocci warnings
On 9/28/2015 19:26, Peter Korsgaard wrote:
"kbuild" == kbuild test robot writes:
> sound/soc/atmel/atmel-classd.c:578:5-11: inconsistent IS_ERR and PTR_ERR,
PTR_ERR on line 579
> PTR_ERR should access the value just tested by IS_ERR
> Semantic patch information:
> There can be false positives in the patch case, where it is the call
> IS_ERR that is wrong.
> Generated by: scripts/coccinelle/tests/odd_ptr_err.cocci
> CC: Songjun Wu
> Signed-off-by: Fengguang Wu
> ---
> atmel-classd.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> --- a/sound/soc/atmel/atmel-classd.c
> +++ b/sound/soc/atmel/atmel-classd.c
> @@ -576,7 +576,7 @@ static int atmel_classd_probe(struct pla
dd-> gclk = devm_clk_get(dev, "gclk");
> if (IS_ERR(dd->aclk)) {
> -ret = PTR_ERR(dd->gclk);
> +ret = PTR_ERR(dd->aclk);
It looks like it is the other way around. It should test
IS_ERR(dd->gclk) instead.
Thank you, you are right.
It should test IS_ERR(dd->gclk) instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ASoC: atmel-classd: DT binding for Class D audio amplifier driver
On 9/17/2015 03:42, Mark Brown wrote: On Tue, Sep 15, 2015 at 11:11:53AM +0800, Wu, Songjun wrote: I try to use one entry, but there is a problem. It's about 'driver_data' in struct device. In function snd_soc_register_card, the parameter 'card' will be set to 'driver_data' by the code 'dev_set_drvdata(card->dev, card)'. Then some resources(eg. regmap, clock) also need be recorded by 'driver_data'. One entry could only has one 'driver_data'. I think the best way is to create two entries, like the current dts. What's your opinion? Look at the recently applied sunxi driver for an example of how to do this - it's a similar piece of hardware (entirely in the SoC and so on). Thank you, It really helps me. I will make a second version soon. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] [media] atmel-isc: mark PM functions as __maybe_unused
Hi Arnd,
Thank you for your patch.
I think it's better to add switch CONFIG_PM, but the PM feature is a
must, or the ISC can not work, maybe the best choice is to add 'depends
on PM' in Kconfig.
#ifdef CONFIG_PM
isc_runtime_suspend
{
XXX
}
isc_runtime_resume
{
XXX
}
static const struct dev_pm_ops atmel_isc_dev_pm_ops = {
SET_RUNTIME_PM_OPS(isc_runtime_suspend, isc_runtime_resume, NULL)
};
#define ATMEL_ISC_PM_OPS(&atmel_isc_dev_pm_ops)
#else
#define ATMEL_ISC_PM_OPSNULL
#endif
static struct platform_driver atmel_isc_driver = {
.probe = atmel_isc_probe,
.remove = atmel_isc_remove,
.driver = {
.name = ATMEL_ISC_NAME,
.pm = ATMEL_ISC_PM_OPS,
.of_match_table = of_match_ptr(atmel_isc_of_match),
},
};
On 9/12/2016 23:32, Arnd Bergmann wrote:
The newly added atmel-isc driver uses SET_RUNTIME_PM_OPS() to
refer to its suspend/resume functions, causing a warning when
CONFIG_PM is not set:
media/platform/atmel/atmel-isc.c:1477:12: error: 'isc_runtime_resume' defined
but not used [-Werror=unused-function]
media/platform/atmel/atmel-isc.c:1467:12: error: 'isc_runtime_suspend' defined
but not used [-Werror=unused-function]
This adds __maybe_unused annotations to avoid the warning without
adding an error-prone #ifdef around it.
Signed-off-by: Arnd Bergmann
---
drivers/media/platform/atmel/atmel-isc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index db6773de92f0..a9ab7ae89f04 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1464,7 +1464,7 @@ static int atmel_isc_remove(struct platform_device *pdev)
return 0;
}
-static int isc_runtime_suspend(struct device *dev)
+static int __maybe_unused isc_runtime_suspend(struct device *dev)
{
struct isc_device *isc = dev_get_drvdata(dev);
@@ -1474,7 +1474,7 @@ static int isc_runtime_suspend(struct device *dev)
return 0;
}
-static int isc_runtime_resume(struct device *dev)
+static int __maybe_unused isc_runtime_resume(struct device *dev)
{
struct isc_device *isc = dev_get_drvdata(dev);
int ret;
Re: [PATCH v9 0/2] [media] atmel-isc: add driver for Atmel ISC
On 8/12/2016 15:32, Hans Verkuil wrote: One quick question: On 08/11/2016 09:06 AM, Songjun Wu wrote: The Image Sensor Controller driver includes two parts. 1) Driver code to implement the ISC function. 2) Device tree binding documentation, it describes how to add the ISC in device tree. Test result with v4l-utils. # v4l2-compliance -f v4l2-compliance SHA : not available Driver Info: Driver name : atmel_isc Card type : Atmel Image Sensor Controller Bus info : platform:atmel_isc f0008000.isc Driver version: 4.7.0 Capabilities : 0x8421 Video Capture Streaming Extended Pix Format Device Capabilities Device Caps : 0x0421 Video Capture Streaming Extended Pix Format Compliance test for device /dev/video0 (not using libv4l2): Required ioctls: test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second video open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Test input 0: Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Can you confirm that the sensor subdevice you are using does not have any controls? I ask since that is fairly unusual, so I want to make sure that controls are really not supported in this setup. Sorry for the late reply. The subdevice I use supports controls, but I did not develop the v4l2 controls in the sensor driver. Should I add the v4l2 controls and test again? Regards, Hans
Re: [PATCH v9 0/2] [media] atmel-isc: add driver for Atmel ISC
On 8/15/2016 15:15, Hans Verkuil wrote: On 08/15/2016 08:09 AM, Wu, Songjun wrote: On 8/12/2016 15:32, Hans Verkuil wrote: One quick question: On 08/11/2016 09:06 AM, Songjun Wu wrote: The Image Sensor Controller driver includes two parts. 1) Driver code to implement the ISC function. 2) Device tree binding documentation, it describes how to add the ISC in device tree. Test result with v4l-utils. # v4l2-compliance -f v4l2-compliance SHA : not available Driver Info: Driver name : atmel_isc Card type : Atmel Image Sensor Controller Bus info : platform:atmel_isc f0008000.isc Driver version: 4.7.0 Capabilities : 0x8421 Video Capture Streaming Extended Pix Format Device Capabilities Device Caps : 0x0421 Video Capture Streaming Extended Pix Format Compliance test for device /dev/video0 (not using libv4l2): Required ioctls: test VIDIOC_QUERYCAP: OK Allow for multiple opens: test second video open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Test input 0: Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Can you confirm that the sensor subdevice you are using does not have any controls? I ask since that is fairly unusual, so I want to make sure that controls are really not supported in this setup. Sorry for the late reply. The subdevice I use supports controls, but I did not develop the v4l2 controls in the sensor driver. So you mean the sensor hardware has controls, but the sensor driver doesn't implement them? Do I understand you correctly? Yes, your understanding is correct. Should I add the v4l2 controls and test again? If the sensor driver does not implement controls (i.e. has a struct v4l2_ctrl_handler), then everything is fine and the v4l2-compliance output is correct. Please confirm this. I just want to be 100% certain about this before I make the pull request. I can confirm this. I use the sensor ov7740, and the driver is developed by myself, I did not add the v4l2 controls into the sensor driver for now. Thanks, Hans
Re: [PATCH v9 0/2] [media] atmel-isc: add driver for Atmel ISC
On 8/15/2016 15:34, Hans Verkuil wrote: On 08/11/2016 09:06 AM, Songjun Wu wrote: The Image Sensor Controller driver includes two parts. 1) Driver code to implement the ISC function. 2) Device tree binding documentation, it describes how to add the ISC in device tree. So close... Running checkpatch gives me: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #133: new file mode 100644 Can you make a patch adding an entry to MAINTAINERS? No need to repost the other two. Thank you. Nicolas or I will make a patch to add an entry to MAINTAINERS. Regards, Hans
