Re: [PATCH v4 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

[Enric Balletbo Serra]

Hi Neil,

2018-05-21 16:21 GMT+02:00 Neil Armstrong :
> The EC can expose a CEC bus, this patch adds the CEC related definitions
> needed by the cros-ec-cec driver.
> Having a 16 byte mkbp event size makes it possible to send CEC
> messages from the EC to the AP directly inside the mkbp event
> instead of first doing a notification and then a read.
>
> Signed-off-by: Stefan Adolfsson 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/platform/chrome/cros_ec_proto.c |  40 ++---
>  include/linux/mfd/cros_ec.h |   2 +-
>  include/linux/mfd/cros_ec_commands.h| 103 
> 
>  3 files changed, 135 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c 
> b/drivers/platform/chrome/cros_ec_proto.c
> index e7bbdf9..c4f6c44 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -504,10 +504,31 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device 
> *ec_dev,
>  }
>  EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
>
> +static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> +  struct cros_ec_command *msg,
> +  int version, uint32_t size)
> +{
> +   int ret;
> +
> +   msg->version = version;
> +   msg->command = EC_CMD_GET_NEXT_EVENT;
> +   msg->insize = size;
> +   msg->outsize = 0;
> +
> +   ret = cros_ec_cmd_xfer(ec_dev, msg);
> +   if (ret > 0) {
> +   ec_dev->event_size = ret - 1;
> +   memcpy(_dev->event_data, msg->data, ec_dev->event_size);
> +   }
> +
> +   return ret;
> +}
> +
>  static int get_next_event(struct cros_ec_device *ec_dev)
>  {
> u8 buffer[sizeof(struct cros_ec_command) + 
> sizeof(ec_dev->event_data)];
> struct cros_ec_command *msg = (struct cros_ec_command *)
> +   static int cmd_version = 1;
> int ret;
>
> if (ec_dev->suspended) {
> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> return -EHOSTDOWN;
> }
>
> -   msg->version = 0;
> -   msg->command = EC_CMD_GET_NEXT_EVENT;
> -   msg->insize = sizeof(ec_dev->event_data);
> -   msg->outsize = 0;
> +   if (cmd_version == 1) {
> +   ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> +   sizeof(struct ec_response_get_next_event_v1));
> +   if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
> +   return ret;
>

Thinking a bit more, there is a command to ask supported command
versions (EC_CMD_GET_CMD_VERSION). So, i theory you should be able to
ask for the versions supported by the command, see for example what is
done in [1], [2] and [3]. Anyway I am fine with this for now, and
after do some test seems that nothing breaks, so

Tested-by: Enric Balletbo i Serra 

Thanks,
 Enric

[1] 
https://chromium.googlesource.com/chromiumos/platform/ec/+/master/util/misc_util.c#98
[2] 
https://chromium.googlesource.com/chromiumos/platform/ec/+/master/util/misc_util.c#131
[3] 
https://chromium.googlesource.com/chromiumos/platform/ec/+/master/util/ectool.c#786


> -   ret = cros_ec_cmd_xfer(ec_dev, msg);
> -   if (ret > 0) {
> -   ec_dev->event_size = ret - 1;
> -   memcpy(_dev->event_data, msg->data,
> -  sizeof(ec_dev->event_data));
> +   /* Fallback to version 0 for future send attempts */
> +   cmd_version = 0;
> }
>
> +   ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> + sizeof(struct ec_response_get_next_event));
> +
> return ret;
>  }
>
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index f36125e..32caef3 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -147,7 +147,7 @@ struct cros_ec_device {
> bool mkbp_event_supported;
> struct blocking_notifier_head event_notifier;
>
> -   struct ec_response_get_next_event event_data;
> +   struct ec_response_get_next_event_v1 event_data;
> int event_size;
> u32 host_event_wake_mask;
>  };
> diff --git a/include/linux/mfd/cros_ec_commands.h 
> b/include/linux/mfd/cros_ec_commands.h
> index f2edd99..9b8bc4a 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -804,6 +804,8 @@ enum ec_feature_code {
> EC_FEATURE_MOTION_SENSE_FIFO = 24,
> /* EC has RTC feature that can be controlled by host commands */
> EC_FEATURE_RTC = 27,
> +   /* EC supports CEC commands */
> +   EC_FEATURE_CEC = 35,
>  };
>
>  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> @@ -2078,6 +2080,12 @@ enum ec_mkbp_event {
> /* EC sent a sysrq command */
> 

Re: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

[Enric Balletbo Serra]

Hi Neil,

2018-05-18 15:05 GMT+02:00 Neil Armstrong :
> The EC can expose a CEC bus, this patch adds the CEC related definitions
> needed by the cros-ec-cec driver.
> Having a 16 byte mkbp event size makes it possible to send CEC
> messages from the EC to the AP directly inside the mkbp event
> instead of first doing a notification and then a read.
>
> Signed-off-by: Stefan Adolfsson 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 40 +
>  include/linux/mfd/cros_ec.h |  2 +-
>  include/linux/mfd/cros_ec_commands.h| 80 
> +
>  3 files changed, 112 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c 
> b/drivers/platform/chrome/cros_ec_proto.c
> index e7bbdf9..c4f6c44 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -504,10 +504,31 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device 
> *ec_dev,
>  }
>  EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
>
> +static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> +  struct cros_ec_command *msg,
> +  int version, uint32_t size)
> +{
> +   int ret;
> +
> +   msg->version = version;
> +   msg->command = EC_CMD_GET_NEXT_EVENT;
> +   msg->insize = size;
> +   msg->outsize = 0;
> +
> +   ret = cros_ec_cmd_xfer(ec_dev, msg);
> +   if (ret > 0) {
> +   ec_dev->event_size = ret - 1;
> +   memcpy(_dev->event_data, msg->data, ec_dev->event_size);
> +   }
> +
> +   return ret;
> +}
> +
>  static int get_next_event(struct cros_ec_device *ec_dev)
>  {
> u8 buffer[sizeof(struct cros_ec_command) + 
> sizeof(ec_dev->event_data)];
> struct cros_ec_command *msg = (struct cros_ec_command *)
> +   static int cmd_version = 1;

Personal opinion, but I don't like this static here, and also I don't
think this is scalable. Could we ask for the command version?

> int ret;
>
> if (ec_dev->suspended) {
> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> return -EHOSTDOWN;
> }
>
> -   msg->version = 0;
> -   msg->command = EC_CMD_GET_NEXT_EVENT;
> -   msg->insize = sizeof(ec_dev->event_data);
> -   msg->outsize = 0;
> +   if (cmd_version == 1) {
> +   ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> +   sizeof(struct ec_response_get_next_event_v1));
> +   if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
> +   return ret;
>
> -   ret = cros_ec_cmd_xfer(ec_dev, msg);
> -   if (ret > 0) {
> -   ec_dev->event_size = ret - 1;
> -   memcpy(_dev->event_data, msg->data,
> -  sizeof(ec_dev->event_data));
> +   /* Fallback to version 0 for future send attempts */
> +   cmd_version = 0;
> }
>

So we always do a failed transfer on all these EC devices that does
not support CEC. I am wondering if wouldn't be better pass the command
version to the cros_ec_get_next_event function. The driver should know
which version to use, just a random idea.

> +   ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> + sizeof(struct ec_response_get_next_event));
> +
> return ret;
>  }
>
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index f36125e..32caef3 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -147,7 +147,7 @@ struct cros_ec_device {
> bool mkbp_event_supported;
> struct blocking_notifier_head event_notifier;
>
> -   struct ec_response_get_next_event event_data;
> +   struct ec_response_get_next_event_v1 event_data;
> int event_size;
> u32 host_event_wake_mask;
>  };
> diff --git a/include/linux/mfd/cros_ec_commands.h 
> b/include/linux/mfd/cros_ec_commands.h
> index f2edd99..16c3a2b 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h

