Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200615034715.11438-1-zhang.lyra@gmail.com> <CADBw62oCNrvTgu9k-iykaX4bbpVWjbg3GN+v5yPBCc9R2bR3Pg@mail.gmail.com>
 <CA+H2tpF95rLRb8qbgCsrKBtjTV7JMTenuaYBHf19tpAXFqmqMQ@mail.gmail.com>
In-Reply-To: <CA+H2tpF95rLRb8qbgCsrKBtjTV7JMTenuaYBHf19tpAXFqmqMQ@mail.gmail.com>
From:   Baolin Wang <baolin.wang7@gmail.com>
Date:   Tue, 16 Jun 2020 07:30:14 +0800
Message-ID: <CADBw62r2bUxM4hxwN86aOJ9-2j0Zo8H3PMB67A=e8DUB1c0F+Q@mail.gmail.com>
Subject: Re: [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI
To:     Orson Zhai <orsonzhai@gmail.com>
Cc:     Chunyan Zhang <zhang.lyra@gmail.com>,
        Lee Jones <lee.jones@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Chunyan Zhang <chunyan.zhang@unisoc.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 15, 2020 at 11:08 PM Orson Zhai <orsonzhai@gmail.com> wrote:
>
> On Mon, Jun 15, 2020 at 10:12 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >
> > On Mon, Jun 15, 2020 at 11:47 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> > >
> > > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > >
> > > SC27XX-SPI added subdevices according to a pre-defined mfd_cell array,
> > > no matter these devices were really included on board. So with this
> > > patch we switch to a new way of detecting subdevices which are
> > > defined in the devicetree.
> > >
> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > ---
> > >  drivers/mfd/sprd-sc27xx-spi.c | 102 +++++++++++-----------------------
> > >  1 file changed, 31 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
> > > index 33336cde4724..aa3daa0cfcf5 100644
> > > --- a/drivers/mfd/sprd-sc27xx-spi.c
> > > +++ b/drivers/mfd/sprd-sc27xx-spi.c
> > > @@ -93,73 +93,6 @@ enum usb_charger_type sprd_pmic_detect_charger_type(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(sprd_pmic_detect_charger_type);
> > >
> > > -static const struct mfd_cell sprd_pmic_devs[] = {
> > > -       {
> > > -               .name = "sc27xx-wdt",
> > > -               .of_compatible = "sprd,sc2731-wdt",
> > > -       }, {
> > > -               .name = "sc27xx-rtc",
> > > -               .of_compatible = "sprd,sc2731-rtc",
> > > -       }, {
> > > -               .name = "sc27xx-charger",
> > > -               .of_compatible = "sprd,sc2731-charger",
> > > -       }, {
> > > -               .name = "sc27xx-chg-timer",
> > > -               .of_compatible = "sprd,sc2731-chg-timer",
> > > -       }, {
> > > -               .name = "sc27xx-fast-chg",
> > > -               .of_compatible = "sprd,sc2731-fast-chg",
> > > -       }, {
> > > -               .name = "sc27xx-chg-wdt",
> > > -               .of_compatible = "sprd,sc2731-chg-wdt",
> > > -       }, {
> > > -               .name = "sc27xx-typec",
> > > -               .of_compatible = "sprd,sc2731-typec",
> > > -       }, {
> > > -               .name = "sc27xx-flash",
> > > -               .of_compatible = "sprd,sc2731-flash",
> > > -       }, {
> > > -               .name = "sc27xx-eic",
> > > -               .of_compatible = "sprd,sc2731-eic",
> > > -       }, {
> > > -               .name = "sc27xx-efuse",
> > > -               .of_compatible = "sprd,sc2731-efuse",
> > > -       }, {
> > > -               .name = "sc27xx-thermal",
> > > -               .of_compatible = "sprd,sc2731-thermal",
> > > -       }, {
> > > -               .name = "sc27xx-adc",
> > > -               .of_compatible = "sprd,sc2731-adc",
> > > -       }, {
> > > -               .name = "sc27xx-audio-codec",
> > > -               .of_compatible = "sprd,sc2731-audio-codec",
> > > -       }, {
> > > -               .name = "sc27xx-regulator",
> > > -               .of_compatible = "sprd,sc2731-regulator",
> > > -       }, {
> > > -               .name = "sc27xx-vibrator",
> > > -               .of_compatible = "sprd,sc2731-vibrator",
> > > -       }, {
> > > -               .name = "sc27xx-keypad-led",
> > > -               .of_compatible = "sprd,sc2731-keypad-led",
> > > -       }, {
> > > -               .name = "sc27xx-bltc",
> > > -               .of_compatible = "sprd,sc2731-bltc",
> > > -       }, {
> > > -               .name = "sc27xx-fgu",
> > > -               .of_compatible = "sprd,sc2731-fgu",
> > > -       }, {
> > > -               .name = "sc27xx-7sreset",
> > > -               .of_compatible = "sprd,sc2731-7sreset",
> > > -       }, {
> > > -               .name = "sc27xx-poweroff",
> > > -               .of_compatible = "sprd,sc2731-poweroff",
> > > -       }, {
> > > -               .name = "sc27xx-syscon",
> > > -               .of_compatible = "sprd,sc2731-syscon",
> > > -       },
> > > -};
> > > -
> > >  static int sprd_pmic_spi_write(void *context, const void *data, size_t count)
> > >  {
> > >         struct device *dev = context;
> > > @@ -205,6 +138,35 @@ static const struct regmap_config sprd_pmic_config = {
> > >         .max_register = 0xffff,
> > >  };
> > >
> > > +static int sprd_pmic_add_subdevices(struct device *dev, int id,
> > > +                        struct irq_domain *domain)
> > > +{
> > > +       int ret = 0;
> > > +       struct device_node *child, *parent = dev->of_node;
> > > +       struct mfd_cell cell = {0};
> > > +       const char *comp;
> > > +       unsigned int prefix_len = strlen("sprd,");
> > > +       char buf[30];
> > > +
> > > +       for_each_child_of_node(parent, child) {
> > > +               comp = of_get_property(child, "compatible", NULL);
> > > +               if (!comp || strncmp("sprd,", comp, prefix_len))
> > > +                       return -EINVAL;
> > > +
> > > +               memcpy(buf, comp, strlen(comp) + 1);
> > > +               cell.of_compatible = buf;
> > > +               cell.name = buf + prefix_len;
> >
> > I feel it is a little hackish, you can create any device nodes from DT
> > even the PMIC does not support.
>
> In the old code, if sub cells have wrong compatible strings, he will not get
> an opportunity to be probed.

Yes, that's correct, and that's what PMIC asks.

> It is confused to user for  there is no any error and warning message.

The sub-nodes of PMIC should describe their standard compatible string
in their DT bindings.

> I think it maybe better to let them being probed even when they might be wrong.
> The cell will check by itself --  return success in probing if in the
> right place or return error if any
> thing is wrong there.

I do not think this is reasonable, since I can hack the DT to create
any device nodes that PMIC does not support. But if Lee agrees with
Chunyan's change, I am fine with that.

> >
> > I am not sure the problem caused by the original method, you can use
> > the compatible string to be compatible with different PMICs, such as
> > SC2730 ADC:
> > compatible = "sprd,sc2731-adc", "sprd,sc2730-adc";
> >
> > Moreover I think you can add the "sprd,sc2730-xxx" compatible string
> > for the mfd_cell in this driver if you are sure we want to support
> > SC2730 PMIC.
> >
> > > +
> > > +               ret = devm_mfd_add_devices(dev, id, &cell, 1, NULL, 0, domain);
> > > +               if (ret) {
> > > +                       pr_err("devm_mfd_add_devices return fail ret=%d\n", ret);
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  static int sprd_pmic_probe(struct spi_device *spi)
> > >  {
> > >         struct sprd_pmic *ddata;
> > > @@ -263,10 +225,8 @@ static int sprd_pmic_probe(struct spi_device *spi)
> > >                 return ret;
> > >         }
> > >
> > > -       ret = devm_mfd_add_devices(&spi->dev, PLATFORM_DEVID_AUTO,
> > > -                                  sprd_pmic_devs, ARRAY_SIZE(sprd_pmic_devs),
> > > -                                  NULL, 0,
> > > -                                  regmap_irq_get_domain(ddata->irq_data));
> > > +       ret = sprd_pmic_add_subdevices(&spi->dev, PLATFORM_DEVID_AUTO,
> > > +                                      regmap_irq_get_domain(ddata->irq_data));
> > >         if (ret) {
> > >                 dev_err(&spi->dev, "Failed to register device %d\n", ret);
> > >                 return ret;
> > > --
> > > 2.20.1
> > >
> >
> >
> > --
> > Baolin Wang


-- 
Baolin Wang