Re: [PATCH v2 1/5] media: cec-notifier: Get notifier by device and connector name

2018-05-18 Thread Sean Paul 
 void cec_register_cec_notifier(struct 
> cec_adapter *adap,
>  #endif
>  
>  /**
> + * cec_notifier_get - find or create a new cec_notifier for the given device.
> + * @dev: device that sends the events.
> + *
> + * If a notifier for device @dev already exists, then increase the refcount
> + * and return that notifier.
> + *
> + * If it doesn't exist, then allocate a new notifier struct and return a
> + * pointer to that new struct.

You might also want to cover the case where you have multiple named notifiers
for the same device. It looks like it just grabs the first one?

Sean

> + *
> + * Return NULL if the memory could not be allocated.
> + */
> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> + return cec_notifier_get_conn(dev, NULL);
> +}
> +
> +/**
>   * cec_notifier_phys_addr_invalidate() - set the physical address to INVALID
>   *
>   * @n: the CEC notifier
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [Outreachy kernel] [PATCH] Staging: media: imx: Prefer using BIT macro

2017-09-08 Thread Sean Paul 
On Fri, Sep 8, 2017 at 11:11 AM, Srishti Sharma  wrote:
> Use BIT(x) instead of (1<
> Signed-off-by: Srishti Sharma 
> ---
>  drivers/staging/media/imx/imx-media.h | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media.h 
> b/drivers/staging/media/imx/imx-media.h
> index d409170..e5b8d29 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -310,16 +310,16 @@ void imx_media_capture_device_set_format(struct 
> imx_media_video_dev *vdev,
>  void imx_media_capture_device_error(struct imx_media_video_dev *vdev);
>
>  /* subdev group ids */
> -#define IMX_MEDIA_GRP_ID_SENSOR(1 << 8)
> -#define IMX_MEDIA_GRP_ID_VIDMUX(1 << 9)
> -#define IMX_MEDIA_GRP_ID_CSI2  (1 << 10)
> +#define IMX_MEDIA_GRP_ID_SENSORBIT(8)
> +#define IMX_MEDIA_GRP_ID_VIDMUXBIT(9)
> +#define IMX_MEDIA_GRP_ID_CSI2  BIT(10)
>  #define IMX_MEDIA_GRP_ID_CSI_BIT   11
>  #define IMX_MEDIA_GRP_ID_CSI   (0x3 << IMX_MEDIA_GRP_ID_CSI_BIT)
> -#define IMX_MEDIA_GRP_ID_CSI0  (1 << IMX_MEDIA_GRP_ID_CSI_BIT)
> +#define IMX_MEDIA_GRP_ID_CSI0  BIT(IMX_MEDIA_GRP_ID_CSI_BIT)
>  #define IMX_MEDIA_GRP_ID_CSI1  (2 << IMX_MEDIA_GRP_ID_CSI_BIT)
> -#define IMX_MEDIA_GRP_ID_VDIC  (1 << 13)
> -#define IMX_MEDIA_GRP_ID_IC_PRP(1 << 14)
> -#define IMX_MEDIA_GRP_ID_IC_PRPENC (1 << 15)
> -#define IMX_MEDIA_GRP_ID_IC_PRPVF  (1 << 16)
> +#define IMX_MEDIA_GRP_ID_VDIC  BIT(13)
> +#define IMX_MEDIA_GRP_ID_IC_PRPBIT(14)
> +#define IMX_MEDIA_GRP_ID_IC_PRPENC BIT(15)
> +#define IMX_MEDIA_GRP_ID_IC_PRPVF  BIT(16)

Hi Srishti,
Thanks for your patch.

Perhaps this is just personal preference, but I find the previous
version more readable. Since IMX_MEDIA_GRP_ID_CSI and
IMX_MEDIA_GRP_ID_CSI1 are multi-bit fields, you can't fully eliminate
the bit shift operations, so you end up with a mix, which is kind of
ugly.

Sean

>
>  #endif
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups 
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/1504883469-8127-1-git-send-email-srishtishar%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/3] drm: add support for DisplayPort CEC-Tunneling-over-AUX

2017-08-10 Thread Sean Paul 
 + if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
