| Message ID | 20190131192619.9763-1-sam@ravnborg.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/panel: drmP.h removal and DRM_DEV* | expand |
Hi all. On Thu, Jan 31, 2019 at 08:26:00PM +0100, Sam Ravnborg wrote: > Hi Thierry et al. > > While reviewing a number of new panel drivers there was a > certain pattern in the feedback: > - the now deprecated drmP.h file was used > - dev_err() and friends was used > > This patch-set address the above items in the panel > drivers in drm/panel/ > The hope is that new panel drivers will no longer inherit bad > patterns from the existing drivers. > > The use of DRM_DEV* is not accepted by everyone, so this conversion > was split up in smaller bits. > If some drivers do not want to use DRM_DEV* then just drop the relevant patch. > > All patches are build tested on x86/arm. > > The DRM_DEV* patches depends on the drmP.h removal. > > One extra patch sneaked in "panel-innolux: drop unused variable" > This is a fix for an unused variable and was added to flush my panel patches. > > Note: Waiting for key storage (for gpg key) before I start the process getting > commit rights, so I rely on someone else (Thierry?) to commit this. > > Patches are made on top of drm-misc-next as of a few days ago. Just for the record, all patches are checkpatched - OK. A lot of the DRM_DEV_ERROR() etc lines had to be linewrapped to avoid the maximum linelength. The conversion was done manually, no fancy coincielle script behind. That's something to look into for future changes. Sam
On Thu, Jan 31, 2019 at 08:26:00PM +0100, Sam Ravnborg wrote: > Hi Thierry et al. > > While reviewing a number of new panel drivers there was a > certain pattern in the feedback: > - the now deprecated drmP.h file was used > - dev_err() and friends was used > > This patch-set address the above items in the panel > drivers in drm/panel/ > The hope is that new panel drivers will no longer inherit bad > patterns from the existing drivers. > > The use of DRM_DEV* is not accepted by everyone, so this conversion > was split up in smaller bits. > If some drivers do not want to use DRM_DEV* then just drop the relevant patch. > > All patches are build tested on x86/arm. > > The DRM_DEV* patches depends on the drmP.h removal. > > One extra patch sneaked in "panel-innolux: drop unused variable" > This is a fix for an unused variable and was added to flush my panel patches. > > Note: Waiting for key storage (for gpg key) before I start the process getting > commit rights, so I rely on someone else (Thierry?) to commit this. > > Patches are made on top of drm-misc-next as of a few days ago. > > Comments welcome! Hey Sam, Thanks for the patchset, this will make dmesg grepping easier! One comment, and you're going to hate me for it: Why use DRM_DEV* instead of DRM_*? When I introduced DRM_DEV, it was to cover the case where there are multiple instances of the same driver (ie: dual-channel mipi, multiple crtcs, etc). I suppose that _could_ happen in the panel space, but it seems more unlikely than not. It's quite possible I'm overthinking this, but just something I figured I would point out. Either way, I think this is an improvement over dev_*. Sean > > Sam > > Sam Ravnborg (19): > drm/panel: drop drmP.h usage > drm/panel: panel-innolux: drop unused variable > drm/panel: samsung: use DRM_DEV* > drm/panel: arm-versatile: use DRM_DEV* > drm/panel: truly: use DRM_DEV* > drm/panel: sitronix: use DRM_DEV* > drm/panel: ilitek: use DRM_DEV* > drm/panel: innolux: use DRM_DEV* > drm/panel: jdi: use DRM_DEV* > drm/panel: lg: use DRM_DEV* > drm/panel: lvds: use DRM_DEV* > drm/panel: olimex: use DRM_DEV* > drm/panel: orisetech: use DRM_DEV* > drm/panel: panasonic: use DRM_DEV* > drm/panel: raspberrypi: use DRM_DEV* > drm/panel: raydium: use DRM_DEV* > drm/panel: seiko: use DRM_DEV* > drm/panel: sharp: use DRM_DEV* > drm/panel: simple: use DRM_DEV* > > drivers/gpu/drm/panel/panel-arm-versatile.c | 21 +++-- > drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 97 ++++++++++++---------- > drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 14 ++-- > drivers/gpu/drm/panel/panel-innolux-p079zca.c | 17 ++-- > drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 66 ++++++++------- > drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c | 4 +- > drivers/gpu/drm/panel/panel-lg-lg4573.c | 25 +++--- > drivers/gpu/drm/panel/panel-lvds.c | 39 +++++---- > drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c | 28 ++++--- > drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 15 ++-- > .../gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 19 +++-- > .../gpu/drm/panel/panel-raspberrypi-touchscreen.c | 25 +++--- > drivers/gpu/drm/panel/panel-raydium-rm68200.c | 11 ++- > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 18 ++-- > drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c | 21 +++-- > drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 15 ++-- > drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 32 ++++--- > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ++-- > drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 66 ++++++++++----- > drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 27 +++--- > drivers/gpu/drm/panel/panel-simple.c | 22 +++-- > drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 16 ++-- > drivers/gpu/drm/panel/panel-truly-nt35597.c | 10 ++- > 23 files changed, 375 insertions(+), 251 deletions(-)
Hi Sean. > Hey Sam, > Thanks for the patchset, this will make dmesg grepping easier! One comment, and > you're going to hate me for it: Why use DRM_DEV* instead of DRM_*? > > When I introduced DRM_DEV, it was to cover the case where there are multiple > instances of the same driver (ie: dual-channel mipi, multiple crtcs, etc). I > suppose that _could_ happen in the panel space, but it seems more unlikely than > not. The rationale for using DRM_DEV* are solely that if a struct device * is avalible, then we can use this to provide more information about the origin of the logging. I have not testet it - but from browsing the code I could not see that DRM_ERROR and friends picked up the module name. If DRM_ERROR is the right choice I will redo the patches - no problem. But if we loose the module name then the DRM_DEV* variants are preferable IMO. Note: I missed DRM_DEV_WARN(), so I left one dev_warn() in the code. Sam
On Thu, Jan 31, 2019 at 10:03:12PM +0100, Sam Ravnborg wrote: > Hi Sean. > > > Hey Sam, > > Thanks for the patchset, this will make dmesg grepping easier! One comment, and > > you're going to hate me for it: Why use DRM_DEV* instead of DRM_*? > > > > When I introduced DRM_DEV, it was to cover the case where there are multiple > > instances of the same driver (ie: dual-channel mipi, multiple crtcs, etc). I > > suppose that _could_ happen in the panel space, but it seems more unlikely than > > not. > > The rationale for using DRM_DEV* are solely that if a struct device * is avalible, > then we can use this to provide more information about the origin of the logging. > > I have not testet it - but from browsing the code I could not see that > DRM_ERROR and friends picked up the module name. > If DRM_ERROR is the right choice I will redo the patches - no problem. > > But if we loose the module name then the DRM_DEV* variants are preferable IMO. I personally like the DRM_DEV_* variants better because of the additional information that they provide. That can be useful when grepping logs etc. I'm slightly on the fence about this patch. The unwritten, and admittedly fuzzy, rules that I've been using so far are that dev_*() are used or messages that have to do with the panel device itself, whereas DRM_* variants are used for things that are actually related to DRM. So typically this would mean that roughly everything in ->probe() or ->remove() would be dev_*(), while the rest would be DRM_DEV_*(). The reason for this is that during most of ->probe() there's not really any connection to DRM. In many cases the DRM device doesn't even exist yet. Consider component/master setups where some display component will wait for the panel to be registered before binding the master. So I find it confusing to have the DRM style messages when there's actually no DRM going on yet. That said, I understand that this might not be an immediately obvious rule and it might not be followed rigorously, which both quite possibly contribute to the perception that the messages are all inconsistent. It always made sense to me, but if everyone else thinks that it's totally nuts, I'm sure I can find a way to get over it. Thierry
Hi Thierry. > > I personally like the DRM_DEV_* variants better because of the > additional information that they provide. That can be useful when > grepping logs etc. > > I'm slightly on the fence about this patch. The unwritten, and > admittedly fuzzy, rules that I've been using so far are that dev_*() are > used or messages that have to do with the panel device itself, whereas > DRM_* variants are used for things that are actually related to DRM. So > typically this would mean that roughly everything in ->probe() or > ->remove() would be dev_*(), while the rest would be DRM_DEV_*(). For a rookie like me it is much simpler if one can use the same logging primitives all over or at least the rules when to use what is simple. It is simple to say that everything that exists below drivers/gpu/drm/ relates to drm. Suggested set of rules to follow: - If in drm core, use DRM_XXX where XXX represent the core functionality - If in a driver use DRM_DEV* if a struct device is available - If in a driver and no struct device, use plain DRM_ERROR/INFO If there is a need to distingush before/after one has a drm_device, the best way would be to have a set of logging primitives that take a drm_device. So we could extend the rule set: - If in a driver use DRM_DRM* if a struct drm_device is available (This rule would take precedence over a struct device) DRM_DRM*, or DRM_DDEV* or ... But you get the idea. But this is not where we are today. Shall I redo the patch-set so we go back to dev_*() in probe() / remove()? Sam
On Fri, 01 Feb 2019, Sam Ravnborg <sam@ravnborg.org> wrote: > Hi Thierry. > >> >> I personally like the DRM_DEV_* variants better because of the >> additional information that they provide. That can be useful when >> grepping logs etc. >> >> I'm slightly on the fence about this patch. The unwritten, and >> admittedly fuzzy, rules that I've been using so far are that dev_*() are >> used or messages that have to do with the panel device itself, whereas >> DRM_* variants are used for things that are actually related to DRM. So >> typically this would mean that roughly everything in ->probe() or >> ->remove() would be dev_*(), while the rest would be DRM_DEV_*(). > > For a rookie like me it is much simpler if one can use the same > logging primitives all over or at least the rules when to use what is simple. > It is simple to say that everything that exists below drivers/gpu/drm/ > relates to drm. > > Suggested set of rules to follow: > - If in drm core, use DRM_XXX where XXX represent the core functionality > - If in a driver use DRM_DEV* if a struct device is available > - If in a driver and no struct device, use plain DRM_ERROR/INFO Core and drivers are already pretty conflated: http://patchwork.freedesktop.org/patch/msgid/20181227162310.13023-1-jani.nikula@intel.com --- Side note, I'd like to switch i915 to dev based debugs, but I absolutely hate the idea of changing: DRM_DEBUG_KMS("...") to: DRM_DEV_DEBUG_KMS(dev_priv->drm.dev, "...") I think the dev based macros are way too long, and would serve *most* (though not all) drivers better by having struct drm_device * rather than struct device * as the first param. In the above, just the boilerplate consumes half the line. Basically I'd like to see drm_ prefixed analogues to all the dev_ based logging functions, e.g. drm_dbg that takes drm_device. But it's so much churn that I'm contemplating just making i915 specific wrappers instead. :( BR, Jani. > > If there is a need to distingush before/after one has a drm_device, > the best way would be to have a set of logging primitives that > take a drm_device. So we could extend the rule set: > - If in a driver use DRM_DRM* if a struct drm_device is available > (This rule would take precedence over a struct device) > > DRM_DRM*, or DRM_DDEV* or ... But you get the idea. > > But this is not where we are today. > > Shall I redo the patch-set so we go back to dev_*() in probe() / remove()? > > Sam > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 01.02.2019 11:30, Jani Nikula wrote: > On Fri, 01 Feb 2019, Sam Ravnborg <sam@ravnborg.org> wrote: >> Hi Thierry. >> >>> I personally like the DRM_DEV_* variants better because of the >>> additional information that they provide. That can be useful when >>> grepping logs etc. >>> >>> I'm slightly on the fence about this patch. The unwritten, and >>> admittedly fuzzy, rules that I've been using so far are that dev_*() are >>> used or messages that have to do with the panel device itself, whereas >>> DRM_* variants are used for things that are actually related to DRM. So >>> typically this would mean that roughly everything in ->probe() or >>> ->remove() would be dev_*(), while the rest would be DRM_DEV_*(). >> For a rookie like me it is much simpler if one can use the same >> logging primitives all over or at least the rules when to use what is simple. >> It is simple to say that everything that exists below drivers/gpu/drm/ >> relates to drm. >> >> Suggested set of rules to follow: >> - If in drm core, use DRM_XXX where XXX represent the core functionality >> - If in a driver use DRM_DEV* if a struct device is available >> - If in a driver and no struct device, use plain DRM_ERROR/INFO > Core and drivers are already pretty conflated: > > http://patchwork.freedesktop.org/patch/msgid/20181227162310.13023-1-jani.nikula@intel.com > > --- > > Side note, I'd like to switch i915 to dev based debugs, but I absolutely > hate the idea of changing: > > DRM_DEBUG_KMS("...") > > to: > > DRM_DEV_DEBUG_KMS(dev_priv->drm.dev, "...") > > I think the dev based macros are way too long, and would serve *most* > (though not all) drivers better by having struct drm_device * rather > than struct device * as the first param. In the above, just the > boilerplate consumes half the line. > > Basically I'd like to see drm_ prefixed analogues to all the dev_ based > logging functions, e.g. drm_dbg that takes drm_device. But it's so much > churn that I'm contemplating just making i915 specific wrappers > instead. :( Does it means I am the only one who is not convinced to use all these DRM_DEV helpers. For me classic dev_(err|...) looks fine, if we really want to emphasize that logs comes from DRM dev_* allows format modification, sth like this: #define dev_fmt(fmt) "DRM: %s:%d: " fmt, __func__, __LINE__ but it is still something I do not see very helpful. In general I think we have too many alternatives/flavours and developers do not know what to choose, current usage of all these DRM_* shows it clearly. Regards Andrzej > > BR, > Jani. > > > > >> If there is a need to distingush before/after one has a drm_device, >> the best way would be to have a set of logging primitives that >> take a drm_device. So we could extend the rule set: >> - If in a driver use DRM_DRM* if a struct drm_device is available >> (This rule would take precedence over a struct device) >> >> DRM_DRM*, or DRM_DDEV* or ... But you get the idea. >> >> But this is not where we are today. >> >> Shall I redo the patch-set so we go back to dev_*() in probe() / remove()? >> >> Sam >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, 01 Feb 2019, Andrzej Hajda <a.hajda@samsung.com> wrote: > On 01.02.2019 11:30, Jani Nikula wrote: >> On Fri, 01 Feb 2019, Sam Ravnborg <sam@ravnborg.org> wrote: >>> Hi Thierry. >>> >>>> I personally like the DRM_DEV_* variants better because of the >>>> additional information that they provide. That can be useful when >>>> grepping logs etc. >>>> >>>> I'm slightly on the fence about this patch. The unwritten, and >>>> admittedly fuzzy, rules that I've been using so far are that dev_*() are >>>> used or messages that have to do with the panel device itself, whereas >>>> DRM_* variants are used for things that are actually related to DRM. So >>>> typically this would mean that roughly everything in ->probe() or >>>> ->remove() would be dev_*(), while the rest would be DRM_DEV_*(). >>> For a rookie like me it is much simpler if one can use the same >>> logging primitives all over or at least the rules when to use what is simple. >>> It is simple to say that everything that exists below drivers/gpu/drm/ >>> relates to drm. >>> >>> Suggested set of rules to follow: >>> - If in drm core, use DRM_XXX where XXX represent the core functionality >>> - If in a driver use DRM_DEV* if a struct device is available >>> - If in a driver and no struct device, use plain DRM_ERROR/INFO >> Core and drivers are already pretty conflated: >> >> http://patchwork.freedesktop.org/patch/msgid/20181227162310.13023-1-jani.nikula@intel.com >> >> --- >> >> Side note, I'd like to switch i915 to dev based debugs, but I absolutely >> hate the idea of changing: >> >> DRM_DEBUG_KMS("...") >> >> to: >> >> DRM_DEV_DEBUG_KMS(dev_priv->drm.dev, "...") >> >> I think the dev based macros are way too long, and would serve *most* >> (though not all) drivers better by having struct drm_device * rather >> than struct device * as the first param. In the above, just the >> boilerplate consumes half the line. >> >> Basically I'd like to see drm_ prefixed analogues to all the dev_ based >> logging functions, e.g. drm_dbg that takes drm_device. But it's so much >> churn that I'm contemplating just making i915 specific wrappers >> instead. :( > > > Does it means I am the only one who is not convinced to use all these > DRM_DEV helpers. > > For me classic dev_(err|...) looks fine, if we really want to emphasize > that logs comes from DRM dev_* allows format modification, sth like this: > > #define dev_fmt(fmt) "DRM: %s:%d: " fmt, __func__, __LINE__ > > but it is still something I do not see very helpful. dev_dbg has all the fancy dynamic debug stuff, but no way to filter by category the way drm.debug bitmask allows. BR, Jani. > > > In general I think we have too many alternatives/flavours and developers > do not know what to choose, current usage of all these DRM_* shows it > clearly. > > > Regards > > Andrzej > > > >> >> BR, >> Jani. >> >> >> >> >>> If there is a need to distingush before/after one has a drm_device, >>> the best way would be to have a set of logging primitives that >>> take a drm_device. So we could extend the rule set: >>> - If in a driver use DRM_DRM* if a struct drm_device is available >>> (This rule would take precedence over a struct device) >>> >>> DRM_DRM*, or DRM_DDEV* or ... But you get the idea. >>> >>> But this is not where we are today. >>> >>> Shall I redo the patch-set so we go back to dev_*() in probe() / remove()? >>> >>> Sam >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >
Hi Thierry.
> I'm slightly on the fence about this patch.
Please ignore patch 3-19, there is no consensus on the logging changes.
We do not want to apply these and then have to redo parts/all of
it later.
But the first two patches has not seen any feedback yet:
[PATCH v1 01/19] drm/panel: drop drmP.h usage
[PATCH v1 02/19] drm/panel: panel-innolux: drop unused variable
Please consider these, or maybe wait a little to see if someone
find time to review.
I can resend the patches if this is preferred.
Sam
On Fri, 2019-02-01 at 14:37 +0100, Sam Ravnborg wrote: > Hi Thierry. > > > I'm slightly on the fence about this patch. > > Please ignore patch 3-19, there is no consensus on the logging changes. > We do not want to apply these and then have to redo parts/all of > it later. > > But the first two patches has not seen any feedback yet: > > [PATCH v1 01/19] drm/panel: drop drmP.h usage > [PATCH v1 02/19] drm/panel: panel-innolux: drop unused variable > > Please consider these, or maybe wait a little to see if someone > find time to review. > > I can resend the patches if this is preferred. My preference would also convert all the DRM_DEV_<level> uses to drm_dev_<level> eventually. Also, the macros themselves could change to use a more consistent mechanism. This would make the drm logging mechanisms more like other logging mechanisms used in the kernel. Something like: --- Subject: [PATCH] drm: Improve and standardize logging functions Use a more typical logging style. Add and use drm_printk and drm_dev_printk functions that include the test for KERN_ERR use where '*ERROR*' is added to logging output. Remove the slightly unusual _DRM_PRINTK macro and use the new drm_printk function instead. --- drivers/gpu/drm/drm_print.c | 67 +++++++++++++++++++++++++++++---------------- include/drm/drm_print.h | 51 ++++++++++++++++++++-------------- 2 files changed, 74 insertions(+), 44 deletions(-) diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 0e7fc3e7dfb4..4e3ae7b5cce1 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -174,22 +174,59 @@ void drm_printf(struct drm_printer *p, const char *f, ...) } EXPORT_SYMBOL(drm_printf); -void drm_dev_printk(const struct device *dev, const char *level, - const char *format, ...) +void drm_printk(const char *format, ...) { struct va_format vaf; va_list args; + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = KERN_DEFAULT; + int level = printk_get_level(format); + size_t size = printk_skip_level(format) - format; + + if (size) { + memcpy(lvl, format, size); + lvl[size] = '\0'; + } va_start(args, format); - vaf.fmt = format; + vaf.fmt = format + size; + vaf.va = &args; + + printk("%s" "[" DRM_NAME ":%ps] %s%pV", + lvl, __builtin_return_address(0), + level == LOGLEVEL_ERR ? "*ERROR* " : "", + &vaf); + + va_end(args); +} +EXPORT_SYMBOL(drm_printk); + +void drm_dev_printk(const struct device *dev, const char *format, ...) +{ + struct va_format vaf; + va_list args; + char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = KERN_DEFAULT; + int level = printk_get_level(format); + size_t size = printk_skip_level(format) - format; + + if (size) { + memcpy(lvl, format, size); + lvl[size] = '\0'; + } + + va_start(args, format); + vaf.fmt = format + size; vaf.va = &args; if (dev) - dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV", - __builtin_return_address(0), &vaf); + dev_printk(lvl, dev, "[" DRM_NAME ":%ps] %s%pV", + __builtin_return_address(0), + level == LOGLEVEL_ERR ? "*ERROR* " : "", + &vaf); else - printk("%s" "[" DRM_NAME ":%ps] %pV", - level, __builtin_return_address(0), &vaf); + printk("%s" "[" DRM_NAME ":%ps] %s%pV", + lvl, __builtin_return_address(0), + level == LOGLEVEL_ERR ? "*ERROR* " : "", + &vaf); va_end(args); } @@ -237,19 +274,3 @@ void drm_dbg(unsigned int category, const char *format, ...) va_end(args); } EXPORT_SYMBOL(drm_dbg); - -void drm_err(const char *format, ...) -{ - struct va_format vaf; - va_list args; - - va_start(args, format); - vaf.fmt = format; - vaf.va = &args; - - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV", - __builtin_return_address(0), &vaf); - - va_end(args); -} -EXPORT_SYMBOL(drm_err); diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index afbc3beef089..9d9fd10e6ff8 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -268,9 +268,8 @@ static inline struct drm_printer drm_debug_printer(const char *prefix) #define DRM_UT_LEASE 0x80 #define DRM_UT_DP 0x100 -__printf(3, 4) -void drm_dev_printk(const struct device *dev, const char *level, - const char *format, ...); +__printf(2, 3) +void drm_dev_printk(const struct device *dev, const char *format, ...); __printf(3, 4) void drm_dev_dbg(const struct device *dev, unsigned int category, const char *format, ...); @@ -278,26 +277,38 @@ void drm_dev_dbg(const struct device *dev, unsigned int category, __printf(2, 3) void drm_dbg(unsigned int category, const char *format, ...); __printf(1, 2) -void drm_err(const char *format, ...); +void drm_printk(const char *format, ...); /* Macros to make printk easier */ -#define _DRM_PRINTK(once, level, fmt, ...) \ - printk##once(KERN_##level "[" DRM_NAME "] " fmt, ##__VA_ARGS__) - -#define DRM_INFO(fmt, ...) \ - _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__) -#define DRM_NOTE(fmt, ...) \ - _DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__) +#define DRM_ERROR(fmt, ...) \ + drm_printk(KERN_ERR fmt, ##__VA_ARGS__) +#define drm_err DRM_ERROR #define DRM_WARN(fmt, ...) \ - _DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__) + drm_printk(KERN_WARNING fmt, ##__VA_ARGS__) +#define DRM_NOTE(fmt, ...) \ + drm_printk(KERN_NOTICE fmt, ##__VA_ARGS__) +#define DRM_INFO(fmt, ...) \ + drm_printk(KERN_INFO fmt, ##__VA_ARGS__) + +#define drm_printk_once(fmt, ...) \ +({ \ + static bool __print_once __read_mostly; \ + bool __ret_print_once = !__print_once; \ + \ + if (!__print_once) { \ + __print_once = true; \ + drm_printk(fmt, ##__VA_ARGS__); \ + } \ + unlikely(__ret_print_once); \ +}) -#define DRM_INFO_ONCE(fmt, ...) \ - _DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__) -#define DRM_NOTE_ONCE(fmt, ...) \ - _DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__) #define DRM_WARN_ONCE(fmt, ...) \ - _DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__) + drm_printk_once(KERN_WARNING fmt, ##__VA_ARGS__) +#define DRM_NOTE_ONCE(fmt, ...) \ + drm_printk_once(KERN_NOTICE fmt, ##__VA_ARGS__) +#define DRM_INFO_ONCE(fmt, ...) \ + drm_printk_once(KERN_INFO fmt, ##__VA_ARGS__) /** * Error output. @@ -306,9 +317,7 @@ void drm_err(const char *format, ...); * @fmt: printf() like format string. */ #define DRM_DEV_ERROR(dev, fmt, ...) \ - drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__) -#define DRM_ERROR(fmt, ...) \ - drm_err(fmt, ##__VA_ARGS__) + drm_dev_printk(dev, KERN_ERR fmt, ##__VA_ARGS__) /** * Rate limited error output. Like DRM_ERROR() but won't flood the log. @@ -329,7 +338,7 @@ void drm_err(const char *format, ...); DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__) #define DRM_DEV_INFO(dev, fmt, ...) \ - drm_dev_printk(dev, KERN_INFO, fmt, ##__VA_ARGS__) + drm_dev_printk(dev, KERN_INFO fmt, ##__VA_ARGS__) #define DRM_DEV_INFO_ONCE(dev, fmt, ...) \ ({ \
Hi Joe. > My preference would also convert all the > DRM_DEV_<level> uses to drm_dev_<level> eventually. > > Also, the macros themselves could change to use a > more consistent mechanism. > > This would make the drm logging mechanisms more like > other logging mechanisms used in the kernel. > > Something like: I like your patch, but do not have the time to follow-up on it. It is for now saved and I will re-visit it when I have a bit more time on my hands. Thanks, Sam
Hi Thierry et al. While reviewing a number of new panel drivers there was a certain pattern in the feedback: - the now deprecated drmP.h file was used - dev_err() and friends was used This patch-set address the above items in the panel drivers in drm/panel/ The hope is that new panel drivers will no longer inherit bad patterns from the existing drivers. The use of DRM_DEV* is not accepted by everyone, so this conversion was split up in smaller bits. If some drivers do not want to use DRM_DEV* then just drop the relevant patch. All patches are build tested on x86/arm. The DRM_DEV* patches depends on the drmP.h removal. One extra patch sneaked in "panel-innolux: drop unused variable" This is a fix for an unused variable and was added to flush my panel patches. Note: Waiting for key storage (for gpg key) before I start the process getting commit rights, so I rely on someone else (Thierry?) to commit this. Patches are made on top of drm-misc-next as of a few days ago. Comments welcome! Sam Sam Ravnborg (19): drm/panel: drop drmP.h usage drm/panel: panel-innolux: drop unused variable drm/panel: samsung: use DRM_DEV* drm/panel: arm-versatile: use DRM_DEV* drm/panel: truly: use DRM_DEV* drm/panel: sitronix: use DRM_DEV* drm/panel: ilitek: use DRM_DEV* drm/panel: innolux: use DRM_DEV* drm/panel: jdi: use DRM_DEV* drm/panel: lg: use DRM_DEV* drm/panel: lvds: use DRM_DEV* drm/panel: olimex: use DRM_DEV* drm/panel: orisetech: use DRM_DEV* drm/panel: panasonic: use DRM_DEV* drm/panel: raspberrypi: use DRM_DEV* drm/panel: raydium: use DRM_DEV* drm/panel: seiko: use DRM_DEV* drm/panel: sharp: use DRM_DEV* drm/panel: simple: use DRM_DEV* drivers/gpu/drm/panel/panel-arm-versatile.c | 21 +++-- drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 97 ++++++++++++---------- drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 14 ++-- drivers/gpu/drm/panel/panel-innolux-p079zca.c | 17 ++-- drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 66 ++++++++------- drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c | 4 +- drivers/gpu/drm/panel/panel-lg-lg4573.c | 25 +++--- drivers/gpu/drm/panel/panel-lvds.c | 39 +++++---- drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c | 28 ++++--- drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 15 ++-- .../gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 19 +++-- .../gpu/drm/panel/panel-raspberrypi-touchscreen.c | 25 +++--- drivers/gpu/drm/panel/panel-raydium-rm68200.c | 11 ++- drivers/gpu/drm/panel/panel-samsung-ld9040.c | 18 ++-- drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c | 21 +++-- drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 15 ++-- drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 32 ++++--- drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 18 ++-- drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 66 ++++++++++----- drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 27 +++--- drivers/gpu/drm/panel/panel-simple.c | 22 +++-- drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 16 ++-- drivers/gpu/drm/panel/panel-truly-nt35597.c | 10 ++- 23 files changed, 375 insertions(+), 251 deletions(-)