| Message ID | 1400513044-7968-2-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 19, 2014 at 08:54:03PM +0530, Shashank Sharma wrote: > This patch adds a mmio base address variable for DSI display, > to make the DSI code generic, so that, if required, the same code > can be re-used for future platforms with different mmio base. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> The error message is really a programming error, ie if we hit this error, the the code itself is doing something it shouldn't be doing. This is to be compared to an error caused by external factors. For that reason, I'd use BUG() here, but I'm not sure that's something we've clearly defined. Also the error message is not super clear and uses Mipi instead of MIPI, we always captalize it elsewhere. Depending on how much bikesheddy other peole are: Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
On Mon, May 19, 2014 at 04:45:26PM +0100, Damien Lespiau wrote: > On Mon, May 19, 2014 at 08:54:03PM +0530, Shashank Sharma wrote: > > This patch adds a mmio base address variable for DSI display, > > to make the DSI code generic, so that, if required, the same code > > can be re-used for future platforms with different mmio base. > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > The error message is really a programming error, ie if we hit this > error, the the code itself is doing something it shouldn't be doing. > > This is to be compared to an error caused by external factors. > > For that reason, I'd use BUG() here, but I'm not sure that's something > we've clearly defined. You're only allowed to use BUG if the kernel would oops anyway later on. For normal programming goof-ups a sane default (if it's feasible and doesn't require adding error handling code) is much preferred. My patch application scripts catch new BUGs and I'll review them all. -Daniel > > Also the error message is not super clear and uses Mipi instead of MIPI, > we always captalize it elsewhere. > > Depending on how much bikesheddy other peole are: > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > -- > Damien > > > --- > > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_dsi.c | 8 ++++++++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 3fc2e3d..25cdcf1 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1290,6 +1290,9 @@ struct drm_i915_private { > > */ > > uint32_t gpio_mmio_base; > > > > + /* MMIO base address for MIPI regs */ > > + uint32_t mipi_mmio_base; > > + > > wait_queue_head_t gmbus_wait_queue; > > > > struct pci_dev *bridge_dev; > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 0eff337..c12a858 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -948,6 +948,7 @@ enum punit_power_well { > > #define GFX_PPGTT_ENABLE (1<<9) > > > > #define VLV_DISPLAY_BASE 0x180000 > > +#define VLV_MIPI_BASE VLV_DISPLAY_BASE > > > > #define SCPD0 0x0209c /* 915+ only */ > > #define IER 0x020a0 > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > > index 4e271c7..a454a0b 100644 > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > @@ -639,6 +639,7 @@ bool intel_dsi_init(struct drm_device *dev) > > struct intel_connector *intel_connector; > > struct drm_connector *connector; > > struct drm_display_mode *fixed_mode = NULL; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > const struct intel_dsi_device *dsi; > > unsigned int i; > > > > @@ -658,6 +659,13 @@ bool intel_dsi_init(struct drm_device *dev) > > encoder = &intel_encoder->base; > > intel_dsi->attached_connector = intel_connector; > > > > + if (IS_VALLEYVIEW(dev)) > > + dev_priv->mipi_mmio_base = VLV_MIPI_BASE; > > + else { > > + DRM_ERROR("Unsupported Mipi device to reg base"); > > + return false; > > + } > > + > > connector = &intel_connector->base; > > > > drm_encoder_init(dev, encoder, &intel_dsi_funcs, DRM_MODE_ENCODER_DSI); > > -- > > 1.7.10.4 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 19, 2014 at 04:45:26PM +0100, Damien Lespiau wrote: > On Mon, May 19, 2014 at 08:54:03PM +0530, Shashank Sharma wrote: > > This patch adds a mmio base address variable for DSI display, > > to make the DSI code generic, so that, if required, the same code > > can be re-used for future platforms with different mmio base. > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > The error message is really a programming error, ie if we hit this > error, the the code itself is doing something it shouldn't be doing. > > This is to be compared to an error caused by external factors. > > For that reason, I'd use BUG() here, but I'm not sure that's something > we've clearly defined. > > Also the error message is not super clear and uses Mipi instead of MIPI, > we always captalize it elsewhere. > > Depending on how much bikesheddy other peole are: You might want to add some checkpatch bikesheds, but I've fixed them up while applying. > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3fc2e3d..25cdcf1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1290,6 +1290,9 @@ struct drm_i915_private { */ uint32_t gpio_mmio_base; + /* MMIO base address for MIPI regs */ + uint32_t mipi_mmio_base; + wait_queue_head_t gmbus_wait_queue; struct pci_dev *bridge_dev; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0eff337..c12a858 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -948,6 +948,7 @@ enum punit_power_well { #define GFX_PPGTT_ENABLE (1<<9) #define VLV_DISPLAY_BASE 0x180000 +#define VLV_MIPI_BASE VLV_DISPLAY_BASE #define SCPD0 0x0209c /* 915+ only */ #define IER 0x020a0 diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 4e271c7..a454a0b 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -639,6 +639,7 @@ bool intel_dsi_init(struct drm_device *dev) struct intel_connector *intel_connector; struct drm_connector *connector; struct drm_display_mode *fixed_mode = NULL; + struct drm_i915_private *dev_priv = dev->dev_private; const struct intel_dsi_device *dsi; unsigned int i; @@ -658,6 +659,13 @@ bool intel_dsi_init(struct drm_device *dev) encoder = &intel_encoder->base; intel_dsi->attached_connector = intel_connector; + if (IS_VALLEYVIEW(dev)) + dev_priv->mipi_mmio_base = VLV_MIPI_BASE; + else { + DRM_ERROR("Unsupported Mipi device to reg base"); + return false; + } + connector = &intel_connector->base; drm_encoder_init(dev, encoder, &intel_dsi_funcs, DRM_MODE_ENCODER_DSI);
This patch adds a mmio base address variable for DSI display, to make the DSI code generic, so that, if required, the same code can be re-used for future platforms with different mmio base. Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_dsi.c | 8 ++++++++ 3 files changed, 12 insertions(+)