This file is going to be very big and as requested by Lee I plan to
convert this file to the kernel-doc format, this patch introduces some
new structs so could you document the new structs in the suggested
format?

> @@ -804,6 +804,8 @@ enum ec_feature_code {
> EC_FEATURE_MOTION_SENSE_FIFO = 24,
> /* EC has RTC feature that can be controlled by host commands */
> EC_FEATURE_RTC = 27,
> +   /* EC supports CEC commands */
> +   EC_FEATURE_CEC = 35,
>  };
>
>  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
> @@ -2078,6 +2080,12 @@ enum ec_mkbp_event {
> /* EC sent a sysrq command */
> EC_MKBP_EVENT_SYSRQ = 6,
>
> +   /* Notify the AP that something happened on CEC */
> + 

Re: [PATCH v2 5/5] media: platform: Add Chrome OS EC CEC driver

[Enric Balletbo Serra]

Hi Neil,

2018-05-18 15:05 GMT+02:00 Neil Armstrong :
> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the

A minor nit, there is a "consensus" on tell cros-ec as "ChromeOS
Embedded Controller" or "ChromeOS EC". Yes, I know that you can see in
the kernel many other ways to refer to the ChromeOS EC, but to
standardize a little bit, could you replace all occurrences s/Chrome
OS/ChromeOS/. Thanks.

> driver for such feature of the Embedded Controller.
>
> This driver is part of the cros-ec MFD and will be add as a sub-device when
> the feature bit is exposed by the EC.
>
> The controller will only handle a single logical address and handles
> all the messages retries and will only expose Success or Error.
>
> The controller will be tied to the HDMI CEC notifier by using the platform
> DMI Data and the i915 device name and connector name.
>
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/media/platform/Kconfig   |  11 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/cros-ec-cec/Makefile  |   1 +
>  drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 
> +++
>  4 files changed, 361 insertions(+)
>  create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
>  create mode 100644 drivers/media/platform/cros-ec-cec/cros-ec-cec.c
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c7a1cf8..e55a8ed2 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>
>  if CEC_PLATFORM_DRIVERS
>
> +config VIDEO_CROS_EC_CEC
> +   tristate "Chrome OS EC CEC driver"

here

> +   depends on MFD_CROS_EC || COMPILE_TEST
> +   select CEC_CORE
> +   select CEC_NOTIFIER
> +   ---help---
> + If you say yes here you will get support for the
> + Chrome OS Embedded Controller's CEC.

here

> + The CEC bus is present in the HDMI connector and enables 
> communication
> + between compatible devices.
> +
>  config VIDEO_MESON_AO_CEC
> tristate "Amlogic Meson AO CEC driver"
> depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 932515d..830696f 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)+= 
> qcom/camss-8x16/
>  obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
>
>  obj-y  += meson/
> +
> +obj-y  += cros-ec-cec/
> diff --git a/drivers/media/platform/cros-ec-cec/Makefile 
> b/drivers/media/platform/cros-ec-cec/Makefile
> new file mode 100644
> index 000..9ce97f9
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec-cec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
> diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c 
> b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> new file mode 100644
> index 000..7e1e275
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * CEC driver for Chrome OS Embedded Controller

and here

> + *
> + * Copyright (c) 2018 BayLibre, SAS
> + * Author: Neil Armstrong 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DRV_NAME   "cros-ec-cec"
> +
> +/**
> + * struct cros_ec_cec - Driver data for EC CEC
> + *
> + * @cros_ec: Pointer to EC device
> + * @notifier: Notifier info for responding to EC events
> + * @adap: CEC adapter
> + * @notify: CEC notifier pointer
> + * @rx_msg: storage for a received message
> + */
> +struct cros_ec_cec {
> +   struct cros_ec_device *cros_ec;
> +   struct notifier_block notifier;
> +   struct cec_adapter *adap;
> +   struct cec_notifier *notify;
> +   struct cec_msg rx_msg;
> +};
> +
> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
> +{
> +   struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +   uint8_t *cec_message = cros_ec->event_data.data.cec_message;
> +   unsigned int len = cros_ec->event_size;
> +
> +   cros_ec_cec->rx_msg.len = len;
> +   memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
> +
> +   cec_received_msg(cros_ec_cec->adap, _ec_cec->rx_msg);
> +}
> +
> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
> +{
> +   struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +   uint32_t events = cros_ec->event_data.data.cec_events;
> +
> +   if (events & EC_MKBP_CEC_SEND_OK)
> +   cec_transmit_attempt_done(cros_ec_cec->adap,
> + 

Re: [PATCH v2 0/5] Add ChromeOS EC CEC Support

[Enric Balletbo Serra]

Hi Neil,

2018-05-18 15:04 GMT+02:00 Neil Armstrong :
> Hi All,
>
> The new Google "Fizz" Intel-based ChromeOS device is gaining CEC support
> through it's Embedded Controller, to enable the Linux CEC Core to communicate
> with it and get the CEC Physical Address from the correct HDMI Connector, the
> following must be added/changed:
> - Add the CEC sub-device registration in the ChromeOS EC MFD Driver
> - Add the CEC related commands and events definitions into the EC MFD driver
> - Add a way to get a CEC notifier with it's (optional) connector name
> - Add the CEC notifier to the i915 HDMI driver
> - Add the proper ChromeOS EC CEC Driver
>
> The CEC notifier with the connector name is the tricky point, since even on
> Device-Tree platforms, there is no way to distinguish between multiple HDMI
> connectors from the same DRM driver. The solution I implemented is pretty
> simple and only adds an optional connector name to eventually distinguish
> an HDMI connector notifier from another if they share the same device.
>
> Feel free to comment this patchset !
>
> Changes since v2:
>  - Add i915 port_identifier() and use this stable name as cec_notifier conn 
> name
>  - Fixed and cleaned up the CEC commands and events handling
>  - Rebased the CEC sub-device registration on top of Enric's serie
>  - Fixed comments typo on cec driver
>  - Protected the DMI match only with PCI and DMI Kconfigs
>

Just because I got confused when I saw two v2 in my inbox. This is v3, right?

> Changes since v1:
>  - Added cec_notifier_put to intel_hdmi
>  - Fixed all small reported issues on the EC CEC driver
>  - Moved the cec_notifier_get out of the #if .. #else .. #endif
>
> Changes since RFC:
>  - Moved CEC sub-device registration after CEC commands and events 
> definitions patch
>  - Removed get_notifier_get_byname
>  - Added CEC_CORE select into i915 Kconfig
>  - Removed CEC driver fallback if notifier is not configured on HW, added 
> explicit warn
>  - Fixed CEC core return type on error
>  - Moved to cros-ec-cec media platform directory
>  - Use bus_find_device() to find the pci i915 device instead of 
> get_notifier_get_byname()
>  - Fix Logical Address setup
>  - Added comment about HW support
>  - Removed memset of msg structures
>
> Neil Armstrong (5):
>   media: cec-notifier: Get notifier by device and connector name
>   drm/i915: hdmi: add CEC notifier to intel_hdmi
>   mfd: cros-ec: Introduce CEC commands and events definitions.
>   mfd: cros_ec_dev: Add CEC sub-device registration
>   media: platform: Add Chrome OS EC CEC driver
>
>  drivers/gpu/drm/i915/Kconfig |   1 +
>  drivers/gpu/drm/i915/intel_display.h |  20 ++
>  drivers/gpu/drm/i915/intel_drv.h |   2 +
>  drivers/gpu/drm/i915/intel_hdmi.c|  13 +
>  drivers/media/cec/cec-notifier.c |  11 +-
>  drivers/media/platform/Kconfig   |  11 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/cros-ec-cec/Makefile  |   1 +
>  drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 
> +++
>  drivers/mfd/cros_ec_dev.c|  16 ++
>  drivers/platform/chrome/cros_ec_proto.c  |  40 ++-
>  include/linux/mfd/cros_ec.h  |   2 +-
>  include/linux/mfd/cros_ec_commands.h |  80 ++
>  include/media/cec-notifier.h |  27 +-
>  14 files changed, 557 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
>  create mode 100644 drivers/media/platform/cros-ec-cec/cros-ec-cec.c
>
> --
> 2.7.4
>

Re: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

[Enric Balletbo Serra]

2018-05-18 15:05 GMT+02:00 Neil Armstrong :
> The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
> when the CEC feature bit is present.
>
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/mfd/cros_ec_dev.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 1d6dc5c..272969e 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -383,6 +383,10 @@ static void cros_ec_sensors_register(struct cros_ec_dev 
> *ec)
> kfree(msg);
>  }
>
> +static const struct mfd_cell cros_ec_cec_cells[] = {
> +   { .name = "cros-ec-cec" }
> +};
> +
>  static const struct mfd_cell cros_ec_rtc_cells[] = {
> { .name = "cros-ec-rtc" }
>  };
> @@ -426,6 +430,18 @@ static int ec_device_probe(struct platform_device *pdev)
> if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
> cros_ec_sensors_register(ec);
>
> +   /* Check whether this EC instance has CEC host command support */
> +   if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
> +   retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> +cros_ec_cec_cells,
> +ARRAY_SIZE(cros_ec_cec_cells),
> +NULL, 0, NULL);
> +   if (retval)
> +   dev_err(ec->dev,
> +   "failed to add cros-ec-cec device: %d\n",
> +   retval);
> +   }
> +
> /* Check whether this EC instance has RTC host command support */
> if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
> retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> --
> 2.7.4
>

Reviewed-by: Enric Balletbo i Serra 

Thanks,
 Enric

Re: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

[Enric Balletbo Serra]

Hi Neil,

I suspect that this patch will conflict with some patches that will be
queued for 4.18 that also introduces new devices, well, for now I
don't see these merged in the Lee's tree.

Based on some reviews I got when I send a patch to this file ...

2018-05-15 17:29 GMT+02:00 Hans Verkuil :
> On 05/15/2018 04:42 PM, Neil Armstrong wrote:
>> The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
>> when the CEC feature bit is present.
>>
>> Signed-off-by: Neil Armstrong 
>
> For what it is worth (not an MFD expert):
>
> Acked-by: Hans Verkuil 
>
> Thanks!
>
> Hans
>
>> ---
>>  drivers/mfd/cros_ec_dev.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index eafd06f..57064ec 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -383,6 +383,18 @@ static void cros_ec_sensors_register(struct cros_ec_dev 
>> *ec)
>>   kfree(msg);
>>  }
>>
>> +static void cros_ec_cec_register(struct cros_ec_dev *ec)
>> +{
>> + int ret;
>> + struct mfd_cell cec_cell = {
>> + .name = "cros-ec-cec",
>> + };
>> +
>> + ret = mfd_add_devices(ec->dev, 0, _cell, 1, NULL, 0, NULL);
>> + if (ret)
>> + dev_err(ec->dev, "failed to add EC CEC\n");
>> +}
>> +

Do not create a single function to only call mfd_add_devices, instead
do the following on top:

static const struct mfd_cell cros_ec_cec_cells[] = {
{ .name = "cros-ec-cec" }
};


>>  static int ec_device_probe(struct platform_device *pdev)
>>  {
>>   int retval = -ENOMEM;
>> @@ -422,6 +434,10 @@ static int ec_device_probe(struct platform_device *pdev)
>>   if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
>>   cros_ec_sensors_register(ec);
>>
>> + /* check whether this EC handles CEC. */
>> + if (cros_ec_check_features(ec, EC_FEATURE_CEC))
>> + cros_ec_cec_register(ec);
>> +

and use PLATFORM_DEVID_AUTO and the ARRAY_SIZE macro, something like this.

/* Check whether this EC instance handles CEC */
if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
  cros_ec_cec_cells,
  ARRAY_SIZE(cros_ec_cec_cells),
  NULL, 0, NULL);
if (retval)
dev_err(ec->dev, "failed to add cros-ec-cec device: %d\n",
 retval);
}

Best regards,
  Enric

>>   /* Take control of the lightbar from the EC. */
>>   lb_manual_suspend_ctrl(ec, 1);
>>
>>
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 04/11] v4l2-ctrls: add config store support

[Enric Balletbo Serra]

Dear Hans,

