Re: [PATCH] drm/panfrost: Prefix interrupt handlers' names

[Robin Murphy <robin.murphy@xxxxxxx>]

On 13/12/2019 1:18 pm, Neil Armstrong wrote:
> Hi,
>
> On 13/12/2019 13:39, Ezequiel Garcia wrote:
> > Currently, the interrupt lines requested by Panfrost
> > use ambiguous names, which adds some obscurity
> > to interrupt introspection (i.e. any tool based
> > on procfs' interrupts file).
> >
> > In order to improve this, prefix each requested
> > interrupt with either the module name or the device
> > name, where possible.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +-
> >   drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> >   drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++++--
> >   3 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> > index f67ed925c0ef..0355c4a78eaa 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> > @@ -348,7 +348,7 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
> >   		return -ENODEV;
> >   
> >   	err = devm_request_irq(pfdev->dev, irq, panfrost_gpu_irq_handler,
> > -			       IRQF_SHARED, "gpu", pfdev);
> > +			       IRQF_SHARED, dev_name(pfdev->dev), pfdev);
> >   	if (err) {
> >   		dev_err(pfdev->dev, "failed to request gpu irq");
> >   		return err;
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index 05c85f45a0de..3bd79ebb6c40 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -480,7 +480,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
> >   		return -ENODEV;
> >   
> >   	ret = devm_request_irq(pfdev->dev, irq, panfrost_job_irq_handler,
> > -			       IRQF_SHARED, "job", pfdev);
> > +			       IRQF_SHARED, KBUILD_MODNAME "-job", pfdev);
> >   	if (ret) {
> >   		dev_err(pfdev->dev, "failed to request job irq");
> >   		return ret;
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > index 842bdd7cf6be..806958434726 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > @@ -612,9 +612,11 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
> >   	if (irq <= 0)
> >   		return -ENODEV;
> >   
> > -	err = devm_request_threaded_irq(pfdev->dev, irq, panfrost_mmu_irq_handler,
> > +	err = devm_request_threaded_irq(pfdev->dev, irq,
> > +					panfrost_mmu_irq_handler,
> >   					panfrost_mmu_irq_handler_thread,
> > -					IRQF_SHARED, "mmu", pfdev);
> > +					IRQF_SHARED, KBUILD_MODNAME "-mmu",
> > +					pfdev);
> >   
> >   	if (err) {
> >   		dev_err(pfdev->dev, "failed to request mmu irq");
>
> Why don't you use dev_name(pfdev->dev) everywhere ?

Agreed, while the current implementation may be confusing it is at least 
self-consistent. TBH it would probably be sufficient to save the bother 
of allocating strings and just settle on "panfrost-{gpu,job,mmu}", since 
upstream users are unlikely to ever come across a system with more than 
one Mali in it ;)

And FWIW note that "GPU" really is the specific hardware name of that 
IRQ output; it's not just a generic fill-in for "the one that isn't the 
Job or MMU IRQ".

Thanks,
Robin.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel