| Message ID | 20190726184045.14669-5-jernej.skrabec@siol.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: sun4i: Add support for Allwinner H6 | expand |
On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > Now that sun4i PWM driver supports deasserting reset line and enabling > bus clock, support for H6 PWM can be added. > > Note that while H6 PWM has two channels, only first one is wired to > output pin. Second channel is used as a clock source to companion AC200 > chip which is bundled into same package. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/pwm/pwm-sun4i.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > index 7d3ac3f2dc3f..9e0eca79ff88 100644 > --- a/drivers/pwm/pwm-sun4i.c > +++ b/drivers/pwm/pwm-sun4i.c > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data sun4i_pwm_single_bypass = { > .npwm = 1, > }; > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = { > + .has_bus_clock = true, > + .has_prescaler_bypass = true, > + .has_reset = true, > + .npwm = 2, > +}; > + > static const struct of_device_id sun4i_pwm_dt_ids[] = { > { > .compatible = "allwinner,sun4i-a10-pwm", > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = { > }, { > .compatible = "allwinner,sun8i-h3-pwm", > .data = &sun4i_pwm_single_bypass, > + }, { > + .compatible = "allwinner,sun50i-h6-pwm", > + .data = &sun50i_pwm_dual_bypass_clk_rst, If you follow my suggestion for the two previous patches, you can just use: compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; and drop this patch. Best regards Uwe > }, { > /* sentinel */ > }, > -- > 2.22.0 > >
Hi Uwe, Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König napisal(a): > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > Now that sun4i PWM driver supports deasserting reset line and enabling > > bus clock, support for H6 PWM can be added. > > > > Note that while H6 PWM has two channels, only first one is wired to > > output pin. Second channel is used as a clock source to companion AC200 > > chip which is bundled into same package. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/pwm/pwm-sun4i.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index 7d3ac3f2dc3f..9e0eca79ff88 100644 > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > sun4i_pwm_single_bypass = {> > > .npwm = 1, > > > > }; > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = { > > + .has_bus_clock = true, > > + .has_prescaler_bypass = true, > > + .has_reset = true, > > + .npwm = 2, > > +}; > > + > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > { > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = > > { > > > > }, { > > > > .compatible = "allwinner,sun8i-h3-pwm", > > .data = &sun4i_pwm_single_bypass, > > > > + }, { > > + .compatible = "allwinner,sun50i-h6-pwm", > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > If you follow my suggestion for the two previous patches, you can just > use: > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > and drop this patch. Maxime found out that it's not compatible with A10s due to difference in bypass bit, but yes, I know what you mean. Since H6 requires reset line and bus clock to be specified, it's not compatible from DT binding side. New yaml based binding must somehow know that in order to be able to validate DT node, so it needs standalone compatible. However, depending on conclusions of other discussions, this new compatible can be associated with already available quirks structure or have it's own. Best regards, Jernej > > Best regards > Uwe > > > }, { > > > > /* sentinel */ > > > > },
On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > Hi Uwe, > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König > napisal(a): > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > Now that sun4i PWM driver supports deasserting reset line and enabling > > > bus clock, support for H6 PWM can be added. > > > > > > Note that while H6 PWM has two channels, only first one is wired to > > > output pin. Second channel is used as a clock source to companion AC200 > > > chip which is bundled into same package. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/pwm/pwm-sun4i.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > index 7d3ac3f2dc3f..9e0eca79ff88 100644 > > > --- a/drivers/pwm/pwm-sun4i.c > > > +++ b/drivers/pwm/pwm-sun4i.c > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > sun4i_pwm_single_bypass = {> > > > .npwm = 1, > > > > > > }; > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = { > > > + .has_bus_clock = true, > > > + .has_prescaler_bypass = true, > > > + .has_reset = true, > > > + .npwm = 2, > > > +}; > > > + > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > { > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = > > > { > > > > > > }, { > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > .data = &sun4i_pwm_single_bypass, > > > > > > + }, { > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > If you follow my suggestion for the two previous patches, you can just > > use: > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > > > and drop this patch. > > Maxime found out that it's not compatible with A10s due to difference in bypass > bit, but yes, I know what you mean. > > Since H6 requires reset line and bus clock to be specified, it's not compatible > from DT binding side. New yaml based binding must somehow know that in order > to be able to validate DT node, so it needs standalone compatible. However, > depending on conclusions of other discussions, this new compatible can be > associated with already available quirks structure or have it's own. I cannot follow. You should be able to specify in the binding that the reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm without a reset line and bus clock also verifies, but this doesn't really hurt (and who knows, maybe the next allwinner chip needs exactly this). Best regards Uwe
On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > Hi Uwe, > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König > > napisal(a): > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > Now that sun4i PWM driver supports deasserting reset line and enabling > > > > bus clock, support for H6 PWM can be added. > > > > > > > > Note that while H6 PWM has two channels, only first one is wired to > > > > output pin. Second channel is used as a clock source to companion AC200 > > > > chip which is bundled into same package. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > --- > > > > > > > > drivers/pwm/pwm-sun4i.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > > index 7d3ac3f2dc3f..9e0eca79ff88 100644 > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > sun4i_pwm_single_bypass = {> > > > > .npwm = 1, > > > > > > > > }; > > > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = { > > > > + .has_bus_clock = true, > > > > + .has_prescaler_bypass = true, > > > > + .has_reset = true, > > > > + .npwm = 2, > > > > +}; > > > > + > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > { > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = > > > > { > > > > > > > > }, { > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > + }, { > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > If you follow my suggestion for the two previous patches, you can just > > > use: > > > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > > > > > and drop this patch. > > > > Maxime found out that it's not compatible with A10s due to difference in bypass > > bit, but yes, I know what you mean. > > > > Since H6 requires reset line and bus clock to be specified, it's not compatible > > from DT binding side. New yaml based binding must somehow know that in order > > to be able to validate DT node, so it needs standalone compatible. However, > > depending on conclusions of other discussions, this new compatible can be > > associated with already available quirks structure or have it's own. > > I cannot follow. You should be able to specify in the binding that the > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > without a reset line and bus clock also verifies, but this doesn't > really hurt (and who knows, maybe the next allwinner chip needs exactly > this). It is not optional. It will not work if either the clocks or reset controls are missing. How would these be optional anyway? Either it's connected and thus required, or it's not and therefore should be omitted from the description. ChenYu
Hello, On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König > > > napisal(a): > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > sun4i_pwm_single_bypass = {> > > > > > .npwm = 1, > > > > > > > > > > }; > > > > > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = { > > > > > + .has_bus_clock = true, > > > > > + .has_prescaler_bypass = true, > > > > > + .has_reset = true, > > > > > + .npwm = 2, > > > > > +}; > > > > > + > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > { > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = > > > > > { > > > > > > > > > > }, { > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > + }, { > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > If you follow my suggestion for the two previous patches, you can just > > > > use: > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > > > > > > > and drop this patch. > > > > > > Maxime found out that it's not compatible with A10s due to difference in bypass > > > bit, but yes, I know what you mean. > > > > > > Since H6 requires reset line and bus clock to be specified, it's not compatible > > > from DT binding side. New yaml based binding must somehow know that in order > > > to be able to validate DT node, so it needs standalone compatible. However, > > > depending on conclusions of other discussions, this new compatible can be > > > associated with already available quirks structure or have it's own. > > > > I cannot follow. You should be able to specify in the binding that the > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > > without a reset line and bus clock also verifies, but this doesn't > > really hurt (and who knows, maybe the next allwinner chip needs exactly > > this). > > It is not optional. It will not work if either the clocks or reset controls > are missing. How would these be optional anyway? Either it's connected and > thus required, or it's not and therefore should be omitted from the > description. [Just arguing about the clock here, the argumentation is analogous for the reset control.] From the driver's perspective it's optional: There are devices with and without a bus clock. This doesn't mean that you can just ignore this clock if it's specified. It's optional in the sense "If dt doesn't specify it, then assume this is a device that doesn't have it and so you don't need to handle it." but not in the sense "it doesn't matter if you handle it or not.". Other than that I'm on your side. So for example I think it's not optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n because this hides exactly the kind of problem you point out here. Best regards Uwe
Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König napisal(a): > Hello, > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König > > > > > > > > napisal(a): > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > }; > > > > > > > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst > > > > > > = { > > > > > > + .has_bus_clock = true, > > > > > > + .has_prescaler_bypass = true, > > > > > > + .has_reset = true, > > > > > > + .npwm = 2, > > > > > > +}; > > > > > > + > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > { > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > sun4i_pwm_dt_ids[] = > > > > > > { > > > > > > > > > > > > }, { > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > + }, { > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > If you follow my suggestion for the two previous patches, you can > > > > > just > > > > > > > > > > use: > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > and drop this patch. > > > > > > > > Maxime found out that it's not compatible with A10s due to difference > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's not > > > > compatible from DT binding side. New yaml based binding must somehow > > > > know that in order to be able to validate DT node, so it needs > > > > standalone compatible. However, depending on conclusions of other > > > > discussions, this new compatible can be associated with already > > > > available quirks structure or have it's own.> > > > > I cannot follow. You should be able to specify in the binding that the > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > > > without a reset line and bus clock also verifies, but this doesn't > > > really hurt (and who knows, maybe the next allwinner chip needs exactly > > > this). > > > > It is not optional. It will not work if either the clocks or reset > > controls > > are missing. How would these be optional anyway? Either it's connected and > > thus required, or it's not and therefore should be omitted from the > > description. > > [Just arguing about the clock here, the argumentation is analogous for > the reset control.] > > From the driver's perspective it's optional: There are devices with and > without a bus clock. This doesn't mean that you can just ignore this > clock if it's specified. It's optional in the sense "If dt doesn't > specify it, then assume this is a device that doesn't have it and so you > don't need to handle it." but not in the sense "it doesn't matter if > you handle it or not.". > > Other than that I'm on your side. So for example I think it's not > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > because this hides exactly the kind of problem you point out here. > I think there's misunderstanding. I only argued that we can't use compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; as you suggested and only compatible = "allwinner,sun50i-h6-pwm"; will work. Not because of driver itself (it can still use _optional() variants), but because of DT binding, which should be able to validate H6 PWM node - reset and bus clock references are required in this case. Best regards, Jernej > Best regards > Uwe
On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König > napisal(a): > > Hello, > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König > > > > > > > > > > napisal(a): > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst > > > > > > > = { > > > > > > > + .has_bus_clock = true, > > > > > > > + .has_prescaler_bypass = true, > > > > > > > + .has_reset = true, > > > > > > > + .npwm = 2, > > > > > > > +}; > > > > > > > + > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > > > { > > > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > > sun4i_pwm_dt_ids[] = > > > > > > > { > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > + }, { > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > > > If you follow my suggestion for the two previous patches, you can > > > > > > just > > > > > > > > > > > > use: > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > and drop this patch. > > > > > > > > > > Maxime found out that it's not compatible with A10s due to difference > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's not > > > > > compatible from DT binding side. New yaml based binding must somehow > > > > > know that in order to be able to validate DT node, so it needs > > > > > standalone compatible. However, depending on conclusions of other > > > > > discussions, this new compatible can be associated with already > > > > > available quirks structure or have it's own.> > > > > > I cannot follow. You should be able to specify in the binding that the > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > > > > without a reset line and bus clock also verifies, but this doesn't > > > > really hurt (and who knows, maybe the next allwinner chip needs exactly > > > > this). > > > > > > It is not optional. It will not work if either the clocks or reset > > > controls > > > are missing. How would these be optional anyway? Either it's connected and > > > thus required, or it's not and therefore should be omitted from the > > > description. > > > > [Just arguing about the clock here, the argumentation is analogous for > > the reset control.] > > > > From the driver's perspective it's optional: There are devices with and > > without a bus clock. This doesn't mean that you can just ignore this > > clock if it's specified. It's optional in the sense "If dt doesn't > > specify it, then assume this is a device that doesn't have it and so you > > don't need to handle it." but not in the sense "it doesn't matter if > > you handle it or not.". > > > > Other than that I'm on your side. So for example I think it's not > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > > because this hides exactly the kind of problem you point out here. > > > > I think there's misunderstanding. I only argued that we can't use > > compatible = "allwinner,sun50i-h6-pwm", > "allwinner,sun5i-a10s-pwm"; > > as you suggested and only > > compatible = "allwinner,sun50i-h6-pwm"; > > will work. Not because of driver itself (it can still use _optional() > variants), but because of DT binding, which should be able to validate H6 PWM > node - reset and bus clock references are required in this case. I think I understood. In my eyes there is no need to let validation of the DT bindings catch a missing "optional" property that is needed on H6. You have to draw the line somewhere which information the driver has hard-coded and what is only provided by the device tree and just assumed to be correct by the driver. You argue the driver should know that if it cares for a "allwinner,sun50i-h6-pwm" device it should know (and check) that there is a clock named "bus" and a resets property that links to a reset controller. How is that different from checking that the base address is 0x0300a000 or that the "pwm" clock is the osc24M clock running at 24 MHz? This isn't checked in the driver or the dt schema. Still if the device tree got one of them wrong this yields an non-working pwm device that isn't catched in the driver. Best regards Uwe
Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König napisal(a): > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König > > > > napisal(a): > > > Hello, > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe > > > > > > Kleine-König > > > > > > > > > > > > napisal(a): > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > +static const struct sun4i_pwm_data > > > > > > > > sun50i_pwm_dual_bypass_clk_rst > > > > > > > > = { > > > > > > > > + .has_bus_clock = true, > > > > > > > > + .has_prescaler_bypass = true, > > > > > > > > + .has_reset = true, > > > > > > > > + .npwm = 2, > > > > > > > > +}; > > > > > > > > + > > > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > > > > > { > > > > > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > > > sun4i_pwm_dt_ids[] = > > > > > > > > { > > > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > > > + }, { > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > > > > > If you follow my suggestion for the two previous patches, you > > > > > > > can > > > > > > > just > > > > > > > > > > > > > > use: > > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > > > and drop this patch. > > > > > > > > > > > > Maxime found out that it's not compatible with A10s due to > > > > > > difference > > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's > > > > > > not > > > > > > compatible from DT binding side. New yaml based binding must > > > > > > somehow > > > > > > know that in order to be able to validate DT node, so it needs > > > > > > standalone compatible. However, depending on conclusions of other > > > > > > discussions, this new compatible can be associated with already > > > > > > available quirks structure or have it's own.> > > > > > > > > > > > I cannot follow. You should be able to specify in the binding that > > > > > the > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > > > > > without a reset line and bus clock also verifies, but this doesn't > > > > > really hurt (and who knows, maybe the next allwinner chip needs > > > > > exactly > > > > > this). > > > > > > > > It is not optional. It will not work if either the clocks or reset > > > > controls > > > > are missing. How would these be optional anyway? Either it's connected > > > > and > > > > thus required, or it's not and therefore should be omitted from the > > > > description. > > > > > > [Just arguing about the clock here, the argumentation is analogous for > > > the reset control.] > > > > > > From the driver's perspective it's optional: There are devices with and > > > without a bus clock. This doesn't mean that you can just ignore this > > > clock if it's specified. It's optional in the sense "If dt doesn't > > > specify it, then assume this is a device that doesn't have it and so you > > > don't need to handle it." but not in the sense "it doesn't matter if > > > you handle it or not.". > > > > > > Other than that I'm on your side. So for example I think it's not > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > > > because this hides exactly the kind of problem you point out here. > > > > I think there's misunderstanding. I only argued that we can't use > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > "allwinner,sun5i-a10s-pwm"; > > > > as you suggested and only > > > > compatible = "allwinner,sun50i-h6-pwm"; > > > > will work. Not because of driver itself (it can still use _optional() > > variants), but because of DT binding, which should be able to validate H6 > > PWM node - reset and bus clock references are required in this case. > > I think I understood. In my eyes there is no need to let validation of > the DT bindings catch a missing "optional" property that is needed on > H6. > > You have to draw the line somewhere which information the driver has > hard-coded and what is only provided by the device tree and just assumed > to be correct by the driver. You argue the driver should know that No, in this thread I argue that DT validation tool, executed by make ARCH=arm64 dtbs_check should catch that. This is not a driver, but DT binding described in YAML. Best regards, Jernej > if it > cares for a "allwinner,sun50i-h6-pwm" device it should know (and check) > that there is a clock named "bus" and a resets property that links to a > reset controller. How is that different from checking that the base > address is 0x0300a000 or that the "pwm" clock is the osc24M clock > running at 24 MHz? This isn't checked in the driver or the dt schema. > Still if the device tree got one of them wrong this yields an > non-working pwm device that isn't catched in the driver. > > Best regards > Uwe
On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König > napisal(a): > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote: > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König > > > > > > napisal(a): > > > > Hello, > > > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe > > > > > > > Kleine-König > > > > > > > > > > > > > > napisal(a): > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > +static const struct sun4i_pwm_data > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst > > > > > > > > > = { > > > > > > > > > + .has_bus_clock = true, > > > > > > > > > + .has_prescaler_bypass = true, > > > > > > > > > + .has_reset = true, > > > > > > > > > + .npwm = 2, > > > > > > > > > +}; > > > > > > > > > + > > > > > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > > > > sun4i_pwm_dt_ids[] = > > > > > > > > > { > > > > > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > > > > > + }, { > > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > > > > > > > If you follow my suggestion for the two previous patches, you > > > > > > > > can > > > > > > > > just > > > > > > > > > > > > > > > > use: > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > > > > > and drop this patch. > > > > > > > > > > > > > > Maxime found out that it's not compatible with A10s due to > > > > > > > difference > > > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's > > > > > > > not > > > > > > > compatible from DT binding side. New yaml based binding must > > > > > > > somehow > > > > > > > know that in order to be able to validate DT node, so it needs > > > > > > > standalone compatible. However, depending on conclusions of other > > > > > > > discussions, this new compatible can be associated with already > > > > > > > available quirks structure or have it's own.> > > > > > > > > > > > > > I cannot follow. You should be able to specify in the binding that > > > > > > the > > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > > > > > > without a reset line and bus clock also verifies, but this doesn't > > > > > > really hurt (and who knows, maybe the next allwinner chip needs > > > > > > exactly > > > > > > this). > > > > > > > > > > It is not optional. It will not work if either the clocks or reset > > > > > controls > > > > > are missing. How would these be optional anyway? Either it's connected > > > > > and > > > > > thus required, or it's not and therefore should be omitted from the > > > > > description. > > > > > > > > [Just arguing about the clock here, the argumentation is analogous for > > > > the reset control.] > > > > > > > > From the driver's perspective it's optional: There are devices with and > > > > without a bus clock. This doesn't mean that you can just ignore this > > > > clock if it's specified. It's optional in the sense "If dt doesn't > > > > specify it, then assume this is a device that doesn't have it and so you > > > > don't need to handle it." but not in the sense "it doesn't matter if > > > > you handle it or not.". > > > > > > > > Other than that I'm on your side. So for example I think it's not > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > > > > because this hides exactly the kind of problem you point out here. > > > > > > I think there's misunderstanding. I only argued that we can't use > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > as you suggested and only > > > > > > compatible = "allwinner,sun50i-h6-pwm"; > > > > > > will work. Not because of driver itself (it can still use _optional() > > > variants), but because of DT binding, which should be able to validate H6 > > > PWM node - reset and bus clock references are required in this case. > > > > I think I understood. In my eyes there is no need to let validation of > > the DT bindings catch a missing "optional" property that is needed on > > H6. > > > > You have to draw the line somewhere which information the driver has > > hard-coded and what is only provided by the device tree and just assumed > > to be correct by the driver. You argue the driver should know that > > No, in this thread I argue that DT validation tool, executed by > > make ARCH=arm64 dtbs_check > > should catch that. This is not a driver, but DT binding described in YAML. The argumentation is the same. dtbs_check doesn't notice if the base address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should it catch a missing reset controller phandle? Best regards Uwe
Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König napisal(a): > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König > > > > napisal(a): > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote: > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König > > > > > > > > napisal(a): > > > > > Hello, > > > > > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > > > > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe > > > > > > > > Kleine-König > > > > > > > > > > > > > > > > napisal(a): > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > +static const struct sun4i_pwm_data > > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst > > > > > > > > > > = { > > > > > > > > > > + .has_bus_clock = true, > > > > > > > > > > + .has_prescaler_bypass = true, > > > > > > > > > > + .has_reset = true, > > > > > > > > > > + .npwm = 2, > > > > > > > > > > +}; > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > > > > > sun4i_pwm_dt_ids[] = > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > > > > > > > + }, { > > > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > > > > > > > > > If you follow my suggestion for the two previous patches, > > > > > > > > > you > > > > > > > > > can > > > > > > > > > just > > > > > > > > > > > > > > > > > > use: > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > > > > > > > and drop this patch. > > > > > > > > > > > > > > > > Maxime found out that it's not compatible with A10s due to > > > > > > > > difference > > > > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > > > > > > > Since H6 requires reset line and bus clock to be specified, > > > > > > > > it's > > > > > > > > not > > > > > > > > compatible from DT binding side. New yaml based binding must > > > > > > > > somehow > > > > > > > > know that in order to be able to validate DT node, so it needs > > > > > > > > standalone compatible. However, depending on conclusions of > > > > > > > > other > > > > > > > > discussions, this new compatible can be associated with > > > > > > > > already > > > > > > > > available quirks structure or have it's own.> > > > > > > > > > > > > > > > I cannot follow. You should be able to specify in the binding > > > > > > > that > > > > > > > the > > > > > > > reset line and bus clock is optional. Then > > > > > > > allwinner,sun50i-h6-pwm > > > > > > > without a reset line and bus clock also verifies, but this > > > > > > > doesn't > > > > > > > really hurt (and who knows, maybe the next allwinner chip needs > > > > > > > exactly > > > > > > > this). > > > > > > > > > > > > It is not optional. It will not work if either the clocks or reset > > > > > > controls > > > > > > are missing. How would these be optional anyway? Either it's > > > > > > connected > > > > > > and > > > > > > thus required, or it's not and therefore should be omitted from > > > > > > the > > > > > > description. > > > > > > > > > > [Just arguing about the clock here, the argumentation is analogous > > > > > for > > > > > the reset control.] > > > > > > > > > > From the driver's perspective it's optional: There are devices with > > > > > and > > > > > without a bus clock. This doesn't mean that you can just ignore this > > > > > clock if it's specified. It's optional in the sense "If dt doesn't > > > > > specify it, then assume this is a device that doesn't have it and so > > > > > you > > > > > don't need to handle it." but not in the sense "it doesn't matter if > > > > > you handle it or not.". > > > > > > > > > > Other than that I'm on your side. So for example I think it's not > > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > > > > > because this hides exactly the kind of problem you point out here. > > > > > > > > I think there's misunderstanding. I only argued that we can't use > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > as you suggested and only > > > > > > > > compatible = "allwinner,sun50i-h6-pwm"; > > > > > > > > will work. Not because of driver itself (it can still use _optional() > > > > variants), but because of DT binding, which should be able to validate > > > > H6 > > > > PWM node - reset and bus clock references are required in this case. > > > > > > I think I understood. In my eyes there is no need to let validation of > > > the DT bindings catch a missing "optional" property that is needed on > > > H6. > > > > > > You have to draw the line somewhere which information the driver has > > > hard-coded and what is only provided by the device tree and just assumed > > > to be correct by the driver. You argue the driver should know that > > > > No, in this thread I argue that DT validation tool, executed by > > > > make ARCH=arm64 dtbs_check > > > > should catch that. This is not a driver, but DT binding described in YAML. > > The argumentation is the same. dtbs_check doesn't notice if the base > address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should > it catch a missing reset controller phandle? Of course checking actual values of node properties doesn't make sense in dtbs_check, otherwise we would have million DT bindings. If you have 10 copies of the same IP core, of course you would use same compatible, but actual register ranges, interrupts, etc. would be different in DT nodes. At this point I would make same argument as were made before, but there is no point going in circles. I'm interested what have DT maintainers to say. Best regards, Jernej
Hello Rob and Frank, Maxime and Jernej on one side and me on the other cannot agree about a detail in the change to the bindings here. I'm trying to objectively summarize the situation for you to help deciding what is the right thing to do here. TLDR: The sun4i pwm driver is extended to support a new variant of that device on the H6 SoC. Compared to the earlier supported variants allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an additional clock. The two positions are: - We need a new compatible because only then the driver and/or the dt schema checker can check that each "allwinner,sun50i-h6-pwm" device has a reset property and a "bus" clock; and the earlier variants don't. - The driver can be simpler and the device specific knowledge is only in a single place (the dt) if the device tree is considered valid and a reset property and "bus" clock is used iff it's provided in the device tree without additional comparison for the compatible. Now our arguments seem to go in circles and Jernej was interested in your position. That's something I agree with ;-) Can you please share your view? Find below some context about the arguments. Best regards Uwe On Tue, Jul 30, 2019 at 12:04:47AM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König > napisal(a): > > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote: > > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König > > > napisal(a): > > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote: > > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König > > > > > napisal(a): > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König > > > > > > > > > napisal(a): > > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > +static const struct sun4i_pwm_data > > > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst > > > > > > > > > > > = { > > > > > > > > > > > + .has_bus_clock = true, > > > > > > > > > > > + .has_prescaler_bypass = true, > > > > > > > > > > > + .has_reset = true, > > > > > > > > > > > + .npwm = 2, > > > > > > > > > > > +}; > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > > > > > > sun4i_pwm_dt_ids[] = > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > > > > > > > > > + }, { > > > > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > > > > > > > > > > > If you follow my suggestion for the two previous patches, (i.e. use devm_clk_get_optional instead of using devm_clk_get iff the compatible is allwinner,sun50i-h6-pwm; analogous for the reset controller.) > > > > > > > > > > you can just use: > > > > > > > > > > > > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > > > > > > > > > and drop this patch. > > > > > > > > > > > > > > > > > > Maxime found out that it's not compatible with A10s due to difference > > > > > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's not > > > > > > > > > compatible from DT binding side. New yaml based binding must somehow > > > > > > > > > know that in order to be able to validate DT node, so it needs > > > > > > > > > standalone compatible. However, depending on conclusions of other > > > > > > > > > discussions, this new compatible can be associated with already > > > > > > > > > available quirks structure or have it's own. > > > > > > > > > > > > > > > > I cannot follow. You should be able to specify in the binding that the > > > > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > > > > > > > > without a reset line and bus clock also verifies, but this doesn't > > > > > > > > really hurt (and who knows, maybe the next allwinner chip needs exactly this). > > > > > > > > > > > > > > It is not optional. It will not work if either the clocks or reset controls > > > > > > > are missing. How would these be optional anyway? Either it's connected and > > > > > > > thus required, or it's not and therefore should be omitted from the description. > > > > > > > > > > > > [Just arguing about the clock here, the argumentation is analogous for > > > > > > the reset control.] > > > > > > > > > > > > From the driver's perspective it's optional: There are devices with and > > > > > > without a bus clock. This doesn't mean that you can just ignore this > > > > > > clock if it's specified. It's optional in the sense "If dt doesn't > > > > > > specify it, then assume this is a device that doesn't have it and so you > > > > > > don't need to handle it." but not in the sense "it doesn't matter if > > > > > > you handle it or not.". > > > > > > > > > > > > Other than that I'm on your side. So for example I think it's not > > > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > > > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > > > > > > because this hides exactly the kind of problem you point out here. > > > > > > > > > > I think there's misunderstanding. I only argued that we can't use > > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > as you suggested and only > > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm"; > > > > > > > > > > will work. Not because of driver itself (it can still use _optional() > > > > > variants), but because of DT binding, which should be able to validate H6 > > > > > PWM node - reset and bus clock references are required in this case. > > > > > > > > I think I understood. In my eyes there is no need to let validation of > > > > the DT bindings catch a missing "optional" property that is needed on > > > > H6. > > > > > > > > You have to draw the line somewhere which information the driver has > > > > hard-coded and what is only provided by the device tree and just assumed > > > > to be correct by the driver. You argue the driver should know that > > > > > > No, in this thread I argue that DT validation tool, executed by > > > > > > make ARCH=arm64 dtbs_check > > > > > > should catch that. This is not a driver, but DT binding described in YAML. > > > > The argumentation is the same. dtbs_check doesn't notice if the base > > address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should > > it catch a missing reset controller phandle? > > Of course checking actual values of node properties doesn't make sense in > dtbs_check, otherwise we would have million DT bindings. If you have 10 copies > of the same IP core, of course you would use same compatible, but actual > register ranges, interrupts, etc. would be different in DT nodes. > > At this point I would make same argument as were made before, but there is no > point going in circles. I'm interested what have DT maintainers to say.
On Tue, Jul 30, 2019 at 4:09 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Rob and Frank, > > Maxime and Jernej on one side and me on the other cannot agree about a > detail in the change to the bindings here. I'm trying to objectively > summarize the situation for you to help deciding what is the right thing > to do here. > > TLDR: The sun4i pwm driver is extended to support a new variant of that > device on the H6 SoC. Compared to the earlier supported variants > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an > additional clock. > > The two positions are: > > - We need a new compatible because only then the driver and/or the dt > schema checker can check that each "allwinner,sun50i-h6-pwm" device > has a reset property and a "bus" clock; and the earlier variants > don't. > > - The driver can be simpler and the device specific knowledge is only > in a single place (the dt) if the device tree is considered valid and > a reset property and "bus" clock is used iff it's provided in the > device tree without additional comparison for the compatible. > > Now our arguments seem to go in circles and Jernej was interested in > your position. That's something I agree with ;-) Can you please share > your view? > > Find below some context about the arguments. A bit more context on the failure modes: If the reset control is missing, anything done to hardware will be silently ignored, since any writes to the registers are ignored. On the other hand, if the bus clock is missing and otherwise not enabled, accessing the device's registers could actually stall the whole system. ChenYu > Best regards > Uwe > > On Tue, Jul 30, 2019 at 12:04:47AM +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König > > napisal(a): > > > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote: > > > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König > > > > napisal(a): > > > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote: > > > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König > > > > > > napisal(a): > > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König > > > > > > > > > > napisal(a): > > > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > +static const struct sun4i_pwm_data > > > > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst > > > > > > > > > > > > = { > > > > > > > > > > > > + .has_bus_clock = true, > > > > > > > > > > > > + .has_prescaler_bypass = true, > > > > > > > > > > > > + .has_reset = true, > > > > > > > > > > > > + .npwm = 2, > > > > > > > > > > > > +}; > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > > > > > > > sun4i_pwm_dt_ids[] = > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > > > > > > > > > > > + }, { > > > > > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > > > > > > > > > > > > > If you follow my suggestion for the two previous patches, > > (i.e. use devm_clk_get_optional instead of using devm_clk_get iff the > compatible is allwinner,sun50i-h6-pwm; analogous for the reset > controller.) > > > > > > > > > > > > you can just use: > > > > > > > > > > > > > > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > > > > > > > > > > > and drop this patch. > > > > > > > > > > > > > > > > > > > > Maxime found out that it's not compatible with A10s due to difference > > > > > > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > > > > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's not > > > > > > > > > > compatible from DT binding side. New yaml based binding must somehow > > > > > > > > > > know that in order to be able to validate DT node, so it needs > > > > > > > > > > standalone compatible. However, depending on conclusions of other > > > > > > > > > > discussions, this new compatible can be associated with already > > > > > > > > > > available quirks structure or have it's own. > > > > > > > > > > > > > > > > > > I cannot follow. You should be able to specify in the binding that the > > > > > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm > > > > > > > > > without a reset line and bus clock also verifies, but this doesn't > > > > > > > > > really hurt (and who knows, maybe the next allwinner chip needs exactly this). > > > > > > > > > > > > > > > > It is not optional. It will not work if either the clocks or reset controls > > > > > > > > are missing. How would these be optional anyway? Either it's connected and > > > > > > > > thus required, or it's not and therefore should be omitted from the description. > > > > > > > > > > > > > > [Just arguing about the clock here, the argumentation is analogous for > > > > > > > the reset control.] > > > > > > > > > > > > > > From the driver's perspective it's optional: There are devices with and > > > > > > > without a bus clock. This doesn't mean that you can just ignore this > > > > > > > clock if it's specified. It's optional in the sense "If dt doesn't > > > > > > > specify it, then assume this is a device that doesn't have it and so you > > > > > > > don't need to handle it." but not in the sense "it doesn't matter if > > > > > > > you handle it or not.". > > > > > > > > > > > > > > Other than that I'm on your side. So for example I think it's not > > > > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > > > > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > > > > > > > because this hides exactly the kind of problem you point out here. > > > > > > > > > > > > I think there's misunderstanding. I only argued that we can't use > > > > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > as you suggested and only > > > > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm"; > > > > > > > > > > > > will work. Not because of driver itself (it can still use _optional() > > > > > > variants), but because of DT binding, which should be able to validate H6 > > > > > > PWM node - reset and bus clock references are required in this case. > > > > > > > > > > I think I understood. In my eyes there is no need to let validation of > > > > > the DT bindings catch a missing "optional" property that is needed on > > > > > H6. > > > > > > > > > > You have to draw the line somewhere which information the driver has > > > > > hard-coded and what is only provided by the device tree and just assumed > > > > > to be correct by the driver. You argue the driver should know that > > > > > > > > No, in this thread I argue that DT validation tool, executed by > > > > > > > > make ARCH=arm64 dtbs_check > > > > > > > > should catch that. This is not a driver, but DT binding described in YAML. > > > > > > The argumentation is the same. dtbs_check doesn't notice if the base > > > address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should > > > it catch a missing reset controller phandle? > > > > Of course checking actual values of node properties doesn't make sense in > > dtbs_check, otherwise we would have million DT bindings. If you have 10 copies > > of the same IP core, of course you would use same compatible, but actual > > register ranges, interrupts, etc. would be different in DT nodes. > > > > At this point I would make same argument as were made before, but there is no > > point going in circles. I'm interested what have DT maintainers to say. > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190730080900.hhxrqun7vk4nsj2h%40pengutronix.de.
On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote: > Hello Rob and Frank, > > Maxime and Jernej on one side and me on the other cannot agree about a > detail in the change to the bindings here. I'm trying to objectively > summarize the situation for you to help deciding what is the right thing > to do here. > > TLDR: The sun4i pwm driver is extended to support a new variant of that > device on the H6 SoC. Compared to the earlier supported variants > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an > additional clock. > > The two positions are: > > - We need a new compatible because only then the driver and/or the dt > schema checker can check that each "allwinner,sun50i-h6-pwm" device > has a reset property and a "bus" clock; and the earlier variants > don't. There is two topics here, really. The binding itself really must have those properties as required. You had an analogy before that we shouldn't really do that, since it would be verification and that it would be similar to checking whether the register range was right. This analogy isn't correct, a better one would be checking that the register range exists in the first place. Indeed, if you don't have a register range, you have no register to write to, and that's a showstopper for any driver. I'm pretty sure we all agree on that. But on those SoCs, like Chen-Yu said, having no resets or clocks properties result in an equally bad result: either any write to that device is completely ignored (missing reset), or the system completely (and silently) locks up (missing bus clock). We *have* to catch that somehow and not let anything like that happen. That being said, we can't really validate that the clock pointed is the right one, just like we can't really check that the register range is the proper one. Or rather, we could, but it's completely impractical and we do agree on that as well. Having the bus clock and reset line being required however is only a few lines in the binding though, and is very practical. > - The driver can be simpler and the device specific knowledge is only > in a single place (the dt) if the device tree is considered valid and > a reset property and "bus" clock is used iff it's provided in the > device tree without additional comparison for the compatible. And now we have the discussion on how it's implemented in a driver. Since it's pretty cheap to implement (only a couple of lines: two for the if block, one for the additional field in the structure, one for each SoC using that) and have huge benefits (not silently locking up the system at boot), then I'd say we should go for it. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Jul 30, 2019 at 07:06:01PM +0200, Maxime Ripard wrote: > On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote: > > Hello Rob and Frank, > > > > Maxime and Jernej on one side and me on the other cannot agree about a > > detail in the change to the bindings here. I'm trying to objectively > > summarize the situation for you to help deciding what is the right thing > > to do here. > > > > TLDR: The sun4i pwm driver is extended to support a new variant of that > > device on the H6 SoC. Compared to the earlier supported variants > > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an > > additional clock. > > > > The two positions are: > > > > - We need a new compatible because only then the driver and/or the dt > > schema checker can check that each "allwinner,sun50i-h6-pwm" device > > has a reset property and a "bus" clock; and the earlier variants > > don't. > > There is two topics here, really. The binding itself really must have > those properties as required. > > You had an analogy before that we shouldn't really do that, since it > would be verification and that it would be similar to checking whether > the register range was right. This analogy isn't correct, a better one > would be checking that the register range exists in the first place. The relevant difference is that *all* devices supported by the driver have to have a register range. Compared to that only a subset of the devices have to have a bus clock. > Indeed, if you don't have a register range, you have no register to > write to, and that's a showstopper for any driver. I'm pretty sure we > all agree on that. But on those SoCs, like Chen-Yu said, having no > resets or clocks properties result in an equally bad result: either > any write to that device is completely ignored (missing reset), or the > system completely (and silently) locks up (missing bus clock). > > We *have* to catch that somehow and not let anything like that happen. IIUC both the clock and the reset stuff is SoC specific, so it's the same for all machines with the H6, right? So assuming this is correctly contained in the h6.dtsi, in which cases can this go wrong? I only see the cases that the dts author includes the wrong dtsi or overrides stuff. In the first case a non-working PWM is probably one of the smaller problems and the second is something we're not really able to catch. But even if each machine's dts author has to get this right, I don't think the dts schema is the right place to assert this. > That being said, we can't really validate that the clock pointed is > the right one, just like we can't really check that the register range > is the proper one. Or rather, we could, but it's completely > impractical and we do agree on that as well. > > Having the bus clock and reset line being required however is only a > few lines in the binding though, and is very practical. > > > - The driver can be simpler and the device specific knowledge is only > > in a single place (the dt) if the device tree is considered valid and > > a reset property and "bus" clock is used iff it's provided in the > > device tree without additional comparison for the compatible. > > And now we have the discussion on how it's implemented in a > driver. Since it's pretty cheap to implement (only a couple of lines: > two for the if block, one for the additional field in the structure, > one for each SoC using that) and have huge benefits (not silently > locking up the system at boot), then I'd say we should go for it. Right, it's only a few lines. Still it (IMHO needlessly) complicates the driver. From the driver's POV the device tree defines the characteristics of the device and if the dts defines an h6-pwm without a bus clock then maybe this is the PWM on the next generation SoC that doesn't need it. And maybe you're happy in a few year's time when you don't have to touch the driver again for this next generation SoC because the driver is not only simpler but also flexible enough to handle the new PWM without adaptions. All in all I don't care much about the dt schema stuff, I want to keep the driver simple. So if we agree that the schema ensures that the h6 pwms have a reset and a bus clock and we just use reset_get_optional and clk_get_optional that's a compromise I can agree to. Best regards Uwe
On Wed, Jul 31, 2019 at 08:52:30AM +0200, Uwe Kleine-König wrote: > On Tue, Jul 30, 2019 at 07:06:01PM +0200, Maxime Ripard wrote: > > On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote: > > > Hello Rob and Frank, > > > > > > Maxime and Jernej on one side and me on the other cannot agree about a > > > detail in the change to the bindings here. I'm trying to objectively > > > summarize the situation for you to help deciding what is the right thing > > > to do here. > > > > > > TLDR: The sun4i pwm driver is extended to support a new variant of that > > > device on the H6 SoC. Compared to the earlier supported variants > > > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an > > > additional clock. > > > > > > The two positions are: > > > > > > - We need a new compatible because only then the driver and/or the dt > > > schema checker can check that each "allwinner,sun50i-h6-pwm" device > > > has a reset property and a "bus" clock; and the earlier variants > > > don't. > > > > There is two topics here, really. The binding itself really must have > > those properties as required. > > > > You had an analogy before that we shouldn't really do that, since it > > would be verification and that it would be similar to checking whether > > the register range was right. This analogy isn't correct, a better one > > would be checking that the register range exists in the first place. > > The relevant difference is that *all* devices supported by the driver > have to have a register range. Compared to that only a subset of the > devices have to have a bus clock. That's true, but it still have nothing to do with validating its presence vs its content. We never even mentionned the latter. > > Indeed, if you don't have a register range, you have no register to > > write to, and that's a showstopper for any driver. I'm pretty sure we > > all agree on that. But on those SoCs, like Chen-Yu said, having no > > resets or clocks properties result in an equally bad result: either > > any write to that device is completely ignored (missing reset), or the > > system completely (and silently) locks up (missing bus clock). > > > > We *have* to catch that somehow and not let anything like that happen. > > IIUC both the clock and the reset stuff is SoC specific, so it's the > same for all machines with the H6, right? Indeed > So assuming this is correctly contained in the h6.dtsi, in which > cases can this go wrong? I only see the cases that the dts author > includes the wrong dtsi or overrides stuff. The bootloader passed by the bootloader is not meant for Linux but for another OS, the bootloader loaded a DT not meant for mainline but some BSP that happen to have the same compatible, the user has applied a work in progress patch to its DT, and then updates the kernel, the user applied a poorly written overlay, etc... We really shouldn't support those cases in the first place, but a silent lockup of the system is the worst way to treat those errors. > In the first case a non-working PWM is probably one of the smaller > problems and the second is something we're not really able to catch. > > But even if each machine's dts author has to get this right, I don't > think the dts schema is the right place to assert this. We shouldn't assert this *only* in the schema, but if it's cheap and it can catch some mistakes, then why not? Worst case scenario, the DTSI will be correct all the time, and it will never generate any error. Just like 90% of all the constraints in the schemas. > > That being said, we can't really validate that the clock pointed is > > the right one, just like we can't really check that the register range > > is the proper one. Or rather, we could, but it's completely > > impractical and we do agree on that as well. > > > > Having the bus clock and reset line being required however is only a > > few lines in the binding though, and is very practical. > > > > > - The driver can be simpler and the device specific knowledge is only > > > in a single place (the dt) if the device tree is considered valid and > > > a reset property and "bus" clock is used iff it's provided in the > > > device tree without additional comparison for the compatible. > > > > And now we have the discussion on how it's implemented in a > > driver. Since it's pretty cheap to implement (only a couple of lines: > > two for the if block, one for the additional field in the structure, > > one for each SoC using that) and have huge benefits (not silently > > locking up the system at boot), then I'd say we should go for it. > > Right, it's only a few lines. Still it (IMHO needlessly) complicates the > driver. From the driver's POV the device tree defines the > characteristics of the device and if the dts defines an h6-pwm without a > bus clock then maybe this is the PWM on the next generation SoC that > doesn't need it. And maybe you're happy in a few year's time when you > don't have to touch the driver again for this next generation SoC > because the driver is not only simpler but also flexible enough to > handle the new PWM without adaptions. You've been doing SoC support for a while, how many times did this truly happen to you, whithout a single change to the driver? > All in all I don't care much about the dt schema stuff, I want to keep > the driver simple. So if we agree that the schema ensures that the h6 > pwms have a reset and a bus clock and we just use reset_get_optional and > clk_get_optional that's a compromise I can agree to. Fine, let's do that then Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello Maxime, the idea of my mail was to summarize quickly the discussion for the dt people to give their judgement to stop us circling in a discussion about the always same points. I suggest we stop the discussion here now and wait for a reply from them instead. Best regards Uwe
Dne ponedeljek, 12. avgust 2019 ob 12:47:00 CEST je Uwe Kleine-König napisal(a): > Hello Maxime, > > the idea of my mail was to summarize quickly the discussion for the dt > people to give their judgement to stop us circling in a discussion about > the always same points. > > I suggest we stop the discussion here now and wait for a reply from them > instead. Shouldn't we just go with compromise solution you suggested and Maxime accepted? I would like to send new version in time for 5.4. Best regards, Jernej > > Best regards > Uwe
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c index 7d3ac3f2dc3f..9e0eca79ff88 100644 --- a/drivers/pwm/pwm-sun4i.c +++ b/drivers/pwm/pwm-sun4i.c @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data sun4i_pwm_single_bypass = { .npwm = 1, }; +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = { + .has_bus_clock = true, + .has_prescaler_bypass = true, + .has_reset = true, + .npwm = 2, +}; + static const struct of_device_id sun4i_pwm_dt_ids[] = { { .compatible = "allwinner,sun4i-a10-pwm", @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = { }, { .compatible = "allwinner,sun8i-h3-pwm", .data = &sun4i_pwm_single_bypass, + }, { + .compatible = "allwinner,sun50i-h6-pwm", + .data = &sun50i_pwm_dual_bypass_clk_rst, }, { /* sentinel */ },
Now that sun4i PWM driver supports deasserting reset line and enabling bus clock, support for H6 PWM can be added. Note that while H6 PWM has two channels, only first one is wired to output pin. Second channel is used as a clock source to companion AC200 chip which is bundled into same package. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/pwm/pwm-sun4i.c | 10 ++++++++++ 1 file changed, 10 insertions(+)