2014-11-17 9:46 GMT+01:00 Hans Verkuil :
> On 11/14/2014 04:44 PM, Sakari Ailus wrote:
>> Hi Hans,
>>
>> Some comments below.
>>
>> On Sun, Sep 21, 2014 at 04:48:22PM +0200, Hans Verkuil wrote:
>>> From: Hans Verkuil 
>>>
>>> Signed-off-by: Hans Verkuil 
>>> ---
>>>  drivers/media/v4l2-core/v4l2-ctrls.c | 150 
>>> +--
>>>  drivers/media/v4l2-core/v4l2-ioctl.c |   4 +-
>>>  include/media/v4l2-ctrls.h   |  14 
>>>  3 files changed, 141 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index 35d1f3d..df0f3ee 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -1478,6 +1478,15 @@ static int cur_to_user(struct v4l2_ext_control *c,
>>>  return ptr_to_user(c, ctrl, ctrl->p_cur);
>>>  }
>>>
>>> +/* Helper function: copy the store's control value back to the caller */
>>> +static int store_to_user(struct v4l2_ext_control *c,
>>> +   struct v4l2_ctrl *ctrl, unsigned store)
>>> +{
>>> +if (store == 0)
>>> +return ptr_to_user(c, ctrl, ctrl->p_new);
>>> +return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
>>> +}
>>> +
>>>  /* Helper function: copy the new control value back to the caller */
>>>  static int new_to_user(struct v4l2_ext_control *c,
>>> struct v4l2_ctrl *ctrl)
>>> @@ -1585,6 +1594,14 @@ static void new_to_cur(struct v4l2_fh *fh, struct 
>>> v4l2_ctrl *ctrl, u32 ch_flags)
>>>  }
>>>  }
>>>
>>> +/* Helper function: copy the new control value to the store */
>>> +static void new_to_store(struct v4l2_ctrl *ctrl)
>>> +{
>>> +/* has_changed is set by cluster_changed */
>>> +if (ctrl && ctrl->has_changed)
>>> +ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
>>> +}
>>> +
>>>  /* Copy the current value to the new value */
>>>  static void cur_to_new(struct v4l2_ctrl *ctrl)
>>>  {
>>> @@ -1603,13 +1620,18 @@ static int cluster_changed(struct v4l2_ctrl *master)
>>>
>>>  for (i = 0; i < master->ncontrols; i++) {
>>>  struct v4l2_ctrl *ctrl = master->cluster[i];
>>> +union v4l2_ctrl_ptr ptr;
>>>  bool ctrl_changed = false;
>>>
>>>  if (ctrl == NULL)
>>>  continue;
>>> +if (ctrl->store)
>>> +ptr = ctrl->p_stores[ctrl->store - 1];
>>> +else
>>> +ptr = ctrl->p_cur;
>>>  for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
>>>  ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
>>> -ctrl->p_cur, ctrl->p_new);
>>> +ptr, ctrl->p_new);
>>>  ctrl->has_changed = ctrl_changed;
>>>  changed |= ctrl->has_changed;
>>>  }
>>> @@ -1740,6 +1762,13 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler 
>>> *hdl)
>>>  list_del(>node);
>>>  list_for_each_entry_safe(sev, next_sev, >ev_subs, node)
>>>  list_del(>node);
>>> +if (ctrl->p_stores) {
>>> +unsigned s;
>>> +
>>> +for (s = 0; s < ctrl->nr_of_stores; s++)
>>> +kfree(ctrl->p_stores[s].p);
>>> +}
>>> +kfree(ctrl->p_stores);
>>>  kfree(ctrl);
>>>  }
>>>  kfree(hdl->buckets);
>>> @@ -1970,7 +1999,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct 
>>> v4l2_ctrl_handler *hdl,
>>>  handler_set_err(hdl, -ERANGE);
>>>  return NULL;
>>>  }
>>> -if (is_array &&
>>> +if ((is_array || (flags & V4L2_CTRL_FLAG_CAN_STORE)) &&
>>>  (type == V4L2_CTRL_TYPE_BUTTON ||
>>>   type == V4L2_CTRL_TYPE_CTRL_CLASS)) {
>>>  handler_set_err(hdl, -EINVAL);
>>> @@ -2084,8 +2113,10 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct 
>>> v4l2_ctrl_handler *hdl,
>>>  is_menu ? cfg->menu_skip_mask : step, def,
>>>  cfg->dims, cfg->elem_size,
>>>  flags, qmenu, qmenu_int, priv);
>>> -if (ctrl)
>>> +if (ctrl) {
>>
>> I think it'd be cleaner to return NULL here if ctrl == NULL. Up to you.
>
> Agreed.
>
>>
>>>  ctrl->is_private = cfg->is_private;
>>> +ctrl->can_store = cfg->can_store;
>>> +}
>>>  return ctrl;
>>>  }
>>>  EXPORT_SYMBOL(v4l2_ctrl_new_custom);
>>> @@ -2452,6 +2483,7 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler 
>>> *hdl)
>>>  cur_to_new(master->cluster[i]);
>>>  master->cluster[i]->is_new = 1;
>>>  master->cluster[i]->done = true;
>>> +master->cluster[i]->store = 0;
>>>  }
>>>  }
>>>  ret = 

Re: [RFC PATCH 04/11] v4l2-ctrls: add config store support

[Enric Balletbo Serra]

2015-12-02 13:33 GMT+01:00 Hans Verkuil <hverk...@xs4all.nl>:
> On 12/02/15 13:03, Enric Balletbo Serra wrote:
>> Dear Hans,
>>
>> We've a driver that uses your confstore stuff and we'd like to push
>> upstream. I'm wondering if there is any plan to upstream the confstore
>> patches or if this was abandoned for some reason. Thanks
>
> Ouch, that's really old code you're using.
>
> The latest version is here:
>
> http://git.linuxtv.org/hverkuil/media_tree.git/log/?h=requests
>
> But that too won't be the final version.
>
> There is still work going on in this area (specifically by Laurent Pinchart)
> since we really need this functionality. But we need to make sure that the
> API is good enough to handle complex hardware before it can be upstreamed.
>
> I suspect that once Laurent has it working for his non-trivial use-case we
> can start looking at upstreaming it.
>
> I recommend rebasing to at least the version in my git tree as that will
> be much closer to the final version. I'll try to rebase that branch to
> the latest kernel, but that's a bit difficult and takes more time than I
> have at the moment.
>

Thanks to point me in the right direction.

Regards,
Enric

> Regards,
>
> Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL FOR v3.16] mt9p031 fixes

[Enric Balletbo Serra]

Hi Laurent,

2014-07-10 0:29 GMT+02:00 Laurent Pinchart laurent.pinch...@ideasonboard.com:
 Hi Enric,

 On Wednesday 09 July 2014 17:56:59 Enric Balletbo Serra wrote:
 2014-05-16 2:45 GMT+02:00 Laurent Pinchart wrote:
  Hi Mauro,
 
  The following changes since commit
  ba0d342ecc21fbbe2f6c178f4479944d1fb34f3b:
saa7134-alsa: include vmalloc.h (2014-05-13 23:05:15 -0300)
 
  are available in the git repository at:
git://linuxtv.org/pinchartl/media.git sensors/next
 
  for you to fetch changes up to a3a7145c6cecbd9752311b8ae1e431f6755ad5f3:
mt9p031: Fix BLC configuration restore when disabling test pattern
 
  (2014-05-16 02:43:50 +0200)
 
  
 
  Laurent Pinchart (2):
mt9p031: Really disable Black Level Calibration in test pattern mode
mt9p031: Fix BLC configuration restore when disabling test pattern
 
   drivers/media/i2c/mt9p031.c | 53 ++--
 
   1 file changed, 39 insertions(+), 14 deletions(-)

 I'm trying to test omap3-isp and a board with mt9p031 sensor with
 current mainline. For now I'm using tag version 3.15 (which is close
 to current mainline). First, when I tried to use the test patterns I
 only saw a black screen, but after applying these two patches I saw an
 improvement, although I can see the test pattern correctly.

 After some modifications the subdevs_group for my board is as follows:

 +static struct isp_v4l2_subdevs_group igep00x0_camera_subdevs[] = {
 +   {
 +   .subdevs = cam0020_primary_subdevs,
 +   .interface = ISP_INTERFACE_PARALLEL,
 +   .bus = {
 +   .parallel = {
 +   /* CAM[11:0] */
 +   .data_lane_shift = ISP_LANE_SHIFT_2,
 +   /* Sample on falling edge */
 +   .clk_pol = 1,
 +   }
 +   },
 +   },
 +   { },
 +};

 As I have some problems I would ask some questions, maybe you can help me.

 In the past in the data_lane_shift was ISP_LANE_SHIFT_0, but now, it
 seems I should to use ISP_LANE_SHIFT_2 (CAM[11:0] - as I saw in the
 include file). ISP_LANE_SHIFT_0 is for CAM[13:0] but OMAP3 has only 12
 data bus signals. Is that right ?

 Not really. The CCDC input is actually 16 bits wide. The ISP parallel bus is
 limited to 12 bits, the CSI2 receivers output up to 14 bits, and the bridge
 can merge two 8-bit samples into a 16-bit sample for YUV formats.

 When using a 12 bit parallel sensor, unless you want to restrict the dynamic
 of the input image, you should use a data lane shift value of 0. This will
 cause the CAMEXT[13:0] signal to be mapped to CAM[13:0]. As the parallel bus
 is limited to 12 bits, the CAM[13:12] bits will be set to zero. When capturing
 from the CCDC output to memory each pixel will be stored on 16 bits, with bits
 [15:12] set to zero, and bits [11:0] containing image data. When forwarding
 data to the preview engine, which has an input width of 10 bits, the ISP
 driver will configure the CCDC video port to output bits [11:2] to the preview
 engine, dropping the two LSBs.


Clear now, thank you for the explanations ...

 Another thing is I'm not able to capture the image correctly, also if
 if configure to ouput a test pattern, doesn't looks good. See as
 example [1] and [2]. Do you know what could be the problem ?

 For your information these are the pipeline that I'm using :

   media-ctl -v -r -l 'mt9p031 1-005d:0-OMAP3 ISP CCDC:0[1],
 OMAP3 ISP CCDC:2-OMAP3 ISP preview:0[1], OMAP3 ISP
 preview:1-OMAP3 ISP resizer:0[1], OMAP3 ISP resizer:1-OMAP3
 ISP resizer output:0[1]'

   media-ctl -v -f 'mt9p031 1-005d:0[SGRBG12 720x480], OMAP3 ISP
 CCDC:2[SGRBG8 720x480], OMAP3 ISP preview:1[UYVY 720x480], OMAP3
 ISP resizer:1[UYVY 720x480]'

 I would configure the pipeline with SGRBG10 at the output of the CCDC. The
 resolutions you request through the pipeline can't be achieved exactly, as the
 sensor can only perform binning/skipping to downscale. The resizer will take
 care to scale the image to the requested 720x480, but it will get distorted.
 You should use media-ctl -p to see what resolutions the above command actually
 sets, and fix the configuration with appropriate cropping if you want to keep
 the sensor aspect ratio intact.


Right. with configuration fixed works as expected. Thanks :)

 # Set Vertical Color Bars as test pattern
   yavta -w '0x009f0903 9' /dev/v4l-subdev8

 # Capture data with
   yavta  -f UYVY -s 720x480 --capture=5 --skip=1 --file=image-# /dev/video6

 # And convert with
   raw2rgbpnm -s 720x480 image-0.uyuv image-0.pnm

 Thanks in advance and any help will be appreciate.

 Regards,
   Enric

 [1] http://downloads.isee.biz/pub/files/tmp/9-Vertical Color Bars.pnm
 [2] http://downloads.isee.biz/pub/files/tmp/9-Vertical Color Bars.uyvy

 --
 Regards,

 Laurent Pinchart


Best regards,
   Enric

Re: [GIT PULL FOR v3.16] mt9p031 fixes

[Enric Balletbo Serra]

Hi Laurent,

2014-05-16 2:45 GMT+02:00 Laurent Pinchart laurent.pinch...@ideasonboard.com:
 Hi Mauro,

 The following changes since commit ba0d342ecc21fbbe2f6c178f4479944d1fb34f3b:

   saa7134-alsa: include vmalloc.h (2014-05-13 23:05:15 -0300)

 are available in the git repository at:

   git://linuxtv.org/pinchartl/media.git sensors/next

 for you to fetch changes up to a3a7145c6cecbd9752311b8ae1e431f6755ad5f3:

   mt9p031: Fix BLC configuration restore when disabling test pattern
 (2014-05-16 02:43:50 +0200)

 
 Laurent Pinchart (2):
   mt9p031: Really disable Black Level Calibration in test pattern mode
   mt9p031: Fix BLC configuration restore when disabling test pattern

  drivers/media/i2c/mt9p031.c | 53
 +++--
  1 file changed, 39 insertions(+), 14 deletions(-)


I'm trying to test omap3-isp and a board with mt9p031 sensor with
current mainline. For now I'm using tag version 3.15 (which is close
to current mainline). First, when I tried to use the test patterns I
only saw a black screen, but after applying these two patches I saw an
improvement, although I can see the test pattern correctly.

After some modifications the subdevs_group for my board is as follows:

+static struct isp_v4l2_subdevs_group igep00x0_camera_subdevs[] = {
+   {
+   .subdevs = cam0020_primary_subdevs,
+   .interface = ISP_INTERFACE_PARALLEL,
+   .bus = {
+   .parallel = {
+   /* CAM[11:0] */
+   .data_lane_shift = ISP_LANE_SHIFT_2,
+   /* Sample on falling edge */
+   .clk_pol = 1,
+   }
+   },
+   },
+   { },
+};

As I have some problems I would ask some questions, maybe you can help me.

In the past in the data_lane_shift was ISP_LANE_SHIFT_0, but now, it
seems I should to use ISP_LANE_SHIFT_2 (CAM[11:0] - as I saw in the
include file). ISP_LANE_SHIFT_0 is for CAM[13:0] but OMAP3 has only 12
data bus signals. Is that right ?

Another thing is I'm not able to capture the image correctly, also if
if configure to ouput a test pattern, doesn't looks good. See as
example [1] and [2]. Do you know what could be the problem ?

For your information these are the pipeline that I'm using :

  media-ctl -v -r -l 'mt9p031 1-005d:0-OMAP3 ISP CCDC:0[1],
OMAP3 ISP CCDC:2-OMAP3 ISP preview:0[1], OMAP3 ISP
preview:1-OMAP3 ISP resizer:0[1], OMAP3 ISP resizer:1-OMAP3
ISP resizer output:0[1]'

  media-ctl -v -f 'mt9p031 1-005d:0[SGRBG12 720x480], OMAP3 ISP
CCDC:2[SGRBG8 720x480], OMAP3 ISP preview:1[UYVY 720x480], OMAP3
ISP resizer:1[UYVY 720x480]'

# Set Vertical Color Bars as test pattern
  yavta -w '0x009f0903 9' /dev/v4l-subdev8

# Capture data with
  yavta  -f UYVY -s 720x480 --capture=5 --skip=1 --file=image-# /dev/video6

# And convert with
  raw2rgbpnm -s 720x480 image-0.uyuv image-0.pnm

Thanks in advance and any help will be appreciate.

Regards,
  Enric

[1] http://downloads.isee.biz/pub/files/tmp/9-Vertical Color Bars.pnm
[2] http://downloads.isee.biz/pub/files/tmp/9-Vertical Color Bars.uyvy


 --
 Regards,

 Laurent Pinchart

 --
 To unsubscribe from this list: send the line unsubscribe linux-media in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

omap3isp: possible circular locking dependency

[Enric Balletbo Serra]

Hi all,

