| Message ID | 52F917CEA1B9C64C94833D53889D478C01AA4F33@dor-sms-xch01.digi.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | MFD with multiple PWM chips | expand |
Hello Hector, I'm not sure why I'm in cc but here is my 2 cents: On 05/11/2018 11:50:12+0000, Palacios, Hector wrote: > Hello, > > I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. > > The device tree looks like this: > > &i2c0 { > mca@63 { > ... > mca_pwm { > compatible = "digi,mca-pwm"; > #address-cells = <1>; > #size-cells = <0>; > Why don't you simply have pwm-channels = <10>; #pwm-cells = <2>; at this position? The child nodes don't seems that necessary to me, especially since you have to handle everything from a single driver anyway. > mca_pwm0: pwm@0 { > reg = <0>; > pwm-channels = <6>; > #pwm-cells = <2>; > }; > > mca_pwm1: pwm@1 { > reg = <1>; > pwm-channels = <2>; > #pwm-cells = <2>; > }; > > mca_pwm2: pwm@2 { > reg = <2>; > pwm-channels = <2>; > #pwm-cells = <2>; > }; > }; > > Being an MFD driver, the PWM driver is only probed once for the node "pwms". > I can iterate through the children, initialize them and add the PWM chips with 'pwmchip_add()'. This works and I can then request and use the different channels via the sysfs on each of the three controllers. > > The problem comes when another driver wants to use a PWM, for example: > > backlight { > compatible = "pwm-backlight"; > pwms = <&mca_pwm2 0 50000>; > brightness-levels = <0 4 8 16 32 64 128 255>; > default-brightness-level = <6>; > status = "okay"; > }; > > This doesn't won't work because the PWM framework only knows about the "pwms" node (which is the parent of my PWM chips in the DT), so the call 'of_pwm_get()' returns with "PWM chip not found". > I guess it is not common to have several PWM chips inside a single controller and that is why the framework doesn't support it. Other subsystems, like the regulators, do support declaring them as children, but not the PWM. > > What would be the best approach to support it? > A patch like this makes it work but I guess it's not ideal: > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 172ef8245811..f4a4cbdda0b2 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -627,6 +627,29 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) > return ERR_PTR(-EPROBE_DEFER); > } > > +static struct pwm_chip *of_subnode_to_pwmchip(struct device_node *np) > +{ > + struct pwm_chip *chip; > + struct device_node *node; > + > + mutex_lock(&pwm_lock); > + > + list_for_each_entry(chip, &pwm_chips, list) { > + if (chip->dev && chip->dev->of_node) { > + for_each_child_of_node(chip->dev->of_node, node) { > + if (node == np) { > + mutex_unlock(&pwm_lock); > + return chip; > + } > + } > + } > + } > + > + mutex_unlock(&pwm_lock); > + > + return ERR_PTR(-EPROBE_DEFER); > +} > + > /** > * of_pwm_get() - request a PWM via the PWM framework > * @np: device node to get the PWM from > @@ -669,9 +692,17 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) > > pc = of_node_to_pwmchip(args.np); > if (IS_ERR(pc)) { > - pr_debug("%s(): PWM chip not found\n", __func__); > - pwm = ERR_CAST(pc); > - goto put; > + /* > + * Check the subnodes of the device before returning with error, > + * just in case it is a device with multiple pwmchips defined as > + * subnodes. > + */ > + pc = of_subnode_to_pwmchip(args.np); > + if (IS_ERR(pc)) { > + pr_debug("%s(): PWM chip not found\n", __func__); > + pwm = ERR_CAST(pc); > + goto put; > + } > } > > if (args.args_count != pc->of_pwm_n_cells) { > > Thanks, > -- > Héctor Palacios > >
Hi Alexandre, On 11/5/18 1:11 PM, Alexandre Belloni wrote: > Hello Hector, > > I'm not sure why I'm in cc but here is my 2 cents: > > On 05/11/2018 11:50:12+0000, Palacios, Hector wrote: >> Hello, >> >> I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. >> >> The device tree looks like this: >> >> &i2c0 { >> mca@63 { >> ... >> mca_pwm { >> compatible = "digi,mca-pwm"; >> #address-cells = <1>; >> #size-cells = <0>; >> > > Why don't you simply have > > pwm-channels = <10>; > #pwm-cells = <2>; > > at this position? > > The child nodes don't seems that necessary to me, especially since you > have to handle everything from a single driver anyway. Each PWM controller can have a different frequency (although it is the same for all channels in it) and different implementations of the firmware (this is a micro-controller IC) may have more or less channels in each controller, so the separation is convenient. The registers where the channels are programmed are also different depending on the PWM controller. With a plain DT I would need to hard-code in the driver what PWM controller each channel belongs to. > >> mca_pwm0: pwm@0 { >> reg = <0>; >> pwm-channels = <6>; >> #pwm-cells = <2>; >> }; >> >> mca_pwm1: pwm@1 { >> reg = <1>; >> pwm-channels = <2>; >> #pwm-cells = <2>; >> }; >> >> mca_pwm2: pwm@2 { >> reg = <2>; >> pwm-channels = <2>; >> #pwm-cells = <2>; >> }; >> }; >> >> Being an MFD driver, the PWM driver is only probed once for the node "pwms". >> I can iterate through the children, initialize them and add the PWM chips with 'pwmchip_add()'. This works and I can then request and use the different channels via the sysfs on each of the three controllers. >> >> The problem comes when another driver wants to use a PWM, for example: >> >> backlight { >> compatible = "pwm-backlight"; >> pwms = <&mca_pwm2 0 50000>; >> brightness-levels = <0 4 8 16 32 64 128 255>; >> default-brightness-level = <6>; >> status = "okay"; >> }; >> >> This doesn't won't work because the PWM framework only knows about the "pwms" node (which is the parent of my PWM chips in the DT), so the call 'of_pwm_get()' returns with "PWM chip not found". >> I guess it is not common to have several PWM chips inside a single controller and that is why the framework doesn't support it. Other subsystems, like the regulators, do support declaring them as children, but not the PWM. >> >> What would be the best approach to support it? >> A patch like this makes it work but I guess it's not ideal: >> >> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c >> index 172ef8245811..f4a4cbdda0b2 100644 >> --- a/drivers/pwm/core.c >> +++ b/drivers/pwm/core.c >> @@ -627,6 +627,29 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) >> return ERR_PTR(-EPROBE_DEFER); >> } >> >> +static struct pwm_chip *of_subnode_to_pwmchip(struct device_node *np) >> +{ >> + struct pwm_chip *chip; >> + struct device_node *node; >> + >> + mutex_lock(&pwm_lock); >> + >> + list_for_each_entry(chip, &pwm_chips, list) { >> + if (chip->dev && chip->dev->of_node) { >> + for_each_child_of_node(chip->dev->of_node, node) { >> + if (node == np) { >> + mutex_unlock(&pwm_lock); >> + return chip; >> + } >> + } >> + } >> + } >> + >> + mutex_unlock(&pwm_lock); >> + >> + return ERR_PTR(-EPROBE_DEFER); >> +} >> + >> /** >> * of_pwm_get() - request a PWM via the PWM framework >> * @np: device node to get the PWM from >> @@ -669,9 +692,17 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) >> >> pc = of_node_to_pwmchip(args.np); >> if (IS_ERR(pc)) { >> - pr_debug("%s(): PWM chip not found\n", __func__); >> - pwm = ERR_CAST(pc); >> - goto put; >> + /* >> + * Check the subnodes of the device before returning with error, >> + * just in case it is a device with multiple pwmchips defined as >> + * subnodes. >> + */ >> + pc = of_subnode_to_pwmchip(args.np); >> + if (IS_ERR(pc)) { >> + pr_debug("%s(): PWM chip not found\n", __func__); >> + pwm = ERR_CAST(pc); >> + goto put; >> + } >> } >> >> if (args.args_count != pc->of_pwm_n_cells) { >> Thanks -- Hector Palacios
Hello Hector, On Mon, Nov 05, 2018 at 11:50:12AM +0000, Palacios, Hector wrote: > I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. > > The device tree looks like this: > > &i2c0 { > mca@63 { > ... > mca_pwm { > compatible = "digi,mca-pwm"; > #address-cells = <1>; > #size-cells = <0>; > > mca_pwm0: pwm@0 { > reg = <0>; > pwm-channels = <6>; > #pwm-cells = <2>; > }; > > mca_pwm1: pwm@1 { > reg = <1>; > pwm-channels = <2>; > #pwm-cells = <2>; > }; > > mca_pwm2: pwm@2 { > reg = <2>; > pwm-channels = <2>; > #pwm-cells = <2>; > }; > }; > > Being an MFD driver, the PWM driver is only probed once for the node "pwms". not sure how this relates to being an MFD driver, but you can also do the following: mca_pwm { #address-cells = <2>; #size-cells = <0>; mca_pwm00: pwm@00 { compatible = "..."; reg = <0 0>; #pwm-cells = <...>; } mca_pwm01: pwm@01 { compatible = "..."; reg = <0 1>; #pwm-cells = <...>; } ... mca_pwm05: pwm@05 { compatible = "..."; reg = <0 5>; #pwm-cells = <...>; } mca_pwm05: pwm@10 { compatible = "..."; reg = <1 0>; #pwm-cells = <...>; } ... } Best regards and greetings to Spain, Uwe
On 07/11/2018 10:25:21+0100, Uwe Kleine-König wrote: > Hello Hector, > > On Mon, Nov 05, 2018 at 11:50:12AM +0000, Palacios, Hector wrote: > > I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. > > > > The device tree looks like this: > > > > &i2c0 { > > mca@63 { > > ... > > mca_pwm { > > compatible = "digi,mca-pwm"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > mca_pwm0: pwm@0 { > > reg = <0>; > > pwm-channels = <6>; > > #pwm-cells = <2>; > > }; > > > > mca_pwm1: pwm@1 { > > reg = <1>; > > pwm-channels = <2>; > > #pwm-cells = <2>; > > }; > > > > mca_pwm2: pwm@2 { > > reg = <2>; > > pwm-channels = <2>; > > #pwm-cells = <2>; > > }; > > }; > > > > Being an MFD driver, the PWM driver is only probed once for the node "pwms". > > not sure how this relates to being an MFD driver, but you can also do > the following: > > mca_pwm { I guess you'd need compatible = "simple-mfd" here. > #address-cells = <2>; > #size-cells = <0>; > > mca_pwm00: pwm@00 { > compatible = "..."; > reg = <0 0>; > #pwm-cells = <...>; > } > > mca_pwm01: pwm@01 { > compatible = "..."; > reg = <0 1>; > #pwm-cells = <...>; > } > > ... > > mca_pwm05: pwm@05 { > compatible = "..."; > reg = <0 5>; > #pwm-cells = <...>; > } > > mca_pwm05: pwm@10 { > compatible = "..."; > reg = <1 0>; > #pwm-cells = <...>; > } > > ... > } > > Best regards and greetings to Spain, > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe, On 11/7/18 10:25 AM, Uwe Kleine-König wrote: > Hello Hector, > > On Mon, Nov 05, 2018 at 11:50:12AM +0000, Palacios, Hector wrote: >> I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. >> >> The device tree looks like this: >> >> &i2c0 { >> mca@63 { >> ... >> mca_pwm { >> compatible = "digi,mca-pwm"; >> #address-cells = <1>; >> #size-cells = <0>; >> >> mca_pwm0: pwm@0 { >> reg = <0>; >> pwm-channels = <6>; >> #pwm-cells = <2>; >> }; >> >> mca_pwm1: pwm@1 { >> reg = <1>; >> pwm-channels = <2>; >> #pwm-cells = <2>; >> }; >> >> mca_pwm2: pwm@2 { >> reg = <2>; >> pwm-channels = <2>; >> #pwm-cells = <2>; >> }; >> }; >> >> Being an MFD driver, the PWM driver is only probed once for the node "pwms". > > not sure how this relates to being an MFD driver, but you can also do > the following: > > mca_pwm { > #address-cells = <2>; > #size-cells = <0>; > > mca_pwm00: pwm@00 { > compatible = "..."; > reg = <0 0>; > #pwm-cells = <...>; > } > > mca_pwm01: pwm@01 { > compatible = "..."; > reg = <0 1>; > #pwm-cells = <...>; > } > > ... > > mca_pwm05: pwm@05 { > compatible = "..."; > reg = <0 5>; > #pwm-cells = <...>; > } > > mca_pwm05: pwm@10 { > compatible = "..."; > reg = <1 0>; > #pwm-cells = <...>; > } > > ... > } Being an MFD driver, I have an entry for each cell/driver the micro-controller handles: GPIO, watchdog, RTC, PWM: static const struct mfd_cell mca_devs[] = { { .name = MCA_DRVNAME_RTC, .num_resources = ARRAY_SIZE(mca_rtc_resources), .resources = mca_rtc_resources, .of_compatible = "digi,mca-rtc", }, { .name = MCA_DRVNAME_WATCHDOG, .num_resources = ARRAY_SIZE(mca_watchdog_resources), .resources = mca_watchdog_resources, .of_compatible = "digi,mca-watchdog", }, { .name = MCA_DRVNAME_GPIO, .num_resources = ARRAY_SIZE(mca_gpios_resources), .resources = mca_gpios_resources, .of_compatible = "digi,mca-gpio", }, ... { .name = MCA_DRVNAME_PWM, .num_resources = ARRAY_SIZE(mca_pwm_resources), .resources = mca_pwm_resources, .of_compatible = "digi,mca-gpio", }, }; This means there is only *one probe call* for each entry/cell/driver in the MFD core driver, no matter how many 'compatible' entries I add on the DT. Having only one probe call, I also only have one 'device' and one 'of_node' to refer to (the parent of the PWM controllers). > > Best regards and greetings to Spain, > Uwe > Thanks and regards :) -- Hector Palacios
On 07/11/2018 09:39:16+0000, Palacios, Hector wrote: > Hi Uwe, > > On 11/7/18 10:25 AM, Uwe Kleine-König wrote: > > Hello Hector, > > > > On Mon, Nov 05, 2018 at 11:50:12AM +0000, Palacios, Hector wrote: > >> I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. > >> > >> The device tree looks like this: > >> > >> &i2c0 { > >> mca@63 { > >> ... > >> mca_pwm { > >> compatible = "digi,mca-pwm"; > >> #address-cells = <1>; > >> #size-cells = <0>; > >> > >> mca_pwm0: pwm@0 { > >> reg = <0>; > >> pwm-channels = <6>; > >> #pwm-cells = <2>; > >> }; > >> > >> mca_pwm1: pwm@1 { > >> reg = <1>; > >> pwm-channels = <2>; > >> #pwm-cells = <2>; > >> }; > >> > >> mca_pwm2: pwm@2 { > >> reg = <2>; > >> pwm-channels = <2>; > >> #pwm-cells = <2>; > >> }; > >> }; > >> > >> Being an MFD driver, the PWM driver is only probed once for the node "pwms". > > > > not sure how this relates to being an MFD driver, but you can also do > > the following: > > > > mca_pwm { > > #address-cells = <2>; > > #size-cells = <0>; > > > > mca_pwm00: pwm@00 { > > compatible = "..."; > > reg = <0 0>; > > #pwm-cells = <...>; > > } > > > > mca_pwm01: pwm@01 { > > compatible = "..."; > > reg = <0 1>; > > #pwm-cells = <...>; > > } > > > > ... > > > > mca_pwm05: pwm@05 { > > compatible = "..."; > > reg = <0 5>; > > #pwm-cells = <...>; > > } > > > > mca_pwm05: pwm@10 { > > compatible = "..."; > > reg = <1 0>; > > #pwm-cells = <...>; > > } > > > > ... > > } > > Being an MFD driver, I have an entry for each cell/driver the > micro-controller handles: GPIO, watchdog, RTC, PWM: > > static const struct mfd_cell mca_devs[] = { > { > .name = MCA_DRVNAME_RTC, > .num_resources = ARRAY_SIZE(mca_rtc_resources), > .resources = mca_rtc_resources, > .of_compatible = "digi,mca-rtc", > }, > { > .name = MCA_DRVNAME_WATCHDOG, > .num_resources = ARRAY_SIZE(mca_watchdog_resources), > .resources = mca_watchdog_resources, > .of_compatible = "digi,mca-watchdog", > }, > { > .name = MCA_DRVNAME_GPIO, > .num_resources = ARRAY_SIZE(mca_gpios_resources), > .resources = mca_gpios_resources, > .of_compatible = "digi,mca-gpio", > }, > > ... > > { > .name = MCA_DRVNAME_PWM, > .num_resources = ARRAY_SIZE(mca_pwm_resources), > .resources = mca_pwm_resources, > .of_compatible = "digi,mca-gpio", > }, > }; > > This means there is only *one probe call* for each entry/cell/driver in > the MFD core driver, no matter how many 'compatible' entries I add on > the DT. > Having only one probe call, I also only have one 'device' and one > 'of_node' to refer to (the parent of the PWM controllers). > Yes, Uwe's point was to have one compatible/device/probe per PWM subnode instead of one for all the PWMs.
On 11/7/18 10:48 AM, Alexandre Belloni wrote: > On 07/11/2018 09:39:16+0000, Palacios, Hector wrote: >> Hi Uwe, >> >> On 11/7/18 10:25 AM, Uwe Kleine-König wrote: >>> Hello Hector, >>> >>> On Mon, Nov 05, 2018 at 11:50:12AM +0000, Palacios, Hector wrote: >>>> I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. >>>> >>>> The device tree looks like this: >>>> >>>> &i2c0 { >>>> mca@63 { >>>> ... >>>> mca_pwm { >>>> compatible = "digi,mca-pwm"; >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> >>>> mca_pwm0: pwm@0 { >>>> reg = <0>; >>>> pwm-channels = <6>; >>>> #pwm-cells = <2>; >>>> }; >>>> >>>> mca_pwm1: pwm@1 { >>>> reg = <1>; >>>> pwm-channels = <2>; >>>> #pwm-cells = <2>; >>>> }; >>>> >>>> mca_pwm2: pwm@2 { >>>> reg = <2>; >>>> pwm-channels = <2>; >>>> #pwm-cells = <2>; >>>> }; >>>> }; >>>> >>>> Being an MFD driver, the PWM driver is only probed once for the node "pwms". >>> >>> not sure how this relates to being an MFD driver, but you can also do >>> the following: >>> >>> mca_pwm { >>> #address-cells = <2>; >>> #size-cells = <0>; >>> >>> mca_pwm00: pwm@00 { >>> compatible = "..."; >>> reg = <0 0>; >>> #pwm-cells = <...>; >>> } >>> >>> mca_pwm01: pwm@01 { >>> compatible = "..."; >>> reg = <0 1>; >>> #pwm-cells = <...>; >>> } >>> >>> ... >>> >>> mca_pwm05: pwm@05 { >>> compatible = "..."; >>> reg = <0 5>; >>> #pwm-cells = <...>; >>> } >>> >>> mca_pwm05: pwm@10 { >>> compatible = "..."; >>> reg = <1 0>; >>> #pwm-cells = <...>; >>> } >>> >>> ... >>> } >> >> Being an MFD driver, I have an entry for each cell/driver the >> micro-controller handles: GPIO, watchdog, RTC, PWM: >> >> static const struct mfd_cell mca_devs[] = { >> { >> .name = MCA_DRVNAME_RTC, >> .num_resources = ARRAY_SIZE(mca_rtc_resources), >> .resources = mca_rtc_resources, >> .of_compatible = "digi,mca-rtc", >> }, >> { >> .name = MCA_DRVNAME_WATCHDOG, >> .num_resources = ARRAY_SIZE(mca_watchdog_resources), >> .resources = mca_watchdog_resources, >> .of_compatible = "digi,mca-watchdog", >> }, >> { >> .name = MCA_DRVNAME_GPIO, >> .num_resources = ARRAY_SIZE(mca_gpios_resources), >> .resources = mca_gpios_resources, >> .of_compatible = "digi,mca-gpio", >> }, >> >> ... >> >> { >> .name = MCA_DRVNAME_PWM, >> .num_resources = ARRAY_SIZE(mca_pwm_resources), >> .resources = mca_pwm_resources, >> .of_compatible = "digi,mca-gpio", >> }, >> }; >> >> This means there is only *one probe call* for each entry/cell/driver in >> the MFD core driver, no matter how many 'compatible' entries I add on >> the DT. >> Having only one probe call, I also only have one 'device' and one >> 'of_node' to refer to (the parent of the PWM controllers). >> > > Yes, Uwe's point was to have one compatible/device/probe per PWM subnode > instead of one for all the PWMs. Maybe I'm missing something but wouldn't I then need to add mfd_cells for every PWM device to my MFD core driver? I had already tried Uwe's approach before he mentioned it but then realized that the MFD I2C driver was really only calling probe once per each mfd_cell. I need something like the PMIC DA9063 with its regulators. The MFD core driver only has once cell for the regulators: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/da9063-core.c#n79 but it has multiple regulators defined in the DTS: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx6q-ba16.dtsi#n202 Although none of them has a 'compatible' string, they can all be referenced by other nodes in the DT and be handled separately. The regulator driver probe is only called once (as per the unique cell in the MFD driver) and the DT is parsed to get and register each regulator in a loop: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/da9063-regulator.c#n853 The trick is, in the regulators framework, there is a 'regulator_config' struct that has an of_node field which the driver sets to each regulator node in the DT. That doesn't exist on the PWM framework. Regards, -- Hector Palacios
On Wed, Nov 07, 2018 at 11:41:42AM +0000, Palacios, Hector wrote: > On 11/7/18 10:48 AM, Alexandre Belloni wrote: > > On 07/11/2018 09:39:16+0000, Palacios, Hector wrote: > >> Hi Uwe, > >> > >> On 11/7/18 10:25 AM, Uwe Kleine-König wrote: > >>> Hello Hector, > >>> > >>> On Mon, Nov 05, 2018 at 11:50:12AM +0000, Palacios, Hector wrote: > >>>> I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. > >>>> > >>>> The device tree looks like this: > >>>> > >>>> &i2c0 { > >>>> mca@63 { > >>>> ... > >>>> mca_pwm { > >>>> compatible = "digi,mca-pwm"; > >>>> #address-cells = <1>; > >>>> #size-cells = <0>; > >>>> > >>>> mca_pwm0: pwm@0 { > >>>> reg = <0>; > >>>> pwm-channels = <6>; > >>>> #pwm-cells = <2>; > >>>> }; > >>>> > >>>> mca_pwm1: pwm@1 { > >>>> reg = <1>; > >>>> pwm-channels = <2>; > >>>> #pwm-cells = <2>; > >>>> }; > >>>> > >>>> mca_pwm2: pwm@2 { > >>>> reg = <2>; > >>>> pwm-channels = <2>; > >>>> #pwm-cells = <2>; > >>>> }; > >>>> }; > >>>> > >>>> Being an MFD driver, the PWM driver is only probed once for the node "pwms". > >>> > >>> not sure how this relates to being an MFD driver, but you can also do > >>> the following: > >>> > >>> mca_pwm { > >>> #address-cells = <2>; > >>> #size-cells = <0>; > >>> > >>> mca_pwm00: pwm@00 { > >>> compatible = "..."; > >>> reg = <0 0>; > >>> #pwm-cells = <...>; > >>> } > >>> > >>> mca_pwm01: pwm@01 { > >>> compatible = "..."; > >>> reg = <0 1>; > >>> #pwm-cells = <...>; > >>> } > >>> > >>> ... > >>> > >>> mca_pwm05: pwm@05 { > >>> compatible = "..."; > >>> reg = <0 5>; > >>> #pwm-cells = <...>; > >>> } > >>> > >>> mca_pwm05: pwm@10 { > >>> compatible = "..."; > >>> reg = <1 0>; > >>> #pwm-cells = <...>; > >>> } > >>> > >>> ... > >>> } > >> > >> Being an MFD driver, I have an entry for each cell/driver the > >> micro-controller handles: GPIO, watchdog, RTC, PWM: > >> > >> static const struct mfd_cell mca_devs[] = { > >> { > >> .name = MCA_DRVNAME_RTC, > >> .num_resources = ARRAY_SIZE(mca_rtc_resources), > >> .resources = mca_rtc_resources, > >> .of_compatible = "digi,mca-rtc", > >> }, > >> { > >> .name = MCA_DRVNAME_WATCHDOG, > >> .num_resources = ARRAY_SIZE(mca_watchdog_resources), > >> .resources = mca_watchdog_resources, > >> .of_compatible = "digi,mca-watchdog", > >> }, > >> { > >> .name = MCA_DRVNAME_GPIO, > >> .num_resources = ARRAY_SIZE(mca_gpios_resources), > >> .resources = mca_gpios_resources, > >> .of_compatible = "digi,mca-gpio", > >> }, > >> > >> ... > >> > >> { > >> .name = MCA_DRVNAME_PWM, > >> .num_resources = ARRAY_SIZE(mca_pwm_resources), > >> .resources = mca_pwm_resources, > >> .of_compatible = "digi,mca-gpio", > >> }, > >> }; > >> > >> This means there is only *one probe call* for each entry/cell/driver in > >> the MFD core driver, no matter how many 'compatible' entries I add on > >> the DT. > >> Having only one probe call, I also only have one 'device' and one > >> 'of_node' to refer to (the parent of the PWM controllers). > >> > > > > Yes, Uwe's point was to have one compatible/device/probe per PWM subnode > > instead of one for all the PWMs. > > Maybe I'm missing something but wouldn't I then need to add mfd_cells > for every PWM device to my MFD core driver? > I had already tried Uwe's approach before he mentioned it but then > realized that the MFD I2C driver was really only calling probe once per > each mfd_cell. > > I need something like the PMIC DA9063 with its regulators. The MFD core > driver only has once cell for the regulators: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/da9063-core.c#n79 > > but it has multiple regulators defined in the DTS: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx6q-ba16.dtsi#n202 > Although none of them has a 'compatible' string, they can all be > referenced by other nodes in the DT and be handled separately. > > The regulator driver probe is only called once (as per the unique cell > in the MFD driver) and the DT is parsed to get and register each > regulator in a loop: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/da9063-regulator.c#n853 > > The trick is, in the regulators framework, there is a 'regulator_config' > struct that has an of_node field which the driver sets to each regulator > node in the DT. That doesn't exist on the PWM framework. It should be fairly easy to implement that manually, though. All of the necessary helpers exist (da9063 uses them, though perhaps hidden in the regulator helpers). With the PWM framework you can't attach a PWM chip to just a struct device_node, so you'll have to have a full struct device for it. The easiest way to make this work would probably be to have an MFD driver that dynamically registers subdevices based on the nodes it finds in device tree. Then a PWM driver could bind against each of these devices and set pwm_chip.dev to point to the platform device's ->dev field. You can probably automate most of that using of_platform_populate(), which may already work automatically if you set the compatible string of the top-level node to "simple-bus". Thierry
Hello, On Wed, Nov 07, 2018 at 04:49:30PM +0100, Thierry Reding wrote: > On Wed, Nov 07, 2018 at 11:41:42AM +0000, Palacios, Hector wrote: > > The trick is, in the regulators framework, there is a 'regulator_config' > > struct that has an of_node field which the driver sets to each regulator > > node in the DT. That doesn't exist on the PWM framework. > > It should be fairly easy to implement that manually, though. All of the > necessary helpers exist (da9063 uses them, though perhaps hidden in the > regulator helpers). > > With the PWM framework you can't attach a PWM chip to just a struct > device_node, so you'll have to have a full struct device for it. The > easiest way to make this work would probably be to have an MFD driver > that dynamically registers subdevices based on the nodes it finds in > device tree. Then a PWM driver could bind against each of these devices > and set pwm_chip.dev to point to the platform device's ->dev field. You > can probably automate most of that using of_platform_populate(), which > may already work automatically if you set the compatible string of the > top-level node to "simple-bus". I think technically there is no difference between "simple-bus" and "simple-mfd" (that Alexandre already suggested). Semantically the latter is probably the better. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 172ef8245811..f4a4cbdda0b2 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -627,6 +627,29 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) return ERR_PTR(-EPROBE_DEFER); } +static struct pwm_chip *of_subnode_to_pwmchip(struct device_node *np) +{ + struct pwm_chip *chip; + struct device_node *node; + + mutex_lock(&pwm_lock); + + list_for_each_entry(chip, &pwm_chips, list) { + if (chip->dev && chip->dev->of_node) { + for_each_child_of_node(chip->dev->of_node, node) { + if (node == np) { + mutex_unlock(&pwm_lock); + return chip; + } + } + } + } + + mutex_unlock(&pwm_lock); + + return ERR_PTR(-EPROBE_DEFER); +} + /** * of_pwm_get() - request a PWM via the PWM framework * @np: device node to get the PWM from @@ -669,9 +692,17 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) pc = of_node_to_pwmchip(args.np); if (IS_ERR(pc)) { - pr_debug("%s(): PWM chip not found\n", __func__); - pwm = ERR_CAST(pc); - goto put; + /* + * Check the subnodes of the device before returning with error, + * just in case it is a device with multiple pwmchips defined as + * subnodes. + */ + pc = of_subnode_to_pwmchip(args.np); + if (IS_ERR(pc)) { + pr_debug("%s(): PWM chip not found\n", __func__); + pwm = ERR_CAST(pc); + goto put; + } } if (args.args_count != pc->of_pwm_n_cells) {
Hello, I have an MFD device connected via I2C. This chip has several PWM controllers, each one containing a number of PWM channels. The device tree looks like this: &i2c0 { mca@63 { ... mca_pwm { compatible = "digi,mca-pwm"; #address-cells = <1>; #size-cells = <0>; mca_pwm0: pwm@0 { reg = <0>; pwm-channels = <6>; #pwm-cells = <2>; }; mca_pwm1: pwm@1 { reg = <1>; pwm-channels = <2>; #pwm-cells = <2>; }; mca_pwm2: pwm@2 { reg = <2>; pwm-channels = <2>; #pwm-cells = <2>; }; }; Being an MFD driver, the PWM driver is only probed once for the node "pwms". I can iterate through the children, initialize them and add the PWM chips with 'pwmchip_add()'. This works and I can then request and use the different channels via the sysfs on each of the three controllers. The problem comes when another driver wants to use a PWM, for example: backlight { compatible = "pwm-backlight"; pwms = <&mca_pwm2 0 50000>; brightness-levels = <0 4 8 16 32 64 128 255>; default-brightness-level = <6>; status = "okay"; }; This doesn't won't work because the PWM framework only knows about the "pwms" node (which is the parent of my PWM chips in the DT), so the call 'of_pwm_get()' returns with "PWM chip not found". I guess it is not common to have several PWM chips inside a single controller and that is why the framework doesn't support it. Other subsystems, like the regulators, do support declaring them as children, but not the PWM. What would be the best approach to support it? A patch like this makes it work but I guess it's not ideal: Thanks, -- Héctor Palacios