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>
In-Reply-To: <CADBw62oCNrvTgu9k-iykaX4bbpVWjbg3GN+v5yPBCc9R2bR3Pg@mail.gmail.com>
From:   Orson Zhai <orsonzhai@gmail.com>
Date:   Mon, 15 Jun 2020 23:08:12 +0800
Message-ID: <CA+H2tpF95rLRb8qbgCsrKBtjTV7JMTenuaYBHf19tpAXFqmqMQ@mail.gmail.com>
Subject: Re: [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI
To:     Baolin Wang <baolin.wang7@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 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.
It is confused to user for  there is no any error and warning message.
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.

Best,
Orson

>
> 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