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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120803050352.GA2908@avionic-0098.adnet.avionic-design.de>
Date:	Fri, 3 Aug 2012 07:03:52 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Jingoo Han <jg1.han@...sung.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] pwm: samsung: fix the number of PWMs

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@...sung.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

Content of type "application/pgp-signature" skipped