linux-kernel - Re: [PATCH 1/5] mfd: add Congatec Board Controller mfd driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240912141309.GC24460@google.com>
Date: Thu, 12 Sep 2024 15:13:09 +0100
From: Lee Jones <lee@...nel.org>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Andi Shyti <andi.shyti@...nel.org>,
	Wim Van Sebroeck <wim@...ux-watchdog.org>,
	Guenter Roeck <linux@...ck-us.net>, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
	linux-watchdog@...r.kernel.org, thomas.petazzoni@...tlin.com,
	blake.vermeer@...sight.com
Subject: Re: [PATCH 1/5] mfd: add Congatec Board Controller mfd driver

On Tue, 10 Sep 2024, Thomas Richard wrote:

> On 8/22/24 12:38, Lee Jones wrote:
> > On Fri, 09 Aug 2024, Thomas Richard wrote:
> > 
> >> Add core MFD driver for the Board Controller found on some Congatec SMARC
> >> module. This Board Controller provides functions like watchdog, GPIO, and
> >> I2C busses.
> >>
> >> This commit add support only for the conga-SA7 module.
> >>
> >> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>
> >> ---
> >>  drivers/mfd/Kconfig      |  12 ++
> >>  drivers/mfd/Makefile     |   1 +
> >>  drivers/mfd/cgbc-core.c  | 453 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/mfd/cgbc.h |  44 +++++
> >>  4 files changed, 510 insertions(+)
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >> index bc8be2e593b6..3e0530f30267 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -224,6 +224,18 @@ config MFD_AXP20X_RSB
> >>  	  components like regulators or the PEK (Power Enable Key) under the
> >>  	  corresponding menus.
> >>  
> >> +config MFD_CGBC
> >> +	tristate "Congatec Board Controller"
> >> +	select MFD_CORE
> >> +	depends on X86
> >> +	help
> >> +	  This is the core driver of the Board Controller found on some Congatec
> >> +	  SMARC modules. The Board Controller provides functions like watchdog,
> >> +	  I2C busses, and GPIO controller.
> >> +
> >> +	  To compile this driver as a module, choose M here: the module will be
> >> +	  called cgbc-core.
> >> +
> >>  config MFD_CROS_EC_DEV
> >>  	tristate "ChromeOS Embedded Controller multifunction device"
> >>  	select MFD_CORE
> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >> index 02b651cd7535..d5da3fcd691c 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_SM501)		+= sm501.o
> >>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
> >>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
> >>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
> >> +obj-$(CONFIG_MFD_CGBC)		+= cgbc-core.o
> >>  obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
> >>  obj-$(CONFIG_MFD_CS42L43)	+= cs42l43.o
> >>  obj-$(CONFIG_MFD_CS42L43_I2C)	+= cs42l43-i2c.o
> >> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> >> new file mode 100644
> >> index 000000000000..cca9b1170cc9
> >> --- /dev/null
> >> +++ b/drivers/mfd/cgbc-core.c
> >> @@ -0,0 +1,453 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * Congatec Board Controller MFD core driver.
> > 
> > No such thing as an MFD.
> 
> What should it be if it's not an MFD ?

You should be telling me this. :)

"Board Controller" according to the Kconfig entry.

> >> +static struct platform_device *cgbc_pdev;
> > 
> > No avoidable globals.
> 
> I don't see how can I have cgbc_pdev not global.

Because of the incestuous, "I am my own parent", thing?  Okay.

> >> +static void cgbc_session_release(struct cgbc_device_data *cgbc)
> >> +{
> >> +	if (cgbc_session_command(cgbc, cgbc->session) != cgbc->session)
> > 
> > What does this do?
> > 
> > Are we checking or doing something?  If the latter, then we shouldn't be
> > doing that in an if() statement.  If the former, then what's the point
> > of the function?
> 
> We try to close the session we opened with the device.
> We check that the session is closed correctly.

Please remove it from the if statement and store the result in variable
with suitable nomenclature please.

[...]

In future, please snip everything you do not wish to discuss.

> Thanks for the review !!

No problem.

-- 
Lee Jones [李琼斯]