Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1677200 > unrolled thread

Re: [PATCH v2 04/19] media: camss: Add CSIPHY files

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 04/19] media: camss: Add CSIPHY files Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-28 23:40 +0200
    Re: [PATCH v2 04/19] media: camss: Add CSIPHY files Todor Tomov <todor.tomov@linaro.org> - 2017-06-29 18:40 +0200
      Re: [PATCH v2 04/19] media: camss: Add CSIPHY files Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-30 02:00 +0200
        Re: [PATCH v2 04/19] media: camss: Add CSIPHY files Todor Tomov <todor.tomov@linaro.org> - 2017-06-30 09:10 +0200
          Re: [PATCH v2 04/19] media: camss: Add CSIPHY files Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-30 10:10 +0200

#1677200 — Re: [PATCH v2 04/19] media: camss: Add CSIPHY files

Hi Todor,

It's been a while --- how do you do?

Thanks for the patchset!

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@linaro.org>
> ---
>  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?

> + *
> + * 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.

> +
> +	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()?

> +	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.

> +		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.

> +			return -EBUSY;
> +
> +		sd = container_of(entity, struct v4l2_subdev, entity);

media_entity_to_v4l2_subdev().

> +		csiphy = v4l2_get_subdevdata(sd);
> +
> +		sd = container_of(remote->entity, struct v4l2_subdev, entity);

Ditto.

> +		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.

> +	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 */

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [next] | [standalone]

#1677935

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@linaro.org>
>> ---
>>  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

[toc] | [prev] | [next] | [standalone]

#1678301

Hi Todor,

On Thu, Jun 29, 2017 at 07:36:47PM +0300, Todor Tomov wrote:
> >> +/*
> >> + * 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?

Ah. So you have multiple links here only one of which may be active?

I guess you can well use media_entity_remote_pad(), but then
media_entity_remote_pad() argument needs to be made const. Feel free to
spin a patch. I don't think it'd have further implications elsewhere.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1678543

Hi Sakari,

On 06/30/2017 02:53 AM, Sakari Ailus wrote:
> Hi Todor,
> 
> On Thu, Jun 29, 2017 at 07:36:47PM +0300, Todor Tomov wrote:
>>>> +/*
>>>> + * 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?
> 
> Ah. So you have multiple links here only one of which may be active?

Exactly. Below I'm adding the output of media-ctl --print-dot as you have
requested. I can add it in the driver document as well.

> 
> I guess you can well use media_entity_remote_pad(), but then
> media_entity_remote_pad() argument needs to be made const. Feel free to
> spin a patch. I don't think it'd have further implications elsewhere.
> 

Well media_entity_remote_pad() accepts struct media_pad *pad, not a
const and trying to pass a const triggers a warning. This is why I had
to cast. Or did I misunderstand you?


# media-ctl -d /dev/media1 --print-dot
digraph board {
        rankdir=TB
        n00000001 [label="msm_csiphy0\n/dev/v4l-subdev0", shape=box, style=filled, fillcolor=yellow]
        n00000001 -> n00000007 [style=dashed]
        n00000001 -> n0000000a [style=dashed]
        n00000004 [label="msm_csiphy1\n/dev/v4l-subdev1", shape=box, style=filled, fillcolor=yellow]
        n00000004 -> n00000007 [style=dashed]
        n00000004 -> n0000000a [style=dashed]
        n00000007 [label="msm_csid0\n/dev/v4l-subdev2", shape=box, style=filled, fillcolor=yellow]
        n00000007 -> n0000000d [style=dashed]
        n00000007 -> n00000010 [style=dashed]
        n0000000a [label="msm_csid1\n/dev/v4l-subdev3", shape=box, style=filled, fillcolor=yellow]
        n0000000a -> n0000000d [style=dashed]
        n0000000a -> n00000010 [style=dashed]
        n0000000d [label="msm_ispif0\n/dev/v4l-subdev4", shape=box, style=filled, fillcolor=yellow]
        n0000000d -> n00000013:port0 [style=dashed]
        n0000000d -> n0000001c:port0 [style=dashed]
        n0000000d -> n00000025:port0 [style=dashed]
        n0000000d -> n0000002e:port0 [style=dashed]
        n00000010 [label="msm_ispif1\n/dev/v4l-subdev5", shape=box, style=filled, fillcolor=yellow]
        n00000010 -> n00000013:port0 [style=dashed]
        n00000010 -> n0000001c:port0 [style=dashed]
        n00000010 -> n00000025:port0 [style=dashed]
        n00000010 -> n0000002e:port0 [style=dashed]
        n00000013 [label="{{<port0> 0} | msm_vfe0_rdi0\n/dev/v4l-subdev6 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000013:port1 -> n00000016 [style=bold]
        n00000016 [label="msm_vfe0_video0\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
        n0000001c [label="{{<port0> 0} | msm_vfe0_rdi1\n/dev/v4l-subdev7 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        n0000001c:port1 -> n0000001f [style=bold]
        n0000001f [label="msm_vfe0_video1\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
        n00000025 [label="{{<port0> 0} | msm_vfe0_rdi2\n/dev/v4l-subdev8 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000025:port1 -> n00000028 [style=bold]
        n00000028 [label="msm_vfe0_video2\n/dev/video2", shape=box, style=filled, fillcolor=yellow]
        n0000002e [label="{{<port0> 0} | msm_vfe0_pix\n/dev/v4l-subdev9 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        n0000002e:port1 -> n00000031 [style=bold]
        n00000031 [label="msm_vfe0_video3\n/dev/video3", shape=box, style=filled, fillcolor=yellow]
        n00000057 [label="{{} | ov5645 1-0076\n/dev/v4l-subdev10 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000057:port0 -> n00000001 [style=bold]
        n00000059 [label="{{} | ov5645 1-0074\n/dev/v4l-subdev11 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000059:port0 -> n00000004 [style=bold]
}


--
Best regards,
Todor Tomov

[toc] | [prev] | [next] | [standalone]

#1678587

Hi Todor,

On Fri, Jun 30, 2017 at 10:00:25AM +0300, Todor Tomov wrote:
> Hi Sakari,
> 
> On 06/30/2017 02:53 AM, Sakari Ailus wrote:
> > Hi Todor,
> > 
> > On Thu, Jun 29, 2017 at 07:36:47PM +0300, Todor Tomov wrote:
> >>>> +/*
> >>>> + * 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?
> > 
> > Ah. So you have multiple links here only one of which may be active?
> 
> Exactly. Below I'm adding the output of media-ctl --print-dot as you have
> requested. I can add it in the driver document as well.

Hmm. I think it could be useful there as an example. I wonder what others
think.

> 
> > 
> > I guess you can well use media_entity_remote_pad(), but then
> > media_entity_remote_pad() argument needs to be made const. Feel free to
> > spin a patch. I don't think it'd have further implications elsewhere.
> > 
> 
> Well media_entity_remote_pad() accepts struct media_pad *pad, not a
> const and trying to pass a const triggers a warning. This is why I had
> to cast. Or did I misunderstand you?

No, you don't cast to non-const. Instead, you change the function to accept
a const argument.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web