> + num_las = CEC_MAX_LOG_ADDRS;
> +
> + if (!IS_ERR_OR_NULL(aux->cec_adap)) {
> + if (aux->cec_adap->capabilities == cec_caps &&
> + aux->cec_adap->available_log_addrs == num_las)
> + return 0;
> + cec_unregister_adapter(aux->cec_adap);
> + }
> +
> + aux->cec_adap = cec_allocate_adapter(_dp_cec_adap_ops,
> +  aux, name, cec_caps, num_las);
> + if (IS_ERR(aux->cec_adap)) {
> + err = PTR_ERR(aux->cec_adap);
> + aux->cec_adap = NULL;
> + return err;
> + }
> + err = cec_register_adapter(aux->cec_adap, parent);
> + if (err) {
> + cec_delete_adapter(aux->cec_adap);
> + aux->cec_adap = NULL;
> + }
> + return err;
> +}
> +EXPORT_SYMBOL(drm_dp_cec_configure_adapter);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index b17476a6909c..0e236dd40b42 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -952,6 +952,8 @@ struct drm_dp_aux_msg {
>   size_t size;
>  };
>  
> +struct cec_adapter;
> +
>  /**
>   * struct drm_dp_aux - DisplayPort AUX channel
>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
> @@ -1010,6 +1012,10 @@ struct drm_dp_aux {
>* @i2c_defer_count: Counts I2C DEFERs, used for DP validation.
>*/
>   unsigned i2c_defer_count;
> + /**
> +  * @cec_adap: the CEC adapter for CEC-Tunneling-over-AUX support.
> +  */
> + struct cec_adapter *cec_adap;
>  };
>  
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> @@ -1132,4 +1138,22 @@ drm_dp_has_quirk(const struct drm_dp_desc *desc, enum 
> drm_dp_quirk quirk)
>   return desc->quirks & BIT(quirk);
>  }
>  
> +#ifdef CONFIG_DRM_DP_CEC
> +bool drm_dp_cec_irq(struct drm_dp_aux *aux);
> +int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux, const char *name,
> +  struct device *parent);
> +#else
> +static inline bool drm_dp_cec_irq(struct drm_dp_aux *aux)
> +{
> + return false;
> +}
> +
> +static inline int drm_dp_cec_configure_adapter(struct drm_dp_aux *aux,
> +const char *name,
> +struct device *parent)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 2.11.0
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH] dma-buf/sw_sync: Fix timeline/pt overflow cases

2017-06-28 Thread Sean Paul 
On Wed, Jun 28, 2017 at 08:45:55PM +0100, Chris Wilson wrote:
> Quoting Sean Paul (2017-06-28 17:47:24)
> > On Wed, Jun 28, 2017 at 05:00:20PM +0100, Chris Wilson wrote:
> > > Quoting Sean Paul (2017-06-28 16:51:11)
> > > > Protect against long-running processes from overflowing the timeline
> > > > and creating fences that go back in time. While we're at it, avoid
> > > > overflowing while we're incrementing the timeline.
> > > > 
> > > > Signed-off-by: Sean Paul <seanp...@chromium.org>
> > > > ---
> > > >  drivers/dma-buf/sw_sync.c | 7 ++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > > > index 69c5ff36e2f9..40934619ed88 100644
> > > > --- a/drivers/dma-buf/sw_sync.c
> > > > +++ b/drivers/dma-buf/sw_sync.c
> > > > @@ -142,7 +142,7 @@ static void sync_timeline_signal(struct 
> > > > sync_timeline *obj, unsigned int inc)
> > > >  
> > > > spin_lock_irqsave(>child_list_lock, flags);
> > > >  
> > > > -   obj->value += inc;
> > > > +   obj->value += min(inc, ~0x0U - obj->value);
> > > 
> > > The timeline uses u32 seqno, so just obj->value += min(inc, INT_MAX);
> > > 
> > Hi Chris,
> > Thanks for the review.
> > 
> > I don't think that solves the same problem I was trying to solve. The issue 
> > is
> > that android userspace increments value by 0x7fff twice in order to 
> > ensure
> > all fences have signaled. This is causing value to overflow and is_signaled 
> > will
> > never be true. With your snippet, the possibility of overflow still exists.
> > 
> > > Better of course would be to report the error,
> > 
> > AFAIK, it's not an error to jump the timeline, perhaps just bad taste. 
> > Capping
> > value at UINT_MAX will ensure all fences are signaled, and the check below 
> > ensures
> > that fences can't be created beyond that (returning an error at that point 
> > in
> > time).
> 
> UINT_MAX doesn't imply all fences will be signaled either, the timeline
> is supposed to wrap.
> 
> The issue is timeline_fence_signaled() is using the wrong test, it
> should be return (int)(fence->seqno - parent->value) <= 0; If it helps
> extract a little helper from dma_fence_is_later().

