Re: [PATCH v2 4/7] media: vb2-dma-contig: add helper for setting dma max seg size
Hi Marek,
On Monday 14 December 2015 10:20:22 Marek Szyprowski wrote:
> On 2015-12-13 20:57, Laurent Pinchart wrote:
> > On Wednesday 09 December 2015 14:58:19 Marek Szyprowski wrote:
> >> Add a helper function for device drivers to set DMA's max_seg_size.
> >> Setting it to largest possible value lets DMA-mapping API always create
> >> contiguous mappings in DMA address space. This is essential for all
> >> devices, which use dma-contig videobuf2 memory allocator and shared
> >> buffers.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> >> ---
> >>
> >> drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++
> >> include/media/videobuf2-dma-contig.h | 1 +
> >> 2 files changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index c331272..628518d
> >> 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -742,6 +742,21 @@ void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
> >>
> >> }
> >> EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
> >>
> >> +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int
> >> size)
> >> +{
> >> + if (!dev->dma_parms) {
> >
> > When can this function be called with dev->dma_parms not NULL ?
>
> When one loads a module with multimedia driver (which calls this
> function), then unloads and loads it again. It is rather safe to have this
> check.
Don't you have a much bigger problem in that case ? When you unload the module
the device will be unbound from the driver, causing the memory allocated by
devm_kzalloc to be freed. dev->dma_parms will then point to freed memory,
which will get reused by all subsequent calls to dma_get_max_seg_size(),
dma_get_max_seg_size() & co (including the ones in this function).
> >> + dev->dma_parms = devm_kzalloc(dev, sizeof(dev->dma_parms),
> >> +GFP_KERNEL);
> >> + if (!dev->dma_parms)
> >> + return -ENOMEM;
> >> + }
> >> + if (dma_get_max_seg_size(dev) < size)
> >> + return dma_set_max_seg_size(dev, size);
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
> >> +
> >> MODULE_DESCRIPTION("DMA-contig memory handling routines for
> >> videobuf2");
> >> MODULE_AUTHOR("Pawel Osciak <pa...@osciak.com>");
> >> MODULE_LICENSE("GPL");
> >> diff --git a/include/media/videobuf2-dma-contig.h
> >> b/include/media/videobuf2-dma-contig.h index c33dfa6..0e6ba64 100644
> >> --- a/include/media/videobuf2-dma-contig.h
> >> +++ b/include/media/videobuf2-dma-contig.h
> >> @@ -26,6 +26,7 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb,
> >> unsigned int plane_no)
> >> void *vb2_dma_contig_init_ctx(struct device *dev);
> >> void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);
> >> +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int
> >> size);
> >> extern const struct vb2_mem_ops vb2_dma_contig_memops;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/7] media: vb2-dma-contig: add helper for setting dma max seg size
Hi Marek,
Thank you for the patch.
On Wednesday 09 December 2015 14:58:19 Marek Szyprowski wrote:
> Add a helper function for device drivers to set DMA's max_seg_size.
> Setting it to largest possible value lets DMA-mapping API always create
> contiguous mappings in DMA address space. This is essential for all
> devices, which use dma-contig videobuf2 memory allocator and shared
> buffers.
>
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---
> drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++
> include/media/videobuf2-dma-contig.h | 1 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index c331272..628518d
> 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -742,6 +742,21 @@ void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
> }
> EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
>
> +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> + if (!dev->dma_parms) {
When can this function be called with dev->dma_parms not NULL ?
> + dev->dma_parms = devm_kzalloc(dev, sizeof(dev->dma_parms),
> + GFP_KERNEL);
> + if (!dev->dma_parms)
> + return -ENOMEM;
> + }
> + if (dma_get_max_seg_size(dev) < size)
> + return dma_set_max_seg_size(dev, size);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
> +
> MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
> MODULE_AUTHOR("Pawel Osciak <pa...@osciak.com>");
> MODULE_LICENSE("GPL");
> diff --git a/include/media/videobuf2-dma-contig.h
> b/include/media/videobuf2-dma-contig.h index c33dfa6..0e6ba64 100644
> --- a/include/media/videobuf2-dma-contig.h
> +++ b/include/media/videobuf2-dma-contig.h
> @@ -26,6 +26,7 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb,
> unsigned int plane_no)
>
> void *vb2_dma_contig_init_ctx(struct device *dev);
> void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);
> +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size);
>
> extern const struct vb2_mem_ops vb2_dma_contig_memops;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/7] Exynos: MFC driver: reserved memory cleanup and IOMMU support
Hi Marek, Thank you for the patches. On Wednesday 09 December 2015 14:58:15 Marek Szyprowski wrote: > Hello, > > This patchset finally perform cleanup of custom code in s5p-mfc codec > driver. The first part is removal of custom, driver specific code for > intializing and handling of reserved memory. Instead, a generic code for > reserved memory regions is used. Should you update the reserved memory bindings documentation (Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt) to document usage of the memory-region-names property ? > Then, once it is done, the proper setup > of DMA parameters (max segment size) is applied for all multimedia > devices found on Exynos SoCs to let them properly handle shared buffers > mapped into contiguous DMA address space. The last patch adds support > for IOMMU to MFC driver. Some additional code is needed because of > specific requirements of MFC device firmware (see patch 7 for more > details). When no IOMMU is available, the code fallbacks to generic > reserved memory regions. > > After applying this patchset, MFC device works correctly when IOMMU is > either enabled or disabled. > > Patches have been tested on top of linux-next from 20151207. I would > prefer to merge patches 1-2 via Samsung tree and patches 3-7 via media > tree (there are no compile-time dependencies between patches 1-2 and > 3-7). Patches have been tested on Odroid U3 (Exynos 4412 based) and > Odroid XU3 (Exynos 5422 based) boards. > > Best regards > Marek Szyprowski > Samsung R Institute Poland > > > Changelog: > v2: > - reworked of_reserved_mem_init* functions on request from Rob Herring, > added separate index and name based selection of reserved region > - adapted for of_reserved_mem_init* related changes > > v1: https://www.mail-archive.com/linux-media@vger.kernel.org/msg94100.html > - initial version of another approach for this problem, rewrote driver code > for new reserved memory bindings, which finally have been merged some > time ago > > v0: > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/189259.ht > ml - old patchset solving the same problem, abandoned due to other tasks and > long time of merging reserved memory bindings and support code for it > > Patch summary: > > Marek Szyprowski (7): > ARM: Exynos: convert MFC device to generic reserved memory bindings > ARM: dts: exynos4412-odroid*: enable MFC device > of: reserved_mem: add support for named reserved mem nodes > media: vb2-dma-contig: add helper for setting dma max seg size > media: set proper max seg size for devices on Exynos SoCs > media: s5p-mfc: replace custom reserved memory init code with generic > one > media: s5p-mfc: add iommu support > > .../devicetree/bindings/media/s5p-mfc.txt | 16 +-- > arch/arm/boot/dts/exynos4210-origen.dts| 22 ++- > arch/arm/boot/dts/exynos4210-smdkv310.dts | 22 ++- > arch/arm/boot/dts/exynos4412-odroid-common.dtsi| 24 > arch/arm/boot/dts/exynos4412-origen.dts| 22 ++- > arch/arm/boot/dts/exynos4412-smdk4412.dts | 22 ++- > arch/arm/boot/dts/exynos5250-arndale.dts | 22 ++- > arch/arm/boot/dts/exynos5250-smdk5250.dts | 22 ++- > arch/arm/boot/dts/exynos5250-spring.dts| 22 ++- > arch/arm/boot/dts/exynos5420-arndale-octa.dts | 22 ++- > arch/arm/boot/dts/exynos5420-smdk5420.dts | 22 ++- > arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi | 22 ++- > arch/arm/mach-exynos/Makefile | 2 - > arch/arm/mach-exynos/exynos.c | 19 --- > arch/arm/mach-exynos/mfc.h | 16 --- > arch/arm/mach-exynos/s5p-dev-mfc.c | 94 - > drivers/media/platform/exynos-gsc/gsc-core.c | 1 + > drivers/media/platform/exynos4-is/fimc-core.c | 1 + > drivers/media/platform/exynos4-is/fimc-is.c| 1 + > drivers/media/platform/exynos4-is/fimc-lite.c | 1 + > drivers/media/platform/s5p-g2d/g2d.c | 1 + > drivers/media/platform/s5p-jpeg/jpeg-core.c| 1 + > drivers/media/platform/s5p-mfc/s5p_mfc.c | 153 ++ > drivers/media/platform/s5p-mfc/s5p_mfc_iommu.h | 79 +++ > drivers/media/platform/s5p-tv/mixer_video.c| 1 + > drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 ++ > drivers/of/of_reserved_mem.c | 104 +++--- > include/linux/of_reserved_mem.h| 31 - > include/media/videobuf2-dma-contig.h | 1 + > 29 files changed, 533 insertions(+), 248 deletions(-) > delete mode 100644 arch/arm/mach
Re: [PATCH v8 32/55] [media] media: use macros to check for V4L2 subdev entities
; diff --git a/drivers/media/platform/xilinx/xilinx-dma.c
> b/drivers/media/platform/xilinx/xilinx-dma.c index
> 88cd789cdaf7..8e14841bf445 100644
> --- a/drivers/media/platform/xilinx/xilinx-dma.c
> +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> @@ -49,8 +49,7 @@ xvip_dma_remote_subdev(struct media_pad *local, u32 *pad)
> struct media_pad *remote;
>
> remote = media_entity_remote_pad(local);
> - if (remote == NULL ||
> - media_entity_type(remote->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> + if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> return NULL;
>
> if (pad)
> @@ -113,8 +112,7 @@ static int xvip_pipeline_start_stop(struct xvip_pipeline
> *pipe, bool start) break;
>
> pad = media_entity_remote_pad(pad);
> - if (pad == NULL ||
> - media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> break;
>
> entity = pad->entity;
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> b/drivers/media/v4l2-core/v4l2-subdev.c index e6e1115d8215..60da43772de9
> 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -526,7 +526,7 @@ static int
> v4l2_subdev_link_validate_get_format(struct media_pad *pad,
>struct v4l2_subdev_format *fmt)
> {
> - if (media_entity_type(pad->entity) == MEDIA_ENT_T_V4L2_SUBDEV) {
> + if (is_media_entity_v4l2_subdev(pad->entity)) {
> struct v4l2_subdev *sd =
> media_entity_to_v4l2_subdev(pad->entity);
>
> diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c
> b/drivers/staging/media/davinci_vpfe/vpfe_video.c index
> 92573fa852a9..16763e0831f2 100644
> --- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
> +++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
> @@ -148,7 +148,7 @@ static void vpfe_prepare_pipeline(struct
> vpfe_video_device *video) while ((entity =
> media_entity_graph_walk_next())) {
> if (entity == >video_dev.entity)
> continue;
> - if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE)
> + if ((!is_media_entity_v4l2_io(remote->entity))
> continue;
> far_end = to_vpfe_video(media_entity_to_video_device(entity));
> if (far_end->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> @@ -293,7 +293,7 @@ static int vpfe_pipeline_enable(struct vpfe_pipeline
> *pipe) media_entity_graph_walk_start(, entity);
> while ((entity = media_entity_graph_walk_next())) {
>
> - if (media_entity_type(entity) == MEDIA_ENT_T_DEVNODE)
> + if !is_media_entity_v4l2_subdev(entity))
With these two chunks fixed,
Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
I'm wondering, however, why you replace some occurrences of ==
MEDIA_ENT_T_DEVNODE with !is_media_entity_v4l2_subdev and some other with
is_media_entity_v4l2_io.
> continue;
> subdev = media_entity_to_v4l2_subdev(entity);
> ret = v4l2_subdev_call(subdev, video, s_stream, 1);
> @@ -334,7 +334,7 @@ static int vpfe_pipeline_disable(struct vpfe_pipeline
> *pipe)
>
> while ((entity = media_entity_graph_walk_next())) {
>
> - if (media_entity_type(entity) == MEDIA_ENT_T_DEVNODE)
> + if (!is_media_entity_v4l2_subdev(entity))
> continue;
> subdev = media_entity_to_v4l2_subdev(entity);
> ret = v4l2_subdev_call(subdev, video, s_stream, 0);
> diff --git a/drivers/staging/media/omap4iss/iss.c
> b/drivers/staging/media/omap4iss/iss.c index 40591963b42b..44b88ff3ba83
> 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -397,7 +397,7 @@ static int iss_pipeline_pm_use_count(struct media_entity
> *entity) media_entity_graph_walk_start(, entity);
>
> while ((entity = media_entity_graph_walk_next())) {
> - if (media_entity_type(entity) == MEDIA_ENT_T_DEVNODE)
> + if (is_media_entity_v4l2_io(entity))
> use += entity->use_count;
> }
>
> @@ -419,7 +419,7 @@ static int iss_pipeline_pm_power_one(struct media_entity
> *entity, int change) {
> struct v4l2_subdev *subdev;
>
> - subdev = media_entity_type(entity) == MEDIA_ENT_T_V4L2_SUBDEV
> + subdev = is_media_entity_v4l2_subdev(entity)
> ? media_entity_to_v4l2_subdev(entity) : NULL;
>
> if (entity->use_count == 0 && change
Re: [PATCH v2 9/9] [media] omap3isp: support 64-bit version of omap3isp_stat_data
a->ts.tv_usec = data64.ts.tv_usec;
> + memcpy(>buf, , sizeof(*data) - sizeof(data->ts));
> +
> + return ret;
> +}
> +
> /*
> * omap3isp_stat_config - Receives new statistic engine configuration.
> * @new_conf: Pointer to config structure.
> diff --git a/drivers/media/platform/omap3isp/ispstat.h
> b/drivers/media/platform/omap3isp/ispstat.h index
> 7b4f136567a3..b19ea6c8f733 100644
> --- a/drivers/media/platform/omap3isp/ispstat.h
> +++ b/drivers/media/platform/omap3isp/ispstat.h
> @@ -130,6 +130,8 @@ struct ispstat_generic_config {
> int omap3isp_stat_config(struct ispstat *stat, void *new_conf);
> int omap3isp_stat_request_statistics(struct ispstat *stat,
>struct omap3isp_stat_data *data);
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data);
> int omap3isp_stat_init(struct ispstat *stat, const char *name,
> const struct v4l2_subdev_ops *sd_ops);
> void omap3isp_stat_cleanup(struct ispstat *stat);
> diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
> index c090cf9249bb..4bff66aefca5 100644
> --- a/include/uapi/linux/omap3isp.h
> +++ b/include/uapi/linux/omap3isp.h
> @@ -54,6 +54,8 @@
> _IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config)
> #define VIDIOC_OMAP3ISP_STAT_REQ \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data)
> +#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
> + _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
> #define VIDIOC_OMAP3ISP_STAT_EN \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
>
> @@ -164,7 +166,11 @@ struct omap3isp_h3a_aewb_config {
> * @config_counter: Number of the configuration associated with the data.
> */
> struct omap3isp_stat_data {
> +#ifdef __KERNEL__
> + struct v4l2_timeval ts;
> +#else
> struct timeval ts;
> +#endif
> void __user *buf;
> __u32 buf_size;
> __u16 frame_number;
> @@ -172,6 +178,19 @@ struct omap3isp_stat_data {
> __u16 config_counter;
> };
>
> +#ifdef __KERNEL__
> +struct omap3isp_stat_data_time32 {
> + struct {
> + __s32 tv_sec;
> + __s32 tv_usec;
> + } ts;
> + __u32 buf;
> + __u32 buf_size;
> + __u16 frame_number;
> + __u16 cur_frame;
> + __u16 config_counter;
> +};
> +#endif
>
> /* Histogram related structs */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/37] ARM: dts: s5pv210-goni: Fix typo in regulator enable GPIO property
The property name should be "gpio", not "gpios". Fix it. Signed-off-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> --- arch/arm/boot/dts/s5pv210-goni.dts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Cc: linux-samsung-soc@vger.kernel.org Cc: Kukjin Kim <kg...@kernel.org> Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com> diff --git a/arch/arm/boot/dts/s5pv210-goni.dts b/arch/arm/boot/dts/s5pv210-goni.dts index a3d4643b202e..3b76eeeb8410 100644 --- a/arch/arm/boot/dts/s5pv210-goni.dts +++ b/arch/arm/boot/dts/s5pv210-goni.dts @@ -47,7 +47,7 @@ regulator-min-microvolt = <280>; regulator-max-microvolt = <280>; reg = <0>; - gpios = < 4 0>; + gpio = < 4 0>; enable-active-high; }; @@ -73,7 +73,7 @@ regulator-min-microvolt = <280>; regulator-max-microvolt = <280>; reg = <3>; - gpios = < 3 0>; + gpio = < 3 0>; enable-active-high; }; }; -- 2.4.9 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity
Hello,
While working on regulators, GPIOs and DT I noticed that many of our DT source
files incorrectly describe fixed regulators. The common error patterns are
- Usage of the undefined (and never parsed) enable-active-low property
- Usage of the enable-active-high property without specifying an enable GPIO
- Typos in the enabl GPIO property name (gpios instead of gpio)
- Mismatch between the enable-active-high property (or the lack thereof) and
the enable GPIO flags
This patch series fixes those issues in all the DT sources after locating the
errors using the following script.
--
#!/bin/sh
echo $1
cat $1 | awk '
BEGIN {
open_drain = 0;
active_high = 0;
gpio = 0;
flags = 0;
}
match($0, /([a-zA-Z0-9@_-]*) {/, ary) {
name = ary[1];
}
/compatible.*"regulator-fixed"/ {
found = 1;
}
/enable-active-high/ {
active_high = 1;
}
/gpio-open-drain/ {
open_drain = 1;
}
match($0, /gpio += <.* ([^ ]*)>/, ary) {
gpio = 1;
flags = ary[1];
if (flags == 0)
flags = "GPIO_ACTIVE_HIGH";
}
/}/ {
if (found) {
if (gpio) {
print "\t" name ": active high " active_high " " flags
" open drain " open_drain;
if ((active_high && flags == "GPIO_ACTIVE_LOW") ||
(!active_high && flags == "GPIO_ACTIVE_HIGH"))
print "WARNING: enable-active-high and flags do
not match"
} else {
if (active_high)
print "WARNING: active high without GPIO"
if (open_drain)
print "WARNING: open drain without GPIO"
}
}
gpio = 0;
found = 0;
active_high = 0;
open_drain = 0;
flags = 0;
}
'
--
All patches except for the ones touching omap3-beagle-xm and omap3-overo-base
are untested as I lack test hardware.
As there's no dependency between the patches touching different source files
the appropriate maintainers could take their share of the patches in their
tree. Alternatively I could send a single pull request after collecting all
acks but that might be more complex.
Cc: devicet...@vger.kernel.org
Cc: linux-o...@vger.kernel.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-te...@vger.kernel.org
Cc: linux-g...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: Benoit Cousson <bcous...@baylibre.com>
Cc: Tony Lindgren <t...@atomide.com>
Cc: Jason Cooper <ja...@lakedaemon.net>
Cc: Andrew Lunn <and...@lunn.ch>
Cc: Gregory Clement <gregory.clem...@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselba...@gmail.com>
Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Cc: Shawn Guo <shawn...@kernel.org>
Cc: Sascha Hauer <ker...@pengutronix.de>
Cc: Stephen Warren <swar...@wwwdotorg.org>
Cc: Thierry Reding <thierry.red...@gmail.com>
Cc: Alexandre Courbot <gnu...@gmail.com>
Cc: Liam Girdwood <lgirdw...@gmail.com>
Cc: Mark Brown <broo...@kernel.org>
Cc: Linus Walleij <linus.wall...@linaro.org>
Laurent Pinchart (37):
ARM: dts: am437x-gp-evm: Remove unneeded regulator property
ARM: dts: am43xx-epos-evm: Remove unneeded regulator property
ARM: mvebu: Armada 388 GP: Remove unneeded regulator property
ARM: imx6sx-sdb: Fix typo in regulator enable GPIO property
ARM: dts: s5pv210-aquila: Fix typo in regulator enable GPIO property
ARM: dts: s5pv210-goni: Fix typo in regulator enable GPIO property
ARM: dts: omap3-evm: Remove invalid enable-active-low regulator
property
ARM: dts: omap3-sb-t35: Remove invalid enable-active-low regulator
property
ARM: dts: omap3-tao3530: Remove invalid enable-active-low regulator
property
ARM: dts: imx6qdl-tx6: Fix regulator enable GPIO polarity
ARM: dts: dove-cm-a510: Fix regulator enable GPIO polarity
ARM: dts: dove-sbc-a510: Fix regulator enable GPIO polarity
ARM: dts: exynos5250-arndale: Fix regulator enable GPIO polarity
ARM: dts: imx23-evk: Fix regulator enable GPIO polarity
ARM: dts: imx23-stmp378x_devb: Fix regulator enable GPIO polarity
ARM: dts: imx25-pdk: Fix regulator enable GPIO polarity
ARM: dts: imx28-cfa10036: Fix regulator enable GPIO polarity
ARM: dts: imx28-evk: Fix regulator enable GPIO polarity
ARM: dts: imx28-m28cu3: Fix regulator enable GPIO polarity
ARM: dts: imx28-m28evk: Fix regulator enable GPIO polarity
ARM: dts: imx28-sps1: Fix regulator enable GPIO polarity
ARM: dts: imx28-tx28: Fix regulator enable GPIO polarity
A
Re: [PATCH v4 1/6] media: get rid of unused extra_links param on media_entity_init()
Hi Mauro,
Thank you for the patch.
On Friday 14 August 2015 11:56:38 Mauro Carvalho Chehab wrote:
Currently, media_entity_init() creates an array with the links,
allocated at init time. It provides a parameter (extra_links)
that would allocate more links than the current needs, but this
is not used by any driver.
As we want to be able to do dynamic link allocation/removal,
we'll need to change the implementation of the links. So,
before doing that, let's first remove that extra unused
parameter, in order to cleanup the interface first.
Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com
Acked-by: Sakari Ailus sakari.ai...@linux.intel.com
diff --git a/Documentation/media-framework.txt
b/Documentation/media-framework.txt index f552a75c0e70..2cc6019f7147 100644
--- a/Documentation/media-framework.txt
+++ b/Documentation/media-framework.txt
@@ -104,7 +104,7 @@ although drivers can allocate entities directly.
Drivers initialize entities by calling
media_entity_init(struct media_entity *entity, u16 num_pads,
- struct media_pad *pads, u16 extra_links);
+ struct media_pad *pads);
The media_entity name, type, flags, revision and group_id fields can be
initialized before or after calling media_entity_init. Entities embedded in
The extra_links parameter is documented later on in this file. Could you
replace the paragraph
Unlike the number of pads, the total number of links isn't always known in
advance by the entity driver. As an initial estimate, media_entity_init
pre-allocates a number of links equal to the number of pads plus an optional
number of extra links. The links array will be reallocated if it grows beyond
the initial estimate.
with
Unlike the number of pads, the total number of links isn't always known in
advance by the entity driver. As an initial estimate, media_entity_init
pre-allocates a number of links equal to the number of pads. The links array
will be reallocated if it grows beyond the initial estimate.
?
[snip]
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 4d8e01c7b1b2..78440c7aad94 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -30,7 +30,6 @@
* media_entity_init - Initialize a media entity
*
* @num_pads: Total number of sink and source pads.
- * @extra_links: Initial estimate of the number of extra links.
* @pads: Array of 'num_pads' pads.
*
* The total number of pads is an intrinsic property of entities known by
the
Similarly here, I would rewrite the documentation as
* The total number of pads is an intrinsic property of entities known by the
* entity driver, while the total number of links depends on hardware design
* and is an extrinsic property unknown to the entity driver. However, in most
* use cases the number of links can safely be assumed to be equal to or
* larger than the number of pads.
*
* For those reasons the links array can be preallocated based on the number
* of pads and will be reallocated later if extra links need to be created.
*
* This function allocates a links array with enough space to hold at least
* 'num_pads' elements. The media_entity::max_links field will be set to the
* number of allocated elements.
With that fixed,
Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
And feel free to merge this patch for v4.3 independently of the rest.
@@ -52,10 +51,10 @@
*/
int
media_entity_init(struct media_entity *entity, u16 num_pads,
- struct media_pad *pads, u16 extra_links)
+ struct media_pad *pads)
{
struct media_link *links;
- unsigned int max_links = num_pads + extra_links;
+ unsigned int max_links = num_pads;
unsigned int i;
links = kzalloc(max_links * sizeof(links[0]), GFP_KERNEL);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 03/25] iommu: Init iommu-groups support earlier, in core_initcall
Hi Marek, Thank you for the patch. On Tuesday 19 May 2015 15:20:23 Marek Szyprowski wrote: iommu_group_alloc might be called very early in case of iommu controllers activated from of_iommu, so ensure that this part of subsystem is ready when devices are being populated from device-tree (core_initcall seems to be okay for this case). Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com --- drivers/iommu/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index d4f527e56679..37a6aa8f318b 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1207,7 +1207,7 @@ static int __init iommu_init(void) return 0; } -arch_initcall(iommu_init); +core_initcall(iommu_init); I'll let Joerg comment on this, but this initcall ordering dance always makes me feel that something isn't quite right. Have you had a chance to look at the patch series I posted about a week ago to implement IOMMU probe deferral support ? int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr attr, void *data) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/25] iommu: exynos: remove useless device_add/remove callbacks
Hi Joerg, On Monday 18 May 2015 19:04:30 Joerg Roedel wrote: On Mon, May 18, 2015 at 02:09:14PM +0200, Marek Szyprowski wrote: Hmm, will this remove support for iommu-groups in the exynos driver? I am still working on the default-domain patch-set which makes iommu-group support mandatory for iommu-drivers. I you wish, I can leave this code. iommu-groups were not used at all on Exynos, so I thought that there is no point keeping useless code. Yes, please keep the iommu-goups support, it will be required at some point. I think the concept of iommu groups isn't very well understood, hence the little love it receives from developers who conclude it's useless. I can't blame anyone, I don't do any better. Could I convince you to include documentation in your next patch series that touches iommu groups ? Could you please have a look at my [RFC/PATCH 0/9] IOMMU probe deferral support patch series in the context of iommu groups ? The series pretty much drops groups support from the ipmmu-vmsa driver, not because of a real desire to do so, but because I wasn't sure how to keep them. Tips would be appreciated. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 11/18] iommu: exynos: remove useless device_add/remove callbacks
Hi Joerg, On Monday 26 January 2015 12:00:59 Joerg Roedel wrote: Hi Laurent, On Sun, Jan 25, 2015 at 05:38:22PM +0200, Laurent Pinchart wrote: IOMMU groups still seem a bit unclear to me. Will Deacon has nicely explained what they represent in http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310816 .html. The IOMMU core doesn't make groups mandatory, but requires them in some code paths. For example the coldplug device add function add_iommu_group() called for all devices already registered when bus_set_iommu() is called will try to warn of devices added multiple times with a WARN_ON(dev-iommu_group). Another example is the iommu_bus_notifier() function which will call the remove_device() operation only when dev-iommu_group isn't NULL. I'm thus unsure whether groups should be made mandatory, or whether the IOMMU core should be fixed to make them really optional (or, third option, whether there's something I haven't understood properly). My plan is to make IOMMU groups mandatory. I am currently preparing and RFC patch-set to introduce default-domains (which will be per group). So when all IOMMU drivers are converted to make use of default domains the iommu groups will be mandatory. Could the default domain policy be configured by the IOMMU driver ? The ipmmu- vmsa driver supports four domains only, and serves up to 32 masters, with one group per master. A default of one domain per group wouldn't be usable. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 17/18] iommu: exynos: init from dt-specific callback instead of initcall
Hi Will,
On Monday 19 January 2015 11:33:31 Will Deacon wrote:
On Mon, Jan 19, 2015 at 01:11:07AM +, Laurent Pinchart wrote:
On Friday 16 January 2015 10:13:11 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the
driver, which replaces subsys_initcall-based procedure.
exynos_iommu_of_setup ensures that each sysmmu controller is probed
before its master device.
[...]
+static int __init exynos_iommu_of_setup(struct device_node *np)
+{
+ struct platform_device *pdev;
+
+ if (!init_done)
+ exynos_iommu_init();
+
+ pdev = of_platform_device_create(np, NULL,
platform_bus_type.dev_root);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
This feels like a hack to me. What happens here is that you're using the
IOMMU_OF_DECLARE mechanism to make sure that the iommu platform device
will be created and registered before the normal OF bus populate
mechanism kicks in, thus ensuring that the iommu gets probed before other
devices. In practice this is pretty similar to using different init
levels, which is what Will's patch set was trying to avoid in the first
place. Creating a new kind of init levels mechanism doesn't sound very
good to me.
The existing exynos-iommu driver is based on classic instantiation of a
platform device from DT, using the normal device probing mechanism. As
such it relies on the availability of a struct device for various helper
functions. I thus understand why you want a struct device being
registered for the iommu, instead of initializing the device right from
the exynos_iommu_of_setup() function without a corresponding struct
device being registered.
This leads me to question whether we should really introduce
IOMMU_OF_DECLARE. Using regular deferred probing seems more and more like
a better solution to me.
We seem to be going round and round on this argument. I said before that
I'm not against changing this [1], but somebody would need to propose
patches, which hasn't happened in recent history.
Arnd also makes some good arguments against using probing [2], which would
need further discussion.
Basically, it looks like there are two sides to this argument and I don't
see anything changing without patch proposals. The only thing that the
current discussions seem to be achieving is blocking people like Marek,
who are trying to make use of what we have in mainline today!
To be perfectly clear, I won't block patches here without submitting a
counterproposal (unless there's something fundamentally wrong of course). I
still believe the deferred probe approach should be given at least a try, but
as I don't have time to implement that myself now, I won't try to block
anything.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310783.
html
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310992.
html
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V8 00/14] drm/exynos: few patches to enhance bridge chip support
Hi, On Monday 19 January 2015 18:05:07 Javier Martinez Canillas wrote: On 01/19/2015 05:30 PM, Thierry Reding wrote: I know you probably are very busy but it would be great if you can take a look to this series to avoid another kernel release to be missed since we are already at v3.19-rc5. Only this version was posted 2 months ago and the first version of the series was sent on May, 2014 so it has been on the list for almost a year now... Tomi and Laurent had already agreed with the DT binging so I think that we can already merge these and if there are any remaining issues, those can be fixed during the 3.20 -rc cycle. I see only a single Tested-by on this series and that seems to be with a bunch of out-of-tree patches applied on top. Does this now actually work applied on top of upstream? Also it seems like many people actually want this to get merged but there's been no Reviewed-bys and only a single Tested-by? Is that as good as it's going to get? Good point, I provided some feedback on an early version of the series but I'm not an DRM expert so I couldn't review it in detail to provide my Reviewed-by. I did provide my Tested-by a long time ago though but looking at the patches it seems those were dropped so here goes again: For the whole series on an Exynos5420 Peach Pit and an Exynos5250 Snow Chromebooks: Tested-by: Javier Martinez Canillas javier.marti...@collabora.co.uk Also I think the last update was that Ajay was going to resend this based on v3.19-rc1 or something. Is that still going to happen? Otherwise I can probably try to apply as-is, but not sure how well it will. Ajay, Are you going to rebase and resend this series with all the provided tags? Gustavo and I have a rebased branch on top of 3.19-rc5 and one of us can post your series if you are not planning to do it. Laurent and Tomi, You said that you are OK with the DT bindings now, does that count as an Acked-by or Reviewed-by for you at least for the DT bindings changes in the series? It counts as an Acked-by for the DT bindings. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 17/18] iommu: exynos: init from dt-specific callback instead of initcall
Hi Marek,
Thank you for the patch.
On Friday 16 January 2015 10:13:11 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the
driver, which replaces subsys_initcall-based procedure.
exynos_iommu_of_setup ensures that each sysmmu controller is probed
before its master device.
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
drivers/iommu/exynos-iommu.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index c53cc8f61176..ea2659159e63 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -13,16 +13,21 @@
#endif
#include linux/clk.h
+#include linux/dma-mapping.h
#include linux/err.h
#include linux/io.h
#include linux/iommu.h
#include linux/interrupt.h
#include linux/list.h
+#include linux/of.h
+#include linux/of_iommu.h
+#include linux/of_platform.h
#include linux/platform_device.h
#include linux/pm_runtime.h
#include linux/slab.h
#include asm/cacheflush.h
+#include asm/dma-iommu.h
#include asm/pgtable.h
typedef u32 sysmmu_iova_t;
@@ -1084,6 +1089,8 @@ static const struct iommu_ops exynos_iommu_ops = {
.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
};
+static int init_done;
+
static int __init exynos_iommu_init(void)
{
int ret;
@@ -1116,6 +1123,8 @@ static int __init exynos_iommu_init(void)
goto err_set_iommu;
}
+ init_done = true;
+
return 0;
err_set_iommu:
kmem_cache_free(lv2table_kmem_cache, zero_lv2_table);
@@ -1125,4 +1134,21 @@ err_reg_driver:
kmem_cache_destroy(lv2table_kmem_cache);
return ret;
}
-subsys_initcall(exynos_iommu_init);
+
+static int __init exynos_iommu_of_setup(struct device_node *np)
+{
+ struct platform_device *pdev;
+
+ if (!init_done)
+ exynos_iommu_init();
+
+ pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
This feels like a hack to me. What happens here is that you're using the
IOMMU_OF_DECLARE mechanism to make sure that the iommu platform device will be
created and registered before the normal OF bus populate mechanism kicks in,
thus ensuring that the iommu gets probed before other devices. In practice
this is pretty similar to using different init levels, which is what Will's
patch set was trying to avoid in the first place. Creating a new kind of init
levels mechanism doesn't sound very good to me.
The existing exynos-iommu driver is based on classic instantiation of a
platform device from DT, using the normal device probing mechanism. As such it
relies on the availability of a struct device for various helper functions. I
thus understand why you want a struct device being registered for the iommu,
instead of initializing the device right from the exynos_iommu_of_setup()
function without a corresponding struct device being registered.
This leads me to question whether we should really introduce IOMMU_OF_DECLARE.
Using regular deferred probing seems more and more like a better solution to
me.
+ of_iommu_set_ops(np, exynos_iommu_ops);
+ return 0;
+}
+
+IOMMU_OF_DECLARE(exynos_iommu_of, samsung,exynos-sysmmu,
+ exynos_iommu_of_setup);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Arnd, On Wednesday 17 December 2014 16:56:53 Arnd Bergmann wrote: On Wednesday 17 December 2014 15:53:25 Lucas Stach wrote: Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann: On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices. This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea. I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses. This is similar to the problems we discussed with the componentized device framework and in the end everybody agreed on a simple rule: if the device node is enabled in the DT there must be a driver bound to it before other devices depending on this node can be probed. If we apply the same logic to the IOMMU situation we get two possibilities to allow bypassing the IOMMU: 1. completely disable the IOMMU node in the DT 2. leave node enabled in DT, but add a bypass property Obviously the second option still requires to have the IOMMU driver available, but is more flexible. Right now everything depends on the IOMMU being in passthrough mode at reset with no driver bound. If a device comes around where this isn't the case thing will break with the current assumptions or the first option. Having the IOMMU node enabled in DT with no driver available to the kernel seems like an invalid configuration which should be expected to break. Exactly the same thing as with componentized devices... One differences here is that for the IOMMU, we should generally be able to fall back to swiotlb Or to nothing at all if the device can DMA to the allocated memory directly. (currently only on ARM64, not ARM32, until someone adds support). I can see your point regarding machines that have a mandatory IOMMU with no fallback when there is no driver, but we can support them by making the iommu driver selected through Kconfig for that platform, while still allowing other platforms to work with drivers left out of the kernel. The question is how to tell the kernel not to wait for an IOMMU that will never be there. Would a kernel command line argument be an acceptable solution or do we need something more automatic ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V8 00/14] drm/exynos: few patches to enhance bridge chip support
Hi Javier, On Wednesday 17 December 2014 10:31:41 Javier Martinez Canillas wrote: On 12/16/2014 12:37 AM, Laurent Pinchart wrote: Hi Javier, Tomi and Laurent, You asked Ajay to change his series to use the video port and enpoints DT bindings instead of phandles, could you please review his latest version? I guess is now too late for 3.19 since we are in the middle of the merge window but it would be great if this series can at least made it to 3.20. I don't have time to review the series in details right now, but I'm happy with the DT bindings, and have no big issue with the rest of the patches. I don't really like the of_drm_find_bridge() concept introduced in 03/14 but I won't nack it given lack of time to implement an alternative proposal. It's an internal API, it can always be reworked later anyway. Thanks a lot for taking the time to look at the DT bindings, You're welcome. Sorry for the long delay. then I guess that the series are finally ready to be merged? From my point of view, yes. Ajay's series don't apply cleanly anymore because it has been a while since he posted it but he can rebase on top of 3.19-rc1 once it is released and re-resend. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Arnd, On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices. This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea. I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses. Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Arnd, On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote: On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote: On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices. This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea. I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses. Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ? It's a policy decision that should only depend on the user. Modifying the DT is wrong here IMHO because the device is still connected to the IOMMU in hardware and we should correctly represent that. I was thinking about setting status = disabled on the IOMMU nodes, not removing the IOMMU references in the bus master nodes. You can normally set 'iommu=off' or 'iommu=force' on the command line to force it on or off rather than letting the IOMMU driver decide whether the device turns it on. Not enabling the IOMMU at all should really just behave the same way as 'iommu=off', anything else would be very confusing. Setting iommu=off sounds fine to me. The IOMMU core would then return a no IOMMU present error instead of -EPROBE_DEFER, and probing of the bus masters would continue without IOMMU support. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Arnd, On Wednesday 17 December 2014 22:58:47 Arnd Bergmann wrote: On Wednesday 17 December 2014 18:02:51 Laurent Pinchart wrote: On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote: On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote: On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices. This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea. I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses. Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ? It's a policy decision that should only depend on the user. Modifying the DT is wrong here IMHO because the device is still connected to the IOMMU in hardware and we should correctly represent that. I was thinking about setting status = disabled on the IOMMU nodes, not removing the IOMMU references in the bus master nodes. But that still requires a modification of the DT. The hardware is the same, so I don't see why we should update the dtb based on kernel configuration. It wouldn't be the first time we encode configuration information in DT, but I agree it's not an optimal solution. Setting iommu=off on the kernel command line is better, and should be easy to implement. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Arnd,
On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote:
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
IOMMUs are not as low-level as system interrupt controllers or
system clocks. I'm beginning to agree with Thierry that they should
be treated as normal platform devices as they're not required
earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm
certainly not wed to the current approach; I just want something that
allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or
could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
I believe we could postpone it to probe time, but I'd rather not.
The way I see the arguments in favor, we have mainly cosmetic arguments:
- Doing it at probe time would eliminate the ugly section magic hack
- iommu drivers could be implemented as loadable modules with platform
drivers, for consistency with most other drivers
The main argument, from my point of view, is that handling IOMMUs are normal
platform devices allow using all the kernel infrastructure that depends on a
struct device. The dev_* print helpers are nice to have, but what would make a
big difference is the ability to use runtime PM.
On the other hand, I see:
- DMA configuration is traditionally done at device creation time, and
changing that is more likely to introduce bugs than leaving it
where it is.
- On a lot of machines, the IOMMU is optional, and the probe function
cannot know the difference between an IOMMU driver that is left out
of the kernel and one that will be initialized later, using a fixed
entry point for initializing the IOMMU makes the behavior consistent
I'm not advocating for IOMMU support being built as a loadable module. It
might be nice from a development point of view, but that's about it.
There is a third option in theory, which is to only enable the IOMMU
as part of dma_set_mask(). I've done that in the past on powerpc, but
the new approach seems cleaner.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Arnd,
On Tuesday 16 December 2014 13:10:59 Arnd Bergmann wrote:
On Tuesday 16 December 2014 14:07:11 Laurent Pinchart wrote:
On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote:
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
IOMMUs are not as low-level as system interrupt controllers or
system clocks. I'm beginning to agree with Thierry that they should
be treated as normal platform devices as they're not required
earlier than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm
certainly not wed to the current approach; I just want something
that allows of_{dma,iommu}_configure to run with the information it
needs.
Do we need of_dma_configure() to run when the device is created, or
could we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
I believe we could postpone it to probe time, but I'd rather not.
The way I see the arguments in favor, we have mainly cosmetic arguments:
- Doing it at probe time would eliminate the ugly section magic hack
- iommu drivers could be implemented as loadable modules with platform
drivers, for consistency with most other drivers
The main argument, from my point of view, is that handling IOMMUs are
normal platform devices allow using all the kernel infrastructure that
depends on a struct device. The dev_* print helpers are nice to have, but
what would make a big difference is the ability to use runtime PM.
Right, I agree that would be nice.
On the other hand, I see:
- DMA configuration is traditionally done at device creation time, and
changing that is more likely to introduce bugs than leaving it
where it is.
- On a lot of machines, the IOMMU is optional, and the probe function
cannot know the difference between an IOMMU driver that is left out
of the kernel and one that will be initialized later, using a fixed
entry point for initializing the IOMMU makes the behavior consistent
I'm not advocating for IOMMU support being built as a loadable module. It
might be nice from a development point of view, but that's about it.
We'd still have to deal with deferred probing, or with ordering the iommu
initcalls before the dma master initcalls in some other way, so the
problem is not that different from loadable modules. Do you have an idea
for how to solve that?
If we forbid the IOMMU driver from being compiled as a module can't we just
rely on deferred probing ? The bus master driver will just be reprobed after
the IOMMU gets probed, like for other devices.
This could fail in case the IOMMU device permanently fails to probe. There
would be something very wrong in the system in that case, Enabling the bus
masters totally transparently without IOMMU support could not be such a good
idea.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Will,
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
On Sun, Dec 14, 2014 at 12:45:36PM +, Laurent Pinchart wrote:
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the
driver, which replaces subsys_initcall-based procedure.
exynos_iommu_of_setup ensures that each sysmmu controller is probed
before its master device.
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
drivers/iommu/exynos-iommu.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index cd28dc09db39..88f9afe641a0 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
[snip]
@@ -1125,4 +1134,21 @@ err_reg_driver:
kmem_cache_destroy(lv2table_kmem_cache);
return ret;
}
-subsys_initcall(exynos_iommu_init);
+
+static int __init exynos_iommu_of_setup(struct device_node *np)
+{
+ struct platform_device *pdev;
+
+ if (!init_done)
+ exynos_iommu_init();
+
+ pdev = of_platform_device_create(np, NULL,
platform_bus_type.dev_root);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
If we end up having to create the IOMMU platform devices from within the
drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a
workaround to me. I wonder whether it wouldn't then be better to let the
driver core instantiate the IOMMU platform device from DT as for all
other devices, and use device notifiers to defer probe of the bus masters
until the required IOMMU(s) are registered.
Will, what's your opinion on that ?
Creating the platform device manually for the IOMMU is indeed grotty, but I
don't really understand why it's needed. Interrupt controllers, for example,
seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at the
moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers
instead. Note that IRQ controllers that are further away from the CPU (such as
GPIO-based IRQ controllers) are real platform devices and use runtime PM.
IOMMUs are not as low-level as system interrupt controllers or system clocks.
I'm beginning to agree with Thierry that they should be treated as normal
platform devices as they're not required earlier than probe time of their bus
master devices.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Will,
On Monday 15 December 2014 17:43:02 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:27:30PM +, Laurent Pinchart wrote:
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
On Sun, Dec 14, 2014 at 12:45:36PM +, Laurent Pinchart wrote:
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
+static int __init exynos_iommu_of_setup(struct device_node *np)
+{
+ struct platform_device *pdev;
+
+ if (!init_done)
+ exynos_iommu_init();
+
+ pdev = of_platform_device_create(np, NULL,
platform_bus_type.dev_root);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
If we end up having to create the IOMMU platform devices from within
the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like
a workaround to me. I wonder whether it wouldn't then be better to let
the driver core instantiate the IOMMU platform device from DT as for
all other devices, and use device notifiers to defer probe of the bus
masters until the required IOMMU(s) are registered.
Will, what's your opinion on that ?
Creating the platform device manually for the IOMMU is indeed grotty,
but I don't really understand why it's needed. Interrupt controllers,
for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think of at
the moment is runtime PM. IRQ controllers close to the CPU use CPU PM
notifiers instead. Note that IRQ controllers that are further away from
the CPU (such as GPIO-based IRQ controllers) are real platform devices
and use runtime PM.
Ok, that's a good point, but the IOMMU will still probe later anyway, right?
That depends on the driver implementation, using OF node match an IOMMU driver
doesn't have to register a struct driver. Assuming we require IOMMU drivers to
register a struct driver, a platform device should then be probed at a later
time.
However, if we wait until the IOMMU gets probed to initialize runtime PM and
make it functional, we'll be back in square one if the IOMMU gets probed after
the bus master, as the bus master could start issuing bus requests at probe
time with the IOMMU not powered yet.
IOMMUs are not as low-level as system interrupt controllers or system
clocks. I'm beginning to agree with Thierry that they should be treated
as normal platform devices as they're not required earlier than probe
time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm
certainly not wed to the current approach; I just want something that
allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could we
postpone it to just before probe time ?
The usual answer is `we should model buses properly', but that's really
not practical afaict.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
On Monday 15 December 2014 18:13:23 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:53:48PM +, Laurent Pinchart wrote:
Hi Will,
Hello again :)
On Monday 15 December 2014 17:43:02 Will Deacon wrote:
On Mon, Dec 15, 2014 at 05:27:30PM +, Laurent Pinchart wrote:
On Monday 15 December 2014 17:17:00 Will Deacon wrote:
Creating the platform device manually for the IOMMU is indeed
grotty, but I don't really understand why it's needed. Interrupt
controllers, for example, seem to get by without one.
There's several reasons, one of the most compelling ones I can think
of at the moment is runtime PM. IRQ controllers close to the CPU use
CPU PM notifiers instead. Note that IRQ controllers that are further
away from the CPU (such as GPIO-based IRQ controllers) are real
platform devices and use runtime PM.
Ok, that's a good point, but the IOMMU will still probe later anyway,
right?
That depends on the driver implementation, using OF node match an IOMMU
driver doesn't have to register a struct driver. Assuming we require
IOMMU drivers to register a struct driver, a platform device should then
be probed at a later time.
However, if we wait until the IOMMU gets probed to initialize runtime PM
and make it functional, we'll be back in square one if the IOMMU gets
probed after the bus master, as the bus master could start issuing bus
requests at probe time with the IOMMU not powered yet.
True, but couldn't the early init code do enough to get the thing
functional? That said, I'm showing my ignorance here as I'm not familiar
with the PM code (and the software models I have for the SMMU clearly don't
implement anything in this regard).
We're reaching the limits of my knowledge as well. If the IOMMU is in a power
domain different than the bus masters the driver would at least need to ensure
that the power domain is turned on, which might be a bit hackish without
runtime PM.
IOMMUs are not as low-level as system interrupt controllers or system
clocks. I'm beginning to agree with Thierry that they should be
treated as normal platform devices as they're not required earlier
than probe time of their bus master devices.
Well, I think you'd have to propose patches for discussion since I'm
certainly not wed to the current approach; I just want something that
allows of_{dma,iommu}_configure to run with the information it needs.
Do we need of_dma_configure() to run when the device is created, or could
we postpone it to just before probe time ?
I'm not sure I can answer that one... Arnd?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V8 00/14] drm/exynos: few patches to enhance bridge chip support
Hi Javier, On Friday 12 December 2014 10:51:50 Javier Martinez Canillas wrote: Hello, On 11/18/2014 07:20 AM, Ajay kumar wrote: On Sat, Nov 15, 2014 at 3:24 PM, Ajay Kumar wrote: This series is based on master branch of Linus tree at: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git This series has been in the mailing lists for months and have been tested by many people. What else is missing before it can be merged? It would be great to have proper display support on platforms that have a eDP to LVDS bridge chip (e.g: Snow, Peach Pit and Spring Chromebooks) and everything is in place but this series which had been missing many kernel releases already. Changes since V7: -- Address comments from Tomi and Laurent: -- Use videoports and endpoints to represent the connection between encoder, bridge and the panel, instead of using phandles. -- Address comments from Daniel: -- Make the patch description more descriptive. -- remove device pointer from drm_bridge, and just use device_node instead. -- don't pass encoder pointer to bridge_attach. -- Address comments from Sean Paul: -- Remove bridge from mode_config, and pull out drm_bridge functions from drm_crtc.c to drm_bridge.c Tomi and Laurent, You asked Ajay to change his series to use the video port and enpoints DT bindings instead of phandles, could you please review his latest version? I guess is now too late for 3.19 since we are in the middle of the merge window but it would be great if this series can at least made it to 3.20. I don't have time to review the series in details right now, but I'm happy with the DT bindings, and have no big issue with the rest of the patches. I don't really like the of_drm_find_bridge() concept introduced in 03/14 but I won't nack it given lack of time to implement an alternative proposal. It's an internal API, it can always be reworked later anyway. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 18/19] iommu: exynos: init from dt-specific callback instead of initcall
Hi Marek,
Thank you for the patch.
On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote:
This patch introduces IOMMU_OF_DECLARE-based initialization to the
driver, which replaces subsys_initcall-based procedure.
exynos_iommu_of_setup ensures that each sysmmu controller is probed
before its master device.
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
drivers/iommu/exynos-iommu.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index cd28dc09db39..88f9afe641a0 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
[snip]
@@ -1125,4 +1134,21 @@ err_reg_driver:
kmem_cache_destroy(lv2table_kmem_cache);
return ret;
}
-subsys_initcall(exynos_iommu_init);
+
+static int __init exynos_iommu_of_setup(struct device_node *np)
+{
+ struct platform_device *pdev;
+
+ if (!init_done)
+ exynos_iommu_init();
+
+ pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
If we end up having to create the IOMMU platform devices from within the
drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround
to me. I wonder whether it wouldn't then be better to let the driver core
instantiate the IOMMU platform device from DT as for all other devices, and
use device notifiers to defer probe of the bus masters until the required
IOMMU(s) are registered.
Will, what's your opinion on that ?
+
+ of_iommu_set_ops(np, exynos_iommu_ops);
+ return 0;
+}
+
+IOMMU_OF_DECLARE(exynos_iommu_of, samsung,exynos-sysmmu,
+ exynos_iommu_of_setup);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
Hi Ajay, On Friday 10 October 2014 18:33:05 Ajay kumar wrote: On Wed, Oct 8, 2014 at 12:39 PM, Thierry Reding wrote: On Tue, Oct 07, 2014 at 05:49:24PM +0300, Laurent Pinchart wrote: On Tuesday 07 October 2014 16:06:55 Ajay kumar wrote: On Tue, Oct 7, 2014 at 4:00 PM, Tomi Valkeinen wrote: On 20/09/14 14:22, Ajay kumar wrote: Well, I am okay with using video ports to describe the relationship between the encoder, bridge and the panel. But, its just that I need to make use of 2 functions when phandle does it using just one function ;) -panel_node = of_parse_phandle(dev-of_node, panel, 0) + endpoint = of_graph_get_next_endpoint(dev-of_node, NULL); + if (!endpoint) + return -EPROBE_DEFER; + + panel_node = of_graph_get_remote_port_parent(endpoint); + if (!panel_node) + return -EPROBE_DEFER; If nobody else has objections over using of_graph functions instead of phandles, I can respin this patchset by making use of video ports. The discussion did digress somewhat. As a clarification, I'm in no way nack'ing this series because it doesn't use the graphs for video connections. I don't see the simple phandle bindings used here as broken as such. Well, I am okay with any approach you guys decide on. I desperately want this to get this in since it has been floating around for quite sometime. The more we drag this, the more rework for me since the number of platforms using bridge support is increasing daily! I won't nack this patch either. I'm however concerned that we'll run straight into the wall if we don't come up with an agreement on a standard way to describe connections in DT for display devices, which is why I would prefer the ps8622 bindings to use OF graph to describe connections. I think there's not really an easy way out here. It's pretty bold trying to come up with a common way to describe bridges when we have only a single one (and a single use-case at that). The worst that can happen is that we need to change the binding at some point, in which case we may have to special-case early drivers, but I really don't think that's as much of an issue as everybody seems to think. This series has been floating around for months because we're being overly prudent to accept a binding that /may/ turn out to not be generic enough. Right. It would be great if you guys come to agreement ASAP! I don't think we'll agree any time soon, so I believe it's up to you to decide which option is best based on all arguments that have been presented. If you ask me, of course, OF graph is best :-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
Hi Tomi,
On Tuesday 07 October 2014 10:06:10 Tomi Valkeinen wrote:
On 06/10/14 17:40, Laurent Pinchart wrote:
But seriously speaking, I was thinking about this. I'd really like to
have a generic video-mux node, that would still somehow allow us to have
device specific configurations for the video sources and sinks (which
the endpoints provide us), without endpoints.
The reason endpoints don't feel right in this case is that it makes
sense that the mux has a single input and two outputs, but with
endpoints we need two endpoints from the bridge (which is still ok), but
we also need two endpoitns in the mux's input side, which doesn't feel
right.
I came up with the following. It's quite ugly, though. I hope someone
has ideas how it could be done in a neater way:
bridge1234 {
bridge1234-cfg1: config1 {
high-voltage;
};
bridge1234-cfg2: config2 {
low-voltage;
};
output = mux;
};
mux: video-gpio-mux {
gpio = 123;
outputs = panel1 panel2;
input-configs = bridge1234-cfg1 bridge1234-cfg2;
};
panel1: panel-foo {
};
panel2: panel-foo {
};
So the bridge node has configuration sets. These might be compared to
pinmux nodes. And the mux has a list of input-configs. When panel1 is to
be enabled, bridge1234-cfg1 config becomes active, and the bridge is
given this configuration.
I have to say the endpoint system feels cleaner than the above, but
perhaps it's possible to improve the method above somehow.
Isn't it possible for the bridge to compute its configuration at runtime
by querying properties of the downstream video entities ? That would solve
the problem neatly.
You mean the bridge driver would somehow take a peek into panel1 and
panel2 nodes, looking for bridge specific properties? Sounds somewhat
fragile to me... How would the bridge driver know a property is for the
bridge?
No, I mean the bridge driver should call the API provided by the panel objects
to get information about the panels, and compute its configuration parameters
from those. I'm not sure if that's possible though, it depends on whether the
bridge parameters can be computed from information provided by the panel.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
Hi Ajay, On Tuesday 07 October 2014 16:06:55 Ajay kumar wrote: On Tue, Oct 7, 2014 at 4:00 PM, Tomi Valkeinen wrote: On 20/09/14 14:22, Ajay kumar wrote: Well, I am okay with using video ports to describe the relationship between the encoder, bridge and the panel. But, its just that I need to make use of 2 functions when phandle does it using just one function ;) -panel_node = of_parse_phandle(dev-of_node, panel, 0) + endpoint = of_graph_get_next_endpoint(dev-of_node, NULL); + if (!endpoint) + return -EPROBE_DEFER; + + panel_node = of_graph_get_remote_port_parent(endpoint); + if (!panel_node) + return -EPROBE_DEFER; If nobody else has objections over using of_graph functions instead of phandles, I can respin this patchset by making use of video ports. The discussion did digress somewhat. As a clarification, I'm in no way nack'ing this series because it doesn't use the graphs for video connections. I don't see the simple phandle bindings used here as broken as such. Well, I am okay with any approach you guys decide on. I desperately want this to get this in since it has been floating around for quite sometime. The more we drag this, the more rework for me since the number of platforms using bridge support is increasing daily! I won't nack this patch either. I'm however concerned that we'll run straight into the wall if we don't come up with an agreement on a standard way to describe connections in DT for display devices, which is why I would prefer the ps8622 bindings to use OF graph to describe connections. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
Hi Tomi, On Tuesday 07 October 2014 11:25:56 Tomi Valkeinen wrote: On 07/10/14 10:23, Laurent Pinchart wrote: You mean the bridge driver would somehow take a peek into panel1 and panel2 nodes, looking for bridge specific properties? Sounds somewhat fragile to me... How would the bridge driver know a property is for the bridge? No, I mean the bridge driver should call the API provided by the panel objects to get information about the panels, and compute its configuration parameters from those. I'm not sure if that's possible though, it depends on whether the bridge parameters can be computed from information provided by the panel. Right. My example tried to show a case where they can't be queried. The board or the wiring causes signal degradation, which can be fixed by increasing the bridge output voltage slightly. So it has nothing really to do with the panel, but the board. I fully admit that it is a purely theoretical case, and I don't have any real use cases in mind right now. Still, I agree with you that we might need to configure the bridge differently depending on the selected output with parameters that are not specific to the bridge. There's two use cases I can think of. In the first one the panels are connected directly to the bridge. The bridge should have two links from its output port, one to each panel. If we use the OF graph bindings then the bridge output port node would have two endpoint nodes, and configuration parameters can be stored in the endpoint nodes. In the second use case another element would be present between the bridge and the two panels. In that case there would be a single link between the bridge and that other element. If the bridge needs to be configured differently depending on the selected panel using parameters that can't be queried from the panels, then we'll have a problem. I'm not sure whether this use case has practical applications as board-related parameters seem to me that they pertain to physical links. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
is always done downstream, so you'd start by looking at some type of bus interface and query it for what devices are present on the bus. Take for example PCI: the CPU only needs to know how to access the host bridge and will then probe devices behind each of the ports on that bridge. Some of those devices will be bridges, too, so it will continue to probe down the hierarchy. Without DT you don't have a means to know that there was a panel before you've actually gone and probed your whole hierarchy and found a GPU with some sort of output that a panel can be connected to. I think it makes a lot of sense to describe things in the same way in DT. Maybe I don't quite follow, but it sounds to be you are mixing control and data. For control, all you say is true. The CPU probes the devices on control busses, either with the aid of HW or the aid of DT, going downstream. But the data paths are a different matter. The CPU/SoC may not even be involved in the whole data path. You could have a sensor on the board directly connected to a panel. Both are controlled by the CPU, but the data path goes from the sensor to the panel (or vice versa). There's no way the data paths can be CPU centric in that case. Also, a difference with the data paths compared to control paths is that they are not strictly needed for operation. An encoder can generate an output without enabling its input (test pattern or maybe blank screen, or maybe a screen with company logo). Or an encoder with two inputs might only get the second input when the user requests a very high res mode. So it is possible that the data paths are lazily initialized. You do know that there is a panel right after the device is probed according to its control bus. It doesn't mean that the data paths are there yet. In some cases the user space needs to reconfigure the data paths before a panel has an input and can be used to show an image from the SoC's display subsystem. The point here being that the data path bindings don't really relate to the probing part. You can probe no matter which way the data path bindings go, and no matter if there actually exists (yet) a probed device on the other end of a data path phandle. While I think having video data connections in DT either way, downstream or upstream, would work, it has felt most natural for me to have the phandles from video sinks to video sources. The reason for that is that I think the video sink has to be in control of its source. It's the sink that tells the source to start or stop or reconfigure. So I have had need to get the video source from a video sink, but I have never had the need to get the video sinks from video sources. We could decide to model all data connections are phandles that go in the data flow direction (source to sink), opposite to the data flow direction (sink to source), or in both directions. The problem with the sink to source direction is that it raises the complexity of implementations for display drivers, as the master driver that will bind all the components together will have a hard time locating the components in DT if all the components point towards it. Modeling the connections in the source to sink direction only would create the exact same problem for video capture (camera) devices. That's why I believe that bidirectional connections would be a better choice. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
Hi Thierry, On Tuesday 23 September 2014 16:49:38 Thierry Reding wrote: On Tue, Sep 23, 2014 at 02:52:24PM +0300, Laurent Pinchart wrote: On Tuesday 23 September 2014 13:47:40 Andrzej Hajda wrote: On 09/23/2014 01:23 PM, Laurent Pinchart wrote: [...] This becomes an issue even on Linux when considering video-related devices that can be part of either a capture pipeline or a display pipeline. If the link always goes in the data flow direction, then it will be easy to locate the downstream device (bridge or panel) from the display controller driver, but it would be much more difficult to locate the same device from a camera driver as all of a sudden the device would become an upstream device. Why? If you have graph: sensor -- camera Then camera register itself in some framework as a destination device and sensor looks in this framework for the device identified by remote endpoint. Then sensor tells camera it is connected to it and voila. Except that both kernelspace and userspace deal with cameras the other way around, the master device is the camera receiver, not the camera sensor. DRM is architected the same way, with the component that performs DMA operations being the master device. I don't see what's wrong with having the camera reference the sensor by phandle instead. That's much more natural in my opinion. The problem, as explained by Tomi in a more recent e-mail (let's thus discuss the issue there) is that making the phandles pointing outwards from the CPU point of view stops working when the same chip or IP core can be used in both a camera and a display pipeline (and we have real use cases for that), or when the CPU isn't involved at all in the pipeline. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
Hi Tomi,
On Wednesday 24 September 2014 11:42:06 Tomi Valkeinen wrote:
On 23/09/14 17:45, Thierry Reding wrote:
On Tue, Sep 23, 2014 at 02:31:35PM +0300, Tomi Valkeinen wrote:
On 23/09/14 12:39, Thierry Reding wrote:
My point is that if you use plain phandles you usually have the
meta-data already. Referring to the above example, bridge0 knows that it
should look for a bridge with phandle bridge1, whereas bridge1 knows
that the device it is connected to is a panel.
The bridge should not care what kind of device is there on the other
end. The bridge just has an output, going to another video component.
You'll need to know at some point that one of the devices in a panel,
otherwise you can't treat it like a panel.
The bridge doesn't need to treat it like a panel. Someone else might,
though. But the panel driver knows it is a panel, and can thus register
needed services, or mark itself as a panel.
Well, I can't say about this particular bridge, but afaik you can
connect a parallel RGB signal to multiple panels just like that,
without any muxing.
Right, but in that case you're not reconfiguring the signal in any way
for each of the panels. You send one single signal to all of them. For
Yes, that's one use case, cloning. But I was not talking about that.
all intents and purposes there is only one panel. Well, I guess you
could have separate backlights for the panels. In that case though it
seems better to represent it at least as a virtual mux or bridge, or
perhaps a mux panel.
I was talking about the case where you have two totally different
devices, let's say panels, connected to the same output. One could take
a 16-bit parallel RGB signal, the other 24-bit. Only one of them can be
enabled at a time (from HW perspective both can be enabled at the same
time, but then the other one shows garbage).
So we're essentially back to a mux, albeit maybe a virtual one.
Yes. Are you suggesting to model virtual hardware in .dts? I got the
impression that you wanted to model only the real hardware in .dts =).
But seriously speaking, I was thinking about this. I'd really like to
have a generic video-mux node, that would still somehow allow us to have
device specific configurations for the video sources and sinks (which
the endpoints provide us), without endpoints.
The reason endpoints don't feel right in this case is that it makes
sense that the mux has a single input and two outputs, but with
endpoints we need two endpoints from the bridge (which is still ok), but
we also need two endpoitns in the mux's input side, which doesn't feel
right.
I came up with the following. It's quite ugly, though. I hope someone
has ideas how it could be done in a neater way:
bridge1234 {
bridge1234-cfg1: config1 {
high-voltage;
};
bridge1234-cfg2: config2 {
low-voltage;
};
output = mux;
};
mux: video-gpio-mux {
gpio = 123;
outputs = panel1 panel2;
input-configs = bridge1234-cfg1 bridge1234-cfg2;
};
panel1: panel-foo {
};
panel2: panel-foo {
};
So the bridge node has configuration sets. These might be compared to
pinmux nodes. And the mux has a list of input-configs. When panel1 is to
be enabled, bridge1234-cfg1 config becomes active, and the bridge is
given this configuration.
I have to say the endpoint system feels cleaner than the above, but
perhaps it's possible to improve the method above somehow.
Isn't it possible for the bridge to compute its configuration at runtime by
querying properties of the downstream video entities ? That would solve the
problem neatly.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
On Tuesday 23 September 2014 12:02:45 Andrzej Hajda wrote: On 09/23/2014 11:30 AM, Tomi Valkeinen wrote: On 23/09/14 09:21, Thierry Reding wrote: Well, I can write almost any kind of bindings, and then evidently my device would work. For me, on my board. Well, that's the whole problem with DT. For many devices we only have a single setup to test against. And even when we have several they often are derived from each other. But the alternative would be to defer (possibly indefinitely) merging support for a device until a second, wildly different setup shows up. That's completely unreasonable and we need to start somewhere. Yes, but in this case we know of existing boards that have complex setups. It's not theoretical. I'm not saying we should stop everything until we have a 100% solution for the rare complex cases. But we should keep them in mind and, when possible, solve problems in a way that will work for the complex cases also. I guess non-video devices haven't had need for those. I have had lots of boards with video setup that cannot be represented with simple phandles. I'm not sure if I have just been unlucky or what, but my understand is that other people have encountered such boards also. Usually the problems encountered there have been circumvented with some hacky video driver for that specific board, or maybe a static configuration handled by the boot loader. I have yet to encounter such a setup. Can you point me at a DTS for one such setup? I do remember a couple of hypothetical cases being discussed at one time or another, but I haven't seen any actual DTS content where this was needed. No, I can't point to them as they are not in the mainline (at least the ones I've been working on), for obvious reasons. With a quick glance, I have the following devices in my cabinet that have more complex setups: OMAP 4430 SDP, BeagleBoneBlack + LCD, AM43xx EVM. Many Nokia devices used to have such setups, usually so that the LCD and tv-out were connected to the same video source. Do we have a standard way of representing the video pipeline with simple phandles? Or does everyone just do their own version? If there's no standard way, it sounds it'll be a mess to support in the future. It doesn't matter all that much whether the representation is standard. Again, I disagree. phandles should simply point to the next element in the pipeline and the OS abstractions should be good enough to handle the details about how to chain the elements. I, on the other hand, would rather see the links the other way around. Panel having a link to the video source, etc. The video graphs have two-way links, which of course is the safest options, but also more verbose and redundant. When this was discussed earlier, it was unclear which way the links should be. It's true that only links to one direction are strictly needed, but the question raised was that if in the drivers we end up always going the links the other way, the performance penalty may be somewhat big. (If I recall right). I do not see why performance may drop significantly? If the link is one-way it should probably work as below: - the destination registers itself in some framework, - the source looks for the destination in this framework using phandle, - the source starts to communicate with the destination - since now full two way link can be established dynamically. Where do you see here big performance penalty? The performance-related problems arise when you need to locate the remote device in the direction opposite to the phandle link direction. Traversing a link forward just involves a phandle lookup, but traversing it backwards isn't possible the same way. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
On Tuesday 23 September 2014 11:53:15 Thierry Reding wrote: On Tue, Sep 23, 2014 at 12:30:20PM +0300, Tomi Valkeinen wrote: On 23/09/14 09:21, Thierry Reding wrote: Well, I can write almost any kind of bindings, and then evidently my device would work. For me, on my board. Well, that's the whole problem with DT. For many devices we only have a single setup to test against. And even when we have several they often are derived from each other. But the alternative would be to defer (possibly indefinitely) merging support for a device until a second, wildly different setup shows up. That's completely unreasonable and we need to start somewhere. Yes, but in this case we know of existing boards that have complex setups. It's not theoretical. Complex setups involving /this particular/ bridge and binding are theoretical at this point, however. phandles should simply point to the next element in the pipeline and the OS abstractions should be good enough to handle the details about how to chain the elements. I, on the other hand, would rather see the links the other way around. Panel having a link to the video source, etc. Why? It seems much more natural to describe this using the natural flow of data. The device furthest away from the CPU should be the target of the phandle. That way the DT can be used to trace the flow of data down the pipeline. The video graphs have two-way links, which of course is the safest options, but also more verbose and redundant. Right. If we take that line of thinking to the extreme we end up listing individual registers in DT so that we can safely assume that we can control all possible aspects of the device. And the other extreme would be to have a single DT node for the logical video device with all information pertaining to it stored in C code. Extremes are never good, we need to find a reasonable middle-ground here. I believe OF graph fulfills that requirement, it might be slightly more verbose than a single phandle, but it's also much more versatile. Like I said, this seems to be the latest response to DT ABI stability requirement. Add as much data to a DT node as possible so that it has higher chances of being complete. The result is often overly complex DT content that doesn't add any real value. When this was discussed earlier, it was unclear which way the links should be. It's true that only links to one direction are strictly needed, but the question raised was that if in the drivers we end up always going the links the other way, the performance penalty may be somewhat big. (If I recall right). I doubt that graphs will become so complex that walking it would become a performance bottleneck. In fact if we're constantly walking the graph we're already doing something wrong. It should only be necessary when the pipeline configuration changes, which should hopefully not be very often (i.e. not several times per second). -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
On Tuesday 23 September 2014 13:18:30 Andrzej Hajda wrote: On 09/23/2014 01:10 PM, Laurent Pinchart wrote: On Tuesday 23 September 2014 12:02:45 Andrzej Hajda wrote: On 09/23/2014 11:30 AM, Tomi Valkeinen wrote: On 23/09/14 09:21, Thierry Reding wrote: Well, I can write almost any kind of bindings, and then evidently my device would work. For me, on my board. Well, that's the whole problem with DT. For many devices we only have a single setup to test against. And even when we have several they often are derived from each other. But the alternative would be to defer (possibly indefinitely) merging support for a device until a second, wildly different setup shows up. That's completely unreasonable and we need to start somewhere. Yes, but in this case we know of existing boards that have complex setups. It's not theoretical. I'm not saying we should stop everything until we have a 100% solution for the rare complex cases. But we should keep them in mind and, when possible, solve problems in a way that will work for the complex cases also. I guess non-video devices haven't had need for those. I have had lots of boards with video setup that cannot be represented with simple phandles. I'm not sure if I have just been unlucky or what, but my understand is that other people have encountered such boards also. Usually the problems encountered there have been circumvented with some hacky video driver for that specific board, or maybe a static configuration handled by the boot loader. I have yet to encounter such a setup. Can you point me at a DTS for one such setup? I do remember a couple of hypothetical cases being discussed at one time or another, but I haven't seen any actual DTS content where this was needed. No, I can't point to them as they are not in the mainline (at least the ones I've been working on), for obvious reasons. With a quick glance, I have the following devices in my cabinet that have more complex setups: OMAP 4430 SDP, BeagleBoneBlack + LCD, AM43xx EVM. Many Nokia devices used to have such setups, usually so that the LCD and tv-out were connected to the same video source. Do we have a standard way of representing the video pipeline with simple phandles? Or does everyone just do their own version? If there's no standard way, it sounds it'll be a mess to support in the future. It doesn't matter all that much whether the representation is standard. Again, I disagree. phandles should simply point to the next element in the pipeline and the OS abstractions should be good enough to handle the details about how to chain the elements. I, on the other hand, would rather see the links the other way around. Panel having a link to the video source, etc. The video graphs have two-way links, which of course is the safest options, but also more verbose and redundant. When this was discussed earlier, it was unclear which way the links should be. It's true that only links to one direction are strictly needed, but the question raised was that if in the drivers we end up always going the links the other way, the performance penalty may be somewhat big. (If I recall right). I do not see why performance may drop significantly? If the link is one-way it should probably work as below: - the destination registers itself in some framework, - the source looks for the destination in this framework using phandle, - the source starts to communicate with the destination - since now full two way link can be established dynamically. Where do you see here big performance penalty? The performance-related problems arise when you need to locate the remote device in the direction opposite to the phandle link direction. Traversing a link forward just involves a phandle lookup, but traversing it backwards isn't possible the same way. But you do not need to traverse backwards. You just wait when the source start to communicate with the destination, at this moment destination can build back-link dynamically. Your driver might not need it today for your use cases, but can you be certain that no driver on any OS would need to ? This becomes an issue even on Linux when considering video-related devices that can be part of either a capture pipeline or a display pipeline. If the link always goes in the data flow direction, then it will be easy to locate the downstream device (bridge or panel) from the display controller driver, but it would be much more difficult to locate the same device from a camera driver as all of a sudden the device would become an upstream device. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
Hi Thierry, On Tuesday 23 September 2014 12:10:33 Thierry Reding wrote: On Tue, Sep 23, 2014 at 11:43:47AM +0200, Andrzej Hajda wrote: On 09/23/2014 10:35 AM, Thierry Reding wrote: [...] But I agree that it would be nice to unify bridges and encoders more. It should be possible to make encoder always a bridge (or perhaps even replace encoders with bridges altogether). Then once you're out of the DRM device everything would be a bridge until you get to a panel. I agree that some sort of unification of bridges, (slave) encoders is a good thing, this way we stay only with bridges and panels as receivers. But we will still have to deal with the code like: if (on the other end of the link is panel) do panel framework specific things else do bridge framework specific things The code in both branches usually does similar things but due to framework differences it is difficult to merge. That's because they are inherently different entities. You can perform operations on a panel that don't make sense for a bridge and vice-versa. But a subset of the operations are common, so it's annoying to have to explicitly deal with both objects in code that only cares about the common subset of operations. Ideally it would be best to get rid of such constructs. For example by trying to make frameworks per interface type exposed by device (eg. video_sink) and not by device type (drm_bridge, drm_panel). But then you loose all information about the real type of device. Or you have to create a common base class. And then you're still back to dealing with the two specific cases in many places, so the gain seems rather minimal. We would need to experiment with the concept, but a single abstract class works surprisingly well in V4L. Devices as diverse as camera sensors, HDMI receivers or video scalers expose a single API, and drivers for the logical capture devices (roughly equivalent to the DRM display controller drivers) turned out not to need much knowledge about what the connected devices really are in order to use them. This was implemented with a superset of operations, with many of them being optional to implement for component drivers. Of course knowing the exact type of a component can still be sometimes useful, but that would be pretty easy to implement through a query operation exposed by the components. We're digressing here, but my point is that the encoder/bridge merge that we seem to agree about should, in my opinion, be followed by a merge with panels. I am, however, more interested at this point by the encoder/bridge merge, as having two separate frameworks there is the biggest pain point for my use cases. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
DT property to selectively disable device features (was [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties)
Hi Thierry,
On Tuesday 23 September 2014 07:55:34 Thierry Reding wrote:
On Tue, Sep 23, 2014 at 03:00:37AM +0300, Laurent Pinchart wrote:
On Monday 22 September 2014 13:35:15 Thierry Reding wrote:
On Mon, Sep 22, 2014 at 04:53:22PM +0530, Ajay kumar wrote:
On Mon, Sep 22, 2014 at 4:11 PM, Thierry Reding wrote:
On Mon, Sep 22, 2014 at 02:01:38PM +0530, Ajay kumar wrote:
[snip]
I think you misunderstood what I said, or maybe I didn't explain
clearly what I meant. If the PS8622 provides a backlight there's
nothing wrong with always exposing it. The bridge itself isn't going
to be using the backlight anyway. Rather the panel itself should be
using the backlight device exposed by PS8622 or some separate
backlight device.
To illustrate by an example:
ps8622: ... {
compatible = parade,ps8622;
...
};
panel {
...
backlight = ps8622;
...
};
No, if ps8622 backlight control is used, we need not specify the
backlight phandle for the panel driver. Somehow, ps8622 internal
circuitry keeps the bootup glitch free :)
Backlight control and panel controls can be separate then.
But they shouldn't. Your panel driver should always be the one to
control backlight. How else is the bridge supposed to know when to turn
backlight on or off?
What you did in v6 of this series was look up a backlight device and
then not use it. That seemed unnecessary. Looking at v6 again the
reason for getting a phandle to the backlight was so that the device
itself did not expose its own backlight controlling circuitry if an
external one was being used. But since the bridge has no business
controlling the backlight, having the backlight phandle in the
bridge is not correct.
So I think what you could do in the driver instead is always expose
the backlight device. If the panel used a different backlight, the
PS8622's internal on simply wouldn't be accessed. It would still be
possible to control the backlight in sysfs, but that shouldn't be a
problem (only root can access it)
That would be like simple exposing a feature which cannot be used
by the user, ideally which should not be used by the user.
And it won't be used unless they access the sysfs files directly. There
are a lot of cases where we expose functionality that cannot be
meaningfully used by the user. For example, a GPIO may not be routed to
anything on a board, yet we don't explicitly hide any specific GPIOs
from users.
That said, I have no strong objections to the boolean property if
you feel like it's really necessary.
Won't you think having a boolean property for an optional
feature of the device, is better than all these?
Like I said, I'm indifferent on the matter. I don't think it's necessary
to hide the backlight device, but if you want to, please feel free to do
so.
DT typically use
status = disabled
to disable devices. In this case we don't want to disable the ps8622
completely, but just one of its functions. Maybe a backlight-status
property could be used for that ? If that's considered too verbose, I
would be fine with a disable-feature boolean property too.
Another alternative would be to make the backlight a subnode:
ps8622: bridge@... {
compatible = parade,ps8622;
...
backlight {
...
status = disabled;
...
};
};
I've thought about that as well. I feel it's getting a bit complex for little
added benefit, but I don't have a very strong opinion.
Devices that expose several functions are not the odd case, and the need to
selectively mark some of them as disabled in DT seems pretty common to me. I
wonder if we should try to decide on standard bindings for that. To recap the
proposals, we could have
- a boolean disable-feature property
- a text feature-status property
- a features subnode with a status text property
Anything else ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
On Tuesday 23 September 2014 13:47:40 Andrzej Hajda wrote: On 09/23/2014 01:23 PM, Laurent Pinchart wrote: On Tuesday 23 September 2014 13:18:30 Andrzej Hajda wrote: On 09/23/2014 01:10 PM, Laurent Pinchart wrote: On Tuesday 23 September 2014 12:02:45 Andrzej Hajda wrote: On 09/23/2014 11:30 AM, Tomi Valkeinen wrote: On 23/09/14 09:21, Thierry Reding wrote: Well, I can write almost any kind of bindings, and then evidently my device would work. For me, on my board. Well, that's the whole problem with DT. For many devices we only have a single setup to test against. And even when we have several they often are derived from each other. But the alternative would be to defer (possibly indefinitely) merging support for a device until a second, wildly different setup shows up. That's completely unreasonable and we need to start somewhere. Yes, but in this case we know of existing boards that have complex setups. It's not theoretical. I'm not saying we should stop everything until we have a 100% solution for the rare complex cases. But we should keep them in mind and, when possible, solve problems in a way that will work for the complex cases also. I guess non-video devices haven't had need for those. I have had lots of boards with video setup that cannot be represented with simple phandles. I'm not sure if I have just been unlucky or what, but my understand is that other people have encountered such boards also. Usually the problems encountered there have been circumvented with some hacky video driver for that specific board, or maybe a static configuration handled by the boot loader. I have yet to encounter such a setup. Can you point me at a DTS for one such setup? I do remember a couple of hypothetical cases being discussed at one time or another, but I haven't seen any actual DTS content where this was needed. No, I can't point to them as they are not in the mainline (at least the ones I've been working on), for obvious reasons. With a quick glance, I have the following devices in my cabinet that have more complex setups: OMAP 4430 SDP, BeagleBoneBlack + LCD, AM43xx EVM. Many Nokia devices used to have such setups, usually so that the LCD and tv-out were connected to the same video source. Do we have a standard way of representing the video pipeline with simple phandles? Or does everyone just do their own version? If there's no standard way, it sounds it'll be a mess to support in the future. It doesn't matter all that much whether the representation is standard. Again, I disagree. phandles should simply point to the next element in the pipeline and the OS abstractions should be good enough to handle the details about how to chain the elements. I, on the other hand, would rather see the links the other way around. Panel having a link to the video source, etc. The video graphs have two-way links, which of course is the safest options, but also more verbose and redundant. When this was discussed earlier, it was unclear which way the links should be. It's true that only links to one direction are strictly needed, but the question raised was that if in the drivers we end up always going the links the other way, the performance penalty may be somewhat big. (If I recall right). I do not see why performance may drop significantly? If the link is one-way it should probably work as below: - the destination registers itself in some framework, - the source looks for the destination in this framework using phandle, - the source starts to communicate with the destination - since now full two way link can be established dynamically. Where do you see here big performance penalty? The performance-related problems arise when you need to locate the remote device in the direction opposite to the phandle link direction. Traversing a link forward just involves a phandle lookup, but traversing it backwards isn't possible the same way. But you do not need to traverse backwards. You just wait when the source start to communicate with the destination, at this moment destination can build back-link dynamically. Your driver might not need it today for your use cases, but can you be certain that no driver on any OS would need to ? I have just showed how to create back-link dynamically if we have only forward-link in DT. Ie it is a trivial 'proof' that the direction is not so important. So I do not understand why do you pose such question? This becomes an issue even on Linux when considering video-related devices that can be part of either a capture pipeline or a display pipeline. If the link always goes in the data flow direction, then it will be easy to locate the downstream device (bridge or panel) from the display controller driver, but it would be much more difficult to locate the same device from a camera driver
Re: [PATCH V7 11/12] Documentation: bridge: Add documentation for ps8622 DT properties
they access the sysfs files directly. There are a lot of cases where we expose functionality that cannot be meaningfully used by the user. For example, a GPIO may not be routed to anything on a board, yet we don't explicitly hide any specific GPIOs from users. That said, I have no strong objections to the boolean property if you feel like it's really necessary. Won't you think having a boolean property for an optional feature of the device, is better than all these? Like I said, I'm indifferent on the matter. I don't think it's necessary to hide the backlight device, but if you want to, please feel free to do so. DT typically use status = disabled to disable devices. In this case we don't want to disable the ps8622 completely, but just one of its functions. Maybe a backlight-status property could be used for that ? If that's considered too verbose, I would be fine with a disable-feature boolean property too. Another alternative would be not to expose it at all and not have the boolean property since you don't seem to have a way to test it in the first place (or at least there's no device support upstream where it's needed). -- Regards, Laurent Pinchart signature.asc Description: This is a digitally signed message part.
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Thierry, On Monday 22 September 2014 09:40:38 Thierry Reding wrote: On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote: On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. No. Component framework was designed with multi-device drivers in mind. That is, drivers that need to combine two or more platform devices into a single logical device. Typically that includes display controllers and encoders (in various looks) for DRM. I disagree. AFAIK the component framework was designed to easily combine multiple devices into a single logical device, regardless of which bus each device is connected to. That's what makes the component framework useful : it allows master drivers to build logical devices from heterogeneous components without having to use one API per bus and/or component type. If the only goal had been to combine platform devices on an SoC, simpler device-specific solutions would likely have been used instead. Panels and bridges are in my opinion different because they are outside of the DRM driver. They aren't part of the device complex that an SoC provides. They represent hardware that is external to the SoC and the DRM driver and can be shared across SoCs. They represent hardware external to the SoC, but internal to the logical DRM device. Forcing panels and bridges to register as components will require all drivers to implement master/component support solely for accessing this external hardware. What you're suggesting is like saying that clocks or regulators should register as components so that their users can get them that way. In fact by that argument everything that's referenced by phandle would need to register as component (PHYs, LEDs, GPIOs, I2C controllers, ...). No, that's very different. The device you list are clearly external resources, while the bridges and panels are components part of a logical display device. This patch set introduces yet another framework, without any compelling reason as far as I can see. Today DRM drivers already need to use three different frameworks (component, I2C slave encoder and panel), and we're adding a fourth oneto make the mess even messier. Panel and bridge aren't really frameworks. Rather they are a simple registry to allow drivers to register panels and bridges and display drivers to look them up. Regardless of how you call them, we have three interfaces. This is really a headlong rush, we need to stop and fix the design mistakes. Can you point out specific design mistakes? I don't see any, but I'm obviously biased. The slave encoder / bridge split is what I consider a design mistake. Those two interfaces serve the same purpose, they should really be merged. - The component framework solves the probe ordering problem. Bridges can use deferred probing, but when a bridge requires a resources (such as a clock for instance) provided by the display controller, this will break. Panel and bridges can support deferred probing without the component framework just fine. Not if the bridge requires a clock provided by the display controller, in which case there's a dependency loop. Without this patchset, you cannot bring an X server based display on snow and peach_pit. Also, day by day the number of platforms using drm_bridge is increasing. That's exactly why I'd like to use the component framework now
Re: [PATCH V6 0/8] drm/exynos: few patches to enhance bridge chip support
Hi Ajay, On Wednesday 17 September 2014 15:43:04 Ajay kumar wrote: Hi Laurent, Please find the latest series here: http://www.spinics.net/lists/dri-devel/msg66740.html Thank you. My comment was meant to be general though, not just for your patch series. On Wed, Sep 17, 2014 at 3:23 PM, Laurent Pinchart wrote: On Wednesday 30 July 2014 11:40:54 Thierry Reding wrote: [snip] One other thing: how does the bridge know which mode to drive? I suspect that it can drive more than one mode? Can it freely be configured or does it have a predefined set of modes? If the latter, then according to what you said above there needs to be a way to configure the bridge (via DT?) so that it reports the mode matching the panel. I wonder if that should be handled completely in code, so that for example a bridge has a panel attached it can use the panel's .get_modes() and select a matching mode among the set that it supports. Yes, pretty please :-) I don't think it would be a good idea to duplicate mode information in the bridge DT node, as that's not a property of the bridge. Querying the mode at runtime is in my opinion a much better option, and would also allow switching between different modes at runtime when that makes sense. Now, I'm not sure whether it should be the bridge driver querying the panel driver directly, or the display controller driver doing it and then configuring the bridge accordingly. The latter seems more generic to me and doesn't rely on the assumption that the bridge output will always be directly connected to a panel. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Ajay, On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. This patch set introduces yet another framework, without any compelling reason as far as I can see. Today DRM drivers already need to use three different frameworks (component, I2C slave encoder and panel), and we're adding a fourth one to make the mess even messier. This is really a headlong rush, we need to stop and fix the design mistakes. - The component framework solves the probe ordering problem. Bridges can use deferred probing, but when a bridge requires a resources (such as a clock for instance) provided by the display controller, this will break. The encoder driver should call drm_bridge_attach_encoder to pass on the drm_device and the encoder pointers to the bridge object. Now that the drm_device pointer is available, the encoder then calls bridge-funcs-post_encoder_init to allow the bridge to continue registering itself with the drm core. This is what really bothers me with DRM bridge. The framework assumes that a bridge will always bridge an encoder and a connector. Beside lacking support for chained bridges, this creates an artificial split between bridges and encoders by modeling the same components using drm_encoder or drm_bridge depending on their position in the video output pipeline. I would like to see drm_bridge becoming more self-centric, removing the awareness of the upstream encoder and downstream connector. I'll give this a try, but it will conflict with this patch, so I'd like to share opinions and coordinate efforts sooner than later if possible. I am not really able to understand how you want drm_bridge to be. As of now, there are many platforms using drm_bridge and they don't have a problem with current implementation. Regarding chained bridges: Can't you add this once my patchset is merged? As an additional feature? Yes, as I mentioned in another e-mail this can be fixed later. I want to start discussing it though. To be honest, I have spent quite sometime for working on this patchset. All I started with was to add drm_panel support to drm_bridge. When I sent the first patchset for that, Daniel, Rob and Thierry raised a concern that current bridge framework itself is not proper and hence they asked me to fix that first. And we have reached till here based on their comments only. Without this patchset, you cannot bring an X server based display on snow and peach_pit. Also, day by day the number of platforms using drm_bridge is increasing. That's exactly why I'd like to use the component framework now, as the conversion will become more complex as time goes by. And, I don't really see a problem with the current approach(which is exactly the same way panel framework is). And, I am no decision maker here. I would expect the top guys to comment! -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 0/8] drm/exynos: few patches to enhance bridge chip support
Hi Thierry,
On Wednesday 30 July 2014 11:40:54 Thierry Reding wrote:
On Wed, Jul 30, 2014 at 11:54:00AM +0530, Ajay kumar wrote:
On Tue, Jul 29, 2014 at 5:17 PM, Thierry Reding wrote:
On Tue, Jul 29, 2014 at 01:42:09PM +0200, Andreas Färber wrote:
Am 29.07.2014 13:36, schrieb Thierry Reding:
On Tue, Jul 29, 2014 at 01:21:48PM +0200, Andreas Färber wrote:
Am 28.07.2014 08:13, schrieb Ajay kumar:
On 7/27/14, Andreas Färber afaer...@suse.de wrote:
Am 25.07.2014 21:22, schrieb Ajay Kumar:
This series is based on exynos-drm-next branch of Inki Dae's tree
at:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.
git
I have tested this after adding few DT changes for
exynos5250-snow,
exynos5420-peach-pit and exynos5800-peach-pi boards.
I'm trying to test this with a modified exynos5250-spring DT
[...]
Unfortunately the most I got on Spring with attached DT was a blank
screen with a white horizontal line in the middle.
Do I need to specify a specific panel model for Spring?
[...]
From 9172a26a8f0d0f0d170bd27e1c150ad204d8086a Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Andreas=20F=C3=A4rber?= afaer...@suse.de
Date: Sun, 27 Jul 2014 21:58:06 +0200
Subject: [PATCH] ARM: dts: exynos5250: Add eDP/LVDS bridge to Spring
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Ajay Kumar ajaykumar...@samsung.com
[AF: Redone for v6]
Signed-off-by: Andreas F??rber afaer...@suse.de
---
arch/arm/boot/dts/exynos5250-spring.dts | 32 +-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/exynos5250-spring.dts
b/arch/arm/boot/dts/exynos5250-spring.dts index
687dfab86bc8..517b1ff2bfdf 100644
--- a/arch/arm/boot/dts/exynos5250-spring.dts
+++ b/arch/arm/boot/dts/exynos5250-spring.dts
@@ -64,10 +64,14 @@
vdd_pll-supply = s5m_ldo8_reg;
};
+ panel: panel {
+ compatible = simple-panel;
+ };
You can't do this. simple-panel isn't a valid panel model. It
should probably be removed from the platform_of_match table in the
driver.
Okay, that means the Snow DT is wrong, too:
https://patchwork.kernel.org/patch/4625441/
And the others specify it as fallback:
https://patchwork.kernel.org/patch/4625461/
https://patchwork.kernel.org/patch/4625451/
A quick grep shows that many (all?) devices that use DRM panels provide
simple-panel as fallback. That's probably fine as long as they also do
provide the specific model. But given that simple-panel does not have a
mode or physical size, I don't think even that makes sense.
On snow, the bridge chip provides the display mode instead of the panel.
That is why display was working for me.
Okay, I suppose under some circumstances that might make sense. Although
it's still always the panel that dictates the display timings, so the
panel node needs to have a panel model specific compatible value with a
matching entry in the panel-simple driver so that it can even be used in
setups without a bridge.
One other thing: how does the bridge know which mode to drive? I suspect
that it can drive more than one mode? Can it freely be configured or
does it have a predefined set of modes? If the latter, then according to
what you said above there needs to be a way to configure the bridge (via
DT?) so that it reports the mode matching the panel. I wonder if that
should be handled completely in code, so that for example a bridge has a
panel attached it can use the panel's .get_modes() and select a matching
mode among the set that it supports.
Yes, pretty please :-) I don't think it would be a good idea to duplicate mode
information in the bridge DT node, as that's not a property of the bridge.
Querying the mode at runtime is in my opinion a much better option, and would
also allow switching between different modes at runtime when that makes sense.
Now, I'm not sure whether it should be the bridge driver querying the panel
driver directly, or the display controller driver doing it and then
configuring the bridge accordingly. The latter seems more generic to me and
doesn't rely on the assumption that the bridge output will always be directly
connected to a panel.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V7 00/12] drm/exynos: few patches to enhance bridge chip support
Hi Javier, On Tuesday 16 September 2014 14:44:02 Javier Martinez Canillas wrote: [adding Laurent Pinchart to cc who had concerns with a previous version of this patch-set] Thank you, I've indeed missed v7. On Wed, Aug 27, 2014 at 4:29 PM, Ajay Kumar wrote: This series is based on master branch of Linus tree at: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git I have tested this after adding few DT changes for exynos5250-snow and exynos5420-peach-pit boards. The V4 series of this particular patchset was also tested by: Rahul Sharma rahul.sha...@samsung.com Javier Martinez Canillas jav...@dowhile0.org Changes since V2: -- Address comments from Jingoo Han for ps8622 driver -- Address comments from Daniel, Rob and Thierry regarding bridge chaining -- Address comments from Thierry regarding the names for new drm_panel functions Changes since V3: -- Remove hotplug based initialization of exynos_dp -- Make exynos_dp work directly with drm_panel, remove dependency on panel_binder -- Minor cleanups in panel_binder and panel_lvds driver Changes since V4: -- Use gpiod interface for panel-lvds and ps8622 drivers. -- Address comments from Javier. -- Fix compilation issues when PANEL_BINDER is selected as module. -- Split Documentation patches from driver patches. -- Rebase on top of the tree. Changes since V5: -- Modify bridge drivers to support driver model. -- Drop the concept of bridge chain(sincle there are no 2 real bridges) Hence drop bridge-panel_binder layer. -- Drop panel-lvds driver and accomodate the required changes in panel-simple driver. -- Use gpiod interface in ptn3460 driver. -- Address all comments by Thierry Reding for V5 series. -- Address comments from Sean Paul for exynos_dp_commit issue. Changes since V6: -- Panel patches were seperated and they are merged already. -- Fix few issues with ptn3460, before modifying the bridge core. -- Modify drm_bridge as per Thierry's comments for V6 series. -- Add drm_bridge changes minimally without breaking existing code. -- Add new features for ptn3460, step-by-step. -- Address comments from Thierry and Andreas for ptn3460 and ps8622. -- Split documentation patches from driver patches. I've tested your series on an Exynos5420 Peach Pit and an Exynos5250 Snow Chromebooks and display worked for me on both machines. I also needed [PATCH] drm/panel: simple: Add AUO B116XW03 panel support [0] which does not apply cleanly on linux-next so you may want to do a re-spin for that patch. For Snow I also had to disable CONFIG_FB_SIMPLE, otherwise I just saw a blink on boot and only the backlight remained turned on (no display output). I don't know if that is expected since IIUC it should be possible to do a transition from simplefb to a DRM/KMS driver. I don't have a serial console hooked on this machine so I couldn't debug it further, sorry. Also, I saw that Laurent mentioned some concerns today about the previous version (v6) of your series [1]. I guess he missed this v7 although AFAIU there was no fundamental change on this one so his concerns remains? I was really hoping that this could make it to 3.18 since display support is one of the last major functionalities that is missing on these machines. My main concern as far as merging this patch set goes is why we can't use the component framework instead of introducing yet another new subsystem-specific component registration framework. My other concerns could be addressed as follow-up patches (but I'd like to start discussing them now). [0]: http://patchwork.ozlabs.org/patch/384744/ [1]: http://www.spinics.net/lists/linux-samsung-soc/msg36791.html -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model
Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling drm_bridge_add_for_lookup. The parent encoder driver waits till the bridge is available in the lookup table(by calling of_drm_find_bridge) and then continues with its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. The encoder driver should call drm_bridge_attach_encoder to pass on the drm_device and the encoder pointers to the bridge object. Now that the drm_device pointer is available, the encoder then calls bridge-funcs-post_encoder_init to allow the bridge to continue registering itself with the drm core. This is what really bothers me with DRM bridge. The framework assumes that a bridge will always bridge an encoder and a connector. Beside lacking support for chained bridges, this creates an artificial split between bridges and encoders by modeling the same components using drm_encoder or drm_bridge depending on their position in the video output pipeline. I would like to see drm_bridge becoming more self-centric, removing the awareness of the upstream encoder and downstream connector. I'll give this a try, but it will conflict with this patch, so I'd like to share opinions and coordinate efforts sooner than later if possible. Also, non driver model based ptn3460 driver is removed in this patch. Signed-off-by: Ajay Kumar ajaykumar...@samsung.com --- .../devicetree/bindings/drm/bridge/ptn3460.txt | 27 -- drivers/gpu/drm/Makefile |1 + drivers/gpu/drm/bridge/Kconfig | 12 +- drivers/gpu/drm/bridge/Makefile|2 - drivers/gpu/drm/bridge/ptn3460.c | 343 - drivers/gpu/drm/drm_bridge.c | 89 + drivers/gpu/drm/drm_crtc.c |9 +- drivers/gpu/drm/exynos/Kconfig |3 +- drivers/gpu/drm/exynos/exynos_dp_core.c| 57 ++-- drivers/gpu/drm/exynos/exynos_dp_core.h|1 + drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |3 +- include/drm/bridge/ptn3460.h | 37 --- include/drm/drm_crtc.h | 16 +- 13 files changed, 147 insertions(+), 453 deletions(-) delete mode 100644 Documentation/devicetree/bindings/drm/bridge/ptn3460.txt delete mode 100644 drivers/gpu/drm/bridge/ptn3460.c create mode 100644 drivers/gpu/drm/drm_bridge.c delete mode 100644 include/drm/bridge/ptn3460.h -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/29] drivers: base: add notifier for failed driver bind
Hi Marek,
Thank you for the patch.
On Tuesday 05 August 2014 12:47:32 Marek Szyprowski wrote:
This patch adds support for getting a notify for failed device driver
bind, so all the items done in BUS_NOTIFY_BIND_DRIVER event can be
cleaned if the driver fails to bind.
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
drivers/base/dd.c | 10 +++---
include/linux/device.h | 4 +++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..541a41f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -237,10 +237,14 @@ static int driver_sysfs_add(struct device *dev)
return ret;
}
-static void driver_sysfs_remove(struct device *dev)
+static void driver_sysfs_remove(struct device *dev, int failed)
{
struct device_driver *drv = dev-driver;
+ if (failed dev-bus)
+ blocking_notifier_call_chain(dev-bus-p-bus_notifier,
+ BUS_NOTIFY_DRVBIND_FAILED, dev);
This might be a stupid question, but as you only call driver_sysfs_remove with
failed set to true in a single location (in the failure path of really_probe),
how about moving the blocking_notifier_call_chain to that location ? The code
seems to be a bit out of place here.
+
if (drv) {
sysfs_remove_link(drv-p-kobj, kobject_name(dev-kobj));
sysfs_remove_link(dev-kobj, driver);
@@ -316,7 +320,7 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
probe_failed:
devres_release_all(dev);
- driver_sysfs_remove(dev);
+ driver_sysfs_remove(dev, true);
dev-driver = NULL;
dev_set_drvdata(dev, NULL);
@@ -509,7 +513,7 @@ static void __device_release_driver(struct device *dev)
if (drv) {
pm_runtime_get_sync(dev);
- driver_sysfs_remove(dev);
+ driver_sysfs_remove(dev, false);
if (dev-bus)
blocking_notifier_call_chain(dev-bus-p-bus_notifier,
diff --git a/include/linux/device.h b/include/linux/device.h
index b387710..92daded 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -176,7 +176,7 @@ extern int bus_register_notifier(struct bus_type *bus,
extern int bus_unregister_notifier(struct bus_type *bus,
struct notifier_block *nb);
-/* All 4 notifers below get called with the target struct device *
+/* All 7 notifers below get called with the target struct device *
* as an argument. Note that those functions are likely to be called
* with the device lock held in the core, so be careful.
*/
@@ -189,6 +189,8 @@ extern int bus_unregister_notifier(struct bus_type *bus,
unbound */
#define BUS_NOTIFY_UNBOUND_DRIVER0x0006 /* driver is unbound
from the device */
+#define BUS_NOTIFY_DRVBIND_FAILED0x0007 /* driver failed to bind
+ to device */
extern struct kset *bus_get_kset(struct bus_type *bus);
extern struct klist *bus_get_device_klist(struct bus_type *bus);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] [media] v4l: Add source change event
= v4l2_event_src_replace,
+ .merge = v4l2_event_src_merge,
+};
+
+int v4l2_src_change_event_subscribe(struct v4l2_fh *fh,
+ const struct v4l2_event_subscription *sub)
+{
+ if (sub-type == V4L2_EVENT_SOURCE_CHANGE)
+ return v4l2_event_subscribe(fh, sub, 0, v4l2_event_src_ch_ops);
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(v4l2_src_change_event_subscribe);
+
+int v4l2_src_change_event_subdev_subscribe(struct v4l2_subdev *sd,
+ struct v4l2_fh *fh, struct v4l2_event_subscription *sub)
+{
+ return v4l2_src_change_event_subscribe(fh, sub);
+}
+EXPORT_SYMBOL_GPL(v4l2_src_change_event_subdev_subscribe);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index be05d01..1ab9045 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -132,4 +132,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
int v4l2_event_subdev_unsubscribe(struct v4l2_subdev *sd, struct v4l2_fh
*fh, struct v4l2_event_subscription *sub);
+int v4l2_src_change_event_subscribe(struct v4l2_fh *fh,
+ const struct v4l2_event_subscription *sub);
+int v4l2_src_change_event_subdev_subscribe(struct v4l2_subdev *sd,
+ struct v4l2_fh *fh, struct v4l2_event_subscription *sub);
#endif /* V4L2_EVENT_H */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index ea468ee..b923d91 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1765,6 +1765,7 @@ struct v4l2_streamparm {
#define V4L2_EVENT_EOS 2
#define V4L2_EVENT_CTRL 3
#define V4L2_EVENT_FRAME_SYNC4
+#define V4L2_EVENT_SOURCE_CHANGE 5
My last concern is about the event name (and the name of the related
structure). Either this event applies to inputs only, in which case the
documentation shouldn't mention the output number as it does above, or the
event applies to both inputs and outputs, in which case the name source is
too restrictive. I'd be tempted to go for the second option, even though I
have less use cases in mind on the output side.
#define V4L2_EVENT_PRIVATE_START 0x0800
/* Payload for V4L2_EVENT_VSYNC */
@@ -1796,12 +1797,19 @@ struct v4l2_event_frame_sync {
__u32 frame_sequence;
};
+#define V4L2_EVENT_SRC_CH_RESOLUTION (1 0)
+
+struct v4l2_event_src_change {
+ __u32 changes;
+};
+
struct v4l2_event {
__u32 type;
union {
struct v4l2_event_vsync vsync;
struct v4l2_event_ctrl ctrl;
struct v4l2_event_frame_syncframe_sync;
+ struct v4l2_event_src_changesrc_change;
__u8data[64];
} u;
__u32 pending;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 PATCH v6 11/16] ARM: dts: s6e3fa0: add DT bindings
On Wednesday 07 May 2014 10:05:46 YoungJun Cho wrote:
Hi Andrzej
Thank you for comments.
On 05/05/2014 07:35 PM, Andrzej Hajda wrote:
On 04/27/2014 03:50 AM, YoungJun Cho wrote:
This patch adds DT bindings for s6e3fa0 panel.
The bindings describes panel resources, display timings and cpu mode
timings.
Changelog v2:
- Adds unit address (commented by Sachin Kamat)
Changelog v3:
- Removes optional delay, size properties (commented by Laurent Pinchart)
- Adds OLED detection, TE gpio properties
Changelog v4:
- Moves CPU timings relevant properties from FIMD DT
(commeted by Laurent Pinchart, Andrzej Hajda)
Changelog v5:
- Fixes gpio property names (commented by Andrzej Hajda)
Changelog v6:
- Renames CPU timings to CPU mode timings
- Modifies CPU mode timings internal properties relevant things
(commeted by Laurent Pinchart, Andrzej Hajda)
Signed-off-by: YoungJun Cho yj44@samsung.com
Acked-by: Inki Dae inki@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
.../devicetree/bindings/panel/samsung,s6e3fa0.txt | 68
1 file changed, 68 insertions(+)
create mode 100644
Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
mode 100644
index 000..9f06645
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
@@ -0,0 +1,68 @@
+Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
+
+Required properties:
+ - compatible: samsung,s6e3fa0
+ - reg: the virtual channel number of a DSI peripheral
+ - vdd3-supply: core voltage supply
+ - vci-supply: voltage supply for analog circuits
+ - reset-gpios: a GPIO spec for the reset pin
+ - det-gpios: a GPIO spec for the OLED detection pin
+ - te-gpios: a GPIO spec for the TE pin
+ - display-timings: timings for the connected panel as described by [1]
This still bothers me, it forces users to provide bunch of fake
properties (four porches, two syncs and clock-frequency) just because we
need to pass somehow pixel width and height. And do we really need pixel
dimension to be passed via DT? I guess it could be:
- hardcoded into the driver,
- derived from the panel id,
- maybe read from the panel, this is the best option I guess but I am
not sure if panel provides an API for this.
I have been trying to only use cpu-mode-timings without display-timings
for next RFC v4.
As you mentioned, the only required things in display-timings are
clock-frequency, hactive and vactive.
I put them into cpu-mode-timings, but strictly speaking,
cpu-mode-timings is not related with display timing, it is data
transmission mode switch timing.
And there is no interface for this in drm (display) mode yet.
So I'm not sure what is the generic method to treat them.
If the sync-related properties are not used, they should not be specified. I
don't really like bundling the timing-related properties with the bus timings
though, I would rather make the sync-related properties optional in the
display timing node.
However, I'm pretty sure that your panel does have horizontal and vertical
blanking, and that information is needed in the display timings to compute the
frame rate accurately. The porch and sync length properties might not be
needed individually, but their total value is required.
+ - cpu-mode-timings: CPU interface timings for the connected panel,
+ and it contains following properties.
+Required properties:
+ - wr-active: clock cycles for the active period of CS enable
in CPU + interface.
+Optional properties:
+ - cs-setup: clock cycles for the active period of address
signal
+ enable until chip select is enable in CPU interface.
+ If not specified, the default value(0) will be used.
+ - wr-setup: clock cycles for the active period of CS signal
enable + until write signal is enable in CPU interface.
+ If not specified, the default value(0) will be used.
+ - wr-hold: clock cycles for the active period of CS disable
until + write signal is disable in CPU interface.
+ If not specified, the default value(0) will be used.
+
+Optional properties:
+
+The device node can contain one 'port' child node with one child
'endpoint' +node, according to the bindings defined in [2]. This node
should describe +panel's video bus.
+
+[1]: Documentation/devicetree/bindings/video/display-timing.txt
+[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+ panel@0 {
+ compatible = samsung,s6e3fa0;
+ reg = 0;
+ vdd3-supply = vcclcd_reg;
+ vci-supply = vlcd_reg
Re: [RFC v2 PATCH v3 10/14] drm/panel: add S6E3FA0 driver
Hi Thierry,
On Monday 28 April 2014 23:25:50 Thierry Reding wrote:
On Mon, Apr 28, 2014 at 05:05:24PM +0200, Laurent Pinchart wrote:
On Tuesday 22 April 2014 10:24:39 YoungJun Cho wrote:
On 04/22/2014 08:00 AM, Laurent Pinchart wrote:
Hi YoungJun,
Thank you for the patch.
On Monday 21 April 2014 21:28:37 YoungJun Cho wrote:
This patch adds MIPI-DSI command mode based S6E3FA0 AMOLED LCD Panel
driver.
Changelog v2:
- Declares delay, size properties in probe routine instead of DT
Changelog v3:
- Moves CPU timings relevant properties from FIMD DT
(commented by Laurent Pinchart, Andrzej Hajda)
Signed-off-by: YoungJun Cho yj44@samsung.com
Acked-by: Inki Dae inki@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/gpu/drm/panel/Kconfig |7 +
drivers/gpu/drm/panel/Makefile|1 +
drivers/gpu/drm/panel/panel-s6e3fa0.c | 569 ++
3 files changed, 577 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c
[snip]
diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c
b/drivers/gpu/drm/panel/panel-s6e3fa0.c new file mode 100644
index 000..1282678
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-s6e3fa0.c
@@ -0,0 +1,569 @@
[snip]
+static int s6e3fa0_get_modes(struct drm_panel *panel)
+{
+ struct drm_connector *connector = panel-connector;
+ struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_create(connector-dev);
+ if (!mode) {
+ DRM_ERROR(failed to create a new display mode\n);
+ return 0;
+ }
+
+ drm_display_mode_from_videomode(ctx-vm, mode);
+ mode-width_mm = ctx-width_mm;
+ mode-height_mm = ctx-height_mm;
+ connector-display_info.width_mm = mode-width_mm;
+ connector-display_info.height_mm = mode-height_mm;
+
+ mode-type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+ mode-private = (void *)ctx-cpu_timings;
Wouldn't it make sense to create a new get_interface_params (or
similar)
operation for drm_panel to query interface configuration parameters
instead of shoving it in the mode private field ?
You mean new get_interface_params operation is different from
get_modes() ?
Till now, struct drm_display_mode and most of mode relevant APIs are
only for video interface.
And CPU interface also needs video mode configurations.
I have a plan to implement the CPU interface relevant APIs like video
mode ones, but I think they should be used under current DRM mode APIs
like fill_modes, get_modes and so on.
So after that implementation, this private field will be replaced by
new ones.
Could you explain it in more detail?
The idea is that the interface parameters (RD/WR signals timings in this
case, but this could also include MIPI DSI lane configuration or any
other kind of physical interface parameters) are distinct from the video
modes.
We already have the lanes field in struct mipi_dsi_device.
Seems I've missed the addition of mipi_dsi_device to DRM.
I think in general I'd prefer to not spread these parameters around too
wildly. If this is a general requirement for DBI devices, perhaps what we
need is struct mipi_dbi_device?
That could be done, but I'm not sure we should expose the nature of the panel
device (i.e. I'm a mipi_dsi_device) to the display controller. I would be
worried about using container_of() on panel-dev to get a mipi_dsi_device
pointer, as we would then need a strict guarantee that the panel-dev pointer
is embedded directly in a mipi_dsi_device. This might be doable for DSI, but
other kind of panels might be connected to different control busses (I'm
thinking about I2C and SPI in particular) and still need to expose video
interface parameters to the display controller driver.
--
Regards,
Laurent Pinchart
signature.asc
Description: This is a digitally signed message part.
Re: [RFC v2 PATCH v3 10/14] drm/panel: add S6E3FA0 driver
Hi YoungJun,
On Tuesday 22 April 2014 10:24:39 YoungJun Cho wrote:
On 04/22/2014 08:00 AM, Laurent Pinchart wrote:
Hi YoungJun,
Thank you for the patch.
On Monday 21 April 2014 21:28:37 YoungJun Cho wrote:
This patch adds MIPI-DSI command mode based S6E3FA0 AMOLED LCD Panel
driver.
Changelog v2:
- Declares delay, size properties in probe routine instead of DT
Changelog v3:
- Moves CPU timings relevant properties from FIMD DT
(commented by Laurent Pinchart, Andrzej Hajda)
Signed-off-by: YoungJun Cho yj44@samsung.com
Acked-by: Inki Dae inki@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/gpu/drm/panel/Kconfig |7 +
drivers/gpu/drm/panel/Makefile|1 +
drivers/gpu/drm/panel/panel-s6e3fa0.c | 569 ++
3 files changed, 577 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c
[snip]
diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c
b/drivers/gpu/drm/panel/panel-s6e3fa0.c new file mode 100644
index 000..1282678
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-s6e3fa0.c
@@ -0,0 +1,569 @@
[snip]
+static int s6e3fa0_get_modes(struct drm_panel *panel)
+{
+ struct drm_connector *connector = panel-connector;
+ struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_create(connector-dev);
+ if (!mode) {
+ DRM_ERROR(failed to create a new display mode\n);
+ return 0;
+ }
+
+ drm_display_mode_from_videomode(ctx-vm, mode);
+ mode-width_mm = ctx-width_mm;
+ mode-height_mm = ctx-height_mm;
+ connector-display_info.width_mm = mode-width_mm;
+ connector-display_info.height_mm = mode-height_mm;
+
+ mode-type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+ mode-private = (void *)ctx-cpu_timings;
Wouldn't it make sense to create a new get_interface_params (or similar)
operation for drm_panel to query interface configuration parameters
instead of shoving it in the mode private field ?
You mean new get_interface_params operation is different from
get_modes() ?
Till now, struct drm_display_mode and most of mode relevant APIs are
only for video interface.
And CPU interface also needs video mode configurations.
I have a plan to implement the CPU interface relevant APIs like video
mode ones, but I think they should be used under current DRM mode APIs
like fill_modes, get_modes and so on.
So after that implementation, this private field will be replaced by
new ones.
Could you explain it in more detail?
The idea is that the interface parameters (RD/WR signals timings in this case,
but this could also include MIPI DSI lane configuration or any other kind of
physical interface parameters) are distinct from the video modes.
Do you see a need to tie tie interface parameters with drm_display_mode ? Can
they be mode-specific ? In any case I'd like not to use the private field of
drm_display_mode. If we need to tie both information together then it should
be done in a standard way.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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] s5p-mfc: Add IOMMU support
Hi Arun,
On Tuesday 22 April 2014 17:52:08 Arun Kumar K wrote:
On Tue, Apr 22, 2014 at 5:23 PM, Laurent Pinchart wrote:
On Tuesday 22 April 2014 16:32:48 Arun Kumar K wrote:
The patch adds IOMMU support for MFC driver.
I've been working on an IOMMU driver lately, which led me to think about
how drivers should be interfaced with IOMMUs. Runtime IOMMU handling is
performed by the DMA mapping API, but in many cases (including Exynos
platforms) the arm_iommu_create_mapping() and arm_iommu_attach_device()
functions still need to be called explicitly by drivers, which doesn't
seem a very good idea to me. Ideally IOMMU usage should be completely
transparent for bus master drivers, without requiring any driver
modification to use the IOMMU.
What would you think about improving the Exynos IOMMU driver to create the
mapping and attach the device instead of having to modify all bus master
drivers ? See the ipmmu_add_device() function in
http://www.spinics.net/lists/linux-sh/msg30488.html for a possible
implementation.
Yes that would be a better solution. But as far as I know, exynos platforms
has few more complications where multiple IOMMUs are present for single IP.
The exynos iommu work is still under progress and KyonHo Cho will have some
inputs / comments on this. This seems to me a valid usecase which can be
considered for exynos iommu also.
Thank you. Just to be clear, I don't see a reason to delay this patch until
the Exynos IOMMU driver gets support for creating mappings and attaching
devices, but it would be nice to see that happen as a cleanup in the not too
distant future.
Signed-off-by: Arun Kumar K arun...@samsung.com
---
This patch is tested on IOMMU support series [1] posted
by KyonHo Cho.
[1] https://lkml.org/lkml/2014/3/14/9
---
drivers/media/platform/s5p-mfc/s5p_mfc.c | 33
+++
1 file changed, 33 insertions(+)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc.c index 89356ae..1f248ba 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -32,11 +32,18 @@
#include s5p_mfc_opr.h
#include s5p_mfc_cmd.h
#include s5p_mfc_pm.h
+#ifdef CONFIG_EXYNOS_IOMMU
+#include asm/dma-iommu.h
+#endif
#define S5P_MFC_NAME s5p-mfc
#define S5P_MFC_DEC_NAME s5p-mfc-dec
#define S5P_MFC_ENC_NAME s5p-mfc-enc
+#ifdef CONFIG_EXYNOS_IOMMU
+static struct dma_iommu_mapping *mapping;
+#endif
+
int debug;
module_param(debug, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(debug, Debug level - higher value produces more
verbose
messages); @@ -1013,6 +1020,23 @@ static void *mfc_get_drv_data(struct
platform_device *pdev);
static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev *dev)
{
+#ifdef CONFIG_EXYNOS_IOMMU
+ struct device *mdev = dev-plat_dev-dev;
+
+ mapping = arm_iommu_create_mapping(platform_bus_type, 0x2000,
+ SZ_256M);
+ if (mapping == NULL) {
+ mfc_err(IOMMU mapping failed\n);
+ return -EFAULT;
+ }
+ mdev-dma_parms = devm_kzalloc(dev-plat_dev-dev,
+ sizeof(*mdev-dma_parms), GFP_KERNEL);
+ dma_set_max_seg_size(mdev, 0xu);
+ arm_iommu_attach_device(mdev, mapping);
+
+ dev-mem_dev_l = dev-mem_dev_r = mdev;
+ return 0;
+#else
unsigned int mem_info[2] = { };
dev-mem_dev_l = devm_kzalloc(dev-plat_dev-dev,
@@ -1049,6 +1073,7 @@ static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev
*dev) return -ENOMEM;
}
return 0;
+#endif
}
/* MFC probe function */
@@ -1228,6 +1253,10 @@ err_mem_init_ctx_1:
vb2_dma_contig_cleanup_ctx(dev-alloc_ctx[0]);
err_res:
s5p_mfc_final_pm(dev);
+#ifdef CONFIG_EXYNOS_IOMMU
+ if (mapping)
+ arm_iommu_release_mapping(mapping);
+#endif
pr_debug(%s-- with error\n, __func__);
return ret;
@@ -1256,6 +1285,10 @@ static int s5p_mfc_remove(struct platform_device
*pdev) put_device(dev-mem_dev_r);
}
+#ifdef CONFIG_EXYNOS_IOMMU
+ if (mapping)
+ arm_iommu_release_mapping(mapping);
+#endif
s5p_mfc_final_pm(dev);
return 0;
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 PATCH v4 09/14] ARM: dts: s6e3fa0: add DT bindings
Hi Andrzej,
On Wednesday 23 April 2014 11:02:21 Andrzej Hajda wrote:
On 04/21/2014 02:28 PM, YoungJun Cho wrote:
This patch adds DT bindings for s6e3fa0 panel.
The bindings describes panel resources, display timings and cpu timings.
Changelog v2:
- Adds unit address (commented by Sachin Kamat)
Changelog v3:
- Removes optional delay, size properties (commented by Laurent Pinchart)
- Adds OLED detection, TE gpio properties
Changelog v4:
- Moves CPU timings relevant properties from FIMD DT
(commeted by Laurent Pinchart, Andrzej Hajda)
Signed-off-by: YoungJun Cho yj44@samsung.com
Acked-by: Inki Dae inki@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
.../devicetree/bindings/panel/samsung,s6e3fa0.txt | 63 +++
1 file changed, 63 insertions(+)
create mode 100644
Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file
mode 100644
index 000..9eeb38b
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
@@ -0,0 +1,63 @@
+Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
+
+Required properties:
+ - compatible: samsung,s6e3fa0
+ - reg: the virtual channel number of a DSI peripheral
+ - vdd3-supply: core voltage supply
+ - vci-supply: voltage supply for analog circuits
+ - reset-gpio: a GPIO spec for the reset pin
+ - det-gpio: a GPIO spec for the OLED detection pin
+ - te-gpio: a GPIO spec for the TE pin
+ - display-timings: timings for the connected panel as described by [1]
Which properties of display-timings are relevant for CPU mode?
I guess width and height. Anything more?
+ - cpu-timings: CPU interface timings for the connected panel, and it
contains
+following optional properties.
+ - cs-setup: clock cycles for the active period of address
signal
+enable until chip select is enable in CPU interface
+ - wr-setup: clock cycles for the active period of CS signal
enable
+until write signal is enable in CPU interface
+ - wr-act: clock cycles for the active period of CS enable in
CPU
+interface
What about calling this property wr-active ? I had called this wr-cycle in a
previous implementation, that could be an option as well.
+ - wr-hold: clock cycles for the active period of CS disable
until
+write signal is disable in CPU interface
cpu-timings name does not sound to be related to display.
I wonder if it would not be better to merge cpu-timings into
display-timings but this will require more discussion I guess.
The panel has a memory-like interface with chip select, read/write and access
strobe, address (1 bit) and data signals. The interface is defined in the MIPI
DBI specification (a quick search turned up
http://www.scribd.com/doc/206899854/MIPI-DPI-Specification-v2, there might be
direct PDF downloads available), even if some panels implement a similar
interface that predates MIPI DBI (with names such as i80 or SYS-80 probably
due to the similarities with the 8051 external bus).
The cpu-timings properties describe the read and write access timings for
those signals, unrelated to the display video timings. I thus believe that
they should be separate from the display timings. We will probably need to add
properties for the read signal as well, but that can be done later.
If you want to stay with separate node please consider to make it
optional as whole node or make some properties required. Making node
required with all sub-properties optional is weird.
By the way I hope all timings properties are generic for CPU mode,
if not they should be prefixed by vendor or model.
The timings description should be pretty generic I believe, especially given
that the interface is standardized by the MIPI alliance. Could you have a
quick look at the spec and share your opinion ?
+
+Optional properties:
+
+The device node can contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in [2]. This
+node should describe panel's video bus.
+
+[1]: Documentation/devicetree/bindings/video/display-timing.txt
+[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+ panel@0 {
+ compatible = samsung,s6e3fa0;
+ reg = 0;
+ vdd3-supply = vcclcd_reg;
+ vci-supply = vlcd_reg;
+ reset-gpio = gpy7 4 0;
+ det-gpio = gpg0 6 0;
+ te-gpio = gpd1 7 0;
+
+ display-timings {
+ timing0: timing-0 {
+ clock-frequency = 0;
+ hactive = 1080;
+ vactive = 1920
Re: [PATCH] [media] s5p-mfc: Add IOMMU support
Hi Arun,
Thank you for the patch.
On Tuesday 22 April 2014 16:32:48 Arun Kumar K wrote:
The patch adds IOMMU support for MFC driver.
I've been working on an IOMMU driver lately, which led me to think about how
drivers should be interfaced with IOMMUs. Runtime IOMMU handling is performed
by the DMA mapping API, but in many cases (including Exynos platforms) the
arm_iommu_create_mapping() and arm_iommu_attach_device() functions still need
to be called explicitly by drivers, which doesn't seem a very good idea to me.
Ideally IOMMU usage should be completely transparent for bus master drivers,
without requiring any driver modification to use the IOMMU.
What would you think about improving the Exynos IOMMU driver to create the
mapping and attach the device instead of having to modify all bus master
drivers ? See the ipmmu_add_device() function in
http://www.spinics.net/lists/linux-sh/msg30488.html for a possible
implementation.
Signed-off-by: Arun Kumar K arun...@samsung.com
---
This patch is tested on IOMMU support series [1] posted
by KyonHo Cho.
[1] https://lkml.org/lkml/2014/3/14/9
---
drivers/media/platform/s5p-mfc/s5p_mfc.c | 33 +++
1 file changed, 33 insertions(+)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc.c index 89356ae..1f248ba 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -32,11 +32,18 @@
#include s5p_mfc_opr.h
#include s5p_mfc_cmd.h
#include s5p_mfc_pm.h
+#ifdef CONFIG_EXYNOS_IOMMU
+#include asm/dma-iommu.h
+#endif
#define S5P_MFC_NAME s5p-mfc
#define S5P_MFC_DEC_NAME s5p-mfc-dec
#define S5P_MFC_ENC_NAME s5p-mfc-enc
+#ifdef CONFIG_EXYNOS_IOMMU
+static struct dma_iommu_mapping *mapping;
+#endif
+
int debug;
module_param(debug, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(debug, Debug level - higher value produces more verbose
messages); @@ -1013,6 +1020,23 @@ static void *mfc_get_drv_data(struct
platform_device *pdev);
static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev *dev)
{
+#ifdef CONFIG_EXYNOS_IOMMU
+ struct device *mdev = dev-plat_dev-dev;
+
+ mapping = arm_iommu_create_mapping(platform_bus_type, 0x2000,
+ SZ_256M);
+ if (mapping == NULL) {
+ mfc_err(IOMMU mapping failed\n);
+ return -EFAULT;
+ }
+ mdev-dma_parms = devm_kzalloc(dev-plat_dev-dev,
+ sizeof(*mdev-dma_parms), GFP_KERNEL);
+ dma_set_max_seg_size(mdev, 0xu);
+ arm_iommu_attach_device(mdev, mapping);
+
+ dev-mem_dev_l = dev-mem_dev_r = mdev;
+ return 0;
+#else
unsigned int mem_info[2] = { };
dev-mem_dev_l = devm_kzalloc(dev-plat_dev-dev,
@@ -1049,6 +1073,7 @@ static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev
*dev) return -ENOMEM;
}
return 0;
+#endif
}
/* MFC probe function */
@@ -1228,6 +1253,10 @@ err_mem_init_ctx_1:
vb2_dma_contig_cleanup_ctx(dev-alloc_ctx[0]);
err_res:
s5p_mfc_final_pm(dev);
+#ifdef CONFIG_EXYNOS_IOMMU
+ if (mapping)
+ arm_iommu_release_mapping(mapping);
+#endif
pr_debug(%s-- with error\n, __func__);
return ret;
@@ -1256,6 +1285,10 @@ static int s5p_mfc_remove(struct platform_device
*pdev) put_device(dev-mem_dev_r);
}
+#ifdef CONFIG_EXYNOS_IOMMU
+ if (mapping)
+ arm_iommu_release_mapping(mapping);
+#endif
s5p_mfc_final_pm(dev);
return 0;
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] v4l: Add resolution change event.
Hi Arun,
Thank you for the patch.
On Monday 21 April 2014 14:56:01 Arun Kumar K wrote:
From: Pawel Osciak posc...@chromium.org
This event indicates that the decoder has reached a point in the stream,
at which the resolution changes. The userspace is expected to provide a new
set of CAPTURE buffers for the new format before decoding can continue.
The event can also be used for more generic events involving resolution
or format changes at runtime for all kinds of video devices.
Signed-off-by: Pawel Osciak posc...@chromium.org
Signed-off-by: Arun Kumar K arun...@samsung.com
---
.../DocBook/media/v4l/vidioc-subscribe-event.xml | 16
include/uapi/linux/videodev2.h |6 ++
2 files changed, 22 insertions(+)
diff --git a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml index
5c70b61..0aec831 100644
--- a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
@@ -155,6 +155,22 @@
/entry
/row
row
+ entryconstantV4L2_EVENT_SOURCE_CHANGE/constant/entry
+ entry5/entry
+ entry
+ paraThis event is triggered when a resolution or format change
+is detected during runtime by the video device. It can be a
+runtime resolution change triggered by a video decoder or the
+format change happening on an HDMI connector. Application may
+need to reinitialize buffers before proceeding further./para
+
+ paraThis event has a v4l2-event-source-change; associated
+ with it. This has significance only for v4l2 subdevs where the
+ structfieldpad_num/structfield field will be updated with
+ the pad number on which the event is triggered./para
+ /entry
+ /row
+ row
entryconstantV4L2_EVENT_PRIVATE_START/constant/entry
entry0x0800/entry
entryBase event number for driver-private events./entry
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 6ae7bbe..12e0614 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1733,6 +1733,7 @@ struct v4l2_streamparm {
#define V4L2_EVENT_EOS 2
#define V4L2_EVENT_CTRL 3
#define V4L2_EVENT_FRAME_SYNC4
+#define V4L2_EVENT_SOURCE_CHANGE 5
#define V4L2_EVENT_PRIVATE_START 0x0800
/* Payload for V4L2_EVENT_VSYNC */
@@ -1764,12 +1765,17 @@ struct v4l2_event_frame_sync {
__u32 frame_sequence;
};
+struct v4l2_event_source_change {
+ __u32 pad_num;
I would call the field just pad,
+};
+
struct v4l2_event {
__u32 type;
union {
struct v4l2_event_vsync vsync;
struct v4l2_event_ctrl ctrl;
struct v4l2_event_frame_syncframe_sync;
+ struct v4l2_event_source_change source_change;
__u8data[64];
This looks pretty good to me, but I'm a bit concerned about future
compatibility. We might need to report more information to userspace, and in
particular what has been changed at the source (resolution, format, ...). In
order to do so, we'll need to add a flag field to v4l2_event_source_change.
The next __u32 right after the source_change field must thus be zeroed. I see
two ways of doing so:
- zeroing the whole data array before setting event-specific data
- adding a reserved must-be-zeroed field to v4l2_event_source_change
I like the former better as it's more generic, but we then need to ensure that
all drivers zero the whole data field correctly. Adding a new
v4l2_event_init() function would help with that.
} u;
__u32 pending;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] v4l: Add resolution change event.
Hi Arun,
On Monday 21 April 2014 17:19:26 Arun Kumar K wrote:
On Mon, Apr 21, 2014 at 3:54 PM, Laurent Pinchart wrote:
On Monday 21 April 2014 14:56:01 Arun Kumar K wrote:
From: Pawel Osciak posc...@chromium.org
This event indicates that the decoder has reached a point in the stream,
at which the resolution changes. The userspace is expected to provide a
new
set of CAPTURE buffers for the new format before decoding can continue.
The event can also be used for more generic events involving resolution
or format changes at runtime for all kinds of video devices.
Signed-off-by: Pawel Osciak posc...@chromium.org
Signed-off-by: Arun Kumar K arun...@samsung.com
---
.../DocBook/media/v4l/vidioc-subscribe-event.xml | 16
include/uapi/linux/videodev2.h |6 ++
2 files changed, 22 insertions(+)
diff --git a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml index
5c70b61..0aec831 100644
--- a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
@@ -155,6 +155,22 @@
/entry
/row
row
+ entryconstantV4L2_EVENT_SOURCE_CHANGE/constant/entry
+ entry5/entry
+ entry
+ paraThis event is triggered when a resolution or format
change +is detected during runtime by the video device. It
can be a +runtime resolution change triggered by a video
decoder or the +format change happening on an HDMI
connector. Application may +need to reinitialize buffers
before proceeding further./para +
+ paraThis event has a v4l2-event-source-change;
associated
+ with it. This has significance only for v4l2 subdevs where
the
+ structfieldpad_num/structfield field will be updated with
+ the pad number on which the event is triggered./para
+ /entry
+ /row
+ row
entryconstantV4L2_EVENT_PRIVATE_START/constant/entry
entry0x0800/entry
entryBase event number for driver-private events./entry
diff --git a/include/uapi/linux/videodev2.h
b/include/uapi/linux/videodev2.h index 6ae7bbe..12e0614 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1733,6 +1733,7 @@ struct v4l2_streamparm {
#define V4L2_EVENT_EOS 2
#define V4L2_EVENT_CTRL 3
#define V4L2_EVENT_FRAME_SYNC4
+#define V4L2_EVENT_SOURCE_CHANGE 5
#define V4L2_EVENT_PRIVATE_START 0x0800
/* Payload for V4L2_EVENT_VSYNC */
@@ -1764,12 +1765,17 @@ struct v4l2_event_frame_sync {
__u32 frame_sequence;
};
+struct v4l2_event_source_change {
+ __u32 pad_num;
I would call the field just pad,
Ok.
+};
+
struct v4l2_event {
__u32 type;
union {
struct v4l2_event_vsync vsync;
struct v4l2_event_ctrl ctrl;
struct v4l2_event_frame_syncframe_sync;
+ struct v4l2_event_source_change source_change;
__u8data[64];
This looks pretty good to me, but I'm a bit concerned about future
compatibility. We might need to report more information to userspace, and
in particular what has been changed at the source (resolution, format,
...). In order to do so, we'll need to add a flag field to
v4l2_event_source_change.
Ok a flag can be added with bitfields for reporting specific event type.
I don't think we need to add it now. Just making sure it can be added later
without breaking the userspace API would be enough for me.
The next __u32 right after the source_change field must thus be zeroed. I
see two ways of doing so:
- zeroing the whole data array before setting event-specific data
- adding a reserved must-be-zeroed field to v4l2_event_source_change
I like the former better as it's more generic, but we then need to ensure
that all drivers zero the whole data field correctly. Adding a new
v4l2_event_init() function would help with that.
Is that a good idea to have an init() function just for zeroing the data
field? If this is agreed upon, I can add this, but it can be easily missed
out by drivers.
I'm not sure. On one hand it would be easier to catch missing calls if we use
a dedicated function, as the function could set an initialized flag in the
event structure that would be checked by v4l2_event_queue. On the other hand,
that might be overengineering, and we could just manually check that all
drivers memset the structure to 0 before initializing fields. In both cases
the goal is to make sure that the structure
Re: [RFC v2 PATCH v2 06/14] drm/exynos: support MIPI DSI command mode
Hi YoungJun, Thank you for the patch. On Monday 21 April 2014 21:28:33 YoungJun Cho wrote: This patch adds I80 interface for FIMD to support command mode panel. For this, the below features are added: - Sets display interface mode relevant registers properly according to the interface type from DT - Adds drm_panel_cpu_timings structure . The command mode panel sets them as the private attributes in struct drm_display_mode and FIMD gets them by fimd_mode_set(). - Adds TE interrupt handler . FIMD driver should know TE signal from lcd panel to avoid tearing issue. - Adds trigger feature . In case of command mode panel, FIMD should set trigger bit, so that image data has to be transferred to display bus or lcd panel. Changelog v2: - Moves CPU timings relevant properties to panel DT (commented by Laurent Pinchart, Andrzej Hajda) Signed-off-by: YoungJun Cho yj44@samsung.com Acked-by: Inki Dae inki@samsung.com Acked-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/gpu/drm/exynos/Kconfig |1 + drivers/gpu/drm/exynos/exynos_drm_crtc.c | 11 ++ drivers/gpu/drm/exynos/exynos_drm_crtc.h |2 + drivers/gpu/drm/exynos/exynos_drm_drv.h |2 + drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 ++ drivers/gpu/drm/exynos/exynos_drm_fimd.c | 280 ++- include/drm/drm_mipi_dsi.h |2 + include/drm/drm_panel.h |7 + Could you please split the DRM core changes into two separate standalone patches (as they're unrelated to each other) ? include/video/samsung_fimd.h |3 +- 9 files changed, 277 insertions(+), 44 deletions(-) [snip] -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 PATCH v3 10/14] drm/panel: add S6E3FA0 driver
Hi YoungJun,
Thank you for the patch.
On Monday 21 April 2014 21:28:37 YoungJun Cho wrote:
This patch adds MIPI-DSI command mode based S6E3FA0 AMOLED LCD Panel driver.
Changelog v2:
- Declares delay, size properties in probe routine instead of DT
Changelog v3:
- Moves CPU timings relevant properties from FIMD DT
(commented by Laurent Pinchart, Andrzej Hajda)
Signed-off-by: YoungJun Cho yj44@samsung.com
Acked-by: Inki Dae inki@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/gpu/drm/panel/Kconfig |7 +
drivers/gpu/drm/panel/Makefile|1 +
drivers/gpu/drm/panel/panel-s6e3fa0.c | 569 ++
3 files changed, 577 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c
[snip]
diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c
b/drivers/gpu/drm/panel/panel-s6e3fa0.c new file mode 100644
index 000..1282678
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-s6e3fa0.c
@@ -0,0 +1,569 @@
[snip]
+static int s6e3fa0_get_modes(struct drm_panel *panel)
+{
+ struct drm_connector *connector = panel-connector;
+ struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_create(connector-dev);
+ if (!mode) {
+ DRM_ERROR(failed to create a new display mode\n);
+ return 0;
+ }
+
+ drm_display_mode_from_videomode(ctx-vm, mode);
+ mode-width_mm = ctx-width_mm;
+ mode-height_mm = ctx-height_mm;
+ connector-display_info.width_mm = mode-width_mm;
+ connector-display_info.height_mm = mode-height_mm;
+
+ mode-type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+ mode-private = (void *)ctx-cpu_timings;
Wouldn't it make sense to create a new get_interface_params (or similar)
operation for drm_panel to query interface configuration parameters instead of
shoving it in the mode private field ?
+ drm_mode_probed_add(connector, mode);
+
+ return 1;
+}
[snip]
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l: Add resolution change event.
Hi Arun,
Thank you for the patch.
On Wednesday 16 April 2014 18:29:21 Arun Kumar K wrote:
From: Pawel Osciak posc...@chromium.org
This event indicates that the decoder has reached a point in the stream,
at which the resolution changes. The userspace is expected to provide a new
set of CAPTURE buffers for the new format before decoding can continue.
Signed-off-by: Pawel Osciak posc...@chromium.org
Signed-off-by: Arun Kumar K arun...@samsung.com
---
.../DocBook/media/v4l/vidioc-subscribe-event.xml |8
include/uapi/linux/videodev2.h |1 +
2 files changed, 9 insertions(+)
diff --git a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml index
5c70b61..d848628 100644
--- a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
@@ -155,6 +155,14 @@
/entry
/row
row
+ entryconstantV4L2_EVENT_RESOLUTION_CHANGE/constant/entry
+ entry5/entry
+ entryThis event is triggered when a resolution change is detected
+ during runtime by the video decoder. Application may need to
+ reinitialize buffers before proceeding further.
+ /entry
+ /row
Would it make sense to report the new resolution in the event data ? I suppose
it might not be available in all cases though. If we can't report it, would it
make sense to document how applications should proceed to retrieve it ?
A similar resolution change event might be useful on subdevs, in which case we
would need to add a pad number to the event data. We could possibly leave that
for later, but it would be worth considering the problem already.
+ row
entryconstantV4L2_EVENT_PRIVATE_START/constant/entry
entry0x0800/entry
entryBase event number for driver-private events./entry
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 6ae7bbe..58488b7 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1733,6 +1733,7 @@ struct v4l2_streamparm {
#define V4L2_EVENT_EOS 2
#define V4L2_EVENT_CTRL 3
#define V4L2_EVENT_FRAME_SYNC4
+#define V4L2_EVENT_RESOLUTION_CHANGE 5
#define V4L2_EVENT_PRIVATE_START 0x0800
/* Payload for V4L2_EVENT_VSYNC */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 05/14] ARM: dts: samsung-fimd: add I80 specific properties
Hi YoungJun, Thank you for the patch. On Tuesday 15 April 2014 14:47:33 YoungJun Cho wrote: In case of using CPU interface panel, the relevant registers should be set. So this patch adds relevant dt bindings. Signed-off-by: YoungJun Cho yj44@samsung.com Signed-off-by: Inki Dae inki@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- .../devicetree/bindings/video/samsung-fimd.txt |9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/devicetree/bindings/video/samsung-fimd.txt b/Documentation/devicetree/bindings/video/samsung-fimd.txt index 2dad41b..924c2e1 100644 --- a/Documentation/devicetree/bindings/video/samsung-fimd.txt +++ b/Documentation/devicetree/bindings/video/samsung-fimd.txt @@ -44,6 +44,15 @@ Optional Properties: - display-timings: timing settings for FIMD, as described in document [1]. Can be used in case timings cannot be provided otherwise or to override timings provided by the panel. +- samsung,sysreg-phandle: handle to syscon used to control the system registers +- vidout-i80-ldi: boolean to support i80 interface instead of rgb one +- cs-setup: clock cycles for the active period of address signal enable until +chip select is enable in i80 interface +- wr-setup: clock cycles for the active period of CS signal enable until + write signal is enable in i80 interface +- wr-act: clock cycles for the active period of CS enable in i80 interface +- wr-hold: clock cycles for the active period of CS disable until write signal + is disable in i80 interface Shouldn't the interface parameters be considered as a property of the slave device instead ? The bus master side is programmable, and different slaves would have different timing requirements. I think it would make more sense to specify the timings on the slave (panel) side and query them dynamically at runtime. Depending on the slave the timings could be hardcoded in the driver (as they're usually an intrinsic property of the slave) or partially or fully specified in the slave DT node. The device node can contain 'port' child nodes according to the bindings defined in [2]. The following are properties specific to those nodes: -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 09/14] ARM: dts: s6e3fa0: add DT bindings
Hi YoungJun,
Thank you for the patch.
On Wednesday 16 April 2014 13:38:57 YoungJun Cho wrote:
This patch adds DT bindings for s6e3fa0 panel.
The bindings describes panel resources, display timings, delays
and physical size.
Changelog v2:
- Adds unit address (commented by Sachin Kamat)
Signed-off-by: YoungJun Cho yj44@samsung.com
Signed-off-by: Inki Dae inki@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
.../devicetree/bindings/panel/samsung,s6e3fa0.txt | 52 +
1 file changed, 52 insertions(+)
create mode 100644
Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt new file mode
100644
index 000..889a74d
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
@@ -0,0 +1,52 @@
+Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
+
+Required properties:
+ - compatible: samsung,s6e3fa0
+ - reg: the virtual channel number of a DSI peripheral
+ - vdd3-supply: core voltage supply
+ - vci-supply: voltage supply for analog circuits
+ - reset-gpio: a GPIO spec for the reset pin
+ - display-timings: timings for the connected panel as described by [1]
+
+Optional properties:
+ - power-on-delay: delay after turning regulators on [ms]
+ - reset-delay: delay after reset sequence [ms]
+ - init-delay: delay after initialization sequence [ms]
+ - panel-width-mm: physical panel width [mm]
+ - panel-height-mm: physical panel height [mm]
Based on the next patch the panel seems to require quite a lot of device-
specific handling, although I might be mistaken as I'm not too familiar with
DSI. If my understanding is correct, do we really need to specify all those
properties in DT, or could they be hardcoded in the driver ? I would usually
push for generic drivers that read device information from DT, but I'm
wondering whether that is the right choice here.
If you decide to keep the information in DT I think you should document what
happens when optional properties are not specified. I suppose the driver uses
a default value, I would document it in the DT bindings.
+The device node can contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in [2]. This
+node should describe panel's video bus.
+
+[1]: Documentation/devicetree/bindings/video/display-timing.txt
+[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+ panel@0 {
+ compatible = samsung,s6e3fa0;
+ reg = 0;
+ vdd3-supply = vcclcd_reg;
+ vci-supply = vlcd_reg;
+ reset-gpio = gpy7 4 0;
+ power-on-delay= 10;
+ reset-delay = 5;
+ init-delay = 120;
+ panel-width-mm = 71;
+ panel-height-mm = 126;
+
+ display-timings {
+ timing0: timing-0 {
+ clock-frequency = 57153600;
+ hactive = 1080;
+ vactive = 1920;
+ hfront-porch = 2;
+ hback-porch = 2;
+ hsync-len = 1;
+ vfront-porch = 1;
+ vback-porch = 4;
+ vsync-len = 1;
+ };
+ };
+ };
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] [media] exynos-scaler: Add DT bindings for SCALER driver
Hi Shaik,
Thank you for the patch.
On Wednesday 19 March 2014 12:43:13 Shaik Ameer Basha wrote:
This patch adds the DT binding documentation for the Exynos5420/5410
based SCALER device driver.
Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
Reviewed-by: Sylwester Nawrocki s.nawro...@samsung.com
---
.../devicetree/bindings/media/exynos5-scaler.txt | 24 +
1 file changed, 24 insertions(+)
create mode 100644
Documentation/devicetree/bindings/media/exynos5-scaler.txt
diff --git a/Documentation/devicetree/bindings/media/exynos5-scaler.txt
b/Documentation/devicetree/bindings/media/exynos5-scaler.txt new file mode
100644
index 000..e1dd465
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/exynos5-scaler.txt
@@ -0,0 +1,24 @@
+* Samsung Exynos5 SCALER device
+
+SCALER is used for scaling, blending, color fill and color space
+conversion on EXYNOS[5420/5410] SoCs.
+
+Required properties:
+- compatible: should be samsung,exynos5420-scaler or
+ samsung,exynos5410-scaler
+- reg: should contain SCALER physical address location and length
+- interrupts: should contain SCALER interrupt specifier
+- clocks: should contain the SCALER clock phandle and specifier pair for
+ each clock listed in clock-names property, according to
+ the common clock bindings
+- clock-names: should contain exactly one entry
+ - scaler - IP bus clock
I'm not too familiar with the Exynos platform, but wouldn't it make sense to
use a common name across IP cores for interface and function clocks ?
+Example:
+ scaler_0: scaler@1280 {
+ compatible = samsung,exynos5420-scaler;
+ reg = 0x1280 0x1000;
+ interrupts = 0 220 0;
+ clocks = clock 381;
+ clock-names = scaler;
+ };
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] [media] exynos-scaler: Add DT bindings for SCALER driver
Hi Sylwester, On Wednesday 19 March 2014 12:55:21 Sylwester Nawrocki wrote: On 19/03/14 12:31, Laurent Pinchart wrote: +++ b/Documentation/devicetree/bindings/media/exynos5-scaler.txt @@ -0,0 +1,24 @@ +* Samsung Exynos5 SCALER device + +SCALER is used for scaling, blending, color fill and color space +conversion on EXYNOS[5420/5410] SoCs. + +Required properties: +- compatible: should be samsung,exynos5420-scaler or +samsung,exynos5410-scaler +- reg: should contain SCALER physical address location and length +- interrupts: should contain SCALER interrupt specifier +- clocks: should contain the SCALER clock phandle and specifier pair for +each clock listed in clock-names property, according to +the common clock bindings +- clock-names: should contain exactly one entry +- scaler - IP bus clock I'm not too familiar with the Exynos platform, but wouldn't it make sense to use a common name across IP cores for interface and function clocks ? Certainly it would, I proposed that when the exynos clock controller driver was posted, quite long time ago. Unfortunately it wasn't followed up. One of serious reasons was that there are common drivers for multiple Samsung platforms (also the ones not reworked to support dt) and thus changing the clock names would be problematic - driver would still need to handle multiple clock names. But for this driver a common name like gate could be better IMHO. OMAP uses ick for the interface clock and fck for the function clock. Do you think it would make sense to standardize on fck across SoC families ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
-0 = cam_port_a_clk_active;
+ status = okay;
+ #address-cells = 1;
+ #size-cells = 1;
/* parallel camera ports */
parallel-ports {
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
Hi Sylwester,
On Tuesday 11 March 2014 14:38:37 Sylwester Nawrocki wrote:
Hi Laurent,
Thanks for your review.
You're welcome.
On 11/03/14 13:30, Laurent Pinchart wrote:
[...]
---
.../devicetree/bindings/media/samsung-fimc.txt | 34 +-
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt
b/Documentation/devicetree/bindings/media/samsung-fimc.txt index
96312f6..dbd4020 100644
--- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
@@ -32,6 +32,21 @@ way around.
The 'camera' node must include at least one 'fimc' child node.
+Optional properties:
+
+- #clock-cells: from the common clock bindings
(../clock/clock-bindings.txt),
+ must be 1. A clock provider is associated with the 'camera' node and
it should
+ be referenced by external sensors that use clocks provided by the SoC
on
+ CAM_*_CLKOUT pins. The clock specifier cell stores an index of a
clock.
+ The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks
respectively.
+
+- clock-output-names: from the common clock bindings, should contain
names of
+ clocks registered by the camera subsystem corresponding to
CAM_A_CLKOUT,
+ CAM_B_CLKOUT output clocks respectively.
Wouldn't it be better to document the cam_mclk_a and cam_mclk_b names
explicitly ? Or do you expect different names to be used in different DT
files ? And as they correspond to the CAM_A_CLKOUT and CAM_B_CLKOUT pins,
shouldn't they be named cam_a_clkout and cam_b_clkout ?
Basically I could use fixed names for these clocks, I just wanted to keep
a possibility to override them in dts to avoid any possible clock name
collisions, rather than keep a list of different names per SoC in the
driver. Right now fixed names could also be used for all SoCs I'm aware of,
nevertheless I would prefer to keep the clock-output-names property.
cam_a_clkout, cam_b_clkout may be indeed better names, I'll change
that.
OK, variable names are fine with me.
+Note: #clock-cells and clock-output-names are mandatory properties if
external
+image sensor devices reference 'camera' device node as a clock provider.
+
What's the reason not to make them always mandatory ? Backward
compatibility only ? If so wouldn't it make sense to document the
properties as mandatory from now on, and treating them as optional in the
driver for backward compatibility ?
Yes, it's for backwards compatibility only. It may be a good idea to just
document them as required, since this is how the device is expected to be
described in DT from now. I'll just make these a required properties,
the driver already handles them as optional.
OK.
'fimc' device nodes
---
@@ -97,8 +112,8 @@ Image sensor nodes
The sensor device nodes should be added to their control bus controller
(e.g. I2C0) nodes and linked to a port node in the csis or the
parallel-ports node, using the common video interfaces bindings, defined
in video-interfaces.txt.
-The implementation of this bindings requires clock-frequency property to
be
-present in the sensor device nodes.
+An optional clock-frequency property needs to be present in the sensor
device
+nodes. Default value when this property is not present is 24 MHz.
This bothers me. Having the FIMC driver read the clock-frequence property
from the sensor DT nodes feels like a layering violation. Shouldn't the
sensor drivers call clk_set_rate() explicitly instead ?
It is supposed to do so, after this whole patch series. So the camera
controller driver will not need such properties. What do you think about
removing this sentence altogether ?
Sure. As the FIMC won't access the clock-frequency property of the sensor
anymore after this patch series, let's just drop the mention of clock-
frequency.
Example:
@@ -114,7 +129,7 @@ Example:
vddio-supply = ...;
clock-frequency = 2400;
- clocks = ...;
+ clocks = camera 1;
clock-names = mclk;
port {
@@ -135,7 +150,7 @@ Example:
vddio-supply = ...;
clock-frequency = 2400;
- clocks = ...;
+ clocks = camera 0;
clock-names = mclk;
port {
@@ -149,12 +164,15 @@ Example:
camera {
compatible = samsung,fimc, simple-bus;
- #address-cells = 1;
- #size-cells = 1;
- status = okay;
-
+ clocks = clock 132, clock 133;
+ clock-names = sclk_cam0, sclk_cam1;
The documentation mentions that clock-names must contain
Re: [PATCH v10 1/2] s5k5baf: add camera sensor driver
Hi Sylwester, On Thursday 19 December 2013 14:54:44 Sylwester Nawrocki wrote: On 05/12/13 12:38, Andrzej Hajda wrote: Driver for Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP. The driver exposes the sensor as two V4L2 subdevices: - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format, no controls. - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200, pre/post ISP cropping, downscaling via selection API, controls. Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com Signed-off-by: Andrzej Hajda a.ha...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Hi Laurent, Does this driver look sane to you, at its 10'th version? :) If so I could send a pull request including it this week. Yes, it does. Sorry for the delay. Please send the pull request. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 31/51] DMA-API: media: omap3isp: use dma_coerce_mask_and_coherent()
Hi Russell,
Thank you for the patch.
On Thursday 19 September 2013 22:56:02 Russell King wrote:
The code sequence:
isp-raw_dmamask = DMA_BIT_MASK(32);
isp-dev-dma_mask = isp-raw_dmamask;
isp-dev-coherent_dma_mask = DMA_BIT_MASK(32);
bypasses the architectures check on the DMA mask. It can be replaced
with dma_coerce_mask_and_coherent(), avoiding the direct initialization
of this mask.
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/platform/omap3isp/isp.c |6 +++---
drivers/media/platform/omap3isp/isp.h |3 ---
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c index df3a0ec..1c36080 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2182,9 +2182,9 @@ static int isp_probe(struct platform_device *pdev)
isp-pdata = pdata;
isp-ref_count = 0;
- isp-raw_dmamask = DMA_BIT_MASK(32);
- isp-dev-dma_mask = isp-raw_dmamask;
- isp-dev-coherent_dma_mask = DMA_BIT_MASK(32);
+ ret = dma_coerce_mask_and_coherent(isp-dev, DMA_BIT_MASK(32));
+ if (ret)
+ return ret;
platform_set_drvdata(pdev, isp);
diff --git a/drivers/media/platform/omap3isp/isp.h
b/drivers/media/platform/omap3isp/isp.h index cd3eff4..ce65d3a 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -152,7 +152,6 @@ struct isp_xclk {
* @mmio_base_phys: Array with physical L4 bus addresses for ISP register
* regions.
* @mmio_size: Array with ISP register regions size in bytes.
- * @raw_dmamask: Raw DMA mask
* @stat_lock: Spinlock for handling statistics
* @isp_mutex: Mutex for serializing requests to ISP.
* @crashed: Bitmask of crashed entities (indexed by entity ID)
@@ -190,8 +189,6 @@ struct isp_device {
unsigned long mmio_base_phys[OMAP3_ISP_IOMEM_LAST];
resource_size_t mmio_size[OMAP3_ISP_IOMEM_LAST];
- u64 raw_dmamask;
-
/* ISP Obj */
spinlock_t stat_lock; /* common lock for statistic drivers */
struct mutex isp_mutex; /* For handling ref_count field */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] pinctrl: Pass all configs to driver on pin_config_set()
|| !pfc-info-ops-set_bias) + return -ENOTSUPP; - break; + spin_lock_irqsave(pfc-lock, flags); + pfc-info-ops-set_bias(pfc, _pin, param); + spin_unlock_irqrestore(pfc-lock, flags); - default: - return -ENOTSUPP; - } + break; + + default: + return -ENOTSUPP; + } + } /* for each config */ return 0; } -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
Hi Andrejz,
On Monday 26 August 2013 14:34:21 Andrzej Hajda wrote:
On 08/23/2013 02:53 PM, Laurent Pinchart wrote:
On Wednesday 21 August 2013 16:41:31 Andrzej Hajda wrote:
Driver for Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor
with embedded SoC ISP.
The driver exposes the sensor as two V4L2 subdevices:
- S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
no controls.
- S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
pre/post ISP cropping, downscaling via selection API, controls.
Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Andrzej Hajda a.ha...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
Hi,
This patch incorporates Stephen's suggestions, thanks.
Regards
Andrzej
v7
- changed description of 'clock-frequency' DT property
v6
- endpoint node presence is now optional,
- added asynchronous subdev registration support and clock
handling,
- use named gpios in DT bindings
v5
- removed hflip/vflip device tree properties
v4
- GPL changed to GPLv2,
- bitfields replaced by u8,
- cosmetic changes,
- corrected s_stream flow,
- gpio pins are no longer exported,
- added I2C addresses to subdev names,
- CIS subdev registration postponed after
succesfull HW initialization,
- added enums for pads,
- selections are initialized only during probe,
- default resolution changed to 1600x1200,
- state-error pattern removed from few other functions,
- entity link creation moved to registered callback.
v3:
- narrowed state-error usage to i2c and power errors,
- private gain controls replaced by red/blue balance user controls,
- added checks to devicetree gpio node parsing
v2:
- lower-cased driver name,
- removed underscore from regulator names,
- removed platform data code,
- v4l controls grouped in anonymous structs,
- added s5k5baf_clear_error function,
- private controls definitions moved to uapi header file,
- added v4l2-controls.h reservation for private controls,
- corrected subdev registered/unregistered code,
- .log_status sudbev op set to v4l2 helper,
- moved entity link creation to probe routines,
- added cleanup on error to probe function.
---
.../devicetree/bindings/media/samsung-s5k5baf.txt | 59 +
MAINTAINERS|7 +
drivers/media/i2c/Kconfig |7 +
drivers/media/i2c/Makefile |1 +
drivers/media/i2c/s5k5baf.c| 2045 ++
5 files changed, 2119 insertions(+)
create mode 100644
Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode
100644 drivers/media/i2c/s5k5baf.c
[snip]
diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
new file mode 100644
index 000..f21d9f1
--- /dev/null
+++ b/drivers/media/i2c/s5k5baf.c
[snip]
+static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
+u16 count, const u16 *seq)
+{
+ struct i2c_client *c = v4l2_get_subdevdata(state-sd);
+ u16 buf[count + 1];
+ int ret, n;
+
+ s5k5baf_i2c_write(state, REG_CMDWR_ADDR, addr);
+ if (state-error)
+ return;
I would have a preference for returning an error directly from the write
function instead of storing it in state-error, that would be more
explicit. The same is true for all read/write functions.
I have introduced state-error to avoid code bloat. With this 'pattern'
error is checked in about 10 places in the code, of course without
scarifying code correctness.
Replacing this pattern with classic 'return error directly from function'
would result with adding error checks after all calls to i2c i/o functions
and after calls to many functions which those i2c i/o calls contains.
According to my rough estimates it is about 70 places.
Similar pattern is used already in v4l2_ctrl_handler::error.
+ buf[0] = __constant_htons(REG_CMD_BUF);
+ for (n = 1; n = count; ++n)
+ buf[n] = htons(*seq++);
cpu_to_be16()/be16_to_cpu() here as well ?
OK
+
+ n *= 2;
+ ret = i2c_master_send(c, (char *)buf, n);
+ v4l2_dbg(3, debug, c, i2c_write_seq(count=%d): %*ph\n, count,
+ min(2 * count, 64), seq - count);
+
+ if (ret != n) {
+ v4l2_err(c, i2c_write_seq: error during transfer (%d)\n, ret);
+ state-error = ret;
+ }
+}
[snip]
+static int s5k5baf_hw_set_video_bus(struct s5k5baf *state)
+{
+ u16 en_packets;
+
+ switch (state-bus_type) {
+ case V4L2_MBUS_CSI2:
+ en_packets = EN_PACKETS_CSI2;
+ break;
+ case V4L2_MBUS_PARALLEL:
+ en_packets = 0;
+ break;
+ default:
+ v4l2_err(state-sd, unknown video bus: %d\n,
+ state-bus_type);
+ return -EINVAL
Re: [PATCH v4] pinctrl: Pass all configs to driver on pin_config_set()
On Tuesday 27 August 2013 11:32:12 Sherman Yin wrote:
When setting pin configuration in the pinctrl framework, pin_config_set() or
pin_config_group_set() is called in a loop to set one configuration at a
time for the specified pin or group.
This patch 1) removes the loop and 2) changes the API to pass the whole pin
config array to the driver. It is now up to the driver to loop through the
configs. This allows the driver to potentially combine configs and reduce
the number of writes to pin config registers.
All c files changed have been build-tested to verify the change compiles and
that the corresponding .o is successfully generated.
Signed-off-by: Sherman Yin s...@broadcom.com
Reviewed-by: Christian Daudt c...@broadcom.com
Reviewed-by: Matt Porter matt.por...@linaro.org
drivers/pinctrl/pinconf.c | 42
drivers/pinctrl/pinctrl-tegra.c | 69 ++--
include/linux/pinctrl/pinconf.h |6 +-
Reviewed-by: Stephen Warren swar...@nvidia.com
On Tegra (Tegra114 Dalmore board, on top of next-20130816),
Tested-by: Stephen Warren swar...@nvidia.com
For drivers/pinctrl/sh-pfc/pinctrl.c,
Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
Please refer to the discussion with Linus W. [PATCH] ARM: Adds pin config
API to set all configs in one function here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/166567.html
All c files changed have been build-tested to verify the change compiles and
that the corresponding .o are successfully generated.
[v2] rebased on LinusW's linux-pinctrl.git devel branch. Fixed and
build-tested pinctrl-sunxi.c
[v3] rebased on linux-pinctrl.git:devel as of 2013.08.25. Applied API
change to new driver pinctrl-palmas.c. Re build-tested all files.
[v4] Changed int to unsigned int in sh-pfc/pinctrl.c
---
drivers/pinctrl/mvebu/pinctrl-mvebu.c | 26 +++--
drivers/pinctrl/pinconf.c | 42
drivers/pinctrl/pinctrl-abx500.c | 187 --
drivers/pinctrl/pinctrl-at91.c| 48 +
drivers/pinctrl/pinctrl-bcm2835.c | 43
drivers/pinctrl/pinctrl-exynos5440.c | 113 +++-
drivers/pinctrl/pinctrl-falcon.c | 63 ++-
drivers/pinctrl/pinctrl-imx.c | 28 ++---
drivers/pinctrl/pinctrl-mxs.c | 91
drivers/pinctrl/pinctrl-nomadik.c | 125 --
drivers/pinctrl/pinctrl-palmas.c | 134 ---
drivers/pinctrl/pinctrl-rockchip.c| 57 ++
drivers/pinctrl/pinctrl-samsung.c | 17 ++-
drivers/pinctrl/pinctrl-single.c | 33 --
drivers/pinctrl/pinctrl-st.c | 11 +-
drivers/pinctrl/pinctrl-sunxi.c | 77 +++---
drivers/pinctrl/pinctrl-tegra.c | 69 ++--
drivers/pinctrl/pinctrl-tz1090-pdc.c | 135 ++--
drivers/pinctrl/pinctrl-tz1090.c | 140 +---
drivers/pinctrl/pinctrl-u300.c| 18 ++--
drivers/pinctrl/pinctrl-xway.c| 119 +
drivers/pinctrl/sh-pfc/pinctrl.c | 42
drivers/pinctrl/vt8500/pinctrl-wmt.c | 54 +-
include/linux/pinctrl/pinconf.h |6 +-
24 files changed, 952 insertions(+), 726 deletions(-)
[snip]
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c
b/drivers/pinctrl/sh-pfc/pinctrl.c index 8649ec3..e758af9 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -529,38 +529,44 @@ static int sh_pfc_pinconf_get(struct pinctrl_dev
*pctldev, unsigned _pin, }
static int sh_pfc_pinconf_set(struct pinctrl_dev *pctldev, unsigned _pin,
- unsigned long config)
+ unsigned long *configs, unsigned num_configs)
{
struct sh_pfc_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
struct sh_pfc *pfc = pmx-pfc;
- enum pin_config_param param = pinconf_to_config_param(config);
+ enum pin_config_param param;
unsigned long flags;
+ unsigned int i;
- if (!sh_pfc_pinconf_validate(pfc, _pin, param))
- return -ENOTSUPP;
+ for (i = 0; i num_configs; i++) {
+ param = pinconf_to_config_param(configs[i]);
- switch (param) {
- case PIN_CONFIG_BIAS_PULL_UP:
- case PIN_CONFIG_BIAS_PULL_DOWN:
- case PIN_CONFIG_BIAS_DISABLE:
- if (!pfc-info-ops || !pfc-info-ops-set_bias)
+ if (!sh_pfc_pinconf_validate(pfc, _pin, param))
return -ENOTSUPP;
- spin_lock_irqsave(pfc-lock, flags);
- pfc-info-ops-set_bias(pfc, _pin, param);
- spin_unlock_irqrestore(pfc-lock, flags);
+ switch (param) {
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_BIAS_DISABLE
Re: [PATCH v7] s5k5baf: add camera sensor driver
|
+MEDIA_LNK_FL_ENABLED);
+ return ret;
+}
+
+static void s5k5baf_unregistered(struct v4l2_subdev *sd)
+{
+ struct s5k5baf *state = to_s5k5baf(sd);
+ v4l2_device_unregister_subdev(state-cis_sd);
The unregistered operation is called from v4l2_device_unregister_subdev().
Calling it again will be a no-op, the function will return immediately. You
can thus get rid of the unregistered operation completely.
Similarly, the registered operation is called from
v4l2_device_register_subdev(). You can get rid of it as well and just create
the link in the probe function.
+}
[snip]
+static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device
*dev) +{
+ struct device_node *node = dev-of_node;
+ struct device_node *node_ep;
+ struct v4l2_of_endpoint ep;
+ int ret;
+
+ if (!node) {
+ dev_err(dev, no device-tree node provided\n);
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(node, clock-frequency,
+state-mclk_frequency);
+ if (ret 0) {
+ state-mclk_frequency = S5K5BAF_DEFAULT_MCLK_FREQ;
+ dev_info(dev, using default %u Hz clock frequency\n,
+ state-mclk_frequency);
+ }
+
+ ret = s5k5baf_parse_gpios(state-gpios, dev);
+ if (ret 0)
+ return ret;
+
+ node_ep = v4l2_of_get_next_endpoint(node, NULL);
+ if (!node_ep) {
+ /* Default data bus configuration: MIPI CSI-2, 1 data lane. */
+ state-bus_type = V4L2_MBUS_CSI2;
+ state-nlanes = S5K5BAF_DEF_NUM_LANES;
+ dev_warn(dev, no endpoint defined at node %s\n,
+ node-full_name);
+ return 0;
Shouldn't this be a fatal error ? If there's no endpoint the sensor isn't part
of any media pipeline, so it's unusable anyway.
+ }
+
+ v4l2_of_parse_endpoint(node_ep, ep);
+ of_node_put(node_ep);
+ state-bus_type = ep.bus_type;
+
+ if (state-bus_type == V4L2_MBUS_CSI2)
+ state-nlanes = ep.bus.mipi_csi2.num_data_lanes;
+
+ return 0;
+}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
Hi Arnd, On Wednesday 24 July 2013 20:32:03 Arnd Bergmann wrote: On Tuesday 23 July 2013, Tomasz Figa wrote: On Tuesday 23 of July 2013 17:14:20 Alan Stern wrote: On Tue, 23 Jul 2013, Tomasz Figa wrote: Where would you want to have those phy_address arrays stored? There are no board files when booting with DT. Not even saying that you don't need to use any hacky schemes like this when you have DT that nicely specifies relations between devices. If everybody agrees DT has a nice scheme for specifying relations between devices, why not use that same scheme in the PHY core? It is already used, for cases when consumer device has a DT node attached. In non-DT case this kind lookup translates loosely to something that is being done in regulator framework - you can't bind devices by pointers, because you don't have those pointers, so you need to use device names. Sorry for jumping in to the middle of the discussion, but why does a *new* framework even bother defining an interface for board files? Can't we just drop any interfaces for platform data passing in the phy framework and put the burden of adding those to anyone who actually needs them? All the platforms we are concerned with here (exynos and omap, plus new platforms) can be booted using DT anyway. What about non-DT architectures such as MIPS (still widely used in consumer networking equipments from what I've heard) ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/7] media: V4L2: add temporary clock helpers
Hello,
On Tuesday 19 March 2013 10:52:29 Sylwester Nawrocki wrote:
On 03/19/2013 08:32 AM, Guennadi Liakhovetski wrote:
On Mon, 18 Mar 2013, Sylwester Nawrocki wrote:
On 03/15/2013 10:27 PM, Guennadi Liakhovetski wrote:
[...]
diff --git a/drivers/media/v4l2-core/v4l2-clk.c
b/drivers/media/v4l2-core/v4l2-clk.c
new file mode 100644
index 000..3505972
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-clk.c
[snip]
+static struct v4l2_clk *v4l2_clk_find(const struct v4l2_subdev *sd,
+ const char *dev_id, const char *id)
+{
+ struct v4l2_clk *clk;
+
+ list_for_each_entry(clk,clk_list, list) {
+ if (!sd || !(sd-flags V4L2_SUBDEV_FL_IS_I2C)) {
+ if (strcmp(dev_id, clk-dev_id))
+ continue;
+ } else {
+ char *i2c = strstr(dev_id, clk-dev_id);
+ if (!i2c || i2c == dev_id || *(i2c - 1) != ' ')
+ continue;
+ }
+
+ if (!id || !clk-id || !strcmp(clk-id, id))
+ return clk;
+ }
+
+ return ERR_PTR(-ENODEV);
+}
+
+struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
+{
+ struct v4l2_clk *clk;
+
+ mutex_lock(clk_lock);
+ clk = v4l2_clk_find(sd, sd-name, id);
Couldn't we just pass the I2C client's struct device name to this
function ?
Certainly not. This is a part of the generic V4L2 clock API, it's not I2C
specific.
I have been thinking about something like dev_name(sd-dev), but struct
v4l2_subdev doesn't have struct device associated with it.
But the caller of v4l2_clk_get() will have a struct device * available, so I
think it would make sense to pass a struct device * to v4l2_clk_get() and call
dev_name() on it internally. Clocks would be registered with the device ID as
well. This requires knowledge of the clock user device in the clock provider,
but no knowledge of the clock user module name.
And if the host driver that registers a clock for its sub-device knows
the type of device (I2C, SPI client, etc.) why we need to even bother
with checking the subdev/bus type in v4l2_clk_find() function above, when
the host could properly format dev_id when it registers a clock ?
This has been discussed. The host doesn't know the name of the I2C driver,
that would attach to this subdevice at the time, it registers the clock.
This is the easiest way to oversome this problem.
OK, thanks for reminding. It would be probably much easier to associate
the clock with struct device, not with subdev driver. Devices have more
clear naming rules (at last I2C, SPI clients). And most host drivers
already have information about I2C bus id, just I2C slave address would
need to be passed to the host driver so it can register a clock for its
subdev.
Then the subdev would just pass its struct device pointer to this API to
find its clock. What am I missing here ?
I don't think there's a 1-to-1 correspondence between devices and V4L2
subdevices.
I would expect at least a subdev that needs a clock to have struct device
associated with it. It would be also much easier this way to use generic
clocks API in the device tree instantiated systems.
I agree. Let's not overdesign the v4l2-clock API to support clock users
without a struct device. This is a transitional API only after all.
+ if (!IS_ERR(clk) !try_module_get(clk-ops-owner))
+ clk = ERR_PTR(-ENODEV);
+ mutex_unlock(clk_lock);
+
+ if (!IS_ERR(clk)) {
+ clk-subdev = sd;
Why is this needed ? It seems a strange addition that might potentially
make transition to the common clocks API more difficult.
We got rid of the v4l2_clk_bind() function and the .bind() callback. Now I
need a pointer to subdevice _before_ v4l2_clk_register() (former
v4l2_clk_bound()), that's why I have to store it here.
Hmm, sorry, I'm not following. How can we store a subdev pointer in the
clock data structure that has not been registered yet and thus cannot be
found with v4l2_clk_find() ?
+ atomic_inc(clk-use_count);
+ }
+
+ return clk;
+}
+EXPORT_SYMBOL(v4l2_clk_get);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/7] media: V4L2: add temporary clock helpers
Hi Guennadi,
On Tuesday 19 March 2013 11:27:56 Guennadi Liakhovetski wrote:
On Tue, 19 Mar 2013, Sylwester Nawrocki wrote:
+ if (!IS_ERR(clk) !try_module_get(clk-ops-owner))
+ clk = ERR_PTR(-ENODEV);
+ mutex_unlock(clk_lock);
+
+ if (!IS_ERR(clk)) {
+ clk-subdev = sd;
Why is this needed ? It seems a strange addition that might potentially
make transition to the common clocks API more difficult.
We got rid of the v4l2_clk_bind() function and the .bind() callback. Now
I need a pointer to subdevice _before_ v4l2_clk_register() (former
v4l2_clk_bound()), that's why I have to store it here.
Hmm, sorry, I'm not following. How can we store a subdev pointer in the
clock data structure that has not been registered yet and thus cannot be
found with v4l2_clk_find() ?
sorry, I meant v4l2_async_subdev_register(), not v4l2_clk_register(), my
mistake. And I meant v4l2_async_subdev_bind(), v4l2_async_subdev_unbind().
Before we had in the subdev driver (see imx074 example)
/* Tell the bridge the subdevice is about to bind */
v4l2_async_subdev_bind();
/* get a clock */
clk = v4l2_clk_get();
if (IS_ERR(clk))
return -EPROBE_DEFER;
/*
* enable the clock - this needs a subdev pointer, that we passed
* to the bridge with v4l2_async_subdev_bind() above
*/
v4l2_clk_enable(clk);
do_probe();
v4l2_clk_disable(clk);
/* inform the bridge: binding successful */
v4l2_async_subdev_bound();
Now we have just
/* get a clock */
clk = v4l2_clk_get();
if (IS_ERR(clk))
return -EPROBE_DEFER;
/*
* enable the clock - this needs a subdev pointer, that we stored
* in the clock object for the bridge driver to use with
* v4l2_clk_get() above
*/
v4l2_clk_enable(clk);
do_probe();
v4l2_clk_disable(clk);
I'm sorry, but I still don't understand why you need a pointer to the subdev
in the clock provider implementation of v4l2_clk_enable/disable() :-)
/* inform the bridge: binding successful */
v4l2_async_subdev_bound();
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v4 01/14] [media] Add common video interfaces OF bindings documentation
Hi Sylwester,
On Thursday 24 January 2013 19:30:10 Sylwester Nawrocki wrote:
On 01/24/2013 11:16 AM, Laurent Pinchart wrote:
[...]
+Data interfaces on all video devices are described by their child 'port'
+nodes. Configuration of a port depends on other devices participating in
+the data transfer and is described by 'endpoint' subnodes.
+
+dev {
+ #address-cells = 1;
+ #size-cells = 0;
+ port@0 {
+ endpoint@0 { ... };
+ endpoint@1 { ... };
+ };
+ port@1 { ... };
+};
+
+If a port can be configured to work with more than one other device on
+the same bus, an 'endpoint' child node must be provided for each of
+them. If more than one port is present in a device node or there is more
+than one endpoint at a port, a common scheme, using '#address-cells',
+'#size-cells' and 'reg' properties is used.
Wouldn't this cause problems if the device has both video ports and a
child bus ? Using #address-cells and #size-cells for the video ports would
prevent the child bus from being handled in the usual way.
Indeed, it looks like a serious issue in these bindings.
A possible solution would be to number ports with a dash instead of a @,
as done in pinctrl for instance. We would then get
port-0 {
endpoint-0 { ... };
endpoint-1 { ... };
};
port-1 { ... };
Sounds like a good alternative, I can't think of any better solution at the
moment.
+Two 'endpoint' nodes are linked with each other through their
+'remote-endpoint' phandles. An endpoint subnode of a device contains
+all properties needed for configuration of this device for data exchange
+with the other device. In most cases properties at the peer 'endpoint'
+nodes will be identical, however they might need to be different when
+there is any signal modifications on the bus between two devices, e.g.
+there are logic signal inverters on the lines.
+
+Required properties
+---
+
+If there is more than one 'port' or more than one 'endpoint' node
following +properties are required in relevant parent node:
+
+- #address-cells : number of cells required to define port number,
should be 1.
+- #size-cells: should be zero.
I wonder if we should specify whether a port is a data sink or data
source. A source can be connected to multiple sinks at the same time, but
a sink can only be connected to a single source. If we want to perform
automatic sanity checks in the core knowing the direction might help.
Multiple sources can be linked to a single sink, but only one link can be
active at any time.
So I'm not sure if knowing if a DT port is a data source or data sink would
let us to validate device tree structure statically in general.
Such source/sink property could be useful later at runtime, when data
pipeline is set up for streaming.
Yes, I was mostly thinking about runtime.
How do you think this could be represented ? By just having boolean
properties like: 'source' and 'sink' in the port nodes ? Or perhaps in the
endpoint nodes, since some devices might be bidirectional ? I don't recall
any at the moment though.
Source and sink properties would do. We could also use a direction property
that could take sink, source and bidirectional values, but that might be more
complex.
I don't think we will have bidirectional link (as that would most probably
involve a very different kind of bus, and thus new bindings).
+Optional endpoint properties
+
+
+- remote-endpoint: phandle to an 'endpoint' subnode of the other device
+ node.
+- slave-mode: a boolean property, run the link in slave mode.
+ Default is master mode.
What are master and slave modes ? It might be worth it describing them.
This was originally proposed by Guennadi, I think he knows exactly what's
the meaning of this property. I'll dig into relevant documentation to
find out and provide more detailed description.
Thank you.
+- bus-width: number of data lines, valid for parallel busses.
+- data-shift: on parallel data busses, if bus-width is used to specify
+ the number of data lines, data-shift can be used to specify which data
+ lines are used, e.g. bus-width=10; data-shift=2; means, that
+ lines 9:2 are used.
+- hsync-active: active state of HSYNC signal, 0/1 for LOW/HIGH
+ respectively.
+- vsync-active: active state of VSYNC signal, 0/1 for LOW/HIGH
+ respectively. Note, that if HSYNC and VSYNC polarities are not
+ specified, embedded synchronization may be required, where supported.
+- data-active: similar to HSYNC and VSYNC, specifies data line polarity.
+- field-even-active: field signal level during the even field data
+ transmission.
+- pclk-sample: sample data on rising (1) or falling (0) edge of the
+ pixel clock signal.
+- data-lanes: an array of physical data lane indexes. Position of an
+ entry
Re: [PATCH V6 2/2] video: drm: exynos: Add device tree support
On Monday 24 September 2012 21:35:46 Inki Dae wrote: 2012/9/22 Stephen Warren swar...@wwwdotorg.org: On 09/21/2012 01:22 AM, Inki Dae wrote: 2012/9/21 Stephen Warren swar...@wwwdotorg.org: On 09/21/2012 05:22 AM, Leela Krishna Amudala wrote: This patch adds device tree based discovery support for exynos DRM-FIMD driver which includes driver modification to handle platform data in both the cases with DT and non-DT, Also adds the documentation for bindings. diff --git a/Documentation/devicetree/bindings/drm/exynos/fimd.txt b/Documentation/devicetree/bindings/drm/exynos/fimd.txt ... + - samsung,fimd-display: This property should specify the phandle of the + display device node which holds the video interface timing with the + below mentioned properties. + + - lcd-htiming: Specifies the horizontal timing for the overlay. The + horizontal timing includes four parameters in the following order. + + - horizontal back porch (in number of lcd clocks) + - horizontal front porch (in number of lcd clocks) + - hsync pulse width (in number of lcd clocks) + - Display panels X resolution. + + - lcd-vtiming: Specifies the vertical timing for the overlay. The + vertical timing includes four parameters in the following order. + + - vertical back porch (in number of lcd lines) + - vertical front porch (in number of lcd lines) + - vsync pulse width (in number of lcd clocks) + - Display panels Y resolution. Should this not use the new videomode timings that are under discussion at: http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html ok, I agree with you. then the videomode helper is going to be merged to mainline(3.6)? if so, this patch should be reworked based on the videomode helper. I think the videomode helpers would be merged for 3.7 at the very earliest; 3.6 is cooked already. Given there are still some comments on the binding, perhaps it won't happen until 3.8, but it'd be best to ask on that thread so that people more directly involved with the status can answer. as I mentioned before, it's better to use videomode helper instead but for this, we should wait for that the videomode helper are merged to mainline so I think it's better to merge it as is and then modify it for videomode helper to be used later. Aren't DT bindings considered as an ABI, and required to be supported more or less forever ? If you merge this DT binding you'll have to keep supporting it. That's why DT bindings should not be rushed in. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation
Hi Guennadi, On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote: On Tue, 31 Jul 2012, Laurent Pinchart wrote: On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote: On Thu, 26 Jul 2012, Laurent Pinchart wrote: On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote: On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote: On Fri, 25 May 2012, Sylwester Nawrocki wrote: The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y. Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com Signed-off-by: Bartlomiej Zolnierkiewiczb.zolnier...@samsung.com Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com --- .../bindings/camera/samsung-s5k6aafx.txt | 57 + drivers/media/video/s5k6aa.c | 129 ++-- 2 files changed, 146 insertions(+), 40 deletions(-) create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644 index 000..6685a9c --- /dev/null +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt @@ -0,0 +1,57 @@ +Samsung S5K6AAFX camera sensor +-- + +Required properties: + +- compatible : samsung,s5k6aafx; +- reg : base address of the device on I2C bus; You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git-tree but not in this series? No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention. The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node. This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback.. That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions. Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the device_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we temporarily use references from subdevs to hosts until the clock API is available? I don't think that requires a reference from subdevs to hosts in the DT. The subdev will need the host to be probed before a clock can be available so you won't be able to access the hardware in the probe() function in the generic case. You will need to wait until the registered() subdev operation is called, at which point the host can be accessed through the v4l2_device. Sure, I understand, but that's exactly what we wanted to avoid - succeeding client's i2c .probe() without even touching the hardware. But should we allow host probe() to succeed if the sensor isn't present ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation
Hi Guennadi, On Tuesday 31 July 2012 13:29:55 Guennadi Liakhovetski wrote: On Tue, 31 Jul 2012, Laurent Pinchart wrote: On Tuesday 31 July 2012 13:14:13 Guennadi Liakhovetski wrote: On Tue, 31 Jul 2012, Laurent Pinchart wrote: On Tuesday 31 July 2012 11:56:44 Guennadi Liakhovetski wrote: On Thu, 26 Jul 2012, Laurent Pinchart wrote: On Wednesday 18 July 2012 11:18:33 Sylwester Nawrocki wrote: On 07/16/2012 11:42 AM, Guennadi Liakhovetski wrote: On Fri, 25 May 2012, Sylwester Nawrocki wrote: The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y. Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com Signed-off-by: Bartlomiej Zolnierkiewiczb.zolnier...@samsung.com Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com --- .../bindings/camera/samsung-s5k6aafx.txt | 57 + drivers/media/video/s5k6aa.c | 129 ++-- 2 files changed, 146 insertions(+), 40 deletions(-) create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt new file mode 100644 index 000..6685a9c --- /dev/null +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.t xt @@ -0,0 +1,57 @@ +Samsung S5K6AAFX camera sensor +-- + +Required properties: + +- compatible : samsung,s5k6aafx; +- reg : base address of the device on I2C bus; You said you ended up putting your sensors outside of I2C busses, is this one of changes, that are present in your git- tree but not in this series? No, I must have been not clear enough on that. Our idea was to keep I2C slave device nodes as an I2C controller's child nodes, according to the current convention. The 'sensor' nodes (the 'camera''s children) would only contain a phandle to a respective I2C slave node. This implies that we cannot access I2C bus in I2C client's device probe() callback. An actual H/W access could begin only from within and after invocation of v4l2_subdev .registered callback.. That's how I've envisioned the DT bindings for sensors as well, this sounds good. The real challenge will be to get hold of the subdev to register it without race conditions. Hrm... That's how early pre-subdev versions of soc-camera used to work, that's where all the device_video_probe() functions come from. But then we switched to dynamic i2c device registration. Do we want to switch all drivers back now?... Couldn't we temporarily use references from subdevs to hosts until the clock API is available? I don't think that requires a reference from subdevs to hosts in the DT. The subdev will need the host to be probed before a clock can be available so you won't be able to access the hardware in the probe() function in the generic case. You will need to wait until the registered() subdev operation is called, at which point the host can be accessed through the v4l2_device. Sure, I understand, but that's exactly what we wanted to avoid - succeeding client's i2c .probe() without even touching the hardware. But should we allow host probe() to succeed if the sensor isn't present ? I think we should, yes. The host hardware is there and functional - whether or not all or some of the clients are failing. Theoretically clients can also be hot-plugged. Whether and how many video device nodes we create, that's a different question. I think I can agree with you on this (although I could change my mind if this architecture turns out to result in unsolvable technical issues). That will involve a lot of work though. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support
Hi Sylwester, On Tuesday 17 July 2012 20:16:23 Sylwester Nawrocki wrote: On 07/16/2012 10:55 AM, Guennadi Liakhovetski wrote: Hi Sylwester Thanks for your comments to my RFC and for pointing out to this your earlier patch series. Unfortunately, I missed in in May, let me try to provide some thoughts about this, we should really try to converge our proposals. Maybe a discussion at KS would help too. Thank you for the review. I was happy to see your RFC, as previously there seemed to be not much interest in DT among the media guys. Certainly, we need to work on a common approach to ensure interoperability of existing drivers and to avoid having people inventing different bindings for common features. I would also expect some share of device specific bindings, as diversity of media devices is significant. I'd be great to discuss these things at KS, especially support for proper suspend/resume sequences. Also having common sessions with other subsystems folks, like ASoC, for example, might be a good idea. I'm not sure if I'll be travelling to the KS though. :) On Fri, 25 May 2012, Sylwester Nawrocki wrote: s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC (camera host interface DMA engine and image processor). This patch adds support for instantiating the MIPI-CSIS devices from DT and parsing all SoC and board specific properties from device tree. The MIPI DPHY control callback is now called directly from within the driver, the platform code must ensure this callback does the right thing for each SoC. The cell-index property is used to ensure proper signal routing, from physical camera port, through MIPI-CSI2 receiver to the DMA engine (FIMC?). It's also helpful in exposing the device topology in user space at /dev/media? devnode (Media Controller API). This patch also defines a common property (data-lanes) for MIPI-CSI receivers and transmitters. Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com --- Documentation/devicetree/bindings/video/mipi.txt |5 + .../bindings/video/samsung-mipi-csis.txt | 47 ++ drivers/media/video/s5p-fimc/mipi-csis.c | 97 +++- drivers/media/video/s5p-fimc/mipi-csis.h |1 + 4 files changed, 126 insertions(+), 24 deletions(-) create mode 100644 Documentation/devicetree/bindings/video/mipi.txt create mode 100644 Documentation/devicetree/bindings/video/samsung-mipi-csis.txt diff --git a/Documentation/devicetree/bindings/video/mipi.txt b/Documentation/devicetree/bindings/video/mipi.txt new file mode 100644 index 000..5aed285 --- /dev/null +++ b/Documentation/devicetree/bindings/video/mipi.txt @@ -0,0 +1,5 @@ +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters + + - data-lanes : number of differential data lanes wired and actively used in + communication between the transmitter and the receiver, this + excludes the clock lane; Wouldn't it be better to use the standard bus-width DT property? I can't see any problems with using bus-width. It seems sufficient and could indeed be better, without a need to invent new MIPI-CSI specific names. That was my first RFC on that and my perspective wasn't probably broad enough. :) What about CSI receivers that can reroute the lanes internally ? We would need to specify lane indices for each lane then, maybe with something like clock-lane = 0; data-lanes = 2 3 1; For receivers that can't reroute lanes internally, the data-lanes property would be need to specify lanes in sequence. data-lanes = 1 2 3; -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 01/13] ARM: Samsung: Extend MIPI PHY callback with an index argument
Hi Sylwester, On Friday 25 May 2012 21:52:40 Sylwester Nawrocki wrote: For systems instantiated from device tree struct platform_device id field is always -1, add an 'id' argument to the s5p_csis_phy_enable() function so the MIPI-CSIS hardware instance index can be passed in by driver, for CONFIG_OF=y. Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- arch/arm/plat-s5p/setup-mipiphy.c | 20 arch/arm/plat-samsung/include/plat/mipi_csis.h | 10 ++ 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/arch/arm/plat-s5p/setup-mipiphy.c b/arch/arm/plat-s5p/setup-mipiphy.c index 683c466..146ecc3 100644 --- a/arch/arm/plat-s5p/setup-mipiphy.c +++ b/arch/arm/plat-s5p/setup-mipiphy.c @@ -14,24 +14,19 @@ #include linux/spinlock.h #include mach/regs-clock.h -static int __s5p_mipi_phy_control(struct platform_device *pdev, +static int __s5p_mipi_phy_control(struct platform_device *pdev, int id, bool on, u32 reset) What about removing the pdev argument, as it's now not needed ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 05/13] media: s5p-fimc: Add device tree support for FIMC devices
Hi Sylwester, On Wednesday 18 July 2012 21:53:34 Sylwester Nawrocki wrote: On 07/18/2012 10:17 AM, Guennadi Liakhovetski wrote: On Tue, 17 Jul 2012, Sylwester Nawrocki wrote: On 07/16/2012 11:13 AM, Guennadi Liakhovetski wrote: On Fri, 25 May 2012, Sylwester Nawrocki wrote: Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com Signed-off-by: Karol Lewandowskik.lewando...@samsung.com Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com From the documentation below I think, I understand what it does, but why is it needed? It doesn't describe your video subsystem topology, right? How various subdevices are connected. It just lists them all in one node... A description for this patch would be very welcome IMHO and, maybe, such a node can be completely avoided? Sorry, I'll provide better description in next iteration. It's true it doesn't describe the topology in detail, as there are multiple one-to many possible connections between sub-devices. An exact topology is coded in the driver and can be changed through MC API. The samsung,camif-mux-id and video-bus-type properties at sensor nodes just specify to which physical SoC camera port an image sensor is connected. So, don't you think my media-link child nodes are a good solution for this? Not quite yet ;) It would be good to see some example implementation and how it actually works. Originally the all camera devices were supposed to land under common 'camera' node. And a top level driver would be registering all platform devices. With this approach it would be possible to better control PM handling (which currently depends on an order of registering devices to the driver core). But then we discovered that we couldn't use OF_DEV_AUXDATA in such case, which was required to preserve platform device names, in order for the clock API to work. So I've moved some sub-devices out of 'camera' node and have added only a list of phandles to them in that node. This is rather a cheap workaround.. I think all camera sub-devices should be placed under common node, as there are some properties that don't belong to any sub-node: GPIO config, clocks, to name a few. Of course simpler devices might not need such a composite node. I think we can treat the sub-device interdependencies as an issue separate from a need for a common node. If some devices need to reflect the topology better, we probably could include in some nodes (a list of) phandles to other nodes. This could ease parsing the topology at the drivers, by using existing OF infrastructure. Ok, I think you have some good ideas in your RFC's, an interesting question now is - how to proceed. Do you think we'd be able to work out a combined RFC? Or would you prefer to make two versions and then see what others think? In either case it would be nice, I think, if you could try to separate what you see as common V4L DT bindings, then we could discuss that separately. Whereas what you think is private to your hardware, we can also look at for common ideas, or maybe even some of those properties we'll wake to make common too. I think we need a one combined RFC and continue discussions in one thread. Agreed. Still, our proposals are quite different, but I believe we need something in between. I presume we should focus more to have common bindings for subdevs that are reused among different host/ISP devices, i.e. sensors and encoders. For simple host interfaces we can likely come up with common binding patterns, but more complex processing pipelines may require a sort of individual approach. The suspend/resume handling is still something I don't have an idea on how the solution for might look like.. Instantiating all devices from a top level driver could help, but it is only going to work when platforms are converted to the common clock framework and have their clocks instantiated from device tree. This week I'm out of office, and next one or two I have some pending assignments. So there might be some delay before I can dedicate some reasonable amount of time to carry on with that topic. I unfortunately won't be attending KS this time. That's bad news :-( I still think this topic should be discussed during KS, I expect several developers to be interested. The media workshop might not be the best venue though, as we might need quite a lot of time. Until KS let's continue the discussion by e-mail. I'll try to prepare some summary with topics that appear common. And also to restructure my RFC series to better separate new common features and specific H/W support. In the meantime we could possibly continue discussions in your RFC thread. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo
Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation
(1.65V to 1.95V) + or 2.8V (2.5V to 3.1V); + +Optional properties: + +- clock-frequency : the IP's main (system bus) clock frequency in Hz, the default + value when this property is not specified is 24 MHz; +- data-lanes : number of physical lanes used (default 2 if not specified); bus-width? OK. +- gpio-stby : specifies the S5K6AA_STBY GPIO +- gpio-rst : specifies the S5K6AA_RST GPIO From Documentation/devicetree/bindings/gpio/gpio.txt: quote GPIO properties should be named [name-]gpios. /quote Thanks, these have been already removed in the next version, as can be seen above. +- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level; +- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level; Isn't this provided by GPIO flags as described in include/linux/of_gpio.h by using the OF_GPIO_ACTIVE_LOW bit? Hmm, I wasn't aware of that. I'll see how this flag can be used. +- samsung,s5k6aa-hflip : set the default horizontal image flipping; +- samsung,s5k6aa-vflip : set the default vertical image flipping; This is a board property, specifying how the sensor is wired and mounted on the casing, right? IIRC, libv4l has a database of these for USB Yeah, that's exactly it. It could be used for compensating for an upside down mounting. cameras. So, maybe it belongs to the user-space for embedded systems too? Using library for these things is one of the solutions, which works well for USB webcams. However, I wouldn't like enforcing that. Those properties still describe hardware IHMO, then why not add them into DT blob that is usually distributed with each board ? There wouldn't be a need to rely on any user-space database lookup, before the hardware can actually be used. I agree. For USB devices on x86 we don't have many other options, but with DT we can describe the hardware properly. Or at least this shouldn't be Samsung-specific? Yes, good point. If we agree we can keep them, perhaps we could standardize those as boolean properties: horizontal-flip; and vertical-flip; ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation
Hi Sylwester,
On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote:
The driver initializes all board related properties except the s_power()
callback to board code. The platforms that require this callback are not
supported by this driver yet for CONFIG_OF=y.
Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
.../bindings/camera/samsung-s5k6aafx.txt | 57 +
drivers/media/video/s5k6aa.c | 129 -
2 files changed, 146 insertions(+), 40 deletions(-)
create mode 100644
Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file
mode 100644
index 000..6685a9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt
@@ -0,0 +1,57 @@
+Samsung S5K6AAFX camera sensor
+--
+
+Required properties:
+
+- compatible : samsung,s5k6aafx;
+- reg : base address of the device on I2C bus;
+- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601;
+- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V);
+- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V);
+- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V)
+or 2.8V (2.6V to 3.0);
+- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V)
+ or 2.8V (2.5V to 3.1V);
+
+Optional properties:
+
+- clock-frequency : the IP's main (system bus) clock frequency in Hz, the
default
Is that the input clock frequency ? Can't it vary ? Instead of accessing the
sensor clock frequency from the FIMC driver you should reference a clock in
the sensor DT node. That obviously requires generic clock support, which might
not be available for your platform yet (that's one of the reasons the OMAP3
ISP driver doesn't support DT yet).
+ value when this property is not specified is 24 MHz;
+- data-lanes : number of physical lanes used (default 2 if not specified);
+- gpio-stby : specifies the S5K6AA_STBY GPIO
+- gpio-rst : specifies the S5K6AA_RST GPIO
+- samsung,s5k6aa-inv-stby : set inverted S5K6AA_STBY GPIO level;
+- samsung,s5k6aa-inv-rst : set inverted S5K6AA_RST GPIO level;
+- samsung,s5k6aa-hflip : set the default horizontal image flipping;
+- samsung,s5k6aa-vflip : set the default vertical image flipping;
+
+
+Example:
+
+ gpl2: gpio-controller@0 {
+ };
+
+ reg0: regulator@0 {
+ };
+
+ reg1: regulator@1 {
+ };
+
+ reg2: regulator@2 {
+ };
+
+ reg3: regulator@3 {
+ };
+
+ s5k6aafx@3c {
+ compatible = samsung,s5k6aafx;
+ reg = 0x3c;
+ clock-frequency = 2400;
+ gpio-rst = gpl2 0 2 0 3;
+ gpio-stby = gpl2 1 2 0 3;
+ video-itu-601-bus;
+ vdd_core-supply = reg0;
+ vdda-supply = reg1;
+ vdd_reg-supply = reg2;
+ vddio-supply = reg3;
+ };
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 02/13] media: s5p-csis: Add device tree support
Hi Sylwester, On Thursday 26 July 2012 21:51:30 Sylwester Nawrocki wrote: On 07/26/2012 04:38 PM, Laurent Pinchart wrote: --- /dev/null +++ b/Documentation/devicetree/bindings/video/mipi.txt @@ -0,0 +1,5 @@ +Common properties of MIPI-CSI1 and MIPI-CSI2 receivers and transmitters + + - data-lanes : number of differential data lanes wired and actively used in +communication between the transmitter and the receiver, this +excludes the clock lane; Wouldn't it be better to use the standard bus-width DT property? I can't see any problems with using bus-width. It seems sufficient and could indeed be better, without a need to invent new MIPI-CSI specific names. That was my first RFC on that and my perspective wasn't probably broad enough. :) What about CSI receivers that can reroute the lanes internally ? We would need to specify lane indices for each lane then, maybe with something like clock-lane =0; data-lanes =2 3 1; Sounds good to me. And the clock-lane could be made optional, as not all devices would need it. However, as far as I can see, there is currently no generic API for handling this kind of data structure. E.g. number of cells for the interrupts property is specified with an additional #interrupt-cells property. It would have been much easier to handle something like: data-lanes = 2, 3, 1; i.e. an array of the lane indexes. I'm fine with that. For receivers that can't reroute lanes internally, the data-lanes property would be need to specify lanes in sequence. data-lanes =1 2 3; In this case we would be only interested in the number of cells in this property, but how it could be retrieved ? With an array, it could have been calculated from property length returned by of_property_find() (divided by sizof(u32)). Agreed. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation
Hi Sylwester, On Thursday 26 July 2012 22:39:31 Sylwester Nawrocki wrote: On 07/26/2012 05:21 PM, Laurent Pinchart wrote: On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote: The driver initializes all board related properties except the s_power() callback to board code. The platforms that require this callback are not supported by this driver yet for CONFIG_OF=y. Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com Signed-off-by: Bartlomiej Zolnierkiewiczb.zolnier...@samsung.com Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com --- .../bindings/camera/samsung-s5k6aafx.txt | 57 + drivers/media/video/s5k6aa.c | 129 + 2 files changed, 146 insertions(+), 40 deletions(-) create mode 100644 Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt diff --git a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file mode 100644 index 000..6685a9c --- /dev/null +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt @@ -0,0 +1,57 @@ +Samsung S5K6AAFX camera sensor +-- + +Required properties: + +- compatible : samsung,s5k6aafx; +- reg : base address of the device on I2C bus; +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601; +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V); +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V); +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to 1.9V) + or 2.8V (2.6V to 3.0); +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V) + or 2.8V (2.5V to 3.1V); + +Optional properties: + +- clock-frequency : the IP's main (system bus) clock frequency in Hz, the default Is that the input clock frequency ? Can't it vary ? Instead of accessing the Yes, the description is incorrect in this patch, it should read: +- clock-frequency : the sensor's master clock frequency in Hz; and be a required property. As in this patch: https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7 abe563 It could vary (as this is a PLL input frequency), so probably a range would be a better alternative. Given that host device won't always be able to set this exact value... A range sounds good, or perhaps a list of ranges. Sakari, what would you need for the SMIA++ driver ? sensor clock frequency from the FIMC driver you should reference a clock in the sensor DT node. That obviously requires generic clock support, which might not be available for your platform yet (that's one of the reasons the OMAP3 ISP driver doesn't support DT yet). I agree it might be better, but waiting unknown number of kernel releases for the platforms to get converted to common clock API is not a good alternative either. I guess we could have some transitional solutions while other subsystems are getting adapted. I agree, we need an interim solution. Yet we need to specify the clock frequency range per sensor, so 1. either we specify it at a sensor node and host device driver references it, or 2. it could be added to a sensor specific child node of a host device mode, and then only the host would reference it, and sensor would reference a clock in its DT node; I guess it's not a problem that in most cases the camera host device is a clock provider. The sensor will need to configure the clock rate, so a (list of) clock frequency range(s) will be needed in the sensor node anyway. As an interim solution the host can access that property. When the platform will be ported to the common clock API no modification to the DT will be needed. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] video: s3c-fb: Add window positioning support
Hi Florian, On Sunday 18 September 2011 21:29:57 Florian Tobias Schandinat wrote: On 09/07/2011 03:31 PM, Laurent Pinchart wrote: On Thursday 01 September 2011 18:45:18 Florian Tobias Schandinat wrote: On 08/25/2011 07:51 PM, Ajay Kumar wrote: Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place? Good point. Congratulations for figuring out that I like to standardize things. But I think your suggestion is far from being enough to be useful for userspace (which is our goal so that applications can be reused along drivers and don't need to know about individual drivers). Beside standardizing things, do you also like to take them one level higher to solve challenging issues ? I know the answer must be yes :-) The problem at hand here is something we have solved in V4L2 (theoretically only for part of it) with the media controller API, the V4L2 subdevs and their pad-level format API. In a nutshell, the media controller lets drivers model hardware as a graph of buliding blocks connected through their pads and expose that description to userspace applications. In V4L2 most of those blocks are V4L2 subdevs, which are abstract building blocks that implement sets of standard operations. Those operations are exposed to userspace through the V4L2 subdevs pad-level format API, allowing application to configure sizes and selection rectangles at all pads in the graph. Selection rectangles can be used to configure cropping and composing, which is exactly what the window positioning API needs to do. Instead of creating a new fbdev-specific API to do the same, shouldn't we try to join forces ? Okay, thanks for the pointer. After having a look at your API I understand that it would solve the problem to discover how many windows (in this case) are there and how they can be accessed. It looks fine for this purpose, powerful enough and not too complex. So if I get it correct we still need at least a way to configure the position of the windows/overlays/sink pads similar to what Ajay proposed. Yes, the media controller API can only expose the topology to userspace, it can't be used to configure FB-specific parameters on the pipeline. Additionally a way to get and/or set the z-position of the overlays if multiple overlays overlap and set/get how the overlays work (overdraw, constant alpha, source/destination color keying). Normally I'd consider these link properties but I think implementing them as properties of the source framebuffer or sink pad would work as well. Is this correct or did I miss something? That's correct. What bothers me is that both V4L2 and DRM/KMS have the exact same needs. I don't think it makes sense to implement three different solutions to the same problem in our three video-related APIs. What's your opinion about that ? I've tried to raise the issue on the dri-devel mailing list (Proposal for a low-level Linux display framework), but there's still a long way to go before convincing everybody. Feel free to help me :-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] video: s3c-fb: Add window positioning support
Hi Florian, On Thursday 01 September 2011 18:45:18 Florian Tobias Schandinat wrote: Hi all, On 08/25/2011 07:51 PM, Ajay Kumar wrote: Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place? Good point. Congratulations for figuring out that I like to standardize things. But I think your suggestion is far from being enough to be useful for userspace (which is our goal so that applications can be reused along drivers and don't need to know about individual drivers). Beside standardizing things, do you also like to take them one level higher to solve challenging issues ? I know the answer must be yes :-) The problem at hand here is something we have solved in V4L2 (theoretically only for part of it) with the media controller API, the V4L2 subdevs and their pad-level format API. In a nutshell, the media controller lets drivers model hardware as a graph of buliding blocks connected through their pads and expose that description to userspace applications. In V4L2 most of those blocks are V4L2 subdevs, which are abstract building blocks that implement sets of standard operations. Those operations are exposed to userspace through the V4L2 subdevs pad-level format API, allowing application to configure sizes and selection rectangles at all pads in the graph. Selection rectangles can be used to configure cropping and composing, which is exactly what the window positioning API needs to do. Instead of creating a new fbdev-specific API to do the same, shouldn't we try to join forces ? So let me at first summarize how I understand you implemented those things after having a brief look at some of the drivers: Windows are rectangular screen areas whose pixel data come from other locations. The other locations are accessible via other framebuffer devices (e.g. fb1). So in this area the data of fb1 is shown and not the data of fb0 that would be normally shown. So in addition to your proposed positioning I think we should also have the following to give userspace a useful set of functionality: - a way to discover how the screen is composited (how many windows are there, how they are stacked and how to access those) - a way to enable/disable windows (make them (in)visible) - reporting and selecting how the window content can be mixed with the root screen (overwrite, source or destination color keying) - things like window size and color format could be handled by the usual fb API used on the window. However there might be restrictions which cause them to be not 100% API compatible (for example when changing the color format if the windows are required to have the same format as the root screen) - do we need to worry about hardware (up/down) scaling of the window content? So is that what you need for a standardized window implementation? Any additional things that were useful/needed in this context? Would you consider adding support for this API in your drivers? (as standardizing wouldn't be useful if nobody would implement it) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3 v6] Add v4l2 subdev driver for Samsung S5P MIPI-CSI receivers
Hi Sylwester, On Monday 16 May 2011 14:05:36 Sylwester Nawrocki wrote: Hello, I'm resending this MIPI-CSI slave device driver patch to address review comments and fix a few further minor issues. My apologies for spamming a mailbox to those who are not interested. For the whole patch set, Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Changes since v5: - slightly improved description of struct csis_state - moved the pad number check from __s5pcsis_get_format directly to set_fmt/get_fmt pad level operation handlers - replaced __init attribute of s5pcsis_probe() with __devinit and added __devexit for s5pcsis_remove() - fixed bug in s5pcsis_set_hsync_settle, improved set_fmt handler [PATCH 1/3] v4l: Add V4L2_MBUS_FMT_JPEG_1X8 media bus format [PATCH 2/3] v4l: Move s5p-fimc driver into Video capture devices [PATCH 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI Documentation/DocBook/v4l/subdev-formats.xml | 46 ++ drivers/media/video/Kconfig | 28 +- drivers/media/video/Makefile |1 + drivers/media/video/s5p-fimc/Makefile|6 +- drivers/media/video/s5p-fimc/fimc-capture.c | 10 +- drivers/media/video/s5p-fimc/mipi-csis.c | 726 ++ drivers/media/video/s5p-fimc/mipi-csis.h | 22 + include/linux/v4l2-mediabus.h|3 + 8 files changed, 827 insertions(+), 15 deletions(-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Sylwester,
On Sunday 15 May 2011 11:33:14 Sylwester Nawrocki wrote:
On 05/14/2011 05:29 PM, Laurent Pinchart wrote:
On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote:
[snip]
+static int s5pcsis_suspend(struct device *dev)
+{
+ struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+ struct csis_state *state = sd_to_csis_state(sd);
+ int ret;
+
+ v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+ mutex_lock(state-lock);
+ if (state-flags ST_POWERED) {
+ s5pcsis_stop_stream(state);
+ ret = pdata-phy_enable(state-pdev, false);
+ if (ret)
+ goto unlock;
+
+ if (state-supply regulator_disable(state-supply))
+ goto unlock;
+
+ clk_disable(state-clock[CSIS_CLK_GATE]);
+ state-flags= ~ST_POWERED;
+ }
+ state-flags |= ST_SUSPENDED;
+ unlock:
+ mutex_unlock(state-lock);
+ return ret ? -EAGAIN : 0;
+}
+
+static int s5pcsis_resume(struct device *dev)
+{
+ struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+ struct csis_state *state = sd_to_csis_state(sd);
+ int ret = 0;
+
+ v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+ mutex_lock(state-lock);
+ if (!(state-flags ST_SUSPENDED))
+ goto unlock;
+
+ if (!(state-flags ST_POWERED)) {
If the device wasn't powered before being suspended, it should stay
powered off.
I'm not sure, shortly after system wide resume the device is powered off by
PM runtime core anyway.
There is no other means in this driver to enable power except using
pm_runtime_* calls. The device is being powered on or off only through
these runtime PM helpers, i.e. s5pcsis_resume/s5pcsis_suspend.
(full source can be found here: http://tinyurl.com/6fozx34)
OK, it should be fine then.
The pm_runtime_resume helper is guaranteed by the PM core to be called only
on device in 'suspended' state.
From Documentation/power/runtime_pm.txt:
...
* Once the subsystem-level resume callback has completed successfully, the
PM core regards the device as fully operational, which means that the
device _must_ be able to complete I/O operations as needed. The run-time
PM status of the device is then 'active'.
...
If s5pcsis_resume would return 0 without really enabling device clocks and
the external voltage regulator then the runtime PM core idea about the
device's state would be wrong.
I'm not a PM expert but documentation says that it's better to leave
device fully functional after system wide driver's resume() helper returns.
From Documentation/power/devices.txt:
...
When resuming from standby or memory sleep, the phases are:
resume_noirq, resume, complete.
(...)
At the end of these phases, drivers should be as functional as they were
before suspending: I/O can be performed using DMA and IRQs, and the
relevant clocks are gated on. Even if the device was in a low-power state
before the system sleep because of runtime power management, afterwards it
should be back in its full-power state. There are multiple reasons why
it's best to do this; they are discussed in more detail in
Documentation/power/runtime_pm.txt.
...
Unfortunately there seem to be no standard way to instruct the PM core to
enable power of a few (I2C/client platform) devices forming the video
pipeline in an arbitrary order. The parent devices of my platforms devices
are already the power domain devices.
So it might be a good idea to forbid enabling sub-device's power when
the host driver is not using it, to have full control of the pipeline
devices power enable sequence at any time.
I perhaps need to isolate functions out from of s5pcsis_resume/suspend and
then call that in s_power op and s5pcsis_resume/suspend. Don't really like
this idea though... It would virtually render pm_runtime_* calls unusable
in this sub-device driver, those would make sense only in the host driver..
I think using the pm_runtime_* calls make sense, they could replace the subdev
s_power operation in the long term. We'll need to evaluate that (I'm not sure
if runtime PM is available on all platforms targetted by V4L2 for instance).
I just wanted to put all what is needed to control device's power in the PM
helpers and then use pm_runtime_* calls where required.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Sylwester,
On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote:
Add the subdev driver for the MIPI CSIS units available in S5P and
Exynos4 SoC series. This driver supports both CSIS0 and CSIS1
MIPI-CSI2 receivers.
The driver requires Runtime PM to be enabled for proper operation.
Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
[snip]
diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c
b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644
index 000..d50efcb
--- /dev/null
+++ b/drivers/media/video/s5p-fimc/mipi-csis.c
@@ -0,0 +1,722 @@
[snip]
+static void s5pcsis_enable_interrupts(struct csis_state *state, bool on)
+{
+ u32 val = s5pcsis_read(state, S5PCSIS_INTMSK);
+
+ val = on ? val | S5PCSIS_INTMSK_EN_ALL :
+val ~S5PCSIS_INTMSK_EN_ALL;
+ s5pcsis_write(state, S5PCSIS_INTMSK, val);
Shouldn't you clear all interrupt flags by writing to S5PCSIS_INTSRC before
enabling interrupts, just in case ?
+}
[snip]
+static void s5pcsis_set_hsync_settle(struct csis_state *state, int settle)
+{
+ u32 val = s5pcsis_read(state, S5PCSIS_DPHYCTRL);
+
+ val = ~S5PCSIS_DPHYCTRL_HSS_MASK | (settle 27);
Do you mean
val = (val ~S5PCSIS_DPHYCTRL_HSS_MASK) | (settle 27);
?
+ s5pcsis_write(state, S5PCSIS_DPHYCTRL, val);
+}
[snip]
+static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh
*fh, + struct v4l2_subdev_format *fmt)
+{
+ struct csis_state *state = sd_to_csis_state(sd);
+ struct csis_pix_format const *csis_fmt;
+ struct v4l2_mbus_framefmt *mf;
+
+ mf = __s5pcsis_get_format(state, fh, fmt-pad, fmt-which);
+
+ if (fmt-pad == CSIS_PAD_SOURCE) {
+ if (mf) {
+ mutex_lock(state-lock);
+ fmt-format = *mf;
+ mutex_unlock(state-lock);
+ }
+ return 0;
+ }
+ csis_fmt = s5pcsis_try_format(fmt-format);
+ if (mf) {
+ mutex_lock(state-lock);
+ *mf = fmt-format;
+ if (mf == state-format) /* Store the active format */
I would replace the test by
if (fmt-which == V4L2_SUBDEV_FORMAT_TRY)
It's more explicit.
+ state-csis_fmt = csis_fmt;
+ mutex_unlock(state-lock);
+ }
+ return 0;
+}
[snip]
+static int s5pcsis_suspend(struct device *dev)
+{
+ struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+ struct csis_state *state = sd_to_csis_state(sd);
+ int ret;
+
+ v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+ mutex_lock(state-lock);
+ if (state-flags ST_POWERED) {
+ s5pcsis_stop_stream(state);
+ ret = pdata-phy_enable(state-pdev, false);
+ if (ret)
+ goto unlock;
+
+ if (state-supply regulator_disable(state-supply))
+ goto unlock;
+
+ clk_disable(state-clock[CSIS_CLK_GATE]);
+ state-flags = ~ST_POWERED;
+ }
+ state-flags |= ST_SUSPENDED;
+ unlock:
+ mutex_unlock(state-lock);
+ return ret ? -EAGAIN : 0;
+}
+
+static int s5pcsis_resume(struct device *dev)
+{
+ struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+ struct csis_state *state = sd_to_csis_state(sd);
+ int ret = 0;
+
+ v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+ mutex_lock(state-lock);
+ if (!(state-flags ST_SUSPENDED))
+ goto unlock;
+
+ if (!(state-flags ST_POWERED)) {
If the device wasn't powered before being suspended, it should stay powered
off.
+ if (state-supply)
+ ret = regulator_enable(state-supply);
+ if (ret)
+ goto unlock;
+
+ ret = pdata-phy_enable(state-pdev, true);
+ if (!ret) {
+ state-flags |= ST_POWERED;
+ } else {
+ regulator_disable(state-supply);
+ goto unlock;
+ }
+ clk_enable(state-clock[CSIS_CLK_GATE]);
+ }
+ if (state-flags ST_STREAMING)
+ s5pcsis_start_stream(state);
+
+ state-flags = ~ST_SUSPENDED;
+ unlock:
+ mutex_unlock(state-lock);
+ return ret ? -EAGAIN : 0;
+}
[snip]
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Sylwester, On Thursday 05 May 2011 11:33:27 Sylwester Nawrocki wrote: On 05/04/2011 02:00 PM, Laurent Pinchart wrote: On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote: On 05/03/2011 11:16 AM, Laurent Pinchart wrote: On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote: [snip] +struct media_pad pads[CSIS_PADS_NUM]; +struct v4l2_subdev sd; +struct platform_device *pdev; +struct resource *regs_res; +void __iomem *regs; +struct clk *clock[NUM_CSIS_CLOCKS]; +int irq; +struct regulator *supply; +u32 flags; +/* Common format for the source and sink pad. */ +const struct csis_pix_format *csis_fmt; +struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; As try formats are stored in the file handle, and as the formats on the sink and source pads are identical, a single v4l2_mbus_framefmt will do here. Ok. How about a situation when the caller never provides a file handle? Is it not supposed to happen? Good question :-) The subdev pad-level operations have been designed with a userspace interface in mind, so they require a file handle to store try the formats (and crop rectangles). For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the format and should get_fmt just return -EINVAL when passed fh == NULL ? For such a simple subdev, that should work as a workaround, yes. You can use it as a temporary solution at least. Or should the host driver allocate the file handle just for the sake of set_fmt/get_fmt calls (assuming that cropping ops are not supported by the subdev) ? That's another solution. We could also pass an internal structure that contains formats and crop rectangles to the pad operations handlers, instead of passing the whole file handle. Do you think that would be better ? So it would then be an additional argument for the pad-level operations ? It would replace the file handle argument. Perhaps that would make sense when both format and crop information is needed at the same time in the subdev driver. However this would only be required for TRY formats/crop rectangles which wouldn't be supported anyway because of missing file handle.. or I missed something? The reason why we pass the file handle to the pad operations is to let drivers access formats/crop try settings that are stored in the file handle. If we moved those settings to a separate structure, and embedded that structure into the file handle structure, we could pass fh-settings instead of fh to the pad operations. Drivers that want to call pad operations would then need to allocate a settings structure, instead of a complete fake fh. Furthermore I assume more complex pipelines will be handled in user space The pad-level API has been designed to get/set formats/crop settings directly from userspace, not from inside the kernel, so that would certainly work. and the host drivers could store format and crop information locally directly from v4l2_subdev_format and v4l2_subdev_crop data structures. I'm not sure to understand what you mean there. Quoting one of your comments below, x--- FIMC_0 (/dev/video1) SENSOR - MIPI_CSIS --| x--- FIMC_1 (/dev/video3) How do you expect to configure the MIPI_CSIS block from the FIMC_0 and FIMC_1 blocks, without any help from userspace ? Conflicts will need to be handled, and the best way to handle them is to have userspace configuring the MIPI_CSIS explicitly. My priority is to make work the configurations without device nodes at sensor and MIPI CSIS subdevs. I agree it would be best to leave the negotiation logic to user space, however I need to assure the regular V4L2 application also can use the driver. My idea was to only try format at CSI slave and sensor subdevs when S_FMT is called on a video node. So the sensor and CSIS constraints are taken into account. Then from VIDIOC_STREAMON, formats at pipeline elements would be set on subdevs without device node or validated on subdevs providing a device node. For subdevs without device nodes, why don't you set the active format directly when S_FMT is called, instead of postponing the operation until VIDIOC_STREAMON time ? You wouldn't need to use TRY formats then. Another issue is v4l2 controls. The subdevs are now in my case registered to a v4l2_device instance embedded in the media device driver. The video node drivers (FIMC0...FIMC3) have their own v4l2_device instances. So the control inheritance between video node and a subdevs is gone :/, i.e. I couldn't find a standard way to remove back from a parent control handler the controls which have been added to it with v4l2_ctrl_handler_add(). I think you should expose sensor controls through subdev devices nodes, not through the V4L2 device node. I've had similar issue
Re: [RFC/PATCH v7 1/5] Changes in include/linux/videodev2.h for MFC 5.1
On Friday 04 March 2011 12:26:18 Kamil Debski wrote:
This patch adds fourcc values for compressed video stream formats and
V4L2_CTRL_CLASS_CODEC. Also adds controls used by MFC 5.1 driver.
Signed-off-by: Kamil Debski k.deb...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
include/linux/videodev2.h | 39 +++
1 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index a94c4d5..a48a42e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -369,6 +369,19 @@ struct v4l2_pix_format {
#define V4L2_PIX_FMT_DV v4l2_fourcc('d', 'v', 's', 'd') /* 1394 */
#define V4L2_PIX_FMT_MPEG v4l2_fourcc('M', 'P', 'E', 'G') /* MPEG-1/2/4
*/
+#define V4L2_PIX_FMT_H264 v4l2_fourcc('H', '2', '6', '4') /* H264 */
+#define V4L2_PIX_FMT_H263 v4l2_fourcc('H', '2', '6', '3') /* H263 */
+#define V4L2_PIX_FMT_MPEG12 v4l2_fourcc('M', 'P', '1', '2') /* MPEG-1/2 */
+#define V4L2_PIX_FMT_MPEG4v4l2_fourcc('M', 'P', 'G', '4') /* MPEG-4 */
+#define V4L2_PIX_FMT_DIVX v4l2_fourcc('D', 'I', 'V', 'X') /* DivX */
+#define V4L2_PIX_FMT_DIVX3v4l2_fourcc('D', 'I', 'V', '3') /* DivX 3.11 */
+#define V4L2_PIX_FMT_DIVX4v4l2_fourcc('D', 'I', 'V', '4') /* DivX 4.12 */
+#define V4L2_PIX_FMT_DIVX500 v4l2_fourcc('D', 'X', '5', '2') /* DivX 5.00 -
5.02 */
+#define V4L2_PIX_FMT_DIVX503 v4l2_fourcc('D', 'X', '5', '3') /* DivX 5.03 -
x */
+#define V4L2_PIX_FMT_XVID v4l2_fourcc('X', 'V', 'I', 'D') /* Xvid */
+#define V4L2_PIX_FMT_VC1 v4l2_fourcc('V', 'C', '1', 'A') /* VC-1 */
+#define V4L2_PIX_FMT_VC1_RCV v4l2_fourcc('V', 'C', '1', 'R') /* VC-1 RCV */
+
Hans, you mentioned some time ago that you were against ading H.264 or MPEG4
fourccs, and that drivers should use the MPEG controls instead. Could you
clarify your current position on this ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
