MFD with multiple PWM chips

Message ID 52F917CEA1B9C64C94833D53889D478C01AA4F33@dor-sms-xch01.digi.com
State New
Headers show
Series
  • MFD with multiple PWM chips
Related show

Commit Message

Palacios, Hector Nov. 5, 2018, 11:50 a.m.
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

Comments

Alexandre Belloni Nov. 5, 2018, 12:11 p.m. | #1
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
> 
>
Palacios, Hector Nov. 5, 2018, 4:04 p.m. | #2
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
Uwe Kleine-König Nov. 7, 2018, 9:25 a.m. | #3
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
Alexandre Belloni Nov. 7, 2018, 9:34 a.m. | #4
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/  |
Palacios, Hector Nov. 7, 2018, 9:39 a.m. | #5
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
Alexandre Belloni Nov. 7, 2018, 9:48 a.m. | #6
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.
Palacios, Hector Nov. 7, 2018, 11:41 a.m. | #7
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
Thierry Reding Nov. 7, 2018, 3:49 p.m. | #8
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
Uwe Kleine-König Nov. 7, 2018, 3:59 p.m. | #9
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

Patch

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) {