From: Chunyan Zhang <[email protected]>
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 <[email protected]>
---
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;
+
+ 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
On Mon, Jun 15, 2020 at 11:47 AM Chunyan Zhang <[email protected]> wrote:
>
> From: Chunyan Zhang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
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
On Mon, Jun 15, 2020 at 10:12 PM Baolin Wang <[email protected]> wrote:
>
> On Mon, Jun 15, 2020 at 11:47 AM Chunyan Zhang <[email protected]> wrote:
> >
> > From: Chunyan Zhang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
On Mon, Jun 15, 2020 at 11:08 PM Orson Zhai <[email protected]> wrote:
>
> On Mon, Jun 15, 2020 at 10:12 PM Baolin Wang <[email protected]> wrote:
> >
> > On Mon, Jun 15, 2020 at 11:47 AM Chunyan Zhang <[email protected]> wrote:
> > >
> > > From: Chunyan Zhang <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
On Mon, 15 Jun 2020, Baolin Wang wrote:
> On Mon, Jun 15, 2020 at 11:47 AM Chunyan Zhang <[email protected]> wrote:
> >
> > From: Chunyan Zhang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.
>
> 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.
Why not just use of_platform_populate()?
> > +
> > + 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;
> >
>
>
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 16 Jun 2020, Baolin Wang wrote:
> On Mon, Jun 15, 2020 at 11:08 PM Orson Zhai <[email protected]> wrote:
> >
> > On Mon, Jun 15, 2020 at 10:12 PM Baolin Wang <[email protected]> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 11:47 AM Chunyan Zhang <[email protected]> wrote:
> > > >
> > > > From: Chunyan Zhang <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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.
Obviously I do not agree to this patch, as it's one big hack.
Why not just use of_platform_populate()?
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog