On Mon, 04 May 2020, Lubomir Rintel wrote:
> Hi,
>
> thanks for your review. There are some inline responses below. Where I'm not
> responding it means that I'll be just fixing what you've pointed out.
>
> On Wed, Apr 29, 2020 at 07:00:37AM +0100, Lee Jones wrote:
> > On Sat, 25 Apr 2020, Lubomir Rintel wrote:
> >
> > > This driver provides access to the EC RAM of said embedded controller
> > > attached to the I2C bus as well as optionally supporting its slightly weird
> > > power-off/restart protocol.
> > >
> > > A particular implementation of the EC firmware can be identified by a
> > > model byte. If this driver identifies the Dell Ariel platform, it
> > > registers the appropriate cells.
> > >
> > > Signed-off-by: Lubomir Rintel <lkundrak@xxxxx>
> > > ---
> > > drivers/mfd/Kconfig | 10 ++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/ene-kb3930.c | 209 +++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 220 insertions(+)
> > > create mode 100644 drivers/mfd/ene-kb3930.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 0a59249198d3..dae18a2beab5 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -398,6 +398,16 @@ config MFD_DLN2
> > > etc. must be enabled in order to use the functionality of
> > > the device.
> > >
> > > +config MFD_ENE_KB3930
> > > + tristate "ENE KB3930 Embedded Controller support"
> > > + depends on I2C
> > > + depends on MACH_MMP3_DT || COMPILE_TEST
> > > + select MFD_CORE
> > > + help
> > > + This adds support for accessing the registers on ENE KB3930, Embedded
> > > + Controller. Additional drivers such as LEDS_ARIEL must be enabled in
> > > + order to use the functionality of the device.
> > > +
> > > config MFD_EXYNOS_LPASS
> > > tristate "Samsung Exynos SoC Low Power Audio Subsystem"
> > > depends on ARCH_EXYNOS || COMPILE_TEST
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index f935d10cbf0f..2d2f5bc12841 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> > > obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> > > obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> > > obj-$(CONFIG_MFD_CROS_EC_DEV) += cros_ec_dev.o
> > > +obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o
> > > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o
> > >
> > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
> > > diff --git a/drivers/mfd/ene-kb3930.c b/drivers/mfd/ene-kb3930.c
> > > new file mode 100644
> > > index 000000000000..1123f3a1c816
> > > --- /dev/null
> > > +++ b/drivers/mfd/ene-kb3930.c
> > > @@ -0,0 +1,209 @@
> > > +// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
> > > +/*
> > > + * ENE KB3930 Embedded Controller Driver
> > > + *
> > > + * Copyright (C) 2020 Lubomir Rintel
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/mfd/core.h>
> >
> > Alphabetical.
> >
> > > +enum {
> > > + EC_DATA_IN = 0x00,
> > > + EC_RAM_OUT = 0x80,
> > > + EC_RAM_IN = 0x81,
> > > +};
> >
> > Are these registers?
>
> These are I2C registers that are multiplexing access to the EC RAM.
> Should I add a comment or make it clearer in some other way?
A comment sounds good.
> > > +enum {
> > > + EC_MODEL_ID = 0x30,
> > > + EC_VERSION_MAJ = 0x31,
> > > + EC_VERSION_MIN = 0x32,
> > > +};
> >
> > As above?
>
> These are the locations in EC RAM, multiplexed via EC_DATA_IN and
> EC_RAM_IN/OUT.
As above.
> > > +struct kb3930 {
> > > + struct i2c_client *client;
> > > + struct regmap *ec_ram;
> >
> > This is usually called 'regmap'.
>
> Yes. But the device has a set of registers directly on the I2C bus as
> well as another set of registers in the RAM access to which is
> multiplexed via a pair of I2C registers.
>
> This regmap is for the latter register block which is the only one
> exposed currently. I believe it still makes sense to make it obvious
> this is not the I2C registers in case the driver is extended to expose
> those in future.
ram_regmap or similar.
> > > + struct gpio_descs *off_gpios;
> > > +};
> > > +
> > > +struct kb3930 *global_kb3930;
> >
> > Globals are massively frowned upon. Please move it.
>
> This is necessary for the pm_power_off hook that takes no argument. All
> other MFD drivers that implement power off use a global:
Ah, it's one of those:
static struct kb3920_power_off
> ab8500-sysctrl.c: static struct device *sysctrl_dev;
> axp20x.c: static struct axp20x_dev *axp20x_pm_power_off;
> dm355evm_msp.c: static struct i2c_client *msp430;
> max77620.c: static struct max77620_chip *max77620_scratch;
> max8907.c: static struct max8907 *max8907_pm_off;
> palmas.c: static struct palmas *palmas_dev;
> retu-mfd.c: static struct retu_dev *retu_pm_power_off;
> rk808.c: static struct i2c_client *rk808_i2c_client;
> rn5t618.c: static struct rn5t618 *rn5t618_pm_power_off;
> tps6586x.c: static struct device *tps6586x_dev;
> tps65910.c: static struct i2c_client *tps65910_i2c_client;
> tps80031.c: static struct tps80031 *tps80031_power_off_dev;
> twl-core.c: static struct twl_private *twl_priv;
[...]
> > > + ariel_ec_cells,
> > > + ARRAY_SIZE(ariel_ec_cells),
> > > + NULL, 0, NULL);
> > > + if (ret < 0)
> > > + return ret;
> > > + } else {
> > > + dev_err(dev, "unknown board model: %02x\n", model_id);
> > > + return -ENODEV;
> >
> > If you reverse the logic here, you can put this in the if() and omit
> > the else.
>
> It is intentionally structured this way.
>
> Though the driver currently only supports the 'J' version of the EC
> firmware, other versions are possible, with different cells exposed via
> the EC RAM registers.
It's difficult to review based on whatifs.
I think it's worth doing it right for the current situation and adapt
it *if* things change in the future.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
