From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: linux-media@vger.kernel.org
Subject: Re: [PATCH v2 4/6] media: soc-camera: split struct soc_camera_link into host and subdevice parts
Date: Mon, 07 Jan 2013 22:18:52 +0100 [thread overview]
Message-ID: <2516620.yLmXLDLnye@avalon> (raw)
In-Reply-To: <Pine.LNX.4.64.1301031710470.17494@axis700.grange>
Hi Guennadi,
Thanks for the patch.
On Thursday 03 January 2013 17:13:15 Guennadi Liakhovetski wrote:
> struct soc_camera_link currently contains fields, used both by sensor and
> bridge drivers. To make subdevice driver re-use simpler, split it into a
> host and a subdevice parts.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> v2: following an off-list discussion with Laurent, .add_device() and
> .del_device() callbacks are moved over from the subdevice part of the
> platform data to the host part.
>
> drivers/media/i2c/soc_camera/imx074.c | 14 ++--
> drivers/media/i2c/soc_camera/mt9m001.c | 38 ++++----
> drivers/media/i2c/soc_camera/mt9m111.c | 21 ++--
> drivers/media/i2c/soc_camera/mt9t031.c | 22 ++--
> drivers/media/i2c/soc_camera/mt9t112.c | 18 ++--
> drivers/media/i2c/soc_camera/mt9v022.c | 36 ++++----
> drivers/media/i2c/soc_camera/ov2640.c | 12 +-
> drivers/media/i2c/soc_camera/ov5642.c | 16 ++--
> drivers/media/i2c/soc_camera/ov6650.c | 16 ++--
> drivers/media/i2c/soc_camera/ov772x.c | 14 ++--
> drivers/media/i2c/soc_camera/ov9640.c | 12 +-
> drivers/media/i2c/soc_camera/ov9740.c | 14 ++--
> drivers/media/i2c/soc_camera/rj54n1cb0c.c | 24 +++---
> drivers/media/i2c/soc_camera/tw9910.c | 18 ++--
> drivers/media/platform/soc_camera/soc_camera.c | 98 +++++++++--------
> .../platform/soc_camera/soc_camera_platform.c | 2 +-
> include/media/soc_camera.h | 102 +++++++++++++----
> include/media/soc_camera_platform.h | 10 ++-
> 18 files changed, 279 insertions(+), 208 deletions(-)
[snip]
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> b/drivers/media/platform/soc_camera/soc_camera.c index 04ce718..8ec9805
> 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -50,22 +50,22 @@ static LIST_HEAD(hosts);
> static LIST_HEAD(devices);
> static DEFINE_MUTEX(list_lock); /* Protects the list of hosts */
>
> -int soc_camera_power_on(struct device *dev, struct soc_camera_link *icl)
> +int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc
> *ssdd) {
> - int ret = regulator_bulk_enable(icl->num_regulators,
> - icl->regulators);
> + int ret = regulator_bulk_enable(ssdd->num_regulators,
> + ssdd->regulators);
> if (ret < 0) {
> dev_err(dev, "Cannot enable regulators\n");
> return ret;
> }
>
> - if (icl->power) {
> - ret = icl->power(dev, 1);
> + if (ssdd->power) {
> + ret = ssdd->power(dev, 1);
While we're at it, do you plan to get rid of this callback ? What do the
supported board need to perform there ?
> if (ret < 0) {
> dev_err(dev,
> "Platform failed to power-on the camera.\n");
> - regulator_bulk_disable(icl->num_regulators,
> - icl->regulators);
> + regulator_bulk_disable(ssdd->num_regulators,
> + ssdd->regulators);
> }
> }
>
[snip]
> @@ -552,8 +552,8 @@ static int soc_camera_open(struct file *file)
> };
>
> /* The camera could have been already on, try to reset */
> - if (icl->reset)
> - icl->reset(icd->pdev);
> + if (sdesc->subdev_desc.reset)
> + sdesc->subdev_desc.reset(icd->pdev);
>
> ret = ici->ops->add(icd);
> if (ret < 0) {
Same question here, would a GPIO do ?
> @@ -1072,23 +1072,24 @@ static void scan_add_host(struct soc_camera_host
> *ici)
>
> #ifdef CONFIG_I2C_BOARDINFO
> static int soc_camera_init_i2c(struct soc_camera_device *icd,
> - struct soc_camera_link *icl)
> + struct soc_camera_desc *sdesc)
> {
> struct i2c_client *client;
> struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> - struct i2c_adapter *adap = i2c_get_adapter(icl->i2c_adapter_id);
> + struct soc_camera_host_desc *shd = &sdesc->host_desc;
> + struct i2c_adapter *adap = i2c_get_adapter(shd->i2c_adapter_id);
> struct v4l2_subdev *subdev;
>
> if (!adap) {
> dev_err(icd->pdev, "Cannot get I2C adapter #%d. No driver?\n",
> - icl->i2c_adapter_id);
> + shd->i2c_adapter_id);
> goto ei2cga;
> }
>
> - icl->board_info->platform_data = icl;
> + shd->board_info->platform_data = &sdesc->subdev_desc;
I'm looking forward to async registration here (yes, I know, I need to review
your latest patches :-)).
> subdev = v4l2_i2c_new_subdev_board(&ici->v4l2_dev, adap,
> - icl->board_info, NULL);
> + shd->board_info, NULL);
> if (!subdev)
> goto ei2cnd;
>
> @@ -1116,7 +1117,7 @@ static void soc_camera_free_i2c(struct
> soc_camera_device *icd) i2c_put_adapter(adap);
> }
> #else
> -#define soc_camera_init_i2c(icd, icl) (-ENODEV)
> +#define soc_camera_init_i2c(icd, sdesc) (-ENODEV)
> #define soc_camera_free_i2c(icd) do {} while (0)
> #endif
>
> @@ -1126,7 +1127,9 @@ static int video_dev_create(struct soc_camera_device
> *icd); static int soc_camera_probe(struct soc_camera_device *icd)
> {
> struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> - struct soc_camera_link *icl = to_soc_camera_link(icd);
> + struct soc_camera_desc *sdesc = to_soc_camera_desc(icd);
> + struct soc_camera_host_desc *shd = &sdesc->host_desc;
> + struct soc_camera_subdev_desc *ssdd = &sdesc->subdev_desc;
> struct device *control = NULL;
> struct v4l2_subdev *sd;
> struct v4l2_mbus_framefmt mf;
> @@ -1146,8 +1149,8 @@ static int soc_camera_probe(struct soc_camera_device
> *icd) return ret;
>
> /* The camera could have been already on, try to reset */
> - if (icl->reset)
> - icl->reset(icd->pdev);
> + if (ssdd->reset)
> + ssdd->reset(icd->pdev);
I don't think the core should access the reset method here directly (although
it's obviously fine to keep it for now). This will hopefully be fixed by async
registration.
> mutex_lock(&ici->host_lock);
> ret = ici->ops->add(icd);
> @@ -1161,18 +1164,18 @@ static int soc_camera_probe(struct soc_camera_device
> *icd) goto evdc;
>
> /* Non-i2c cameras, e.g., soc_camera_platform, have no board_info */
> - if (icl->board_info) {
> - ret = soc_camera_init_i2c(icd, icl);
> + if (shd->board_info) {
> + ret = soc_camera_init_i2c(icd, sdesc);
> if (ret < 0)
> goto eadddev;
> - } else if (!icl->add_device || !icl->del_device) {
> + } else if (!shd->add_device || !shd->del_device) {
Once again I hope that async registration will remove the add_device and
del_device methods.
> ret = -EINVAL;
> goto eadddev;
> } else {
> - if (icl->module_name)
> - ret = request_module(icl->module_name);
> + if (shd->module_name)
> + ret = request_module(shd->module_name);
>
> - ret = icl->add_device(icd);
> + ret = shd->add_device(icd);
> if (ret < 0)
> goto eadddev;
>
[snip]
> @@ -1534,24 +1537,25 @@ static int soc_camera_video_start(struct
> soc_camera_device *icd)
>
> static int soc_camera_pdrv_probe(struct platform_device *pdev)
> {
> - struct soc_camera_link *icl = pdev->dev.platform_data;
> + struct soc_camera_desc *sdesc = pdev->dev.platform_data;
> + struct soc_camera_subdev_desc *ssdd = &sdesc->subdev_desc;
> struct soc_camera_device *icd;
> int ret;
>
> - if (!icl)
> + if (!sdesc)
> return -EINVAL;
>
> icd = devm_kzalloc(&pdev->dev, sizeof(*icd), GFP_KERNEL);
> if (!icd)
> return -ENOMEM;
>
> - ret = devm_regulator_bulk_get(&pdev->dev, icl->num_regulators,
> - icl->regulators);
> + ret = devm_regulator_bulk_get(&pdev->dev, ssdd->num_regulators,
> + ssdd->regulators);
Do you plan to move this to the client drivers (or more accurately to a soc-
camera helper function called by client drivers at probe time) ?
> if (ret < 0)
> return ret;
>
> - icd->iface = icl->bus_id;
> - icd->link = icl;
> + icd->iface = sdesc->host_desc.bus_id;
> + icd->sdesc = sdesc;
> icd->pdev = &pdev->dev;
> platform_set_drvdata(pdev, icd);
>
[snip]
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 5a662c9..2cc70cf 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -23,11 +23,11 @@
> #include <media/v4l2-device.h>
>
> struct file;
> -struct soc_camera_link;
> +struct soc_camera_desc;
>
> struct soc_camera_device {
> struct list_head list; /* list of all registered devices */
> - struct soc_camera_link *link;
> + struct soc_camera_desc *sdesc;
> struct device *pdev; /* Platform device */
> struct device *parent; /* Camera host device */
> struct device *control; /* E.g., the i2c client */
> @@ -116,26 +116,72 @@ struct soc_camera_host_ops {
> struct i2c_board_info;
> struct regulator_bulk_data;
>
> -struct soc_camera_link {
> - /* Camera bus id, used to match a camera and a bus */
> - int bus_id;
> +struct soc_camera_subdev_desc {
> /* Per camera SOCAM_SENSOR_* bus flags */
> unsigned long flags;
> - int i2c_adapter_id;
> - struct i2c_board_info *board_info;
> - const char *module_name;
> - void *priv;
> +
> + /* sensor driver private platform data */
> + void *drv_priv;
As discussed privately, just for the record, this should go at some point
(with async registration ? ;-)).
> /* Optional regulators that have to be managed on power on/off events */
> struct regulator_bulk_data *regulators;
> int num_regulators;
>
> + /* Optional callbacks to power on or off and reset the sensor */
> + int (*power)(struct device *, int);
> + int (*reset)(struct device *);
> +
> + /*
> + * some platforms may support different data widths than the sensors
> + * native ones due to different data line routing. Let the board code
> + * overwrite the width flags.
> + */
> + int (*set_bus_param)(struct soc_camera_subdev_desc *, unsigned long
> flags);
> + unsigned long (*query_bus_param)(struct soc_camera_subdev_desc *);
> + void (*free_bus)(struct soc_camera_subdev_desc *);
> +};
The structure otherwise looks pretty good to me (except of course that board
callbacks should go at some point, as discussed above). Nice work.
> +struct soc_camera_host_desc {
> + /* Camera bus id, used to match a camera and a bus */
> + int bus_id;
> + int i2c_adapter_id;
> + struct i2c_board_info *board_info;
> + const char *module_name;
> +
> /*
> * For non-I2C devices platform has to provide methods to add a device
> * to the system and to remove it
> */
> int (*add_device)(struct soc_camera_device *);
> void (*del_device)(struct soc_camera_device *);
> +};
> +
> +/*
> + * This MUST be kept binary-identical to struct soc_camera_link below,
> until + * it is completely replaced by this one, after which we can split
> it into its + * two components.
> + */
> +struct soc_camera_desc {
> + struct soc_camera_subdev_desc subdev_desc;
> + struct soc_camera_host_desc host_desc;
> +};
> +
> +/* Prepare to replace this struct: don't change its layout any more! */
> +struct soc_camera_link {
> + /*
> + * Subdevice part - keep at top and compatible to
> + * struct soc_camera_subdev_desc
> + */
> +
> + /* Per camera SOCAM_SENSOR_* bus flags */
> + unsigned long flags;
> +
> + void *priv;
> +
> + /* Optional regulators that have to be managed on power on/off events */
> + struct regulator_bulk_data *regulators;
> + int num_regulators;
> +
> /* Optional callbacks to power on or off and reset the sensor */
> int (*power)(struct device *, int);
> int (*reset)(struct device *);
> @@ -147,6 +193,24 @@ struct soc_camera_link {
> int (*set_bus_param)(struct soc_camera_link *, unsigned long flags);
> unsigned long (*query_bus_param)(struct soc_camera_link *);
> void (*free_bus)(struct soc_camera_link *);
> +
> + /*
> + * Host part - keep at bottom and compatible to
> + * struct soc_camera_host_desc
> + */
> +
> + /* Camera bus id, used to match a camera and a bus */
> + int bus_id;
> + int i2c_adapter_id;
> + struct i2c_board_info *board_info;
> + const char *module_name;
> +
> + /*
> + * For non-I2C devices platform has to provide methods to add a device
> + * to the system and to remove it
> + */
> + int (*add_device)(struct soc_camera_device *);
> + void (*del_device)(struct soc_camera_device *);
> };
>
> static inline struct soc_camera_host *to_soc_camera_host(
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2013-01-07 21:17 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-26 17:35 [PATCH 0/6] soc-camera: improvements and fixes, prepare for async Guennadi Liakhovetski
2012-12-26 17:35 ` [PATCH 1/6] media: sh_mobile_ceu_camera: fix CSI2 format negotiation Guennadi Liakhovetski
2012-12-26 17:35 ` [PATCH 2/6] media: soc-camera: properly fix camera probing races Guennadi Liakhovetski
2012-12-26 17:35 ` [PATCH 3/6] soc-camera: remove struct soc_camera_device::video_lock Guennadi Liakhovetski
2012-12-26 17:35 ` [PATCH 4/6] media: soc-camera: split struct soc_camera_link into host and subdevice parts Guennadi Liakhovetski
2013-01-03 16:13 ` [PATCH v2 " Guennadi Liakhovetski
2013-01-07 21:18 ` Laurent Pinchart [this message]
2012-12-26 17:35 ` [PATCH 5/6] media: soc-camera: use devm_kzalloc in subdevice drivers Guennadi Liakhovetski
2012-12-26 17:35 ` [PATCH 6/6] soc-camera: fix repeated regulator requesting Guennadi Liakhovetski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2516620.yLmXLDLnye@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=g.liakhovetski@gmx.de \
--cc=linux-media@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.