I've a problem running OMAP3 ISP with current 3.9-rc4. I tried to run
the Laurent's live application to capture data from my mt9v034 sensor
but kernel shows a  possible circular locking dependency. Also the
captured images are wrong and I see garbage. The same environment
worked for me with kernel 3.7. Anyone knows any issue related to this
? Anyone experimented something similar with other sensors ? I tried
to find something in ML and Laurent's git repository but I don't see
anything. Thanks in advance.

Here is the log:

~# live
No compatible input device found, disabling digital zoom
32 bpp
Device /dev/video7 opened: omap_vout ().
/dev/video7: 3 buffers requested.
/dev/video7: buffer 0 mapped at address 0xb6df1000.
/dev/video7: buffer 1 mapped at address 0xb6c71000.
/dev/video7: buffer 2 mapped at address 0xb6af1000.
Device /dev/video6 opened: OMAP3 ISP resizer output (media).
viewfinder configured for 2011 1024x768
/dev/video6: 3 buffers requested.
/dev/video6: buffer 0 valid.
/dev/video6: buffer 1 valid.
/dev/video6: buffer 2 valid.
Device /dev/video6 opened: OMAP3 ISP resizer output (media).
/dev/video6: 2 buffers requested.
/dev/video6: buffer 0 mapped at address 0xb64f1000.
/dev/video6: buffer 1 mapped at address 0xb5ef1000.
snapshot configured for 2011 2048x1536
Device /[   63.557861]
[   63.560577] ==
[   63.567077] [ INFO: possible circular locking dependency detected ]
[   63.573669] 3.9.0-rc4-00152-gba9ce12 #4 Not tainted
[   63.578796] ---
[   63.585388] live/1273 is trying to acquire lock:
[   63.590209]  (mm-mmap_sem){++}, at: [bf06fb24]
omap3isp_video_queue_qbuf+0x280/0x7a4 [omap3_isp]
[   63.600280]
[   63.600280] but task is already holding lock:
[   63.606414]  (queue-lock){+.+.+.}, at: [bf06f8d4]
omap3isp_video_queue_qbuf+0x30/0x7a4 [omap3_isp]
[   63.616271]
[   63.616271] which lock already depends on the new lock.
[   63.616271]
[   63.624877]
[   63.624877] the existing dependency chain (in reverse order) is:
[   63.632751]
- #1 (queue-lock){+.+.+.}:
[   63.637207][c0081948] lock_acquire+0x94/0x100
[   63.642700][c04f02b0] mutex_lock_nested+0x40/0x298
[   63.648681][bf0703a0] omap3isp_video_queue_mmap+0x1c/0xe8
[omap3_isp]
[   63.656372][bf02117c] v4l2_mmap+0x54/0x88 [videodev]
[   63.662597][c00dc740] mmap_region+0x2e0/0x520
[   63.668090][c00dcc38] do_mmap_pgoff+0x2b8/0x340
[   63.673767][c00cdd1c] vm_mmap_pgoff+0x84/0xac
[   63.679260][c00db4a8] sys_mmap_pgoff+0x54/0xb0
[   63.684875][c0013660] ret_fast_syscall+0x0/0x3c
[   63.690551]
- #0 (mm-mmap_sem){++}:
[   63.695068][c0080e28] __lock_acquire+0x14bc/0x1ae8
[   63.701019][c0081948] lock_acquire+0x94/0x100
[   63.706512][c04f095c] down_read+0x30/0x40
[   63.711639][bf06fb24]
omap3isp_video_queue_qbuf+0x280/0x7a4 [omap3_isp]
[   63.719543][bf025ca4] v4l_qbuf+0x3c/0x40 [videodev]
[   63.725646][bf024ba8] __video_do_ioctl+0x240/0x33c [videodev]
[   63.732635][bf025668] video_usercopy+0x114/0x40c [videodev]
[   63.739440][bf0215c0] v4l2_ioctl+0xfc/0x144 [videodev]
[   63.745758][c00fe954] do_vfs_ioctl+0x7c/0x5ac
[   63.751281][c00feee8] sys_ioctl+0x64/0x84
[   63.756408][c0013660] ret_fast_syscall+0x0/0x3c
[   63.762084]
[   63.762084] other info that might help us debug this:
[   63.762084]
[   63.770507]  Possible unsafe locking scenario:
[   63.770507]
[   63.776702]CPU0CPU1
[   63.781463]
[   63.786224]   lock(queue-lock);
[   63.789733]lock(mm-mmap_sem);
[   63.795959]lock(queue-lock);
[   63.802124]   lock(mm-mmap_sem);
[   63.805694]
[   63.805694]  *** DEADLOCK ***
[   63.805694]
[   63.811950] 1 lock held by live/1273:
[   63.815795]  #0:  (queue-lock){+.+.+.}, at: [bf06f8d4]
omap3isp_video_queue_qbuf+0x30/0x7a4 [omap3_isp]
[   63.826141]
[   63.826141] stack backtrace:
[   63.830749] [c00196d4] (unwind_backtrace+0x0/0xf0) from
[c04eb478] (print_circular_bug+0x25c/0x2a8)
[   63.840637] [c04eb478] (print_circular_bug+0x25c/0x2a8) from
[c0080e28] (__lock_acquire+0x14bc/0x1ae8)
[   63.850769] [c0080e28] (__lock_acquire+0x14bc/0x1ae8) from
[c0081948] (lock_acquire+0x94/0x100)
[   63.860290] [c0081948] (lock_acquire+0x94/0x100) from
[c04f095c] (down_read+0x30/0x40)
[   63.869018] [c04f095c] (down_read+0x30/0x40) from [bf06fb24]
(omap3isp_video_queue_qbuf+0x280/0x7a4 [omap3_isp])
[   63.880126] [bf06fb24] (omap3isp_video_queue_qbuf+0x280/0x7a4
[omap3_isp]) from [bf025ca4] (v4l_qbuf+0x3c/0x40 [videodev])
[   63.892181] [bf025ca4] (v4l_qbuf+0x3c/0x40 [videodev]) from
[bf024ba8] (__video_do_ioctl+0x240/0x33c [videodev])
[   63.903289] [bf024ba8] (__video_do_ioctl+0x240/0x33c [videodev])
from [bf025668] 

Re: [PATCH] [media] v4l: Add mt9v034 sensor driver

[Enric Balletbo Serra]

Hi all,

Thanks for all your comments

2012/12/2 Sakari Ailus sakari.ai...@iki.fi:
 Hi Enric,

 Thanks for the patch. (Removing my old e-mail address from cc.)

 On Fri, Oct 05, 2012 at 12:34:47PM +0200, Enric Balletbo i Serra wrote:
 From: Enric Balletbo i Serra eballe...@iseebcn.com

 The MT9V034 is a parallel wide VGA sensor from Aptina (formerly Micron)
 controlled through I2C.

 The driver creates a V4L2 subdevice. It currently supports binning and
 cropping, and the gain, auto gain, exposure, auto exposure and test
 pattern controls.

 The following patch is based on the MT9V032 driver from Laurent Pinchart
 and was tested on IGEP tech based boards with custom expansion board with
 MT9V034 sensor.

 How different is this from the mt9v032 sensor driver? How different are the
 two sensors? Could the mt9v032 driver be used on mt9v034 as well?


