From: Shuah Khan <skhan@linuxfoundation.org>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
linux-media@vger.kernel.org
Cc: helen.koike@collabora.com, hverkuil@xs4all.nl,
kernel@collabora.com, dafna3@gmail.com,
Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH v4 1/2] media: vimc: move the dev field of each entity to vimc_ent_dev
Date: Thu, 24 Oct 2019 17:28:16 -0600 [thread overview]
Message-ID: <e0f24ee9-a3ca-a36f-4d6e-3378476fc3bc@linuxfoundation.org> (raw)
In-Reply-To: <20191024093525.10059-2-dafna.hirschfeld@collabora.com>
On 10/24/19 3:35 AM, Dafna Hirschfeld wrote:
> Since the 'struct device *dev' field exists in each of the
> entity structs, it can be moved to the common struct vimc_ent_devevice.
> It is then used to replace 'pr_err' with 'dev_err' in the streamer
> code.
>
I just see one pr_err() converted to dev_err(). Rest are all enhancing
dev_dbg(). You can either capture the two different changes in this
commit log or make them separate patches.
Also this is a big change without a good enough reason to do so.
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> drivers/media/platform/vimc/vimc-capture.c | 7 +++----
> drivers/media/platform/vimc/vimc-common.h | 2 ++
> drivers/media/platform/vimc/vimc-debayer.c | 15 +++++++--------
> drivers/media/platform/vimc/vimc-scaler.c | 11 +++++------
> drivers/media/platform/vimc/vimc-sensor.c | 5 ++---
> drivers/media/platform/vimc/vimc-streamer.c | 2 +-
> 6 files changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 602f80323031..d9cd6525ba22 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -15,7 +15,6 @@
> struct vimc_cap_device {
> struct vimc_ent_device ved;
> struct video_device vdev;
> - struct device *dev;
> struct v4l2_pix_format format;
> struct vb2_queue queue;
> struct list_head buf_list;
> @@ -124,7 +123,7 @@ static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
> if (ret)
> return ret;
>
> - dev_dbg(vcap->dev, "%s: format update: "
> + dev_dbg(vcap->ved.dev, "%s: format update: "
> "old:%dx%d (0x%x, %d, %d, %d, %d) "
> "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vcap->vdev.name,
> /* old */
> @@ -300,7 +299,7 @@ static int vimc_cap_buffer_prepare(struct vb2_buffer *vb)
> unsigned long size = vcap->format.sizeimage;
>
> if (vb2_plane_size(vb, 0) < size) {
> - dev_err(vcap->dev, "%s: buffer too small (%lu < %lu)\n",
> + dev_err(vcap->ved.dev, "%s: buffer too small (%lu < %lu)\n",
> vcap->vdev.name, vb2_plane_size(vb, 0), size);
> return -EINVAL;
> }
> @@ -451,7 +450,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> vcap->ved.ent = &vcap->vdev.entity;
> vcap->ved.process_frame = vimc_cap_process_frame;
> vcap->ved.vdev_get_format = vimc_cap_get_format;
> - vcap->dev = &vimc->pdev.dev;
> + vcap->ved.dev = &vimc->pdev.dev;
I am not convinced this is a safe change to make.
>
> /* Initialize the video_device struct */
> vdev = &vcap->vdev;
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 698db7c07645..8349e3c68a49 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -92,6 +92,7 @@ struct vimc_pix_map {
> /**
> * struct vimc_ent_device - core struct that represents a node in the topology
> *
> + * @dev: a pointer of the device struct of the driver
> * @ent: the pointer to struct media_entity for the node
> * @pads: the list of pads of the node
> * @process_frame: callback send a frame to that node
> @@ -108,6 +109,7 @@ struct vimc_pix_map {
> * media_entity
> */
> struct vimc_ent_device {
> + struct device *dev;
> struct media_entity *ent;
> struct media_pad *pads;
> void * (*process_frame)(struct vimc_ent_device *ved,
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index feac47d79449..ff7f8b763860 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -34,7 +34,6 @@ struct vimc_deb_pix_map {
> struct vimc_deb_device {
> struct vimc_ent_device ved;
> struct v4l2_subdev sd;
> - struct device *dev;
> /* The active format */
> struct v4l2_mbus_framefmt sink_fmt;
> u32 src_code;
> @@ -263,7 +262,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd,
> /* Set the new format in the sink pad */
> vimc_deb_adjust_sink_fmt(&fmt->format);
>
> - dev_dbg(vdeb->dev, "%s: sink format update: "
> + dev_dbg(vdeb->ved.dev, "%s: sink format update: "
> "old:%dx%d (0x%x, %d, %d, %d, %d) "
> "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vdeb->sd.name,
> /* old */
> @@ -386,7 +385,7 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
>
> /* Sum the values of the colors in the mean window */
>
> - dev_dbg(vdeb->dev,
> + dev_dbg(vdeb->ved.dev,
> "deb: %s: --- Calc pixel %dx%d, window mean %d, seek %d ---\n",
> vdeb->sd.name, lin, col, vdeb->sink_fmt.height, seek);
>
> @@ -419,7 +418,7 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
> vdeb->sink_fmt.width,
> vdeb->sink_bpp);
>
> - dev_dbg(vdeb->dev,
> + dev_dbg(vdeb->ved.dev,
> "deb: %s: RGB CALC: frame index %d, win pos %dx%d, color %d\n",
> vdeb->sd.name, index, wlin, wcol, color);
>
> @@ -430,21 +429,21 @@ static void vimc_deb_calc_rgb_sink(struct vimc_deb_device *vdeb,
> /* Save how many values we already added */
> n_rgb[color]++;
>
> - dev_dbg(vdeb->dev, "deb: %s: RGB CALC: val %d, n %d\n",
> + dev_dbg(vdeb->ved.dev, "deb: %s: RGB CALC: val %d, n %d\n",
> vdeb->sd.name, rgb[color], n_rgb[color]);
> }
> }
>
> /* Calculate the mean */
> for (i = 0; i < 3; i++) {
> - dev_dbg(vdeb->dev,
> + dev_dbg(vdeb->ved.dev,
> "deb: %s: PRE CALC: %dx%d Color %d, val %d, n %d\n",
> vdeb->sd.name, lin, col, i, rgb[i], n_rgb[i]);
>
> if (n_rgb[i])
> rgb[i] = rgb[i] / n_rgb[i];
>
> - dev_dbg(vdeb->dev,
> + dev_dbg(vdeb->ved.dev,
> "deb: %s: FINAL CALC: %dx%d Color %d, val %d\n",
> vdeb->sd.name, lin, col, i, rgb[i]);
> }
> @@ -518,7 +517,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
> }
>
> vdeb->ved.process_frame = vimc_deb_process_frame;
> - vdeb->dev = &vimc->pdev.dev;
> + vdeb->ved.dev = &vimc->pdev.dev;
I am not convinced this is a safe change to make.
>
> /* Initialize the frame format */
> vdeb->sink_fmt = sink_fmt_default;
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index a6a3cc5be872..9b1023525920 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -21,7 +21,6 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> struct vimc_sca_device {
> struct vimc_ent_device ved;
> struct v4l2_subdev sd;
> - struct device *dev;
> /* NOTE: the source fmt is the same as the sink
> * with the width and hight multiplied by mult
> */
> @@ -171,7 +170,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
> /* Set the new format in the sink pad */
> vimc_sca_adjust_sink_fmt(&fmt->format);
>
> - dev_dbg(vsca->dev, "%s: sink format update: "
> + dev_dbg(vsca->ved.dev, "%s: sink format update: "
> "old:%dx%d (0x%x, %d, %d, %d, %d) "
> "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vsca->sd.name,
> /* old */
> @@ -271,7 +270,7 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> vsca->bpp);
> pixel = &sink_frame[index];
>
> - dev_dbg(vsca->dev,
> + dev_dbg(vsca->ved.dev,
> "sca: %s: --- scale_pix sink pos %dx%d, index %d ---\n",
> vsca->sd.name, lin, col, index);
>
> @@ -281,7 +280,7 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> index = VIMC_FRAME_INDEX(lin * sca_mult, col * sca_mult,
> vsca->sink_fmt.width * sca_mult, vsca->bpp);
>
> - dev_dbg(vsca->dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
> + dev_dbg(vsca->ved.dev, "sca: %s: scale_pix src pos %dx%d, index %d\n",
> vsca->sd.name, lin * sca_mult, col * sca_mult, index);
>
> /* Repeat this pixel mult times */
> @@ -290,7 +289,7 @@ static void vimc_sca_scale_pix(const struct vimc_sca_device *const vsca,
> * pixel repetition in a line
> */
> for (j = 0; j < sca_mult * vsca->bpp; j += vsca->bpp) {
> - dev_dbg(vsca->dev,
> + dev_dbg(vsca->ved.dev,
> "sca: %s: sca: scale_pix src pos %d\n",
> vsca->sd.name, index + j);
>
> @@ -377,7 +376,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> }
>
> vsca->ved.process_frame = vimc_sca_process_frame;
> - vsca->dev = &vimc->pdev.dev;
> + vsca->ved.dev = &vimc->pdev.dev;
I am not convinced this is a safe change to make.
> /* Initialize the frame format */
> vsca->sink_fmt = sink_fmt_default;
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 46dc6a535abe..9921993a2b73 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -17,7 +17,6 @@
> struct vimc_sen_device {
> struct vimc_ent_device ved;
> struct v4l2_subdev sd;
> - struct device *dev;
> struct tpg_data tpg;
> struct task_struct *kthread_sen;
> u8 *frame;
> @@ -158,7 +157,7 @@ static int vimc_sen_set_fmt(struct v4l2_subdev *sd,
> /* Set the new format */
> vimc_sen_adjust_fmt(&fmt->format);
>
> - dev_dbg(vsen->dev, "%s: format update: "
> + dev_dbg(vsen->ved.dev, "%s: format update: "
> "old:%dx%d (0x%x, %d, %d, %d, %d) "
> "new:%dx%d (0x%x, %d, %d, %d, %d)\n", vsen->sd.name,
> /* old */
> @@ -368,7 +367,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> goto err_free_hdl;
>
> vsen->ved.process_frame = vimc_sen_process_frame;
> - vsen->dev = &vimc->pdev.dev;
> + vsen->ved.dev = &vimc->pdev.dev;
I am not convinced this is a safe change to make.
>
> /* Initialize the frame format */
> vsen->mbus_format = fmt_default;
> diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
> index faa2879c25df..37150c919fcb 100644
> --- a/drivers/media/platform/vimc/vimc-streamer.c
> +++ b/drivers/media/platform/vimc/vimc-streamer.c
> @@ -96,7 +96,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> sd = media_entity_to_v4l2_subdev(ved->ent);
> ret = v4l2_subdev_call(sd, video, s_stream, 1);
> if (ret && ret != -ENOIOCTLCMD) {
> - pr_err("subdev_call error %s\n",
> + dev_err(ved->dev, "subdev_call error %s\n",
> ved->ent->name);
> vimc_streamer_pipeline_terminate(stream);
> return ret;
>
next prev parent reply other threads:[~2019-10-24 23:28 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-24 9:35 [PATCH v4 0/2] upon streaming, check that the pipeline starts with a source entity Dafna Hirschfeld
2019-10-24 9:35 ` [PATCH v4 1/2] media: vimc: move the dev field of each entity to vimc_ent_dev Dafna Hirschfeld
2019-10-24 9:42 ` Hans Verkuil
2019-10-24 10:49 ` Dafna Hirschfeld
2019-10-24 23:28 ` Shuah Khan [this message]
2019-10-24 9:35 ` [PATCH v4 2/2] media: vimc: upon streaming, check that the pipeline starts with a source entity Dafna Hirschfeld
2019-10-24 9:44 ` Hans Verkuil
2019-10-24 11:48 ` Dafna Hirschfeld
2019-10-25 1:34 ` Shuah Khan
2019-11-01 20:31 ` Dafna Hirschfeld
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=e0f24ee9-a3ca-a36f-4d6e-3378476fc3bc@linuxfoundation.org \
--to=skhan@linuxfoundation.org \
--cc=dafna.hirschfeld@collabora.com \
--cc=dafna3@gmail.com \
--cc=helen.koike@collabora.com \
--cc=hverkuil@xs4all.nl \
--cc=kernel@collabora.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox