Hi Sakari, On 06/29/2017 12:34 AM, Sakari Ailus wrote: > Hi Todor, > > It's been a while --- how do you do? > > Thanks for the patchset! Thank you for the review. I'll focus more on this now, so let's see :) > > On Mon, Jun 19, 2017 at 05:48:24PM +0300, Todor Tomov wrote: >> These files control the CSIPHY modules which are responsible for the physical >> layer of the CSI2 receivers. >> >> Signed-off-by: Todor Tomov <todor.tomov@xxxxxxxxxx> >> --- >> drivers/media/platform/qcom/camss-8x16/csiphy.c | 686 ++++++++++++++++++++++++ >> drivers/media/platform/qcom/camss-8x16/csiphy.h | 77 +++ >> 2 files changed, 763 insertions(+) >> create mode 100644 drivers/media/platform/qcom/camss-8x16/csiphy.c >> create mode 100644 drivers/media/platform/qcom/camss-8x16/csiphy.h >> >> diff --git a/drivers/media/platform/qcom/camss-8x16/csiphy.c b/drivers/media/platform/qcom/camss-8x16/csiphy.c >> new file mode 100644 >> index 0000000..b9d47ca >> --- /dev/null >> +++ b/drivers/media/platform/qcom/camss-8x16/csiphy.c >> @@ -0,0 +1,686 @@ >> +/* >> + * csiphy.c >> + * >> + * Qualcomm MSM Camera Subsystem - CSIPHY Module >> + * >> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. > > How about 2017? How time flies... > >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <media/media-entity.h> >> +#include <media/v4l2-device.h> >> +#include <media/v4l2-subdev.h> >> + > > ... > >> +/* >> + * csiphy_init_formats - Initialize formats on all pads >> + * @sd: CSIPHY V4L2 subdevice >> + * @fh: V4L2 subdev file handle >> + * >> + * Initialize all pad formats with default values. >> + * >> + * Return 0 on success or a negative error code otherwise >> + */ >> +static int csiphy_init_formats(struct v4l2_subdev *sd, >> + struct v4l2_subdev_fh *fh) >> +{ >> + struct v4l2_subdev_format format; > > You can do: > > struct v4l2_subdev_format format = { 0 }; > > And drop the memset. Or even better, assign the fields in declaration. Yes. I'll do so for all memsets in the driver. > >> + >> + memset(&format, 0, sizeof(format)); >> + format.pad = MSM_CSIPHY_PAD_SINK; >> + format.which = fh ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; >> + format.format.code = MEDIA_BUS_FMT_UYVY8_2X8; >> + format.format.width = 1920; >> + format.format.height = 1080; >> + >> + return csiphy_set_format(sd, fh ? fh->pad : NULL, &format); >> +} >> + >> +/* >> + * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources >> + * @csiphy: CSIPHY device >> + * @res: CSIPHY module resources table >> + * @id: CSIPHY module id >> + * >> + * Return 0 on success or a negative error code otherwise >> + */ >> +int msm_csiphy_subdev_init(struct csiphy_device *csiphy, >> + struct resources *res, u8 id) >> +{ >> + struct device *dev = to_device_index(csiphy, id); >> + struct platform_device *pdev = container_of(dev, >> + struct platform_device, dev); > > to_platform_device()? Yes, thanks. > >> + struct resource *r; >> + int i; >> + int ret; >> + >> + csiphy->id = id; >> + csiphy->cfg.combo_mode = 0; >> + >> + /* Memory */ >> + >> + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]); >> + csiphy->base = devm_ioremap_resource(dev, r); >> + if (IS_ERR(csiphy->base)) { >> + dev_err(dev, "could not map memory\n"); >> + return PTR_ERR(csiphy->base); >> + } >> + >> + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[1]); >> + csiphy->base_clk_mux = devm_ioremap_resource(dev, r); >> + if (IS_ERR(csiphy->base_clk_mux)) { >> + dev_err(dev, "could not map memory\n"); >> + return PTR_ERR(csiphy->base_clk_mux); >> + } >> + >> + /* Interrupt */ >> + >> + r = platform_get_resource_byname(pdev, IORESOURCE_IRQ, >> + res->interrupt[0]); >> + if (!r) { >> + dev_err(dev, "missing IRQ\n"); >> + return -EINVAL; >> + } >> + >> + csiphy->irq = r->start; >> + snprintf(csiphy->irq_name, sizeof(csiphy->irq_name), "%s_%s%d", >> + dev_name(dev), MSM_CSIPHY_NAME, csiphy->id); >> + ret = devm_request_irq(dev, csiphy->irq, csiphy_isr, >> + IRQF_TRIGGER_RISING, csiphy->irq_name, csiphy); >> + if (ret < 0) { >> + dev_err(dev, "request_irq failed\n"); > > Printing the error code as well might be nice for debugging if ever needed. Ok. > >> + return ret; >> + } >> + >> + disable_irq(csiphy->irq); >> + >> + /* Clocks */ >> + >> + csiphy->nclocks = 0; >> + while (res->clock[csiphy->nclocks]) >> + csiphy->nclocks++; >> + >> + csiphy->clock = devm_kzalloc(dev, csiphy->nclocks * >> + sizeof(*csiphy->clock), GFP_KERNEL); >> + if (!csiphy->clock) >> + return -ENOMEM; >> + >> + for (i = 0; i < csiphy->nclocks; i++) { >> + csiphy->clock[i] = devm_clk_get(dev, res->clock[i]); >> + if (IS_ERR(csiphy->clock[i])) >> + return PTR_ERR(csiphy->clock[i]); >> + >> + if (res->clock_rate[i]) { >> + long clk_rate = clk_round_rate(csiphy->clock[i], >> + res->clock_rate[i]); >> + if (clk_rate < 0) { >> + dev_err(to_device_index(csiphy, csiphy->id), >> + "clk round rate failed\n"); >> + return -EINVAL; >> + } >> + ret = clk_set_rate(csiphy->clock[i], clk_rate); >> + if (ret < 0) { >> + dev_err(to_device_index(csiphy, csiphy->id), >> + "clk set rate failed\n"); >> + return ret; >> + } >> + } >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * csiphy_link_setup - Setup CSIPHY connections >> + * @entity: Pointer to media entity structure >> + * @local: Pointer to local pad >> + * @remote: Pointer to remote pad >> + * @flags: Link flags >> + * >> + * Rreturn 0 on success >> + */ >> +static int csiphy_link_setup(struct media_entity *entity, >> + const struct media_pad *local, >> + const struct media_pad *remote, u32 flags) >> +{ >> + if ((local->flags & MEDIA_PAD_FL_SOURCE) && >> + (flags & MEDIA_LNK_FL_ENABLED)) { >> + struct v4l2_subdev *sd; >> + struct csiphy_device *csiphy; >> + struct csid_device *csid; >> + >> + if (media_entity_remote_pad((struct media_pad *)local)) > > This is ugly. > > What do you intend to find with media_entity_remote_pad()? The pad flags > haven't been assigned to the pad yet, so media_entity_remote_pad() could > give you something else than remote. This is an attempt to check whether the pad is already linked - to refuse a second active connection from the same src pad. As far as I can say, it was a successful attempt. Do you see any problem with it? > >> + return -EBUSY; >> + >> + sd = container_of(entity, struct v4l2_subdev, entity); > > media_entity_to_v4l2_subdev(). Ok. > >> + csiphy = v4l2_get_subdevdata(sd); >> + >> + sd = container_of(remote->entity, struct v4l2_subdev, entity); > > Ditto. Ok. > >> + csid = v4l2_get_subdevdata(sd); >> + >> + csiphy->cfg.csid_id = csid->id; >> + } >> + >> + return 0; >> +} >> + >> +static const struct v4l2_subdev_core_ops csiphy_core_ops = { >> + .s_power = csiphy_set_power, >> +}; >> + >> +static const struct v4l2_subdev_video_ops csiphy_video_ops = { >> + .s_stream = csiphy_set_stream, >> +}; >> + >> +static const struct v4l2_subdev_pad_ops csiphy_pad_ops = { >> + .enum_mbus_code = csiphy_enum_mbus_code, >> + .enum_frame_size = csiphy_enum_frame_size, >> + .get_fmt = csiphy_get_format, >> + .set_fmt = csiphy_set_format, >> +}; >> + >> +static const struct v4l2_subdev_ops csiphy_v4l2_ops = { >> + .core = &csiphy_core_ops, >> + .video = &csiphy_video_ops, >> + .pad = &csiphy_pad_ops, >> +}; >> + >> +static const struct v4l2_subdev_internal_ops csiphy_v4l2_internal_ops = { >> + .open = csiphy_init_formats, >> +}; >> + >> +static const struct media_entity_operations csiphy_media_ops = { >> + .link_setup = csiphy_link_setup, >> + .link_validate = v4l2_subdev_link_validate, >> +}; >> + >> +/* >> + * msm_csiphy_register_entity - Register subdev node for CSIPHY module >> + * @csiphy: CSIPHY device >> + * @v4l2_dev: V4L2 device >> + * >> + * Return 0 on success or a negative error code otherwise >> + */ >> +int msm_csiphy_register_entity(struct csiphy_device *csiphy, >> + struct v4l2_device *v4l2_dev) >> +{ >> + struct v4l2_subdev *sd = &csiphy->subdev; >> + struct media_pad *pads = csiphy->pads; >> + struct device *dev = to_device_index(csiphy, csiphy->id); >> + int ret; >> + >> + v4l2_subdev_init(sd, &csiphy_v4l2_ops); >> + sd->internal_ops = &csiphy_v4l2_internal_ops; >> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >> + snprintf(sd->name, ARRAY_SIZE(sd->name), "%s%d", >> + MSM_CSIPHY_NAME, csiphy->id); >> + v4l2_set_subdevdata(sd, csiphy); >> + >> + ret = csiphy_init_formats(sd, NULL); >> + if (ret < 0) { >> + dev_err(dev, "Failed to init format\n"); >> + return ret; >> + } >> + >> + pads[MSM_CSIPHY_PAD_SINK].flags = MEDIA_PAD_FL_SINK; >> + pads[MSM_CSIPHY_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE; >> + >> + sd->entity.function = MEDIA_ENT_F_IO_V4L; >> + sd->entity.ops = &csiphy_media_ops; >> + ret = media_entity_pads_init(&sd->entity, MSM_CSIPHY_PADS_NUM, pads); >> + if (ret < 0) { >> + dev_err(dev, "Failed to init media entity\n"); >> + return ret; >> + } >> + >> + ret = v4l2_device_register_subdev(v4l2_dev, sd); >> + if (ret < 0) { >> + dev_err(dev, "Failed to register subdev\n"); >> + media_entity_cleanup(&sd->entity); >> + } >> + >> + return ret; >> +} >> + >> +/* >> + * msm_csiphy_unregister_entity - Unregister CSIPHY module subdev node >> + * @csiphy: CSIPHY device >> + */ >> +void msm_csiphy_unregister_entity(struct csiphy_device *csiphy) >> +{ >> + v4l2_device_unregister_subdev(&csiphy->subdev); >> +} >> diff --git a/drivers/media/platform/qcom/camss-8x16/csiphy.h b/drivers/media/platform/qcom/camss-8x16/csiphy.h >> new file mode 100644 >> index 0000000..60330a8 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/camss-8x16/csiphy.h >> @@ -0,0 +1,77 @@ >> +/* >> + * csiphy.h >> + * >> + * Qualcomm MSM Camera Subsystem - CSIPHY Module >> + * >> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2016 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> +#ifndef QC_MSM_CAMSS_CSIPHY_H >> +#define QC_MSM_CAMSS_CSIPHY_H >> + >> +#include <linux/clk.h> >> +#include <media/media-entity.h> >> +#include <media/v4l2-device.h> >> +#include <media/v4l2-mediabus.h> >> +#include <media/v4l2-subdev.h> >> + >> +#define MSM_CSIPHY_PAD_SINK 0 >> +#define MSM_CSIPHY_PAD_SRC 1 >> +#define MSM_CSIPHY_PADS_NUM 2 >> + >> +struct csiphy_lane { >> + u8 pos; >> + u8 pol; >> +}; >> + >> +struct csiphy_lanes_cfg { >> + int num_data; >> + struct csiphy_lane *data; >> + struct csiphy_lane clk; >> +}; >> + >> +struct csiphy_csi2_cfg { >> + int settle_cnt; >> + struct csiphy_lanes_cfg lane_cfg; >> +}; >> + >> +struct csiphy_config { >> + u8 combo_mode; >> + u8 csid_id; >> + struct csiphy_csi2_cfg *csi2; >> +}; >> + >> +struct csiphy_device { >> + u8 id; >> + struct v4l2_subdev subdev; >> + struct media_pad pads[MSM_CSIPHY_PADS_NUM]; >> + void __iomem *base; >> + void __iomem *base_clk_mux; >> + u32 irq; >> + char irq_name[30]; >> + struct clk **clock; > > You could add a forward declaration and avoid including the header file for > struct clk. Up to you I guess --- for a driver specific header it doesn't > really matter much. > Ok, I can keep it for now then. >> + int nclocks; >> + struct csiphy_config cfg; >> + struct v4l2_mbus_framefmt fmt[MSM_CSIPHY_PADS_NUM]; >> +}; >> + >> +struct resources; >> + >> +int msm_csiphy_subdev_init(struct csiphy_device *csiphy, >> + struct resources *res, u8 id); >> + >> +int msm_csiphy_register_entity(struct csiphy_device *csiphy, >> + struct v4l2_device *v4l2_dev); >> + >> +void msm_csiphy_unregister_entity(struct csiphy_device *csiphy); >> + >> +#endif /* QC_MSM_CAMSS_CSIPHY_H */ > -- Best regards, Todor Tomov -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html