| Message ID | 1431020329-11414-10-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>: > We now have a special macro for those cases. I'm not sure if this patch is an improvement. Before it, we always knew which "switch" statement was bad since we used to print either "PDiv" or "KDiv". After the patch, it will not be possible to know from which switch statement the error came from. Of course, there's the advantage of at least knowing the value. I'd vote to either skip this patch, or improve the MISSING_CASE macro to be able to account for multiple uses on the same function. But I'm open to arugmentation :) > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index b9d5d65..ab327a1 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, > params->pdiv = 4; > break; > default: > - WARN(1, "Incorrect PDiv\n"); > + MISSING_CASE(p0); > } > > switch (p2) { > @@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, > params->kdiv = 3; > break; > default: > - WARN(1, "Incorrect KDiv\n"); > + MISSING_CASE(p2); > } > > params->qdiv_ratio = p1; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, May 27, 2015 at 03:40:32PM -0300, Paulo Zanoni wrote: > 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>: > > We now have a special macro for those cases. > > I'm not sure if this patch is an improvement. Before it, we always > knew which "switch" statement was bad since we used to print either > "PDiv" or "KDiv". After the patch, it will not be possible to know > from which switch statement the error came from. Of course, there's > the advantage of at least knowing the value. I'd vote to either skip > this patch, or improve the MISSING_CASE macro to be able to account > for multiple uses on the same function. But I'm open to arugmentation > :) MISSING_CASE is a WARN, which also prints the line number. Not enough? -Daniel > > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index b9d5d65..ab327a1 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, > > params->pdiv = 4; > > break; > > default: > > - WARN(1, "Incorrect PDiv\n"); > > + MISSING_CASE(p0); > > } > > > > switch (p2) { > > @@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, > > params->kdiv = 3; > > break; > > default: > > - WARN(1, "Incorrect KDiv\n"); > > + MISSING_CASE(p2); > > } > > > > params->qdiv_ratio = p1; > > -- > > 2.1.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-05-28 4:51 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > On Wed, May 27, 2015 at 03:40:32PM -0300, Paulo Zanoni wrote: >> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>: >> > We now have a special macro for those cases. >> >> I'm not sure if this patch is an improvement. Before it, we always >> knew which "switch" statement was bad since we used to print either >> "PDiv" or "KDiv". After the patch, it will not be possible to know >> from which switch statement the error came from. Of course, there's >> the advantage of at least knowing the value. I'd vote to either skip >> this patch, or improve the MISSING_CASE macro to be able to account >> for multiple uses on the same function. But I'm open to arugmentation >> :) > > MISSING_CASE is a WARN, which also prints the line number. Not enough? Line numbers are not very useful unless you're absolutely sure which tree/commit someone is running. And it still takes a lot of work to checkout the correct tree/commit and discover which of the WARNs is on that specific line. > -Daniel > >> >> > >> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_ddi.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> > index b9d5d65..ab327a1 100644 >> > --- a/drivers/gpu/drm/i915/intel_ddi.c >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c >> > @@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, >> > params->pdiv = 4; >> > break; >> > default: >> > - WARN(1, "Incorrect PDiv\n"); >> > + MISSING_CASE(p0); >> > } >> > >> > switch (p2) { >> > @@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, >> > params->kdiv = 3; >> > break; >> > default: >> > - WARN(1, "Incorrect KDiv\n"); >> > + MISSING_CASE(p2); >> > } >> > >> > params->qdiv_ratio = p1; >> > -- >> > 2.1.0 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Paulo Zanoni >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, May 28, 2015 at 11:06:57AM -0300, Paulo Zanoni wrote: > 2015-05-28 4:51 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > > On Wed, May 27, 2015 at 03:40:32PM -0300, Paulo Zanoni wrote: > >> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>: > >> > We now have a special macro for those cases. > >> > >> I'm not sure if this patch is an improvement. Before it, we always > >> knew which "switch" statement was bad since we used to print either > >> "PDiv" or "KDiv". After the patch, it will not be possible to know > >> from which switch statement the error came from. Of course, there's > >> the advantage of at least knowing the value. I'd vote to either skip > >> this patch, or improve the MISSING_CASE macro to be able to account > >> for multiple uses on the same function. But I'm open to arugmentation > >> :) > > > > MISSING_CASE is a WARN, which also prints the line number. Not enough? > > Line numbers are not very useful unless you're absolutely sure which > tree/commit someone is running. And it still takes a lot of work to > checkout the correct tree/commit and discover which of the WARNs is on > that specific line. Life is too short, let's drop this patch.
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index b9d5d65..ab327a1 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1145,7 +1145,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, params->pdiv = 4; break; default: - WARN(1, "Incorrect PDiv\n"); + MISSING_CASE(p0); } switch (p2) { @@ -1162,7 +1162,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params, params->kdiv = 3; break; default: - WARN(1, "Incorrect KDiv\n"); + MISSING_CASE(p2); } params->qdiv_ratio = p1;
We now have a special macro for those cases. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)