linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: Alistair Francis <alistair23@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>,
	Rob Herring <robh+dt@kernel.org>,
	lgirdwood@gmail.com, Mark Brown <broonie@kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	Sascha Hauer <kernel@pengutronix.de>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 2/5] mfd: sy7636a: Initial commit
Date: Thu, 8 Jul 2021 12:47:08 +0000	[thread overview]
Message-ID: <YObzzE8y2tyiMg5p@google.com> (raw)
In-Reply-To: <CAKmqyKODRkahuUL+SFj8RQYq2rvj=nLgseozAyiwkDmtD8JE+g@mail.gmail.com>

On Thu, 08 Jul 2021, Alistair Francis wrote:

> On Thu, Jun 17, 2021 at 7:20 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 17 Jun 2021, Alistair Francis wrote:
> >
> > > On Wed, Jun 16, 2021 at 8:56 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Tue, 15 Jun 2021, Alistair Francis wrote:
> > > >
> > > > > Initial support for the Silergy SY7636A Power Management chip.
> > > > >
> > > > > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > > > > ---
> > > > >  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@remarkable.com>
> > > > > +//          Alistair Francis <alistair@alistair23.me>
> > > > > +//
> > > > > +// Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
> > > > > +
> > > > > +#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

  reply	other threads:[~2021-07-08 12:47 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15 10:33 [PATCH v6 1/5] dt-bindings: mfd: Initial commit of silergy,sy7636a.yaml Alistair Francis
2021-06-15 10:33 ` [PATCH v6 2/5] mfd: sy7636a: Initial commit Alistair Francis
2021-06-16 10:56   ` Lee Jones
2021-06-16 22:01     ` Alistair Francis
2021-06-17  9:20       ` Lee Jones
2021-07-08 11:16         ` Alistair Francis
2021-07-08 12:47           ` Lee Jones [this message]
2021-07-28  6:57             ` Alistair Francis
2021-08-02  8:45               ` Lee Jones
2021-06-15 10:33 ` [PATCH v6 3/5] regulator: " Alistair Francis
2021-06-15 10:33 ` [PATCH v6 4/5] ARM: imx_v6_v7_defconfig: Enable silergy,sy7636a Alistair Francis
2021-06-15 10:34 ` [PATCH v6 5/5] ARM: dts: imx7d: remarkable2: " Alistair Francis
2021-06-15 17:29 ` (subset) [PATCH v6 1/5] dt-bindings: mfd: Initial commit of silergy,sy7636a.yaml Mark Brown
2021-06-24 20:56 ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YObzzE8y2tyiMg5p@google.com \
    --to=lee.jones@linaro.org \
    --cc=alistair23@gmail.com \
    --cc=alistair@alistair23.me \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=kernel@pengutronix.de \
    --cc=lgirdwood@gmail.com \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v6 2/5] mfd: sy7636a: Initial commit' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).