Well, are similar. Did you thinking that could be merged with mt9v032 ?

My first attempt was use the mt9v032, but there was some registers
that were different and finally I decided
to use a new file to not abuse of if sentence.

IMHO, is more clear use a separate driver, but if you prefer I can
merge with mt9v032.

 Signed-off-by: Enric Balletbo i Serra eballe...@iseebcn.com
 ---
  drivers/media/i2c/Kconfig   |   10 +
  drivers/media/i2c/Makefile  |1 +
  drivers/media/i2c/mt9v034.c |  834 
 +++
  include/media/mt9v034.h |   15 +
  4 files changed, 860 insertions(+), 0 deletions(-)
  create mode 100644 drivers/media/i2c/mt9v034.c
  create mode 100644 include/media/mt9v034.h

 diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
 index 0e0793a..c35efda 100644
 --- a/drivers/media/i2c/Kconfig
 +++ b/drivers/media/i2c/Kconfig
 @@ -475,6 +475,16 @@ config VIDEO_MT9V032
 This is a Video4Linux2 sensor-level driver for the Micron
 MT9V032 752x480 CMOS sensor.

 +config VIDEO_MT9V034
 + tristate Micron MT9V034 sensor support
 + depends on I2C  VIDEO_V4L2  VIDEO_V4L2_SUBDEV_API
 + depends on MEDIA_CAMERA_SUPPORT
 + ---help---
 +   This is a Video4Linux2 sensor-level driver for the Micron
 +   MT9V034 752x480 CMOS sensor. The MT9V034 is a 1/3-inch
 +   wide-VGA format CMOS active-pixel digital image sensor with
 +   TrueSNAP gobal shutter and high dynamic range (HDR) operation.
 +
  config VIDEO_TCM825X
   tristate TCM825x camera sensor support
   depends on I2C  VIDEO_V4L2
 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
 index 4a270f7..9d4c417 100644
 --- a/drivers/media/i2c/Makefile
 +++ b/drivers/media/i2c/Makefile
 @@ -54,6 +54,7 @@ obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
  obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o
  obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
  obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
 +obj-$(CONFIG_VIDEO_MT9V034) += mt9v034.o
  obj-$(CONFIG_VIDEO_SR030PC30)+= sr030pc30.o
  obj-$(CONFIG_VIDEO_NOON010PC30)  += noon010pc30.o
  obj-$(CONFIG_VIDEO_S5K6AA)   += s5k6aa.o
 diff --git a/drivers/media/i2c/mt9v034.c b/drivers/media/i2c/mt9v034.c
 new file mode 100644
 index 000..7bbfeb6
 --- /dev/null
 +++ b/drivers/media/i2c/mt9v034.c
 @@ -0,0 +1,834 @@
 +/*
 + * Driver for MT9V034 CMOS Image Sensor from Micron
 + *
 + * Copyright (C) 2012, Enric Balletbo eballe...@iseebcn.com
 + *
 + * Based on the MT9V032 driver,
 + *
 + * Copyright (C) 2010, Laurent Pinchart laurent.pinch...@ideasonboard.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + */
 +
 +#include linux/delay.h
 +#include linux/i2c.h
 +#include linux/log2.h
 +#include linux/mutex.h
 +#include linux/slab.h
 +#include linux/videodev2.h
 +#include linux/v4l2-mediabus.h
 +#include linux/module.h
 +
 +#include media/mt9v034.h
 +#include media/v4l2-ctrls.h
 +#include media/v4l2-device.h
 +#include media/v4l2-subdev.h
 +
 +#define MT9V034_PIXEL_ARRAY_HEIGHT   499
 +#define MT9V034_PIXEL_ARRAY_WIDTH809
 +
 +#define MT9V034_SYSCLK_FREQ_DEF  2660

 Isn't this purely board/SoC dependent? Typically sensors can accept a range
 instead.


No, is the master clock (SYSCLK) default value from sensor, although
the master clock range is form 13MHz to 27MHz

 +
 +#define MT9V034_CHIP_VERSION 0x00
 +#define  MT9V034_CHIP_ID_REV10x1324
 +#define MT9V034_COLUMN_START 0x01
 +#define  MT9V034_COLUMN_START_MIN1
 +#define  MT9V034_COLUMN_START_DEF1
 +#define  MT9V034_COLUMN_START_MAX752
 +#define MT9V034_ROW_START0x02
 +#define  MT9V034_ROW_START_MIN   4
 +#define  MT9V034_ROW_START_DEF   4
 +#define  

Re: Using omap3-isp-live example application on beagleboard with DVI

[Enric Balletbo Serra]

Hi Laurent,

2012/10/19 Laurent Pinchart laurent.pinch...@ideasonboard.com:
 Hi Enric,

 On Wednesday 17 October 2012 11:35:37 Enric Balletbo Serra wrote:
 2012/10/17 Laurent Pinchart laurent.pinch...@ideasonboard.com:

 [snip]

  Instead of failing what would be more interesting would be to get the
  application to work in 16bpp mode as well. For that you will need to paint
  the frame buffer with a 16bpp color, and set the colorkey to the same
  value. Would you be able to try that ?

 New patch attached, comments are welcome as I'm newbie with video devices.

 Thank you for the patch. In the future could you please send patches inline
 instead of attached (git send-email is a very useful tool for that) ? It would
 make review easier.


Ok, I'll do.

 You can get the bpp value directly from the frame buffer API without going
 through sysfs. I've modified your patch accordingly, have added support for
 24bpp as well and pushed the result to the repository.


Cool, I liked your patch, today I learned a bit more.


Regards,

Enric Balletbo
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Using omap3-isp-live example application on beagleboard with DVI

[Enric Balletbo Serra]

Hi Laurent,

2012/10/17 Laurent Pinchart laurent.pinch...@ideasonboard.com:
 Hi Enric,

 On Monday 15 October 2012 14:03:20 Enric Balletbo Serra wrote:
 2012/10/11 Laurent Pinchart laurent.pinch...@ideasonboard.com:
  On Thursday 11 October 2012 10:14:26 Enric Balletbò i Serra wrote:
  2012/10/10 Enric Balletbò i Serra eballe...@gmail.com:
   2012/9/6 John Weber rjohnwe...@gmail.com:
   Hello,
  
   My goal is to better understand how to write an application that makes
   use of the omap3isp and media controller frameworks and v4l2.  I'm
   attempting to make use of Laurent's omap3-isp-live example application
   as a starting point and play with the AEC/WB capability.
  
   My problem is that when I start the live application, the display
   turns blue (it seems when the chromakey fill is done), but no video
   appears on the display.  I do think that I'm getting good (or at least
   statistics) from the ISP because I can change the view in front of the
   camera (by putting my hand in front of the lens) and the gain settings
   change.

 [snip]

   I've exactly the same problem. Before try to debug the problem I would
   like to know if you solved the problem. Did you solved ?
 
  The first change I made and worked (just luck). I made the following
  change:
 
  -   vo_enable_colorkey(vo, 0x123456);
  +   // vo_enable_colorkey(vo, 0x123456);
 
  And now the live application works like a charm. Seems this function
  enables a chromakey and the live application can't paint over the
  chromakey. Laurent, just to understand what I did, can you explain
  this ? Thanks.
 
  My guess is that the live application fails to paint the frame buffer with
  the color key. If fb_init() failed the live application would stop, so
  the function succeeds. My guess is thus that the application either
  paints the wrong frame buffer (how many /dev/fb* devices do you have on
  your system ?),

 I checked again and no, it opens the correct framebuffer.

  or paints it with the wrong color. The code assumes that the frame buffer
  is configured in 32 bit, maybe that's not the case on your system ?

 This was my problem, and I suspect it's the John problem. My system was
 configured in 16 bit instead of 32 bit.

 FYI, I made a patch that adds this check to the live application. I did not
 know where send the patch so I attached to this email.

 Thank you for the patch.

 Instead of failing what would be more interesting would be to get the
 application to work in 16bpp mode as well. For that you will need to paint the
 frame buffer with a 16bpp color, and set the colorkey to the same value. Would
 you be able to try that ?


