| Message ID | 20200423174543.17161-4-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
Hi Lee, Am 2020-04-23 19:45, schrieb Michael Walle: > There might be multiple children with the device tree compatible, for > example if a MFD has multiple instances of the same function. In this > case only the first is matched and the other children get a wrong > of_node reference. > Add a new option to match also against the unit address of the child > node. Additonally, a new helper OF_MFD_CELL_REG is added. Do you think this is feasible? I guess this is the biggest uncertainty for me at the moment in this patch series. -michael > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/mfd/mfd-core.c | 29 ++++++++++++++++++++--------- > include/linux/mfd/core.h | 26 ++++++++++++++++++++------ > 2 files changed, 40 insertions(+), 15 deletions(-) > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > index e735565969b3..4ecb376338f7 100644 > --- a/drivers/mfd/mfd-core.c > +++ b/drivers/mfd/mfd-core.c > @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, > int id, > struct device_node *np = NULL; > int ret = -ENOMEM; > int platform_id; > + u32 of_reg; > int r; > > if (id == PLATFORM_DEVID_AUTO) > @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent, > int id, > > if (parent->of_node && cell->of_compatible) { > for_each_child_of_node(parent->of_node, np) { > - if (of_device_is_compatible(np, cell->of_compatible)) { > - if (!of_device_is_available(np)) { > - /* Ignore disabled devices error free */ > - ret = 0; > - goto fail_alias; > - } > - pdev->dev.of_node = np; > - pdev->dev.fwnode = &np->fwnode; > - break; > + if (!of_device_is_compatible(np, cell->of_compatible)) > + continue; > + > + /* also match the unit address if set */ > + if (cell->of_reg & MFD_OF_REG_VALID) { > + if (of_property_read_u32(np, "reg", &of_reg)) > + continue; > + if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg) > + continue; > } > + > + if (!of_device_is_available(np)) { > + /* Ignore disabled devices error free */ > + ret = 0; > + goto fail_alias; > + } > + > + pdev->dev.of_node = np; > + pdev->dev.fwnode = &np->fwnode; > + break; > } > } > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h > index d01d1299e49d..c2c0ad6b14f3 100644 > --- a/include/linux/mfd/core.h > +++ b/include/linux/mfd/core.h > @@ -13,8 +13,11 @@ > #include <linux/platform_device.h> > > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) > +#define MFD_OF_REG_VALID BIT(31) > +#define MFD_OF_REG_MASK GENMASK(30, 0) > > -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, > _match)\ > +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ > + _of_reg, _match) \ > { \ > .name = (_name), \ > .resources = (_res), \ > @@ -22,24 +25,32 @@ > .platform_data = (_pdata), \ > .pdata_size = (_pdsize), \ > .of_compatible = (_compat), \ > + .of_reg = (_of_reg), \ > .acpi_match = (_match), \ > .id = (_id), \ > } > > +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat, \ > + _of_reg) \ > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ > + ((_of_reg) | MFD_OF_REG_VALID), NULL) \ > + > #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat) \ > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL) \ > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ > + 0, NULL) \ > > #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match) \ > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match) \ > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, \ > + _match) \ > > #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id) \ > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL) \ > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \ > > #define MFD_CELL_RES(_name, _res) \ > - MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL) \ > + MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL) \ > > #define MFD_CELL_NAME(_name) \ > - MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL) \ > + MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL) \ > > struct irq_domain; > struct property_entry; > @@ -78,6 +89,9 @@ struct mfd_cell { > */ > const char *of_compatible; > > + /* matching the reg property if set */ > + unsigned int of_reg; > + > /* Matches ACPI */ > const struct mfd_cell_acpi_match *acpi_match;
On Thu, 30 Apr 2020, Michael Walle wrote: > Hi Lee, > > Am 2020-04-23 19:45, schrieb Michael Walle: > > There might be multiple children with the device tree compatible, for > > example if a MFD has multiple instances of the same function. In this > > case only the first is matched and the other children get a wrong > > of_node reference. > > Add a new option to match also against the unit address of the child > > node. Additonally, a new helper OF_MFD_CELL_REG is added. > > > Do you think this is feasible? I guess this is the biggest uncertainty > for me at the moment in this patch series. I think it sounds fine in principle. So long as it doesn't change the existing behaviour when of_reg isn't set. > > Signed-off-by: Michael Walle <michael@walle.cc> > > --- > > drivers/mfd/mfd-core.c | 29 ++++++++++++++++++++--------- > > include/linux/mfd/core.h | 26 ++++++++++++++++++++------ > > 2 files changed, 40 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > > index e735565969b3..4ecb376338f7 100644 > > --- a/drivers/mfd/mfd-core.c > > +++ b/drivers/mfd/mfd-core.c > > @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, int > > id, > > struct device_node *np = NULL; > > int ret = -ENOMEM; > > int platform_id; > > + u32 of_reg; > > int r; > > > > if (id == PLATFORM_DEVID_AUTO) > > @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent, > > int id, > > > > if (parent->of_node && cell->of_compatible) { > > for_each_child_of_node(parent->of_node, np) { > > - if (of_device_is_compatible(np, cell->of_compatible)) { > > - if (!of_device_is_available(np)) { > > - /* Ignore disabled devices error free */ > > - ret = 0; > > - goto fail_alias; > > - } > > - pdev->dev.of_node = np; > > - pdev->dev.fwnode = &np->fwnode; > > - break; > > + if (!of_device_is_compatible(np, cell->of_compatible)) > > + continue; > > + > > + /* also match the unit address if set */ Please use correct grammar in comments (leaving off the full-stop). > > + if (cell->of_reg & MFD_OF_REG_VALID) { > > + if (of_property_read_u32(np, "reg", &of_reg)) > > + continue; > > + if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg) > > + continue; > > } > > + > > + if (!of_device_is_available(np)) { > > + /* Ignore disabled devices error free */ > > + ret = 0; > > + goto fail_alias; > > + } > > + > > + pdev->dev.of_node = np; > > + pdev->dev.fwnode = &np->fwnode; > > + break; > > } > > } > > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h > > index d01d1299e49d..c2c0ad6b14f3 100644 > > --- a/include/linux/mfd/core.h > > +++ b/include/linux/mfd/core.h > > @@ -13,8 +13,11 @@ > > #include <linux/platform_device.h> > > > > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) > > +#define MFD_OF_REG_VALID BIT(31) What about 64bit platforms? > > +#define MFD_OF_REG_MASK GENMASK(30, 0) > > > > -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, > > _match)\ > > +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ > > + _of_reg, _match) \ > > { \ > > .name = (_name), \ > > .resources = (_res), \ > > @@ -22,24 +25,32 @@ > > .platform_data = (_pdata), \ > > .pdata_size = (_pdsize), \ > > .of_compatible = (_compat), \ > > + .of_reg = (_of_reg), \ > > .acpi_match = (_match), \ > > .id = (_id), \ > > } > > > > +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat, \ > > + _of_reg) \ > > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ > > + ((_of_reg) | MFD_OF_REG_VALID), NULL) \ > > + > > #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat) \ > > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL) \ > > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ > > + 0, NULL) \ > > > > #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match) \ > > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match) \ > > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, \ > > + _match) \ > > > > #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id) \ > > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL) \ > > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \ > > > > #define MFD_CELL_RES(_name, _res) \ > > - MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL) \ > > + MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL) \ > > > > #define MFD_CELL_NAME(_name) \ > > - MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL) \ > > + MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL) \ > > > > struct irq_domain; > > struct property_entry; > > @@ -78,6 +89,9 @@ struct mfd_cell { > > */ > > const char *of_compatible; > > > > + /* matching the reg property if set */ Proper grammar please. "OF unit address for device matching" > > + unsigned int of_reg; > > + > > /* Matches ACPI */ > > const struct mfd_cell_acpi_match *acpi_match;
Am 2020-05-15 12:28, schrieb Lee Jones: > On Thu, 30 Apr 2020, Michael Walle wrote: > >> Hi Lee, >> >> Am 2020-04-23 19:45, schrieb Michael Walle: >> > There might be multiple children with the device tree compatible, for >> > example if a MFD has multiple instances of the same function. In this >> > case only the first is matched and the other children get a wrong >> > of_node reference. >> > Add a new option to match also against the unit address of the child >> > node. Additonally, a new helper OF_MFD_CELL_REG is added. >> >> >> Do you think this is feasible? I guess this is the biggest uncertainty >> for me at the moment in this patch series. > > I think it sounds fine in principle. So long as it doesn't change the > existing behaviour when of_reg isn't set. > >> > Signed-off-by: Michael Walle <michael@walle.cc> >> > --- >> > drivers/mfd/mfd-core.c | 29 ++++++++++++++++++++--------- >> > include/linux/mfd/core.h | 26 ++++++++++++++++++++------ >> > 2 files changed, 40 insertions(+), 15 deletions(-) >> > >> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c >> > index e735565969b3..4ecb376338f7 100644 >> > --- a/drivers/mfd/mfd-core.c >> > +++ b/drivers/mfd/mfd-core.c >> > @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, int >> > id, >> > struct device_node *np = NULL; >> > int ret = -ENOMEM; >> > int platform_id; >> > + u32 of_reg; >> > int r; >> > >> > if (id == PLATFORM_DEVID_AUTO) >> > @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent, >> > int id, >> > >> > if (parent->of_node && cell->of_compatible) { >> > for_each_child_of_node(parent->of_node, np) { >> > - if (of_device_is_compatible(np, cell->of_compatible)) { >> > - if (!of_device_is_available(np)) { >> > - /* Ignore disabled devices error free */ >> > - ret = 0; >> > - goto fail_alias; >> > - } >> > - pdev->dev.of_node = np; >> > - pdev->dev.fwnode = &np->fwnode; >> > - break; >> > + if (!of_device_is_compatible(np, cell->of_compatible)) >> > + continue; >> > + >> > + /* also match the unit address if set */ > > Please use correct grammar in comments (leaving off the full-stop). > >> > + if (cell->of_reg & MFD_OF_REG_VALID) { >> > + if (of_property_read_u32(np, "reg", &of_reg)) >> > + continue; >> > + if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg) >> > + continue; >> > } >> > + >> > + if (!of_device_is_available(np)) { >> > + /* Ignore disabled devices error free */ >> > + ret = 0; >> > + goto fail_alias; >> > + } >> > + >> > + pdev->dev.of_node = np; >> > + pdev->dev.fwnode = &np->fwnode; >> > + break; >> > } >> > } >> > >> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h >> > index d01d1299e49d..c2c0ad6b14f3 100644 >> > --- a/include/linux/mfd/core.h >> > +++ b/include/linux/mfd/core.h >> > @@ -13,8 +13,11 @@ >> > #include <linux/platform_device.h> >> > >> > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) >> > +#define MFD_OF_REG_VALID BIT(31) > > What about 64bit platforms? The idea was to have this as a logical number. I.e. for now you may only have one subdevice per unique compatible string. In fact, if you have a look at the ab8500.c, there are multiple "stericsson,ab8500-pwm" subdevices. But there is only one DT node for all three of it. I guess this works as long as you don't use phandles to reference the pwm node in the device tree. Or you don't want to use device tree properties per subdevice (for example the "timeout-sec" of a watchdog device). So to circumvent this, I thought of having the unit-address (and thus the "reg" property) to differentiate between multiple subdevices. Now there is one special case for me: this board management controller might be upgradable and it might change internally. Thus I came up with that logical numbering of subdevices. Rob doesn't seem to be a fan of that, though. Therefore, having bit 31 as a valid indicator leaves you with 2^31 logical devices, which should be enough ;) Rob proposed to have the internal offset as the unit-address. But in that case I can also use devm_of_platform_populate() and don't need the OF_MFD_CELL_REG; I'd just parse the reg offset in each individual subdevice driver. But like I said, I wanted to keep the internal offsets out of the device tree. -michael > >> > +#define MFD_OF_REG_MASK GENMASK(30, 0) >> > >> > -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, >> > _match)\ >> > +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ >> > + _of_reg, _match) \ >> > { \ >> > .name = (_name), \ >> > .resources = (_res), \ >> > @@ -22,24 +25,32 @@ >> > .platform_data = (_pdata), \ >> > .pdata_size = (_pdsize), \ >> > .of_compatible = (_compat), \ >> > + .of_reg = (_of_reg), \ >> > .acpi_match = (_match), \ >> > .id = (_id), \ >> > } >> > >> > +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat, \ >> > + _of_reg) \ >> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ >> > + ((_of_reg) | MFD_OF_REG_VALID), NULL) \ >> > + >> > #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat) \ >> > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL) \ >> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ >> > + 0, NULL) \ >> > >> > #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match) \ >> > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match) \ >> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, \ >> > + _match) \ >> > >> > #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id) \ >> > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL) \ >> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \ >> > >> > #define MFD_CELL_RES(_name, _res) \ >> > - MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL) \ >> > + MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL) \ >> > >> > #define MFD_CELL_NAME(_name) \ >> > - MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL) \ >> > + MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL) \ >> > >> > struct irq_domain; >> > struct property_entry; >> > @@ -78,6 +89,9 @@ struct mfd_cell { >> > */ >> > const char *of_compatible; >> > >> > + /* matching the reg property if set */ > > Proper grammar please. > > "OF unit address for device matching" > >> > + unsigned int of_reg; >> > + >> > /* Matches ACPI */ >> > const struct mfd_cell_acpi_match *acpi_match;
On Mon, 25 May 2020, Michael Walle wrote: > Am 2020-05-15 12:28, schrieb Lee Jones: > > On Thu, 30 Apr 2020, Michael Walle wrote: > > > > > Hi Lee, > > > > > > Am 2020-04-23 19:45, schrieb Michael Walle: > > > > There might be multiple children with the device tree compatible, for > > > > example if a MFD has multiple instances of the same function. In this > > > > case only the first is matched and the other children get a wrong > > > > of_node reference. > > > > Add a new option to match also against the unit address of the child > > > > node. Additonally, a new helper OF_MFD_CELL_REG is added. > > > > > > > > > Do you think this is feasible? I guess this is the biggest uncertainty > > > for me at the moment in this patch series. > > > > I think it sounds fine in principle. So long as it doesn't change the > > existing behaviour when of_reg isn't set. > > > > > > Signed-off-by: Michael Walle <michael@walle.cc> > > > > --- > > > > drivers/mfd/mfd-core.c | 29 ++++++++++++++++++++--------- > > > > include/linux/mfd/core.h | 26 ++++++++++++++++++++------ > > > > 2 files changed, 40 insertions(+), 15 deletions(-) [...] > > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h > > > > index d01d1299e49d..c2c0ad6b14f3 100644 > > > > --- a/include/linux/mfd/core.h > > > > +++ b/include/linux/mfd/core.h > > > > @@ -13,8 +13,11 @@ > > > > #include <linux/platform_device.h> > > > > > > > > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) > > > > +#define MFD_OF_REG_VALID BIT(31) > > > > What about 64bit platforms? > > The idea was to have this as a logical number. I.e. for now you may only > have one subdevice per unique compatible string. In fact, if you have a > look at the ab8500.c, there are multiple "stericsson,ab8500-pwm" > subdevices. But there is only one DT node for all three of it. I guess > this works as long as you don't use phandles to reference the pwm node > in the device tree. Or you don't want to use device tree properties > per subdevice (for example the "timeout-sec" of a watchdog device). > > So to circumvent this, I thought of having the unit-address (and thus > the "reg" property) to differentiate between multiple subdevices. Now > there is one special case for me: this board management controller > might be upgradable and it might change internally. Thus I came up > with that logical numbering of subdevices. Rob doesn't seem to be a > fan of that, though. Therefore, having bit 31 as a valid indicator > leaves you with 2^31 logical devices, which should be enough ;) > > Rob proposed to have the internal offset as the unit-address. But > in that case I can also use devm_of_platform_populate() and don't > need the OF_MFD_CELL_REG; I'd just parse the reg offset in each > individual subdevice driver. But like I said, I wanted to keep the > internal offsets out of the device tree. Oh, I see what you're doing. So you're adding an arbitrary ID to the device's reg property in DT? How is this not a hack? Why don't you use the full address for identification?
Am 2020-05-26 09:24, schrieb Lee Jones: > On Mon, 25 May 2020, Michael Walle wrote: > >> Am 2020-05-15 12:28, schrieb Lee Jones: >> > On Thu, 30 Apr 2020, Michael Walle wrote: >> > >> > > Hi Lee, >> > > >> > > Am 2020-04-23 19:45, schrieb Michael Walle: >> > > > There might be multiple children with the device tree compatible, for >> > > > example if a MFD has multiple instances of the same function. In this >> > > > case only the first is matched and the other children get a wrong >> > > > of_node reference. >> > > > Add a new option to match also against the unit address of the child >> > > > node. Additonally, a new helper OF_MFD_CELL_REG is added. >> > > >> > > >> > > Do you think this is feasible? I guess this is the biggest uncertainty >> > > for me at the moment in this patch series. >> > >> > I think it sounds fine in principle. So long as it doesn't change the >> > existing behaviour when of_reg isn't set. >> > >> > > > Signed-off-by: Michael Walle <michael@walle.cc> >> > > > --- >> > > > drivers/mfd/mfd-core.c | 29 ++++++++++++++++++++--------- >> > > > include/linux/mfd/core.h | 26 ++++++++++++++++++++------ >> > > > 2 files changed, 40 insertions(+), 15 deletions(-) > > [...] > >> > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h >> > > > index d01d1299e49d..c2c0ad6b14f3 100644 >> > > > --- a/include/linux/mfd/core.h >> > > > +++ b/include/linux/mfd/core.h >> > > > @@ -13,8 +13,11 @@ >> > > > #include <linux/platform_device.h> >> > > > >> > > > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) >> > > > +#define MFD_OF_REG_VALID BIT(31) >> > >> > What about 64bit platforms? >> >> The idea was to have this as a logical number. I.e. for now you may >> only >> have one subdevice per unique compatible string. In fact, if you have >> a >> look at the ab8500.c, there are multiple "stericsson,ab8500-pwm" >> subdevices. But there is only one DT node for all three of it. I guess >> this works as long as you don't use phandles to reference the pwm node >> in the device tree. Or you don't want to use device tree properties >> per subdevice (for example the "timeout-sec" of a watchdog device). >> >> So to circumvent this, I thought of having the unit-address (and thus >> the "reg" property) to differentiate between multiple subdevices. Now >> there is one special case for me: this board management controller >> might be upgradable and it might change internally. Thus I came up >> with that logical numbering of subdevices. Rob doesn't seem to be a >> fan of that, though. Therefore, having bit 31 as a valid indicator >> leaves you with 2^31 logical devices, which should be enough ;) >> >> Rob proposed to have the internal offset as the unit-address. But >> in that case I can also use devm_of_platform_populate() and don't >> need the OF_MFD_CELL_REG; I'd just parse the reg offset in each >> individual subdevice driver. But like I said, I wanted to keep the >> internal offsets out of the device tree. > > Oh, I see what you're doing. > > So you're adding an arbitrary ID to the device's reg property in DT? Yes. > How is this not a hack? Well IMHO this is not more or less a hack as the current of_node handling of MFD devices, which happens to work only because there is only one device per compatible string (or it doesn't really work, like in the stericsson,ab8500-pwm case). The of_node is assigned according to the compatible string, just like in my case, only that I have two subdevices with the same compatible string. > Why don't you use the full address for identification? Like I said, in the long term I would like to have support for different versions of the board management controller without having to change the device tree and have device tree bindings for the subdevices at the same time. But it seems, that this is not possible and I guess I have to bite the bullet and may need to provide another device tree if the controller might be updated. -michael
On Tue, May 26, 2020 at 05:54:38PM +0200, Michael Walle wrote: > Am 2020-05-26 09:24, schrieb Lee Jones: ... > Like I said, in the long term I would like to have support for > different versions of the board management controller > without having to change the device tree and have device tree bindings for the > subdevices at the same time. But isn't device tree to describe *very specific platform* rather than *class of platforms*? > But it seems, that this is not possible > and I guess I have to bite the bullet and may need to provide another > device tree if the controller might be updated.
On Tue, 26 May 2020, Andy Shevchenko wrote: > On Tue, May 26, 2020 at 05:54:38PM +0200, Michael Walle wrote: > > Am 2020-05-26 09:24, schrieb Lee Jones: > > ... > > > Like I said, in the long term I would like to have support for > > different versions of the board management controller > > > without having to change the device tree and have device tree bindings for the > > subdevices at the same time. > > But isn't device tree to describe *very specific platform* rather than *class > of platforms*? Yes. Device Tree describes the hardware. If the hardware changes, so must the Device Tree.
On Mon, 25 May 2020, Michael Walle wrote: > Am 2020-05-15 12:28, schrieb Lee Jones: > > On Thu, 30 Apr 2020, Michael Walle wrote: > > > > > Hi Lee, > > > > > > Am 2020-04-23 19:45, schrieb Michael Walle: > > > > There might be multiple children with the device tree compatible, for > > > > example if a MFD has multiple instances of the same function. In this > > > > case only the first is matched and the other children get a wrong > > > > of_node reference. > > > > Add a new option to match also against the unit address of the child > > > > node. Additonally, a new helper OF_MFD_CELL_REG is added. [...] > > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h > > > > index d01d1299e49d..c2c0ad6b14f3 100644 > > > > --- a/include/linux/mfd/core.h > > > > +++ b/include/linux/mfd/core.h > > > > @@ -13,8 +13,11 @@ > > > > #include <linux/platform_device.h> > > > > > > > > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) > > > > +#define MFD_OF_REG_VALID BIT(31) > > > > What about 64bit platforms? > > The idea was to have this as a logical number. I.e. for now you may only > have one subdevice per unique compatible string. In fact, if you have a > look at the ab8500.c, there are multiple "stericsson,ab8500-pwm" > subdevices. But there is only one DT node for all three of it. I guess > this works as long as you don't use phandles to reference the pwm node > in the device tree. Or you don't want to use device tree properties > per subdevice (for example the "timeout-sec" of a watchdog device). This is not a good example, as the "stericsson,ab8500-pwm" is legitimate. Here we are registering 3 potential devices, but only instantiating 1 of them.
Am 2020-06-08 16:24, schrieb Lee Jones: > On Mon, 25 May 2020, Michael Walle wrote: >> Am 2020-05-15 12:28, schrieb Lee Jones: >> > On Thu, 30 Apr 2020, Michael Walle wrote: >> > >> > > Hi Lee, >> > > >> > > Am 2020-04-23 19:45, schrieb Michael Walle: >> > > > There might be multiple children with the device tree compatible, for >> > > > example if a MFD has multiple instances of the same function. In this >> > > > case only the first is matched and the other children get a wrong >> > > > of_node reference. >> > > > Add a new option to match also against the unit address of the child >> > > > node. Additonally, a new helper OF_MFD_CELL_REG is added. > > [...] > >> > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h >> > > > index d01d1299e49d..c2c0ad6b14f3 100644 >> > > > --- a/include/linux/mfd/core.h >> > > > +++ b/include/linux/mfd/core.h >> > > > @@ -13,8 +13,11 @@ >> > > > #include <linux/platform_device.h> >> > > > >> > > > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) >> > > > +#define MFD_OF_REG_VALID BIT(31) >> > >> > What about 64bit platforms? >> >> The idea was to have this as a logical number. I.e. for now you may >> only >> have one subdevice per unique compatible string. In fact, if you have >> a >> look at the ab8500.c, there are multiple "stericsson,ab8500-pwm" >> subdevices. But there is only one DT node for all three of it. I guess >> this works as long as you don't use phandles to reference the pwm node >> in the device tree. Or you don't want to use device tree properties >> per subdevice (for example the "timeout-sec" of a watchdog device). > > This is not a good example, as the "stericsson,ab8500-pwm" is > legitimate. Here we are registering 3 potential devices, but only > instantiating 1 of them. Mh? static const struct mfd_cell ab8500_devs[] = { .. OF_MFD_CELL("ab8500-pwm", NULL, NULL, 0, 1, "stericsson,ab8500-pwm"), OF_MFD_CELL("ab8500-pwm", NULL, NULL, 0, 2, "stericsson,ab8500-pwm"), OF_MFD_CELL("ab8500-pwm", NULL, NULL, 0, 3, "stericsson,ab8500-pwm"), .. } And in pwm-ab8500.c there are three offsets based on the pdev->id. Am I missing something here?
On Mon, 08 Jun 2020, Michael Walle wrote: > Am 2020-06-08 16:24, schrieb Lee Jones: > > On Mon, 25 May 2020, Michael Walle wrote: > > > Am 2020-05-15 12:28, schrieb Lee Jones: > > > > On Thu, 30 Apr 2020, Michael Walle wrote: > > > > > > > > > Hi Lee, > > > > > > > > > > Am 2020-04-23 19:45, schrieb Michael Walle: > > > > > > There might be multiple children with the device tree compatible, for > > > > > > example if a MFD has multiple instances of the same function. In this > > > > > > case only the first is matched and the other children get a wrong > > > > > > of_node reference. > > > > > > Add a new option to match also against the unit address of the child > > > > > > node. Additonally, a new helper OF_MFD_CELL_REG is added. > > > > [...] > > > > > > > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h > > > > > > index d01d1299e49d..c2c0ad6b14f3 100644 > > > > > > --- a/include/linux/mfd/core.h > > > > > > +++ b/include/linux/mfd/core.h > > > > > > @@ -13,8 +13,11 @@ > > > > > > #include <linux/plataorm_device.h> > > > > > > > > > > > > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) > > > > > > +#define MFD_OF_REG_VALID BIT(31) > > > > > > > > What about 64bit platforms? > > > > > > The idea was to have this as a logical number. I.e. for now you may > > > only > > > have one subdevice per unique compatible string. In fact, if you > > > have a > > > look at the ab8500.c, there are multiple "stericsson,ab8500-pwm" > > > subdevices. But there is only one DT node for all three of it. I guess > > > this works as long as you don't use phandles to reference the pwm node > > > in the device tree. Or you don't want to use device tree properties > > > per subdevice (for example the "timeout-sec" of a watchdog device). > > > > This is not a good example, as the "stericsson,ab8500-pwm" is > > legitimate. Here we are registering 3 potential devices, but only > > instantiating 1 of them. > > Mh? > > static const struct mfd_cell ab8500_devs[] = { > .. > OF_MFD_CELL("ab8500-pwm", > NULL, NULL, 0, 1, "stericsson,ab8500-pwm"), > OF_MFD_CELL("ab8500-pwm", > NULL, NULL, 0, 2, "stericsson,ab8500-pwm"), > OF_MFD_CELL("ab8500-pwm", > NULL, NULL, 0, 3, "stericsson,ab8500-pwm"), > .. > } > > And in pwm-ab8500.c there are three offsets based on the pdev->id. > > Am I missing something here? Scrap what I said above. For some reason I had of_platform_populate() in my head. This will register and enumerate 3 devices.
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c index e735565969b3..4ecb376338f7 100644 --- a/drivers/mfd/mfd-core.c +++ b/drivers/mfd/mfd-core.c @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, int id, struct device_node *np = NULL; int ret = -ENOMEM; int platform_id; + u32 of_reg; int r; if (id == PLATFORM_DEVID_AUTO) @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent, int id, if (parent->of_node && cell->of_compatible) { for_each_child_of_node(parent->of_node, np) { - if (of_device_is_compatible(np, cell->of_compatible)) { - if (!of_device_is_available(np)) { - /* Ignore disabled devices error free */ - ret = 0; - goto fail_alias; - } - pdev->dev.of_node = np; - pdev->dev.fwnode = &np->fwnode; - break; + if (!of_device_is_compatible(np, cell->of_compatible)) + continue; + + /* also match the unit address if set */ + if (cell->of_reg & MFD_OF_REG_VALID) { + if (of_property_read_u32(np, "reg", &of_reg)) + continue; + if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg) + continue; } + + if (!of_device_is_available(np)) { + /* Ignore disabled devices error free */ + ret = 0; + goto fail_alias; + } + + pdev->dev.of_node = np; + pdev->dev.fwnode = &np->fwnode; + break; } } diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h index d01d1299e49d..c2c0ad6b14f3 100644 --- a/include/linux/mfd/core.h +++ b/include/linux/mfd/core.h @@ -13,8 +13,11 @@ #include <linux/platform_device.h> #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource)) +#define MFD_OF_REG_VALID BIT(31) +#define MFD_OF_REG_MASK GENMASK(30, 0) -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, _match)\ +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ + _of_reg, _match) \ { \ .name = (_name), \ .resources = (_res), \ @@ -22,24 +25,32 @@ .platform_data = (_pdata), \ .pdata_size = (_pdsize), \ .of_compatible = (_compat), \ + .of_reg = (_of_reg), \ .acpi_match = (_match), \ .id = (_id), \ } +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat, \ + _of_reg) \ + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ + ((_of_reg) | MFD_OF_REG_VALID), NULL) \ + #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat) \ - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL) \ + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \ + 0, NULL) \ #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match) \ - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match) \ + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, \ + _match) \ #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id) \ - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL) \ + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \ #define MFD_CELL_RES(_name, _res) \ - MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL) \ + MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL) \ #define MFD_CELL_NAME(_name) \ - MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL) \ + MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL) \ struct irq_domain; struct property_entry; @@ -78,6 +89,9 @@ struct mfd_cell { */ const char *of_compatible; + /* matching the reg property if set */ + unsigned int of_reg; + /* Matches ACPI */ const struct mfd_cell_acpi_match *acpi_match;
There might be multiple children with the device tree compatible, for example if a MFD has multiple instances of the same function. In this case only the first is matched and the other children get a wrong of_node reference. Add a new option to match also against the unit address of the child node. Additonally, a new helper OF_MFD_CELL_REG is added. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/mfd/mfd-core.c | 29 ++++++++++++++++++++--------- include/linux/mfd/core.h | 26 ++++++++++++++++++++------ 2 files changed, 40 insertions(+), 15 deletions(-)