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