New patch attached, comments are welcome as I'm newbie with video devices.

Regards,
Enric


0001-live-Get-the-application-to-work-in-16bpp-mode.patch
Description: Binary data

Re: Using omap3-isp-live example application on beagleboard with DVI

[Enric Balletbo Serra]

Hi Laurent,

2012/10/11 Laurent Pinchart laurent.pinch...@ideasonboard.com:
 Hi Enric,

 On Thursday 11 October 2012 10:14:26 Enric Balletbò i Serra wrote:
 2012/10/10 Enric Balletbò i Serra eballe...@gmail.com:
  2012/9/6 John Weber rjohnwe...@gmail.com:
  Hello,
 
  My goal is to better understand how to write an application that makes
  use of the omap3isp and media controller frameworks and v4l2.  I'm
  attempting to make use of Laurent's omap3-isp-live example application as
  a starting point and play with the AEC/WB capability.
 
  My problem is that when I start the live application, the display turns
  blue (it seems when the chromakey fill is done), but no video appears on
  the display.  I do think that I'm getting good (or at least statistics)
  from the ISP because I can change the view in front of the camera (by
  putting my hand in front of the lens) and the gain settings change.
 
  root@beagleboard:~# live
  Device /dev/video6 opened: OMAP3 ISP resizer output (media).
  viewfinder configured for 2011 1024x768
  AEWB: #win 10x7 start 16x74 size 256x256 inc 30x30
  Device /dev/video7 opened: omap_vout ().
  3 buffers requested.
  Buffer 0 mapped at address 0x40279000.
  Buffer 1 mapped at address 0x40402000.
  Buffer 2 mapped at address 0x4059e000.
  3 buffers requested.
  Buffer 0 valid.
  Buffer 1 valid.
  Buffer 2 valid.
  AE: factor 3.1250 exposure 2000 sensor gain 12
  AE: factor 1.6018 exposure 2000 sensor gain 19
  AE: factor 1.1346 exposure 2000 sensor gain 21
  AE: factor 1.0446 exposure 2000 sensor gain 21
  AE: factor 1.0448 exposure 2000 sensor gain 21
  AE: factor 1.0444 exposure 2000 sensor gain 21
  AE: factor 1.0443 exposure 2000 sensor gain 21
  AE: factor 1.0445 exposure 2000 sensor gain 21
  AE: factor 1.0438 exposure 2000 sensor gain 21
  AE: factor 1.0448 exposure 2000 sensor gain 21
  AE: factor 1.0461 exposure 2000 sensor gain 21
  AE: factor 1.0897 exposure 2000 sensor gain 22
  AE: factor 2.6543 exposure 2000 sensor gain 58  Me obstructing
  the camera FOV using my hand causes the factor and gain to rise
  AE: factor 1.2345 exposure 2000 sensor gain 71   
  AE: factor 1.1631 exposure 2000 sensor gain 82   
  AE: factor 0.9797 exposure 2000 sensor gain 80   
  AE: factor 0.9709 exposure 2000 sensor gain 77   
  frame rate: 6.597745 fps
  AE: factor 0.9633 exposure 2000 sensor gain 74   
  AE: factor 0.6130 exposure 2000 sensor gain 45   
  AE: factor 0.9271 exposure 2000 sensor gain 41   
  AE: factor 1.0130 exposure 2000 sensor gain 41   
  AE: factor 1.0504 exposure 2000 sensor gain 43   
  AE: factor 1.0411 exposure 2000 sensor gain 44   
  AE: factor 1.0271 exposure 2000 sensor gain 45   
  AE: factor 1.0602 exposure 2000 sensor gain 47   
  AE: factor 1.1278 exposure 2000 sensor gain 53   
  AE: factor 1.1870 exposure 2000 sensor gain 62   
  AE: factor 1.1074 exposure 2000 sensor gain 68   
  AE: factor 1.0716 exposure 2000 sensor gain 72   
  AE: factor 0.4074 exposure 2000 sensor gain 29   
  AE: factor 0.8033 exposure 2000 sensor gain 23
  AE: factor 0.9741 exposure 2000 sensor gain 22
  AE: factor 1.0115 exposure 2000 sensor gain 22
 
  I did have to change the omap_vout driver slightly to increase the buffer
  size.  I was getting errors in the application attempted to allocate
  USERPTR buffers for 1024x768 frames:
 
  root@beagleboard:~# live
  Device /dev/video6 opened: OMAP3 ISP resizer output (media).
  viewfinder configured for 2011 1024x768
  AEWB: #win 10x7 start 16x74 size 256x256 inc 30x30
  Device /dev/video7 opened: omap_vout ().
  3 buffers requested.
  Buffer 0 mapped at address 0x40302000.
  Buffer 1 mapped at address 0x404df000.
  Buffer 2 mapped at address 0x4066e000.
  3 buffers requested.
  Buffer 0 too small (1572864 bytes required, 1474560 bytes available.)
 
  So, I changed drivers/media/video/omap/omap_voutdef.h to increase the
  buffer size slightly.
 
  /* Max Resolution supported by the driver */
  #define VID_MAX_WIDTH   1280/* Largest width */
  #define VID_MAX_HEIGHT  768 /* Largest height */ -- Was 720
 
  I'm pretty sure that wasn't the only way to solve the problem, but it did
  allow the live application to run without errors.
 
  I am using a patched variant of the current Angstrom mainline (3.2.16)
  with the MT9P031 sensor and a DVI display on Beagleboard-xM and am able
  to run the following commands and see a live video stream on the display.
  I suspect that this indicates that hardware setup works:
 
  media-ctl -v -r -l 'mt9p031 2-0048:0-OMAP3 ISP CCDC:0[1], OMAP3 ISP
  CCDC:2-OMAP3 ISP preview:0[1], OMAP3 ISP preview:1-OMAP3 ISP
  resizer:0[1], OMAP3 ISP resizer:1-OMAP3 ISP resizer output:0[1]'
 
  media-ctl -v -f 'mt9p031 2-0048:0 [SGRBG12 1024x768], OMAP3 ISP
  CCDC:2
  [SGRBG10 1024x768], OMAP3 ISP preview:1 [UYVY 10006x760], OMAP3 ISP
  resizer:1 [UYVY 1024x768]'
 
  yavta -f UYVY -s 1024x768 -n 8 --skip 3