Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

From: Greg KH <gregkh@linuxfoundation.org>
To: Christian Gromm <christian.gromm@microchip.com>
Cc: driverdev-devel@linuxdriverproject.org
Subject: Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area
Date: Tue, 17 Dec 2019 14:05:48 +0100	[thread overview]
Message-ID: <20191217130548.GA3227907@kroah.com> (raw)
In-Reply-To: <1576238662-16512-8-git-send-email-christian.gromm@microchip.com>

On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> This patch moves the core module to the /drivers/most directory
> and makes all necessary changes in order to not break the build.
> 
> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>

I've applied the patches up to this one in the series, but I still have
questions about the file you are trying to move here.

It's not in this patch, but I'll just quote from the file
drivers/staging/most/core.c directly:

 * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co. KG

You've touched this file since 2015 :)

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

No need for this, You have drivers here, no need to use any pr_* calls,
as you always have a device structure.
Along with that, almost all of your pr_info() calls are really
errors/warnigns, so use dev_err() or dev_warn() instead please.

The one:
pr_info("registered new core component %s\n", comp->name);

Should at best be a dev_info() line, but really, you don't need to be
loud if all goes well, right?

pr_info("deregistering component %s\n", comp->name);

Should be dev_dbg().

static void release_interface(struct device *dev)
{
	pr_info("releasing interface dev %s...\n", dev_name(dev));
}

static void release_channel(struct device *dev)
{
	pr_info("releasing channel dev %s...\n", dev_name(dev));
}

How did I miss this before?

The driver core documentation used to have a line saying I was allowed
to make fun of programmers who did this, but that had to be removed :(

Anyway, this is totally wrong, first off, delete the debugging lines.
Secondly how are you really releasing anything?  You have to free the
memory here.  You can not have an "empty" release function, the driver
core requires you to actually do something here.

Same for release_most_sub() and anywhere else I missed in my review.

That's a good start, fix that up and I'll be glad to look at the code
again.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel