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

[Lee Jones <lee@xxxxxxxxxx>]

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@xxxxxxxxxxx>
> >> ---
> >>  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 [李琼斯]