[PATCH 2/2] pwm: samsung: fix the number of PWMs

[PATCH 2/2] pwm: samsung: fix the number of PWMs

[Jingoo Han]

Samsung SoC can provide 4 PWMs; thus, the number of PWMs should be
set as 4.

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/pwm/pwm-samsung.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index e5187c0..32562c6 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -228,7 +228,7 @@ static int s3c_pwm_probe(struct platform_device *pdev)
 	s3c->chip.dev = &pdev->dev;
 	s3c->chip.ops = &s3c_pwm_ops;
 	s3c->chip.base = -1;
-	s3c->chip.npwm = 1;
+	s3c->chip.npwm = 4;
 
 	s3c->clk = devm_clk_get(dev, "pwm-tin");
 	if (IS_ERR(s3c->clk)) {
-- 
1.7.1

Re: [PATCH 2/2] pwm: samsung: fix the number of PWMs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Thu, Aug 02, 2012 at 05:56:27PM +0900, Jingoo Han wrote:
> Samsung SoC can provide 4 PWMs; thus, the number of PWMs should be
> set as 4.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> ---
>  drivers/pwm/pwm-samsung.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> index e5187c0..32562c6 100644
> --- a/drivers/pwm/pwm-samsung.c
> +++ b/drivers/pwm/pwm-samsung.c
> @@ -228,7 +228,7 @@ static int s3c_pwm_probe(struct platform_device *pdev)
>  	s3c->chip.dev = &pdev->dev;
>  	s3c->chip.ops = &s3c_pwm_ops;
>  	s3c->chip.base = -1;
> -	s3c->chip.npwm = 1;
> +	s3c->chip.npwm = 4;

I don't think this is correct. The driver seems to be using the platform
device id as index currently, which indicates that the driver is bound
against 4 different platform devices, each representing a single PWM. If
you want to service multiple PWM devices by a single instance you need
to make further changes to the driver. For instance the tcon_base is
initialized based on the platform id. This could easily be replaced by
making it depend on the .hwpwm member of pwm_device.

You may also want to consider making the driver a proper module if that
works on the platforms that need it. I see that it is currently an
arch_initcall, but it might be cleaner to use deferred driver probe
instead.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] pwm: samsung: fix the number of PWMs

[Jingoo Han]

On Thursday, August 02, 2012 6:54 PM Thierry Reding wrote:
> 
> On Thu, Aug 02, 2012 at 05:56:27PM +0900, Jingoo Han wrote:
> > Samsung SoC can provide 4 PWMs; thus, the number of PWMs should be
> > set as 4.
> >
> > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > ---
> >  drivers/pwm/pwm-samsung.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> > index e5187c0..32562c6 100644
> > --- a/drivers/pwm/pwm-samsung.c
> > +++ b/drivers/pwm/pwm-samsung.c
> > @@ -228,7 +228,7 @@ static int s3c_pwm_probe(struct platform_device *pdev)
> >  	s3c->chip.dev = &pdev->dev;
> >  	s3c->chip.ops = &s3c_pwm_ops;
> >  	s3c->chip.base = -1;
> > -	s3c->chip.npwm = 1;
> > +	s3c->chip.npwm = 4;
> 
> I don't think this is correct. The driver seems to be using the platform
> device id as index currently, which indicates that the driver is bound
> against 4 different platform devices, each representing a single PWM. If
> you want to service multiple PWM devices by a single instance you need
> to make further changes to the driver. For instance the tcon_base is
> initialized based on the platform id. This could easily be replaced by
> making it depend on the .hwpwm member of pwm_device.

I just want to use pwm backlight on Samsung SoC boards.
After moving samusng pwm driver from 'arch/arm/plat-samsung/'
to 'drivers/pwm/', pwm bakclight does not work properly.

When pwm_id is '1' and PWM backlight calls pwm_request(data->pwm_id,
"pwm-backlight"), pwm_to_device() returns NULL.

Then, do you mean that?
	s3c->chip.npwm = id + 1;

> 
> You may also want to consider making the driver a proper module if that
> works on the platforms that need it. I see that it is currently an
> arch_initcall, but it might be cleaner to use deferred driver probe
> instead.

Sorry, I cannot understand what you say.

> 
> Thierry

Re: [PATCH 2/2] pwm: samsung: fix the number of PWMs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3142 bytes --]

On Fri, Aug 03, 2012 at 08:17:59AM +0900, Jingoo Han wrote:
> On Thursday, August 02, 2012 6:54 PM Thierry Reding wrote:
> > 
> > On Thu, Aug 02, 2012 at 05:56:27PM +0900, Jingoo Han wrote:
> > > Samsung SoC can provide 4 PWMs; thus, the number of PWMs should be
> > > set as 4.
> > >
> > > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > > ---
> > >  drivers/pwm/pwm-samsung.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> > > index e5187c0..32562c6 100644
> > > --- a/drivers/pwm/pwm-samsung.c
> > > +++ b/drivers/pwm/pwm-samsung.c
> > > @@ -228,7 +228,7 @@ static int s3c_pwm_probe(struct platform_device *pdev)
> > >  	s3c->chip.dev = &pdev->dev;
> > >  	s3c->chip.ops = &s3c_pwm_ops;
> > >  	s3c->chip.base = -1;
> > > -	s3c->chip.npwm = 1;
> > > +	s3c->chip.npwm = 4;
> > 
> > I don't think this is correct. The driver seems to be using the platform
> > device id as index currently, which indicates that the driver is bound
> > against 4 different platform devices, each representing a single PWM. If
> > you want to service multiple PWM devices by a single instance you need
> > to make further changes to the driver. For instance the tcon_base is
> > initialized based on the platform id. This could easily be replaced by
> > making it depend on the .hwpwm member of pwm_device.
> 
> I just want to use pwm backlight on Samsung SoC boards.
> After moving samusng pwm driver from 'arch/arm/plat-samsung/'
> to 'drivers/pwm/', pwm bakclight does not work properly.
> 
> When pwm_id is '1' and PWM backlight calls pwm_request(data->pwm_id,
> "pwm-backlight"), pwm_to_device() returns NULL.

I don't have the hardware to test, but I'll take a look at the code to
see if there's something obvious as to why this fails. I have at least
tested the legacy paths on Tegra and they still work. In fact the PWM
framework is specifically designed such that the transition shouldn't
cause any side-effects.

Just as a quick check, you could add debug output to find out at what
point in time the s3c_pwm_probe() function is called. AFAICT the only
reason why pwm_to_device() would return NULL is if pwmchip_add() hasn't
been called. You could also attach output from dmesg, which may be
useful to diagnose the problem.

> Then, do you mean that?
> 	s3c->chip.npwm = id + 1;

No. If your PWM controller support more than one PWM device (sometimes
also called channels), then you should be setting .npwm to that number
(in your case this seems to be 4). Look for example at the pwm-tegra
driver, which does just that.

> > You may also want to consider making the driver a proper module if that
> > works on the platforms that need it. I see that it is currently an
> > arch_initcall, but it might be cleaner to use deferred driver probe
> > instead.
> 
> Sorry, I cannot understand what you say.

pwm-samsung is currently not a proper module. It uses arch_initcall() to
initialize the module. If possible it should be converted to using
module_platform_driver().

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]