* [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 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
* 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
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).