Understood, thank you for clarifying. This still doesn't solve the issue of 
userspace
jumping the timeline by INT_MAX multiple times. In that case, value will 
rollover and
even the new signaled() will fail to report.

Sean

> -Chris

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH] dma-buf/sw_sync: Fix timeline/pt overflow cases

2017-06-28 Thread Sean Paul 
On Wed, Jun 28, 2017 at 05:00:20PM +0100, Chris Wilson wrote:
> Quoting Sean Paul (2017-06-28 16:51:11)
> > Protect against long-running processes from overflowing the timeline
> > and creating fences that go back in time. While we're at it, avoid
> > overflowing while we're incrementing the timeline.
> > 
> > Signed-off-by: Sean Paul <seanp...@chromium.org>
> > ---
> >  drivers/dma-buf/sw_sync.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > index 69c5ff36e2f9..40934619ed88 100644
> > --- a/drivers/dma-buf/sw_sync.c
> > +++ b/drivers/dma-buf/sw_sync.c
> > @@ -142,7 +142,7 @@ static void sync_timeline_signal(struct sync_timeline 
> > *obj, unsigned int inc)
> >  
> > spin_lock_irqsave(>child_list_lock, flags);
> >  
> > -   obj->value += inc;
> > +   obj->value += min(inc, ~0x0U - obj->value);
> 
> The timeline uses u32 seqno, so just obj->value += min(inc, INT_MAX);
> 
Hi Chris,
Thanks for the review.

I don't think that solves the same problem I was trying to solve. The issue is
that android userspace increments value by 0x7fff twice in order to ensure
all fences have signaled. This is causing value to overflow and is_signaled will
never be true. With your snippet, the possibility of overflow still exists.

> Better of course would be to report the error,

AFAIK, it's not an error to jump the timeline, perhaps just bad taste. Capping
value at UINT_MAX will ensure all fences are signaled, and the check below 
ensures
that fences can't be created beyond that (returning an error at that point in
time).

Sean

> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 69c5ff36e2f9..2503cf884018 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -345,6 +345,9 @@ static long sw_sync_ioctl_inc(struct sync_timeline *obj, 
> unsigned long arg)
> if (copy_from_user(, (void __user *)arg, sizeof(value)))
> return -EFAULT;
>  
> +   if (value > INT_MAX)
> +   return -EINVAL;
> +
> sync_timeline_signal(obj, value);
> 
> >  
> > list_for_each_entry_safe(pt, next, >active_list_head,
> >  active_list) {
> > @@ -178,6 +178,11 @@ static struct sync_pt *sync_pt_create(struct 
> > sync_timeline *obj, int size,
> > return NULL;
> >  
> > spin_lock_irqsave(>child_list_lock, flags);
> > +   if (value < obj->value) {
> > +   spin_unlock_irqrestore(>child_list_lock, flags);
> > +   return NULL;
> > +   }
> 
> Needs a u32 check. if ((int)(value - obj->value) < 0) return some_error;
> -Chris

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH] dma-buf/sw_sync: Fix timeline/pt overflow cases

2017-06-28 Thread Sean Paul 
Protect against long-running processes from overflowing the timeline
and creating fences that go back in time. While we're at it, avoid
overflowing while we're incrementing the timeline.

Signed-off-by: Sean Paul <seanp...@chromium.org>
---
 drivers/dma-buf/sw_sync.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 69c5ff36e2f9..40934619ed88 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -142,7 +142,7 @@ static void sync_timeline_signal(struct sync_timeline *obj, 
unsigned int inc)
 
spin_lock_irqsave(>child_list_lock, flags);
 
-   obj->value += inc;
+   obj->value += min(inc, ~0x0U - obj->value);
 
list_for_each_entry_safe(pt, next, >active_list_head,
 active_list) {
@@ -178,6 +178,11 @@ static struct sync_pt *sync_pt_create(struct sync_timeline 
*obj, int size,
return NULL;
 
spin_lock_irqsave(>child_list_lock, flags);
+   if (value < obj->value) {
+   spin_unlock_irqrestore(>child_list_lock, flags);
+   return NULL;
+   }
+
sync_timeline_get(obj);
dma_fence_init(>base, _fence_ops, >child_list_lock,
   obj->context, value);
-- 
2.13.2.725.g09c95d1e9-goog

[PULL] Synopsys Media Formats

2017-04-03 Thread Sean Paul 
Hi Mauro,

Here's the pull for Neil's new media formats. We're using a topic branch in
drm-misc, so it will not change. Once you have acked, we'll pull this in and
apply the rest of Neil's set.

Thanks,

Sean


The following changes since commit a71c9a1c779f2499fb2afc0553e543f18aff6edf:

  Linux 4.11-rc5 (2017-04-02 17:23:54 -0700)

are available in the git repository at:

  git://anongit.freedesktop.org/git/drm-misc 
tags/topic/synopsys-media-formats-2017-04-03

for you to fetch changes up to 3c2507d308afb233dd41387b41512e7aa97535f0:

  documentation: media: Add documentation for new RGB and YUV bus formats 
(2017-04-03 11:51:40 -0400)


Media formats for synopsys HDMI  TX Controller


Neil Armstrong (2):
  media: uapi: Add RGB and YUV bus formats for Synopsys HDMI TX Controller
  documentation: media: Add documentation for new RGB and YUV bus formats

 Documentation/media/uapi/v4l/subdev-formats.rst | 3000 +++
 include/uapi/linux/media-bus-format.h   |   13 +-
 2 files changed, 1990 insertions(+), 1023 deletions(-)

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 1/1] video: drm: exynos: Adds display-timing node parsing using video helper function

2013-02-01 Thread Sean Paul 
On Fri, Feb 1, 2013 at 6:59 AM, Vikas Sajjan vikas.saj...@linaro.org wrote:
 This patch adds display-timing node parsing using video helper function

 Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
 Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
 ---
  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   39 
 +++---
  1 file changed, 35 insertions(+), 4 deletions(-)

 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
 b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 index bf0d9ba..8eee13f 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 @@ -19,6 +19,7 @@
  #include linux/clk.h
  #include linux/of_device.h
  #include linux/pm_runtime.h
 +#include linux/pinctrl/consumer.h

  #include video/samsung_fimd.h
  #include drm/exynos_drm.h
 @@ -905,16 +906,46 @@ static int __devinit fimd_probe(struct platform_device 
 *pdev)
 struct exynos_drm_subdrv *subdrv;
 struct exynos_drm_fimd_pdata *pdata;
 struct exynos_drm_panel_info *panel;
 +   struct fb_videomode *fbmode;
 +   struct pinctrl *pctrl;
 struct resource *res;
 int win;
 int ret = -EINVAL;

 DRM_DEBUG_KMS(%s\n, __FILE__);

 -   pdata = pdev-dev.platform_data;
 -   if (!pdata) {
 -   dev_err(dev, no platform data specified\n);
 -   return -EINVAL;
 +   if (pdev-dev.of_node) {
 +   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 +   if (!pdata) {
 +   DRM_ERROR(memory allocation for pdata failed\n);
 +   return -ENOMEM;
 +   }
 +
 +   fbmode = devm_kzalloc(dev, sizeof(*fbmode), GFP_KERNEL);
 +   if (!fbmode) {
 +   DRM_ERROR(memory allocation for fbmode failed\n);
 +   return -ENOMEM;
 +   }
 +
 +   ret = of_get_fb_videomode(dev-of_node, fbmode, -1);
 +   if (ret) {
 +   DRM_ERROR(failed to get fb_videomode\n);
 +   return -EINVAL;

This should probably return ret instead of -EINVAL. It would also be
useful to print ret in the error msg.

 +   }
 +   pdata-panel.timing = (struct fb_videomode) *fbmode;
 +
 +   pctrl = devm_pinctrl_get_select_default(dev);
 +   if (IS_ERR(pctrl)) {
 +   DRM_ERROR(no pinctrl data provided.\n);
 +   return -EINVAL;

I think the error message here is misleading. If there's no pinctrl
provided, doesn't get_select_default return NULL? In which case, this
error would never be printed?

I think this behavior is actually desired since there is no pinctrl
for exynos5 and we don't want it to break. However I think your error
should be more along the lines of pinctrl_get_select_default failed.
You should also print and return PTR_RET(pctrl) instead of EINVAL.

Sean

 +   }
 +
 +   } else {
 +   pdata = pdev-dev.platform_data;
 +   if (!pdata) {
 +   DRM_ERROR(no platform data specified\n);
 +   return -EINVAL;
 +   }
 }

 panel = pdata-panel;
 --
 1.7.9.5

 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/1] video: drm: exynos: Adds display-timing node parsing using video helper function

2013-01-30 Thread Sean Paul 
On Wed, Jan 30, 2013 at 1:30 AM, Vikas Sajjan vikas.saj...@linaro.org wrote:
 This patch adds display-timing node parsing using video helper function

 Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
 Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
 ---
  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   38 
 +++---
  1 file changed, 35 insertions(+), 3 deletions(-)

 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
 b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 index bf0d9ba..94729ed 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 @@ -19,6 +19,7 @@
  #include linux/clk.h
  #include linux/of_device.h
  #include linux/pm_runtime.h
 +#include linux/pinctrl/consumer.h

  #include video/samsung_fimd.h
  #include drm/exynos_drm.h
 @@ -905,15 +906,46 @@ static int __devinit fimd_probe(struct platform_device 
 *pdev)
 struct exynos_drm_subdrv *subdrv;
 struct exynos_drm_fimd_pdata *pdata;
 struct exynos_drm_panel_info *panel;
 +   struct fb_videomode *fbmode;
 +   struct device *disp_dev = pdev-dev;

Isn't this the same as dev (maybe I'm missing some dependent patch)?

 +   struct pinctrl *pctrl;
 struct resource *res;
 int win;
 int ret = -EINVAL;

 DRM_DEBUG_KMS(%s\n, __FILE__);

 -   pdata = pdev-dev.platform_data;
 -   if (!pdata) {
 -   dev_err(dev, no platform data specified\n);
 +   if (pdev-dev.of_node) {
 +   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 +   if (!pdata) {
 +   DRM_ERROR(memory allocation for pdata failed\n);
 +   return -ENOMEM;
 +   }
 +
 +   fbmode = devm_kzalloc(dev, sizeof(*fbmode), GFP_KERNEL);
 +   if (!fbmode) {
 +   DRM_ERROR(memory allocation for fbmode failed\n);
 +   return -ENOMEM;
 +   }
 +
 +   ret = of_get_fb_videomode(disp_dev-of_node, fbmode, -1);
 +   if (ret) {
 +   DRM_ERROR(failed to get fb_videomode\n);
 +   return -EINVAL;
 +   }
 +   pdata-panel.timing = (struct fb_videomode) *fbmode;
 +
 +   } else {
 +   pdata = pdev-dev.platform_data;
 +   if (!pdata) {
 +   DRM_ERROR(no platform data specified\n);
 +   return -EINVAL;
 +   }
 +   }
 +
 +   pctrl = devm_pinctrl_get_select_default(dev);
 +   if (IS_ERR(pctrl)) {

Will this work for exynos5? AFAICT, there's no pinctrl setup for it.

Sean

 +   DRM_ERROR(no pinctrl data provided.\n);
 return -EINVAL;
 }

 --
 1.7.9.5

 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [RFC] video: exynos dp: Making Exynos DP Compliant with CDF

2013-01-11 Thread Sean Paul 
On Fri, Jan 11, 2013 at 5:35 AM, Leela Krishna Amudala
l.kris...@samsung.com wrote:
 The Exynos DP transmitter is treated as an end entity in the display pipeline
 and made this RFC patch compliant with CDF.

 Any suggestions are welcome.


A few comments below. It's hard to get too much of an appreciation for
what you're trying to do since a bunch of the interesting parts are
stubbed out.

 Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
 ---
  drivers/video/display/display-core.c  |  2 +-
  drivers/video/exynos/exynos_dp_core.c | 88 
 +++
  drivers/video/exynos/exynos_dp_core.h |  6 +++
  3 files changed, 95 insertions(+), 1 deletion(-)

 diff --git a/drivers/video/display/display-core.c 
 b/drivers/video/display/display-core.c
 index 5f8be30..dbad7e9 100644
 --- a/drivers/video/display/display-core.c
 +++ b/drivers/video/display/display-core.c
 @@ -15,7 +15,7 @@
  #include linux/list.h
  #include linux/module.h
  #include linux/mutex.h
 -#include linux/videomode.h
 +#include video/videomode.h

  #include video/display.h

 diff --git a/drivers/video/exynos/exynos_dp_core.c 
 b/drivers/video/exynos/exynos_dp_core.c
 index 4ef18e2..0f8de27b 100644
 --- a/drivers/video/exynos/exynos_dp_core.c
 +++ b/drivers/video/exynos/exynos_dp_core.c
 @@ -23,6 +23,9 @@
  #include video/exynos_dp.h

  #include exynos_dp_core.h
 +#include video/videomode.h
 +#include video/display.h
 +#define to_panel(p) container_of(p, struct exynos_dp_device, entity)

  static int exynos_dp_init_dp(struct exynos_dp_device *dp)
  {
 @@ -1033,6 +1036,81 @@ static void exynos_dp_phy_exit(struct exynos_dp_device 
 *dp)
  }
  #endif /* CONFIG_OF */

 +static int exynos_dp_power_on(struct exynos_dp_device *dp)
 +{
 +   struct platform_device *pdev = to_platform_device(dp-dev);
 +   struct exynos_dp_platdata *pdata = pdev-dev.platform_data;
 +
 +   if (dp-dev-of_node) {
 +   if (dp-phy_addr)
 +   exynos_dp_phy_init(dp);
 +   } else {
 +   if (pdata-phy_init)
 +   pdata-phy_init();
 +   }
 +
 +   clk_prepare_enable(dp-clock);
 +   exynos_dp_init_dp(dp);
 +   enable_irq(dp-irq);
 +
 +   return 0;
 +}
 +
 +static int dp_set_state(struct display_entity *entity,
 +   enum display_entity_state state)
 +{
 +   struct exynos_dp_device *dp = to_panel(entity);
 +   struct platform_device *pdev = to_platform_device(dp-dev);
 +   int ret = 0;
 +
 +   switch (state) {
 +   case DISPLAY_ENTITY_STATE_OFF:
 +   case DISPLAY_ENTITY_STATE_STANDBY:
 +   ret = exynos_dp_remove(pdev);

This is incorrect, that is the module remove function. It seems like
it works right now since there's nothing permanent happening (like
platform data being freed), but there's no guarantee that this will
remain like that in the future.

IMO, you should factor out the common bits from remove and suspend
into a new function which is called from all three.

 +   break;
 +   case DISPLAY_ENTITY_STATE_ON:
 +   ret = exynos_dp_power_on(dp);
 +   break;
 +   }
 +   return ret;
 +}
 +
 +static int dp_get_modes(struct display_entity *entity,
 +   const struct videomode **modes)
 +{
 +   /* Rework has to be done here*/
 +   return 1;

Returning 1 here is pretty risky since you didn't actually allocate or
populate a mode. I'm surprised this isn't causing some weird
side-effects for you.

 +}
 +
 +static int dp_get_size(struct display_entity *entity,
 +   unsigned int *width, unsigned int *height)
 +{
 +   struct exynos_dp_device *dp = to_panel(entity);
 +   struct platform_device *pdev = to_platform_device(dp-dev);
 +   /*getting pdata in older way, rework has to be done  here to
 + parse it from dt node */
 +   struct exynos_dp_platdata *pdata = pdev-dev.platform_data;
 +
 +   /*Rework has to be done here */
 +   *width = 1280;
 +   *height = 800;
 +   return 0;
 +}
 +
 +static int dp_update(struct display_entity *entity,
 +   void (*callback)(int, void *), void *data)
 +{
 +   /*Rework has to be done here*/
 +   return 0;
 +}
 +
 +static const struct display_entity_control_ops dp_control_ops = {
 +   .set_state = dp_set_state,
 +   .get_modes = dp_get_modes,
 +   .get_size = dp_get_size,
 +   .update = dp_update,
 +};
 +
  static int exynos_dp_probe(struct platform_device *pdev)
  {
 struct resource *res;
 @@ -,6 +1189,16 @@ static int exynos_dp_probe(struct platform_device 
 *pdev)

 platform_set_drvdata(pdev, dp);

 +   /* setup panel entity */
 +   dp-entity.dev = pdev-dev;
 +   dp-entity.release = exynos_dp_remove;
 +   dp-entity.ops = dp_control_ops;
 +
 +   ret = display_entity_register(dp-entity);
 +   if (ret  0) {
 +   pr_err(failed to register display entity\n);