Hi Steve, On 01/03/2017 10:57 PM, Steve Longerbeam wrote: > Add the core media driver for i.MX SOC. > > Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx> > --- > Documentation/devicetree/bindings/media/imx.txt | 205 +++++ v2 was sent before getting Rob's review comments, but still they should be addressed in v3. Also I would suggest to separate device tree binding documentation change and place it as the first patch in the series, this should make the following DTS changes valid. > Documentation/media/v4l-drivers/imx.rst | 430 ++++++++++ > drivers/staging/media/Kconfig | 2 + > drivers/staging/media/Makefile | 1 + > drivers/staging/media/imx/Kconfig | 8 + > drivers/staging/media/imx/Makefile | 6 + > drivers/staging/media/imx/TODO | 18 + > drivers/staging/media/imx/imx-media-common.c | 985 ++++++++++++++++++++++ > drivers/staging/media/imx/imx-media-dev.c | 479 +++++++++++ > drivers/staging/media/imx/imx-media-fim.c | 509 +++++++++++ > drivers/staging/media/imx/imx-media-internal-sd.c | 457 ++++++++++ > drivers/staging/media/imx/imx-media-of.c | 291 +++++++ > drivers/staging/media/imx/imx-media-of.h | 25 + > drivers/staging/media/imx/imx-media.h | 299 +++++++ > include/media/imx.h | 15 + > include/uapi/Kbuild | 1 + > include/uapi/linux/v4l2-controls.h | 4 + > include/uapi/media/Kbuild | 2 + > include/uapi/media/imx.h | 30 + Probably Greg should ack the UAPI changes, you may consider to split them into a separate patch. > 19 files changed, 3767 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/imx.txt > create mode 100644 Documentation/media/v4l-drivers/imx.rst > create mode 100644 drivers/staging/media/imx/Kconfig > create mode 100644 drivers/staging/media/imx/Makefile > create mode 100644 drivers/staging/media/imx/TODO > create mode 100644 drivers/staging/media/imx/imx-media-common.c > create mode 100644 drivers/staging/media/imx/imx-media-dev.c > create mode 100644 drivers/staging/media/imx/imx-media-fim.c > create mode 100644 drivers/staging/media/imx/imx-media-internal-sd.c > create mode 100644 drivers/staging/media/imx/imx-media-of.c > create mode 100644 drivers/staging/media/imx/imx-media-of.h > create mode 100644 drivers/staging/media/imx/imx-media.h > create mode 100644 include/media/imx.h > create mode 100644 include/uapi/media/Kbuild > create mode 100644 include/uapi/media/imx.h > [snip] > + > +struct imx_media_subdev * > +imx_media_find_subdev_by_sd(struct imx_media_dev *imxmd, > + struct v4l2_subdev *sd) > +{ > + struct imx_media_subdev *imxsd; > + int i, ret = -ENODEV; > + > + for (i = 0; i < imxmd->num_subdevs; i++) { > + imxsd = &imxmd->subdev[i]; > + if (sd == imxsd->sd) { This can be simplifed: ... if (sd == imxsd->sd) return imxsd; } return ERR_PTR(-ENODEV); > + ret = 0; > + break; > + } > + } > + > + return ret ? ERR_PTR(ret) : imxsd; > +} > +EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_sd); > + > +struct imx_media_subdev * > +imx_media_find_subdev_by_id(struct imx_media_dev *imxmd, u32 grp_id) > +{ > + struct imx_media_subdev *imxsd; > + int i, ret = -ENODEV; > + > + for (i = 0; i < imxmd->num_subdevs; i++) { > + imxsd = &imxmd->subdev[i]; > + if (imxsd->sd && imxsd->sd->grp_id == grp_id) { > + ret = 0; > + break; This can be simplifed: ... if (imxsd->sd && imxsd->sd->grp_id == grp_i) return imxsd; } return ERR_PTR(-ENODEV); > + } > + } > + > + return ret ? ERR_PTR(ret) : imxsd; > +} > +EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_id); > + [snip] > diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c > new file mode 100644 > index 0000000..8d22730 > --- /dev/null > +++ b/drivers/staging/media/imx/imx-media-dev.c > @@ -0,0 +1,479 @@ > +/* > + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC > + * > + * Copyright (c) 2016 Mentor Graphics Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include <linux/module.h> > +#include <linux/delay.h> > +#include <linux/fs.h> > +#include <linux/timer.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/platform_device.h> > +#include <linux/pinctrl/consumer.h> > +#include <linux/of_platform.h> > +#include <media/v4l2-ioctl.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-event.h> > +#include <media/v4l2-mc.h> Please sort out the list of headers alphabetically. > +#include <video/imx-ipu-v3.h> > +#include <media/imx.h> > +#include "imx-media.h" > +#include "imx-media-of.h" > + > +#define DEVICE_NAME "imx-media" I suppose you don't need this macro. [snip] > + */ > +static int imx_media_create_links(struct imx_media_dev *imxmd) > +{ > + struct imx_media_subdev *local_sd; > + struct imx_media_subdev *remote_sd; > + struct v4l2_subdev *source, *sink; > + struct imx_media_link *link; > + struct imx_media_pad *pad; > + u16 source_pad, sink_pad; > + int num_pads, i, j, k; > + int ret = 0; > + > + for (i = 0; i < imxmd->num_subdevs; i++) { > + local_sd = &imxmd->subdev[i]; > + num_pads = local_sd->num_sink_pads + local_sd->num_src_pads; > + > + for (j = 0; j < num_pads; j++) { > + pad = &local_sd->pad[j]; > + > + for (k = 0; k < pad->num_links; k++) { > + link = &pad->link[k]; > + > + remote_sd = imx_media_find_async_subdev( > + imxmd, link->remote_sd_node, > + link->remote_devname); > + if (!remote_sd) { > + v4l2_warn(&imxmd->v4l2_dev, > + "%s: no remote for %s:%d\n", > + __func__, local_sd->sd->name, > + link->local_pad); > + continue; > + } > + > + /* only create the source->sink links */ > + if (pad->pad.flags & MEDIA_PAD_FL_SINK) > + continue; > + > + source = local_sd->sd; > + sink = remote_sd->sd; > + source_pad = link->local_pad; > + sink_pad = link->remote_pad; > + > + v4l2_info(&imxmd->v4l2_dev, > + "%s: %s:%d -> %s:%d\n", __func__, > + source->name, source_pad, > + sink->name, sink_pad); > + > + ret = media_create_pad_link(&source->entity, > + source_pad, > + &sink->entity, > + sink_pad, > + 0); > + if (ret) { > + v4l2_err(&imxmd->v4l2_dev, > + "create_pad_link failed: %d\n", > + ret); > + goto out; Indentation depth is quite terrific. > + } > + } > + } > + } > + > +out: > + return ret; > [snip] > + > +static const struct of_device_id imx_media_dt_ids[] = { > + { .compatible = "fsl,imx-media" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, imx_media_dt_ids); > + > +static struct platform_driver imx_media_pdrv = { > + .probe = imx_media_probe, > + .remove = imx_media_remove, > + .driver = { > + .name = DEVICE_NAME, > + .owner = THIS_MODULE, Setting of .owner is not needed nowadays IIRC. > + .of_match_table = imx_media_dt_ids, > + }, > +}; > + > +module_platform_driver(imx_media_pdrv); > + > +MODULE_DESCRIPTION("i.MX5/6 v4l2 media controller driver"); > +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c > new file mode 100644 > index 0000000..52bfa8d > --- /dev/null > +++ b/drivers/staging/media/imx/imx-media-fim.c > @@ -0,0 +1,509 @@ > +/* > + * Frame Interval Monitor. > + * > + * Copyright (c) 2016 Mentor Graphics Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include <linux/module.h> > +#include <linux/delay.h> > +#include <linux/slab.h> > +#include <linux/platform_device.h> > +#ifdef CONFIG_IMX_GPT_ICAP > +#include <linux/mxc_icap.h> > +#endif This looks clumsy. Include it unconditionally, if needed do #ifdef's inside the header file. > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-of.h> > +#include <media/v4l2-ctrls.h> Please sort out the list alphabetically. > +#include <media/imx.h> > +#include "imx-media.h" > + [snip] > + > +static int of_parse_fim(struct imx_media_fim *fim, struct device_node *np) > +{ > + struct device_node *fim_np; > + u32 val, tol[2], icap[2]; > + int ret; > + > + fim_np = of_get_child_by_name(np, "fim"); > + if (!fim_np) { > + /* set to the default defaults */ > + fim->of_defaults[FIM_CL_ENABLE] = FIM_CL_ENABLE_DEF; > + fim->of_defaults[FIM_CL_NUM] = FIM_CL_NUM_DEF; > + fim->of_defaults[FIM_CL_NUM_SKIP] = FIM_CL_NUM_SKIP_DEF; > + fim->of_defaults[FIM_CL_TOLERANCE_MIN] = > + FIM_CL_TOLERANCE_MIN_DEF; > + fim->of_defaults[FIM_CL_TOLERANCE_MAX] = > + FIM_CL_TOLERANCE_MAX_DEF; > + fim->icap_channel = -1; > + return 0; > + } > + > + ret = of_property_read_u32(fim_np, "enable", &val); > + if (ret) > + val = FIM_CL_ENABLE_DEF; > + fim->of_defaults[FIM_CL_ENABLE] = val; > + > + ret = of_property_read_u32(fim_np, "num-avg", &val); > + if (ret) > + val = FIM_CL_NUM_DEF; > + fim->of_defaults[FIM_CL_NUM] = val; > + > + ret = of_property_read_u32(fim_np, "num-skip", &val); > + if (ret) > + val = FIM_CL_NUM_SKIP_DEF; > + fim->of_defaults[FIM_CL_NUM_SKIP] = val; > + > + ret = of_property_read_u32_array(fim_np, "tolerance-range", tol, 2); > + if (ret) { > + tol[0] = FIM_CL_TOLERANCE_MIN_DEF; > + tol[1] = FIM_CL_TOLERANCE_MAX_DEF; > + } > + fim->of_defaults[FIM_CL_TOLERANCE_MIN] = tol[0]; > + fim->of_defaults[FIM_CL_TOLERANCE_MAX] = tol[1]; > + > + fim->icap_channel = -1; > + if (IS_ENABLED(CONFIG_IMX_GPT_ICAP)) { > + ret = of_property_read_u32_array(fim_np, > + "input-capture-channel", > + icap, 2); > + if (!ret) { > + fim->icap_channel = icap[0]; > + fim->icap_flags = icap[1]; > + } Should you return error otherwise? > + } > + > + of_node_put(fim_np); > + return 0; > +} [snip] > diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c > new file mode 100644 > index 0000000..018d05a > --- /dev/null > +++ b/drivers/staging/media/imx/imx-media-of.c > @@ -0,0 +1,291 @@ > +/* > + * Media driver for Freescale i.MX5/6 SOC > + * > + * Open Firmware parsing. > + * > + * Copyright (c) 2016 Mentor Graphics Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include <linux/of_platform.h> > +#include <media/v4l2-device.h> > +#include <media/videobuf2-dma-contig.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-of.h> > +#include <media/v4l2-ctrls.h> Please sort out the list alphabetically. > +#include <video/imx-ipu-v3.h> > +#include "imx-media.h" > + > +static int of_add_pad_link(struct imx_media_dev *imxmd, > + struct imx_media_pad *pad, > + struct device_node *local_sd_node, > + struct device_node *remote_sd_node, > + int local_pad, int remote_pad) > +{ > + dev_dbg(imxmd->dev, "%s: adding %s:%d -> %s:%d\n", __func__, > + local_sd_node->name, local_pad, > + remote_sd_node->name, remote_pad); > + > + return imx_media_add_pad_link(imxmd, pad, remote_sd_node, NULL, > + local_pad, remote_pad); > +} > + > +/* parse inputs property from a sensor node */ > +static void of_parse_sensor_inputs(struct imx_media_dev *imxmd, > + struct imx_media_subdev *sensor, > + struct device_node *sensor_np) > +{ > + struct imx_media_sensor_input *sinput = &sensor->input; > + int ret, i; > + > + for (i = 0; i < IMX_MEDIA_MAX_SENSOR_INPUTS; i++) { > + const char *input_name; > + u32 val; > + > + ret = of_property_read_u32_index(sensor_np, "inputs", i, &val); > + if (ret) > + break; > + > + sinput->value[i] = val; > + > + ret = of_property_read_string_index(sensor_np, "input-names", > + i, &input_name); > + /* > + * if input-names not provided, they will be set using > + * the subdev name once the sensor is known during > + * async bind > + */ > + if (!ret) > + strncpy(sinput->name[i], input_name, > + sizeof(sinput->name[i])); > + } > + > + sinput->num = i; > + > + /* if no inputs provided just assume a single input */ > + if (sinput->num == 0) > + sinput->num = 1; > +} > + > +static void of_parse_sensor(struct imx_media_dev *imxmd, > + struct imx_media_subdev *sensor, > + struct device_node *sensor_np) > +{ > + struct device_node *endpoint; > + > + of_parse_sensor_inputs(imxmd, sensor, sensor_np); > + > + endpoint = of_graph_get_next_endpoint(sensor_np, NULL); > + if (endpoint) { > + v4l2_of_parse_endpoint(endpoint, &sensor->sensor_ep); > + of_node_put(endpoint); > + } > +} > + > +static int of_get_port_count(const struct device_node *np) > +{ > + struct device_node *child; > + int num = 0; > + > + /* if this node is itself a port, return 1 */ > + if (of_node_cmp(np->name, "port") == 0) > + return 1; > + > + for_each_child_of_node(np, child) { > + if (of_node_cmp(child->name, "port") == 0) > + num++; > + } Unneeded bracers. > + return num; > +} > + > +/* > + * find the remote device node and remote port id (remote pad #) > + * given local endpoint node > + */ > +static void of_get_remote_pad(struct device_node *epnode, > + struct device_node **remote_node, > + int *remote_pad) > +{ > + struct device_node *rp, *rpp; > + struct device_node *remote; > + > + rp = of_graph_get_remote_port(epnode); > + rpp = of_graph_get_remote_port_parent(epnode); > + > + if (of_device_is_compatible(rpp, "fsl,imx6q-ipu")) { > + /* the remote is one of the CSI ports */ > + remote = rp; > + *remote_pad = 0; > + of_node_put(rpp); > + } else { > + remote = rpp; > + of_property_read_u32(rp, "reg", remote_pad); > + of_node_put(rp); > + } > + > + if (!remote || !of_device_is_available(remote)) { > + of_node_put(remote); > + *remote_node = NULL; > + } else { > + *remote_node = remote; > + } > +} > + > +static struct imx_media_subdev * > +of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np, > + bool is_csi_port) > +{ > + struct imx_media_subdev *imxsd; > + int i, num_pads, ret; > + > + if (!of_device_is_available(sd_np)) { > + dev_dbg(imxmd->dev, "%s: %s not enabled\n", __func__, > + sd_np->name); > + return NULL; > + } > + > + /* register this subdev with async notifier */ > + imxsd = imx_media_add_async_subdev(imxmd, sd_np, NULL); > + if (!imxsd) > + return NULL; > + if (IS_ERR(imxsd)) > + return imxsd; if (IS_ERR_OR_NULL(imxsd)) return imxsd; > + > + if (is_csi_port) { > + /* > + * the ipu-csi has one sink port and one source port. > + * The source port is not represented in the device tree, > + * but is described by the internal pads and links later. > + */ > + num_pads = 2; > + imxsd->num_sink_pads = 1; > + } else if (of_device_is_compatible(sd_np, "fsl,imx-mipi-csi2")) { > + num_pads = of_get_port_count(sd_np); > + /* the mipi csi2 receiver has only one sink port */ > + imxsd->num_sink_pads = 1; > + } else if (of_device_is_compatible(sd_np, "imx-video-mux")) { > + num_pads = of_get_port_count(sd_np); > + /* for the video mux, all but the last port are sinks */ > + imxsd->num_sink_pads = num_pads - 1; > + } else { > + /* must be a sensor */ > + num_pads = 1; > + imxsd->num_sink_pads = 0; > + } > + > + if (imxsd->num_sink_pads >= num_pads) > + return ERR_PTR(-EINVAL); > + > + imxsd->num_src_pads = num_pads - imxsd->num_sink_pads; > + > + dev_dbg(imxmd->dev, "%s: %s has %d pads (%d sink, %d src)\n", > + __func__, sd_np->name, num_pads, > + imxsd->num_sink_pads, imxsd->num_src_pads); > + > + if (imxsd->num_sink_pads == 0) { > + /* this might be a sensor */ > + of_parse_sensor(imxmd, imxsd, sd_np); > + } Unneeded bracers. > + > + for (i = 0; i < num_pads; i++) { > + struct device_node *epnode = NULL, *port, *remote_np; > + struct imx_media_subdev *remote_imxsd; > + struct imx_media_pad *pad; > + int remote_pad; Too deep indentation, may be move the cycle body into a separate function? > + > + /* init this pad */ > + pad = &imxsd->pad[i]; > + pad->pad.flags = (i < imxsd->num_sink_pads) ? > + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE; > + > + if (is_csi_port) > + port = (i < imxsd->num_sink_pads) ? sd_np : NULL; > + else > + port = of_graph_get_port_by_id(sd_np, i); > + if (!port) > + continue; > + > + while ((epnode = of_get_next_child(port, epnode))) { Please reuse for_each_child_of_node() here. > + of_get_remote_pad(epnode, &remote_np, &remote_pad); > + if (!remote_np) { > + of_node_put(epnode); Please remove of_node_put() here, of_get_next_child() does it. > + continue; > + } > + > + ret = of_add_pad_link(imxmd, pad, sd_np, remote_np, > + i, remote_pad); > + if (ret) { > + imxsd = ERR_PTR(ret); > + break; > + } > + > + if (i < imxsd->num_sink_pads) { > + /* follow sink endpoints upstream */ > + remote_imxsd = of_parse_subdev(imxmd, > + remote_np, > + false); > + if (IS_ERR(remote_imxsd)) { > + imxsd = remote_imxsd; > + break; > + } > + } > + > + of_node_put(remote_np); > + of_node_put(epnode); > + } > + > + if (port != sd_np) > + of_node_put(port); > + if (IS_ERR(imxsd)) { > + of_node_put(remote_np); > + of_node_put(epnode); > + break; > + } > + } > + > + return imxsd; > +} > + > +int imx_media_of_parse(struct imx_media_dev *imxmd, > + struct imx_media_subdev *(*csi)[4], > + struct device_node *np) > +{ > + struct device_node *csi_np; > + struct imx_media_subdev *lcsi; Please swap two lines above to get the reverse christmas tree ordering. > + u32 ipu_id, csi_id; > + int i, ret; > + > + for (i = 0; ; i++) { > + csi_np = of_parse_phandle(np, "ports", i); > + if (!csi_np) > + break; > + > + lcsi = of_parse_subdev(imxmd, csi_np, true); > + if (IS_ERR(lcsi)) { > + ret = PTR_ERR(lcsi); > + goto err_put; > + } > + > + of_property_read_u32(csi_np, "reg", &csi_id); Not sure if it is safe enough to ignore return value and potentially left csi_id uninitialized. > + ipu_id = of_alias_get_id(csi_np->parent, "ipu"); > + > + if (ipu_id > 1 || csi_id > 1) { > + dev_err(imxmd->dev, "%s: invalid ipu/csi id (%u/%u)\n", > + __func__, ipu_id, csi_id); > + ret = -EINVAL; > + goto err_put; > + } > + > + of_node_put(csi_np); You can put the node right after of_alias_get_id() call, then in case of error return right from the if block and remove the goto label. > + > + (*csi)[ipu_id * 2 + csi_id] = lcsi; > + } > + > + return 0; > +err_put: > + of_node_put(csi_np); > + return ret; > +} > diff --git a/drivers/staging/media/imx/imx-media-of.h b/drivers/staging/media/imx/imx-media-of.h > new file mode 100644 > index 0000000..0c61b05 > --- /dev/null > +++ b/drivers/staging/media/imx/imx-media-of.h > @@ -0,0 +1,25 @@ > +/* > + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC > + * > + * Open Firmware parsing. > + * > + * Copyright (c) 2016 Mentor Graphics Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#ifndef _IMX_MEDIA_OF_H > +#define _IMX_MEDIA_OF_H > + I do believe you should include some headers or add declarations of "struct imx_media_dev", "struct imx_media_subdev", "struct device_node". > +struct imx_media_subdev * > +imx_media_of_find_subdev(struct imx_media_dev *imxmd, > + struct device_node *np, > + const char *name); > + > +int imx_media_of_parse(struct imx_media_dev *dev, > + struct imx_media_subdev *(*csi)[4], > + struct device_node *np); > + > +#endif > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h > new file mode 100644 > index 0000000..6a018a9 > --- /dev/null > +++ b/drivers/staging/media/imx/imx-media.h > @@ -0,0 +1,299 @@ > +/* > + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC > + * > + * Copyright (c) 2016 Mentor Graphics Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#ifndef _IMX_MEDIA_H > +#define _IMX_MEDIA_H Please insert here an empty line to improve readability. > +#include <media/v4l2-device.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-of.h> Please sort out the list alphabetically. > +#include <media/videobuf2-dma-contig.h> > +#include <video/imx-ipu-v3.h> > + > +/* > + * This is somewhat arbitrary, but we need at least: > + * - 2 camera interface subdevs > + * - 3 IC subdevs > + * - 2 CSI subdevs > + * - 1 mipi-csi2 receiver subdev > + * - 2 video-mux subdevs > + * - 3 camera sensor subdevs (2 parallel, 1 mipi-csi2) > + * > + * And double the above numbers for quad i.mx! > + */ [snip] -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html