Re: [PATCH v6 2/5] mfd: sy7636a: Initial commit

From: Lee Jones
Date: Thu Jul 08 2021 - 08:47:15 EST

On Thu, 08 Jul 2021, Alistair Francis wrote:

> On Thu, Jun 17, 2021 at 7:20 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> >
> > On Thu, 17 Jun 2021, Alistair Francis wrote:
> >
> > > On Wed, Jun 16, 2021 at 8:56 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, 15 Jun 2021, Alistair Francis wrote:
> > > >
> > > > > Initial support for the Silergy SY7636A Power Management chip.
> > > > >
> > > > > Signed-off-by: Alistair Francis <alistair@xxxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/mfd/Kconfig | 9 ++++
> > > > > drivers/mfd/Makefile | 1 +
> > > > > drivers/mfd/sy7636a.c | 82 +++++++++++++++++++++++++++++++++++++
> > > > > include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++
> > > > > 4 files changed, 139 insertions(+)
> > > > > create mode 100644 drivers/mfd/sy7636a.c
> > > > > create mode 100644 include/linux/mfd/sy7636a.h
> > > > >
> > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > index 5c7f2b100191..7d6cf32b1549 100644
> > > > > --- a/drivers/mfd/Kconfig
> > > > > +++ b/drivers/mfd/Kconfig
> > > > > @@ -1339,6 +1339,15 @@ config MFD_SYSCON
> > > > > Select this option to enable accessing system control registers
> > > > > via regmap.
> > > > >
> > > > > +config MFD_SY7636A
> > > > > + tristate "Silergy SY7636A Power Management chip"
> > > > > + select MFD_CORE
> > > > > + select REGMAP_I2C
> > > > > + depends on I2C
> > > > > + help
> > > > > + Select this option to enable support for the Silergy SY7636A
> > > > > + Power Management chip.
> > > > > +
> > > > > config MFD_DAVINCI_VOICECODEC
> > > > > tristate
> > > > > select MFD_CORE
> > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > > index 4f6d2b8a5f76..f95e1e725a95 100644
> > > > > --- a/drivers/mfd/Makefile
> > > > > +++ b/drivers/mfd/Makefile
> > > > > @@ -265,6 +265,7 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o
> > > > > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o
> > > > > obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.o
> > > > >
> > > > > +obj-$(CONFIG_MFD_SY7636A) += sy7636a.o
> > > > > obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
> > > > > obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.o
> > > > > obj-$(CONFIG_MFD_INTEL_M10_BMC) += intel-m10-bmc.o
> > > > > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> > > > > new file mode 100644
> > > > > index 000000000000..e08f29ea63f8
> > > > > --- /dev/null
> > > > > +++ b/drivers/mfd/sy7636a.c
> > > > > @@ -0,0 +1,82 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +//
> > > >
> > > > Only the SPDX with C++ style comments please.
> > > >
> > > > > +// MFD parent driver for SY7636A chip
> > > >
> > > > Drop the MFD part. It's a Linuxisum that doesn't really exist.
> > > >
> > > > > +// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
> > > > > +//
> > > > > +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@xxxxxxxxxxxxxx>
> > > > > +// Alistair Francis <alistair@xxxxxxxxxxxxx>
> > > > > +//
> > > > > +// Based on the lp87565 driver by Keerthy <j-keerthy@xxxxxx>
> > > > > +
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/mfd/core.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of_device.h>
> > > > > +
> > > > > +#include <linux/mfd/sy7636a.h>
> > > > > +
> > > > > +static const struct regmap_config sy7636a_regmap_config = {
> > > > > + .reg_bits = 8,
> > > > > + .val_bits = 8,
> > > > > +};
> > > > > +
> > > > > +static const struct mfd_cell sy7636a_cells[] = {
> > > > > + { .name = "sy7636a-regulator", },
> > > > > + { .name = "sy7636a-temperature", },
> > > > > + { .name = "sy7636a-thermal", },
> > > > > +};
> > > > > +
> > > > > +static const struct of_device_id of_sy7636a_match_table[] = {
> > > > > + { .compatible = "silergy,sy7636a", },
> > > > > + {}
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);
> > > >
> > > > Hold on. This driver doesn't really do anything. If you create OF
> > > > nodes for all the sub-devices, you can use simple-mfd-i2c.
> > > >
> > > > Any reason you can't do that?
> > >
> > > Just to confirm, you mean something like this?
> > >
> > > diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts
> > > b/arch/arm/boot/dts/imx7d-remarkable2.dts
> > > index 9327d1c06c96..3577104b3853 100644
> > > --- a/arch/arm/boot/dts/imx7d-remarkable2.dts
> > > +++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
> > > @@ -382,6 +382,21 @@ epd_pmic: sy7636a@62 {
> > > pinctrl-0 = <&pinctrl_epdpmic>;
> > > #thermal-sensor-cells = <0>;
> > >
> > > + regulator@0 {
> > > + compatible = "sy7636a-regulator";
> > > + reg = <0>;
> > > + };
> > > +
> > > + temperature@0 {
> > > + compatible = "sy7636a-temperature";
> > > + reg = <0>;
> > > + };
> > > +
> > > + thermal@0 {
> > > + compatible = "sy7636a-thermal";
> > > + reg = <0>;
> > > + };
> > > +
> > > regulators {
> > > compatible = "silergy,sy7636a-regulator";
> > > diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> > > index 87f684cff9a1..622a05318cff 100644
> > > --- a/drivers/mfd/simple-mfd-i2c.c
> > > +++ b/drivers/mfd/simple-mfd-i2c.c
> > > @@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
> > >
> > > static const struct of_device_id simple_mfd_i2c_of_match[] = {
> > > { .compatible = "kontron,sl28cpld" },
> > > + { .compatible = "silergy,sy7636a" },
> > > {}
> > > };
> > > MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
> >
> > Essentially. Take a look at how the other users are implementing.
> >
> > The reg entries look bogus to me though. Maybe just leave them out?
>
> So I tried this and didn't have any luck.
>
> After some Kconfig changes to allow it to build, I managed to get it
> probing, but I never got it to power up. It doesn't seem to be the
> same.

I need a more technical reason why this is not the correct approach
for you. "I can't get it to work" doesn't quite reach the quality
line I'm afraid.

Did you try enabling the debug prints in of_platform_bus_create() and
friends to see if your devices are probing correctly?

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog