* [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI @ 2020-06-15 3:47 Chunyan Zhang 2020-06-15 14:11 ` Baolin Wang 0 siblings, 1 reply; 6+ messages in thread From: Chunyan Zhang @ 2020-06-15 3:47 UTC (permalink / raw) To: Lee Jones Cc: linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang, Chunyan Zhang 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; + + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI 2020-06-15 3:47 [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI Chunyan Zhang @ 2020-06-15 14:11 ` Baolin Wang 2020-06-15 15:08 ` Orson Zhai 2020-06-16 7:12 ` Lee Jones 0 siblings, 2 replies; 6+ messages in thread From: Baolin Wang @ 2020-06-15 14:11 UTC (permalink / raw) To: Chunyan Zhang; +Cc: Lee Jones, LKML, Orson Zhai, Chunyan Zhang 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. 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI 2020-06-15 14:11 ` Baolin Wang @ 2020-06-15 15:08 ` Orson Zhai 2020-06-15 23:30 ` Baolin Wang 2020-06-16 7:12 ` Lee Jones 1 sibling, 1 reply; 6+ messages in thread From: Orson Zhai @ 2020-06-15 15:08 UTC (permalink / raw) To: Baolin Wang; +Cc: Chunyan Zhang, Lee Jones, LKML, Chunyan Zhang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI 2020-06-15 15:08 ` Orson Zhai @ 2020-06-15 23:30 ` Baolin Wang 2020-06-16 8:12 ` Lee Jones 0 siblings, 1 reply; 6+ messages in thread From: Baolin Wang @ 2020-06-15 23:30 UTC (permalink / raw) To: Orson Zhai; +Cc: Chunyan Zhang, Lee Jones, LKML, Chunyan Zhang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI 2020-06-15 23:30 ` Baolin Wang @ 2020-06-16 8:12 ` Lee Jones 0 siblings, 0 replies; 6+ messages in thread From: Lee Jones @ 2020-06-16 8:12 UTC (permalink / raw) To: Baolin Wang; +Cc: Orson Zhai, Chunyan Zhang, LKML, Chunyan Zhang On Tue, 16 Jun 2020, Baolin Wang wrote: > 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. 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI 2020-06-15 14:11 ` Baolin Wang 2020-06-15 15:08 ` Orson Zhai @ 2020-06-16 7:12 ` Lee Jones 1 sibling, 0 replies; 6+ messages in thread From: Lee Jones @ 2020-06-16 7:12 UTC (permalink / raw) To: Baolin Wang; +Cc: Chunyan Zhang, LKML, Orson Zhai, Chunyan Zhang On Mon, 15 Jun 2020, Baolin Wang 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. > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-16 8:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-15 3:47 [PATCH] mfd: sprd: get subdevices from DT instead for SC27XX SPI Chunyan Zhang 2020-06-15 14:11 ` Baolin Wang 2020-06-15 15:08 ` Orson Zhai 2020-06-15 23:30 ` Baolin Wang 2020-06-16 8:12 ` Lee Jones 2020-06-16 7:12 ` Lee Jones
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).