Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Grant Likely
On Fri, Nov 30, 2012 at 11:04 AM, Thierry Reding
 wrote:

>> > One other problem is that some PWM devices cannot be setup to achieve a
>> > 0% or 100% duty-cycle but instead will toggle for at least one period.
>> > This would be another argument in favour of moving the functionality to
>> > the individual drivers, perhaps with some functionality provided by the
>> > core to do the gpio_chip registration (a period could be passed to that
>> > function at registration time), which will likely be the same for all
>> > hardware that can and wants to support this feature.
>>
>> It is a bit of an oddball case where if the hardware engineer wires up a
>> PWM to use as a GPIO, then he better be sure that it is actually fit for
>> the purpose.
>
> Yes, I agree. So what we really want is to make this configurable in
> some way. For DT this could just be controlled by the gpio-controller
> property. The PWM core could easily setup the gpio_chip in the presence
> of that property.
>
> For non-DT it could probably be done via a flag that is passed to the
> driver via platform data, in which case the driver would have to call
> the helper explicitly based on the setting of this flag.
>
>> That doesn't prevent the PWM core having some gpio-emulation helper
>> functionality, but do need to be careful about it.
>
> On the other hand, if we turn that support into a helper maybe it is
> easier to leave it up to the driver whether to call it or not. A big
> advantage of this would be that the driver could pass a period along
> that it can choose sensibly according to the chip's capabilities.
>
> Something as simple as:
>
> int pwmchip_emulate_gpio(struct pwm_chip *chip, unsigned long period);
>
> could do. Cleanup could be done automatically in pwmchip_remove().

Looks reasonable.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Thierry Reding
On Fri, Nov 30, 2012 at 10:20:38AM +, Grant Likely wrote:
> On Fri, 30 Nov 2012 07:47:52 +0100, Thierry Reding 
>  wrote:
> > On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
> > > On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi 
> > >  wrote:
> > > > Hi Grant, Lars, Thierry,
> > > > 
> > > > On 11/26/2012 04:46 PM, Grant Likely wrote:
> > > > > You're effectively asking the pwm layer to behave like a gpio (which
> > > > > is completely reasonable). Having a completely separate translation 
> > > > > node
> > > > > really doesn't make sense because it is entirely a software construct.
> > > > > In fact, the way your using it is *entirely* to make the Linux driver
> > > > > model instantiate the translation code. It has *nothing* to do with 
> > > > > the
> > > > > structure of the hardware. It makes complete sense that if a PWM is
> > > > > going to be used as a GPIO, then the PWM node should conform to the 
> > > > > GPIO
> > > > > binding.
> > > > 
> > > > I understand your point around this. I might say I agree with it as 
> > > > well...
> > > > I spent yesterday with prototyping and I'm not really convinced that it 
> > > > is a
> > > > good approach from C code point of view. I got it working, yes.
> > > > In essence this is what I have on top of the slightly modified 
> > > > gpio-pwm.c
> > > > driver I have submitted:
> > > > 
> > > > DTS files:
> > > > twl_pwm: pwm {
> > > > /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
> > > > compatible = "ti,twl6030-pwm";
> > > > #pwm-cells = <2>;
> > > > 
> > > > /* Enable GPIO us of the PWMs */
> > > > gpio-controller = <1>;
> > > 
> > > This line should be simply (the property shouldn't have any data):
> > >   gpio-controller;
> > > 
> > > > #gpio-cells = <2>;
> > > > pwm,period_ns = <7812500>;
> > > 
> > > Nit: property names should use '-' instead of '_'.
> > > 
> > > > };
> > > > 
> > > > leds {
> > > > compatible = "gpio-leds";
> > > > backlight {
> > > > label = "omap4::backlight";
> > > > gpios = <_pwm 1 0>; /* PWM1 of twl6030 */
> > > > };
> > > > 
> > > > keypad {
> > > > label = "omap4::keypad";
> > > > gpios = <_pwm 0 0>; /* PWM0 of twl6030 */
> > > > };
> > > > };
> > > > 
> > > > The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device 
> > > > when
> > > > it is requested going to look something like this. I have removed the 
> > > > error
> > > > checks for now and I still don't have the code to clean up the allocated
> > > > memory for the created device on error, or in case the module is 
> > > > unloaded. We
> > > > should also prevent the pwm core from removal when the pwm-gpo driver 
> > > > is loaded.
> > > > We need to create the platform device for gpo-pwm, create the pdata 
> > > > structure
> > > > for it and fill it in. We also need to hand craft the pwm_lookup table 
> > > > so we
> > > > can use pwm_get() to request the PWM. I have other minor changes around 
> > > > this
> > > > to get things working when we booted with DT.
> > > > So the function to do the heavy lifting is something like this:
> > > > static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> > > > {
> > > > struct platform_device *pdev;
> > > > struct gpio_pwm *gpos;
> > > > struct gpio_pwm_pdata *pdata;
> > > > struct pwm_lookup *lookup;
> > > > char gpodev_name[15];
> > > > int i;
> > > > u32 gpio_mode = 0;
> > > > u32 period_ns = 0;
> > > > 
> > > > of_property_read_u32(chip->dev->of_node, "gpio-controller",
> > > >  _mode);
> > > > if (!gpio_mode)
> > > > return;
> > > > 
> > > > of_property_read_u32(chip->dev->of_node, "pwm,period_ns", 
> > > > _ns);
> > > > if (!period_ns) {
> > > > dev_err(chip->dev,
> > > > "period_ns is not specified for GPIO use\n");
> > > > return;
> > > > }
> > > 
> > > This property name seems ambiguous. What do you need to encode here? It
> > > looks like there is a specific PWM period used for the 'on' state. What
> > > about the 'off' state? Would different pwm outputs have different
> > > frequencies required for GPIO usage.
> > > 
> > > Actually, I'm a bit surprised here that a period value is needed at all.
> > > I would expect if a PWM is used as a GPIO then the driver would already
> > > know how to set it up that way.
> > 
> > Just to make sure we're talking about the same thing here: if a PWM is
> > used as GPIO the assumption is that it would be set to 0% duty-cycle
> > when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
> > The period will still need to be set here, otherwise how would the PWM
> > core know what the hardware even supports?
> 
> Umm, I agree with you on duty cycle, but that's got nothing to 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Peter Ujfalusi
On 11/29/2012 05:10 PM, Grant Likely wrote:
>>  /* Enable GPIO us of the PWMs */
>>  gpio-controller = <1>;
> 
> This line should be simply (the property shouldn't have any data):
>   gpio-controller;

Yes I know. It is like this already in my code. I just mixed up things while
hacking it together.

> 
>>  #gpio-cells = <2>;
>>  pwm,period_ns = <7812500>;
> 
> Nit: property names should use '-' instead of '_'.

Yes, true.

>>  of_property_read_u32(chip->dev->of_node, "pwm,period_ns", _ns);
>>  if (!period_ns) {
>>  dev_err(chip->dev,
>>  "period_ns is not specified for GPIO use\n");
>>  return;
>>  }
> 
> This property name seems ambiguous. What do you need to encode here? It
> looks like there is a specific PWM period used for the 'on' state. What
> about the 'off' state? Would different pwm outputs have different
> frequencies required for GPIO usage.
> 
> Actually, I'm a bit surprised here that a period value is needed at all.
> I would expect if a PWM is used as a GPIO then the driver would already
> know how to set it up that way.

Some PWM hw can select between different frequencies. They do that based on
the period_ns.
We can not just pass random non 0 number to them since they might fail with
the check for supported frequencies.

> Ugh, yeah this isn't the way to go. Don't register a new platform_device
> for the gpio functionality. It should be inline with the rest of the PWM
> setup and should trigger the registration of a gpio_chip directly.

We also need to take care of the non DT booted cases as well.
What we could do:
this driver with removed DT support for legacy booted systems.
GPO layer implementation within drivers/pwm/ to handle DT boot.

When we boot without DT we need to tell back to the machine driver about the
GPIO start of the gpio chip we just allocated.

Hrm, we could do this from the pwm-twl* drivers as well by adding an optional
callback and flag to the pwm core to register the gpio chip.
In this case we can move the whole whole gpio-pwm code under drivers/pwm, add
the 'struct gpio_chip' to struct pwm_chip{}
and register the gpio chip from within the pwm core.
We still have to provide the period_ns in same way to the PWM instance used as
GPIO, probably via platform data. Not sure about it right now.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Peter Ujfalusi
On 11/30/2012 11:20 AM, Grant Likely wrote:
> Umm, I agree with you on duty cycle, but that's got nothing to do with
> period. 100% duty cycle looks exactly the same whether the period is
> 10ns or 100s.

Yes this is true. But some PWM hw can select it's clock based on the period_ns
provided.
In most cases we could hardwire 1 as period_ns but there might be HW which
checks this and only allow the use of supported frequencies.

>> Unless you're proposing to not include that in the PWM core but rather
>> in individual drivers. Then I suppose the driver could choose some
>> sensible default.
> 
> Mostly I'm asking questions because I'm not familiar with the subsystem.
> If the property is needed then I have no objections, but at the moment
> it doesn't make any sense to me.
> 
>> One other problem is that some PWM devices cannot be setup to achieve a
>> 0% or 100% duty-cycle but instead will toggle for at least one period.
>> This would be another argument in favour of moving the functionality to
>> the individual drivers, perhaps with some functionality provided by the
>> core to do the gpio_chip registration (a period could be passed to that
>> function at registration time), which will likely be the same for all
>> hardware that can and wants to support this feature.
> 
> It is a bit of an oddball case where if the hardware engineer wires up a
> PWM to use as a GPIO, then he better be sure that it is actually fit for
> the purpose.

If we inspect the situation from a different angle: a standard GPIO is a PWM
with either turned off state or with full duty cycle (where the duty cycle
means nothing since the signal on the line is constantly high).

> That doesn't prevent the PWM core having some
> gpio-emulation helper functionality, but do need to be careful about it.

If you give designers a way to take short cuts, they will take it whenever
they can. Even if it makes no sense. IMHO. But the BeagleBoard has been
designed before we had any indication from the SW that we could handle this
case. I guess the thinking was that in the bootloader the SW will configure
the PWM to always on and forget about it in the running code.

As a note: I noticed this during my cleanup work around the twl-core. If you
take a look at the kernel code there are other drivers configuring the PWMs of
twl in various places to handle them.
I wrote the pwm-twl and pwm-twl-led drivers so we can get rid of this whole 
mess:
- the gpio-twl4030 extends itself to handle the PWMA/B (LEDA/B) as GPIO. So
drivers are doing OMAP_MAX_GPIO + TWL_NUM_GPIO + 1/2 to control the PWMA/B.
- mach-omap2/board-zoom-display.c have custom code to control the same thing
and the list goes.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Grant Likely
On Fri, 30 Nov 2012 07:47:52 +0100, Thierry Reding 
 wrote:
> On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
> > On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi  
> > wrote:
> > > Hi Grant, Lars, Thierry,
> > > 
> > > On 11/26/2012 04:46 PM, Grant Likely wrote:
> > > > You're effectively asking the pwm layer to behave like a gpio (which
> > > > is completely reasonable). Having a completely separate translation node
> > > > really doesn't make sense because it is entirely a software construct.
> > > > In fact, the way your using it is *entirely* to make the Linux driver
> > > > model instantiate the translation code. It has *nothing* to do with the
> > > > structure of the hardware. It makes complete sense that if a PWM is
> > > > going to be used as a GPIO, then the PWM node should conform to the GPIO
> > > > binding.
> > > 
> > > I understand your point around this. I might say I agree with it as 
> > > well...
> > > I spent yesterday with prototyping and I'm not really convinced that it 
> > > is a
> > > good approach from C code point of view. I got it working, yes.
> > > In essence this is what I have on top of the slightly modified gpio-pwm.c
> > > driver I have submitted:
> > > 
> > > DTS files:
> > > twl_pwm: pwm {
> > >   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
> > >   compatible = "ti,twl6030-pwm";
> > >   #pwm-cells = <2>;
> > > 
> > >   /* Enable GPIO us of the PWMs */
> > >   gpio-controller = <1>;
> > 
> > This line should be simply (the property shouldn't have any data):
> > gpio-controller;
> > 
> > >   #gpio-cells = <2>;
> > >   pwm,period_ns = <7812500>;
> > 
> > Nit: property names should use '-' instead of '_'.
> > 
> > > };
> > > 
> > > leds {
> > >   compatible = "gpio-leds";
> > >   backlight {
> > >   label = "omap4::backlight";
> > >   gpios = <_pwm 1 0>; /* PWM1 of twl6030 */
> > >   };
> > > 
> > >   keypad {
> > >   label = "omap4::keypad";
> > >   gpios = <_pwm 0 0>; /* PWM0 of twl6030 */
> > >   };
> > > };
> > > 
> > > The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device 
> > > when
> > > it is requested going to look something like this. I have removed the 
> > > error
> > > checks for now and I still don't have the code to clean up the allocated
> > > memory for the created device on error, or in case the module is 
> > > unloaded. We
> > > should also prevent the pwm core from removal when the pwm-gpo driver is 
> > > loaded.
> > > We need to create the platform device for gpo-pwm, create the pdata 
> > > structure
> > > for it and fill it in. We also need to hand craft the pwm_lookup table so 
> > > we
> > > can use pwm_get() to request the PWM. I have other minor changes around 
> > > this
> > > to get things working when we booted with DT.
> > > So the function to do the heavy lifting is something like this:
> > > static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> > > {
> > >   struct platform_device *pdev;
> > >   struct gpio_pwm *gpos;
> > >   struct gpio_pwm_pdata *pdata;
> > >   struct pwm_lookup *lookup;
> > >   char gpodev_name[15];
> > >   int i;
> > >   u32 gpio_mode = 0;
> > >   u32 period_ns = 0;
> > > 
> > >   of_property_read_u32(chip->dev->of_node, "gpio-controller",
> > >_mode);
> > >   if (!gpio_mode)
> > >   return;
> > > 
> > >   of_property_read_u32(chip->dev->of_node, "pwm,period_ns", _ns);
> > >   if (!period_ns) {
> > >   dev_err(chip->dev,
> > >   "period_ns is not specified for GPIO use\n");
> > >   return;
> > >   }
> > 
> > This property name seems ambiguous. What do you need to encode here? It
> > looks like there is a specific PWM period used for the 'on' state. What
> > about the 'off' state? Would different pwm outputs have different
> > frequencies required for GPIO usage.
> > 
> > Actually, I'm a bit surprised here that a period value is needed at all.
> > I would expect if a PWM is used as a GPIO then the driver would already
> > know how to set it up that way.
> 
> Just to make sure we're talking about the same thing here: if a PWM is
> used as GPIO the assumption is that it would be set to 0% duty-cycle
> when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
> The period will still need to be set here, otherwise how would the PWM
> core know what the hardware even supports?

Umm, I agree with you on duty cycle, but that's got nothing to do with
period. 100% duty cycle looks exactly the same whether the period is
10ns or 100s.

> Unless you're proposing to not include that in the PWM core but rather
> in individual drivers. Then I suppose the driver could choose some
> sensible default.

Mostly I'm asking questions because I'm not familiar with the subsystem.
If the property is needed then I have no objections, but at the moment
it doesn't make any sense to me.

> One other problem is that some PWM devices cannot be setup to achieve a
> 0% or 100% duty-cycle but 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Grant Likely
On Fri, 30 Nov 2012 07:47:52 +0100, Thierry Reding 
thierry.red...@avionic-design.de wrote:
 On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
  On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi peter.ujfal...@ti.com 
  wrote:
   Hi Grant, Lars, Thierry,
   
   On 11/26/2012 04:46 PM, Grant Likely wrote:
You're effectively asking the pwm layer to behave like a gpio (which
is completely reasonable). Having a completely separate translation node
really doesn't make sense because it is entirely a software construct.
In fact, the way your using it is *entirely* to make the Linux driver
model instantiate the translation code. It has *nothing* to do with the
structure of the hardware. It makes complete sense that if a PWM is
going to be used as a GPIO, then the PWM node should conform to the GPIO
binding.
   
   I understand your point around this. I might say I agree with it as 
   well...
   I spent yesterday with prototyping and I'm not really convinced that it 
   is a
   good approach from C code point of view. I got it working, yes.
   In essence this is what I have on top of the slightly modified gpio-pwm.c
   driver I have submitted:
   
   DTS files:
   twl_pwm: pwm {
 /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
 compatible = ti,twl6030-pwm;
 #pwm-cells = 2;
   
 /* Enable GPIO us of the PWMs */
 gpio-controller = 1;
  
  This line should be simply (the property shouldn't have any data):
  gpio-controller;
  
 #gpio-cells = 2;
 pwm,period_ns = 7812500;
  
  Nit: property names should use '-' instead of '_'.
  
   };
   
   leds {
 compatible = gpio-leds;
 backlight {
 label = omap4::backlight;
 gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
 };
   
 keypad {
 label = omap4::keypad;
 gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
 };
   };
   
   The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device 
   when
   it is requested going to look something like this. I have removed the 
   error
   checks for now and I still don't have the code to clean up the allocated
   memory for the created device on error, or in case the module is 
   unloaded. We
   should also prevent the pwm core from removal when the pwm-gpo driver is 
   loaded.
   We need to create the platform device for gpo-pwm, create the pdata 
   structure
   for it and fill it in. We also need to hand craft the pwm_lookup table so 
   we
   can use pwm_get() to request the PWM. I have other minor changes around 
   this
   to get things working when we booted with DT.
   So the function to do the heavy lifting is something like this:
   static void of_pwmchip_as_gpio(struct pwm_chip *chip)
   {
 struct platform_device *pdev;
 struct gpio_pwm *gpos;
 struct gpio_pwm_pdata *pdata;
 struct pwm_lookup *lookup;
 char gpodev_name[15];
 int i;
 u32 gpio_mode = 0;
 u32 period_ns = 0;
   
 of_property_read_u32(chip-dev-of_node, gpio-controller,
  gpio_mode);
 if (!gpio_mode)
 return;
   
 of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
 if (!period_ns) {
 dev_err(chip-dev,
 period_ns is not specified for GPIO use\n);
 return;
 }
  
  This property name seems ambiguous. What do you need to encode here? It
  looks like there is a specific PWM period used for the 'on' state. What
  about the 'off' state? Would different pwm outputs have different
  frequencies required for GPIO usage.
  
  Actually, I'm a bit surprised here that a period value is needed at all.
  I would expect if a PWM is used as a GPIO then the driver would already
  know how to set it up that way.
 
 Just to make sure we're talking about the same thing here: if a PWM is
 used as GPIO the assumption is that it would be set to 0% duty-cycle
 when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
 The period will still need to be set here, otherwise how would the PWM
 core know what the hardware even supports?

Umm, I agree with you on duty cycle, but that's got nothing to do with
period. 100% duty cycle looks exactly the same whether the period is
10ns or 100s.

 Unless you're proposing to not include that in the PWM core but rather
 in individual drivers. Then I suppose the driver could choose some
 sensible default.

Mostly I'm asking questions because I'm not familiar with the subsystem.
If the property is needed then I have no objections, but at the moment
it doesn't make any sense to me.

 One other problem is that some PWM devices cannot be setup to achieve a
 0% or 100% duty-cycle but instead will toggle for at least one period.
 This would be another argument in favour of moving the functionality to
 the individual drivers, perhaps with some functionality provided by the
 core to do the gpio_chip registration (a period could be passed to that
 function at 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Peter Ujfalusi
On 11/30/2012 11:20 AM, Grant Likely wrote:
 Umm, I agree with you on duty cycle, but that's got nothing to do with
 period. 100% duty cycle looks exactly the same whether the period is
 10ns or 100s.

Yes this is true. But some PWM hw can select it's clock based on the period_ns
provided.
In most cases we could hardwire 1 as period_ns but there might be HW which
checks this and only allow the use of supported frequencies.

 Unless you're proposing to not include that in the PWM core but rather
 in individual drivers. Then I suppose the driver could choose some
 sensible default.
 
 Mostly I'm asking questions because I'm not familiar with the subsystem.
 If the property is needed then I have no objections, but at the moment
 it doesn't make any sense to me.
 
 One other problem is that some PWM devices cannot be setup to achieve a
 0% or 100% duty-cycle but instead will toggle for at least one period.
 This would be another argument in favour of moving the functionality to
 the individual drivers, perhaps with some functionality provided by the
 core to do the gpio_chip registration (a period could be passed to that
 function at registration time), which will likely be the same for all
 hardware that can and wants to support this feature.
 
 It is a bit of an oddball case where if the hardware engineer wires up a
 PWM to use as a GPIO, then he better be sure that it is actually fit for
 the purpose.

If we inspect the situation from a different angle: a standard GPIO is a PWM
with either turned off state or with full duty cycle (where the duty cycle
means nothing since the signal on the line is constantly high).

 That doesn't prevent the PWM core having some
 gpio-emulation helper functionality, but do need to be careful about it.

If you give designers a way to take short cuts, they will take it whenever
they can. Even if it makes no sense. IMHO. But the BeagleBoard has been
designed before we had any indication from the SW that we could handle this
case. I guess the thinking was that in the bootloader the SW will configure
the PWM to always on and forget about it in the running code.

As a note: I noticed this during my cleanup work around the twl-core. If you
take a look at the kernel code there are other drivers configuring the PWMs of
twl in various places to handle them.
I wrote the pwm-twl and pwm-twl-led drivers so we can get rid of this whole 
mess:
- the gpio-twl4030 extends itself to handle the PWMA/B (LEDA/B) as GPIO. So
drivers are doing OMAP_MAX_GPIO + TWL_NUM_GPIO + 1/2 to control the PWMA/B.
- mach-omap2/board-zoom-display.c have custom code to control the same thing
and the list goes.

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Peter Ujfalusi
On 11/29/2012 05:10 PM, Grant Likely wrote:
  /* Enable GPIO us of the PWMs */
  gpio-controller = 1;
 
 This line should be simply (the property shouldn't have any data):
   gpio-controller;

Yes I know. It is like this already in my code. I just mixed up things while
hacking it together.

 
  #gpio-cells = 2;
  pwm,period_ns = 7812500;
 
 Nit: property names should use '-' instead of '_'.

Yes, true.

  of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
  if (!period_ns) {
  dev_err(chip-dev,
  period_ns is not specified for GPIO use\n);
  return;
  }
 
 This property name seems ambiguous. What do you need to encode here? It
 looks like there is a specific PWM period used for the 'on' state. What
 about the 'off' state? Would different pwm outputs have different
 frequencies required for GPIO usage.
 
 Actually, I'm a bit surprised here that a period value is needed at all.
 I would expect if a PWM is used as a GPIO then the driver would already
 know how to set it up that way.

Some PWM hw can select between different frequencies. They do that based on
the period_ns.
We can not just pass random non 0 number to them since they might fail with
the check for supported frequencies.

 Ugh, yeah this isn't the way to go. Don't register a new platform_device
 for the gpio functionality. It should be inline with the rest of the PWM
 setup and should trigger the registration of a gpio_chip directly.

We also need to take care of the non DT booted cases as well.
What we could do:
this driver with removed DT support for legacy booted systems.
GPO layer implementation within drivers/pwm/ to handle DT boot.

When we boot without DT we need to tell back to the machine driver about the
GPIO start of the gpio chip we just allocated.

Hrm, we could do this from the pwm-twl* drivers as well by adding an optional
callback and flag to the pwm core to register the gpio chip.
In this case we can move the whole whole gpio-pwm code under drivers/pwm, add
the 'struct gpio_chip' to struct pwm_chip{}
and register the gpio chip from within the pwm core.
We still have to provide the period_ns in same way to the PWM instance used as
GPIO, probably via platform data. Not sure about it right now.

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Thierry Reding
On Fri, Nov 30, 2012 at 10:20:38AM +, Grant Likely wrote:
 On Fri, 30 Nov 2012 07:47:52 +0100, Thierry Reding 
 thierry.red...@avionic-design.de wrote:
  On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
   On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi 
   peter.ujfal...@ti.com wrote:
Hi Grant, Lars, Thierry,

On 11/26/2012 04:46 PM, Grant Likely wrote:
 You're effectively asking the pwm layer to behave like a gpio (which
 is completely reasonable). Having a completely separate translation 
 node
 really doesn't make sense because it is entirely a software construct.
 In fact, the way your using it is *entirely* to make the Linux driver
 model instantiate the translation code. It has *nothing* to do with 
 the
 structure of the hardware. It makes complete sense that if a PWM is
 going to be used as a GPIO, then the PWM node should conform to the 
 GPIO
 binding.

I understand your point around this. I might say I agree with it as 
well...
I spent yesterday with prototyping and I'm not really convinced that it 
is a
good approach from C code point of view. I got it working, yes.
In essence this is what I have on top of the slightly modified 
gpio-pwm.c
driver I have submitted:

DTS files:
twl_pwm: pwm {
/* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
compatible = ti,twl6030-pwm;
#pwm-cells = 2;

/* Enable GPIO us of the PWMs */
gpio-controller = 1;
   
   This line should be simply (the property shouldn't have any data):
 gpio-controller;
   
#gpio-cells = 2;
pwm,period_ns = 7812500;
   
   Nit: property names should use '-' instead of '_'.
   
};

leds {
compatible = gpio-leds;
backlight {
label = omap4::backlight;
gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
};

keypad {
label = omap4::keypad;
gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
};
};

The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device 
when
it is requested going to look something like this. I have removed the 
error
checks for now and I still don't have the code to clean up the allocated
memory for the created device on error, or in case the module is 
unloaded. We
should also prevent the pwm core from removal when the pwm-gpo driver 
is loaded.
We need to create the platform device for gpo-pwm, create the pdata 
structure
for it and fill it in. We also need to hand craft the pwm_lookup table 
so we
can use pwm_get() to request the PWM. I have other minor changes around 
this
to get things working when we booted with DT.
So the function to do the heavy lifting is something like this:
static void of_pwmchip_as_gpio(struct pwm_chip *chip)
{
struct platform_device *pdev;
struct gpio_pwm *gpos;
struct gpio_pwm_pdata *pdata;
struct pwm_lookup *lookup;
char gpodev_name[15];
int i;
u32 gpio_mode = 0;
u32 period_ns = 0;

of_property_read_u32(chip-dev-of_node, gpio-controller,
 gpio_mode);
if (!gpio_mode)
return;

of_property_read_u32(chip-dev-of_node, pwm,period_ns, 
period_ns);
if (!period_ns) {
dev_err(chip-dev,
period_ns is not specified for GPIO use\n);
return;
}
   
   This property name seems ambiguous. What do you need to encode here? It
   looks like there is a specific PWM period used for the 'on' state. What
   about the 'off' state? Would different pwm outputs have different
   frequencies required for GPIO usage.
   
   Actually, I'm a bit surprised here that a period value is needed at all.
   I would expect if a PWM is used as a GPIO then the driver would already
   know how to set it up that way.
  
  Just to make sure we're talking about the same thing here: if a PWM is
  used as GPIO the assumption is that it would be set to 0% duty-cycle
  when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
  The period will still need to be set here, otherwise how would the PWM
  core know what the hardware even supports?
 
 Umm, I agree with you on duty cycle, but that's got nothing to do with
 period. 100% duty cycle looks exactly the same whether the period is
 10ns or 100s.

Yes, that's true. My concern was more that any value for period that we
choose more or less arbitrarily as the default for GPIO operation would
end up not being supported by some hardware.

  Unless you're proposing to not include that in the PWM core but rather
  in individual drivers. Then I suppose the 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-30 Thread Grant Likely
On Fri, Nov 30, 2012 at 11:04 AM, Thierry Reding
thierry.red...@avionic-design.de wrote:

  One other problem is that some PWM devices cannot be setup to achieve a
  0% or 100% duty-cycle but instead will toggle for at least one period.
  This would be another argument in favour of moving the functionality to
  the individual drivers, perhaps with some functionality provided by the
  core to do the gpio_chip registration (a period could be passed to that
  function at registration time), which will likely be the same for all
  hardware that can and wants to support this feature.

 It is a bit of an oddball case where if the hardware engineer wires up a
 PWM to use as a GPIO, then he better be sure that it is actually fit for
 the purpose.

 Yes, I agree. So what we really want is to make this configurable in
 some way. For DT this could just be controlled by the gpio-controller
 property. The PWM core could easily setup the gpio_chip in the presence
 of that property.

 For non-DT it could probably be done via a flag that is passed to the
 driver via platform data, in which case the driver would have to call
 the helper explicitly based on the setting of this flag.

 That doesn't prevent the PWM core having some gpio-emulation helper
 functionality, but do need to be careful about it.

 On the other hand, if we turn that support into a helper maybe it is
 easier to leave it up to the driver whether to call it or not. A big
 advantage of this would be that the driver could pass a period along
 that it can choose sensibly according to the chip's capabilities.

 Something as simple as:

 int pwmchip_emulate_gpio(struct pwm_chip *chip, unsigned long period);

 could do. Cleanup could be done automatically in pwmchip_remove().

Looks reasonable.

g.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-29 Thread Thierry Reding
On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
> On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi  
> wrote:
> > Hi Grant, Lars, Thierry,
> > 
> > On 11/26/2012 04:46 PM, Grant Likely wrote:
> > > You're effectively asking the pwm layer to behave like a gpio (which
> > > is completely reasonable). Having a completely separate translation node
> > > really doesn't make sense because it is entirely a software construct.
> > > In fact, the way your using it is *entirely* to make the Linux driver
> > > model instantiate the translation code. It has *nothing* to do with the
> > > structure of the hardware. It makes complete sense that if a PWM is
> > > going to be used as a GPIO, then the PWM node should conform to the GPIO
> > > binding.
> > 
> > I understand your point around this. I might say I agree with it as well...
> > I spent yesterday with prototyping and I'm not really convinced that it is a
> > good approach from C code point of view. I got it working, yes.
> > In essence this is what I have on top of the slightly modified gpio-pwm.c
> > driver I have submitted:
> > 
> > DTS files:
> > twl_pwm: pwm {
> > /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
> > compatible = "ti,twl6030-pwm";
> > #pwm-cells = <2>;
> > 
> > /* Enable GPIO us of the PWMs */
> > gpio-controller = <1>;
> 
> This line should be simply (the property shouldn't have any data):
>   gpio-controller;
> 
> > #gpio-cells = <2>;
> > pwm,period_ns = <7812500>;
> 
> Nit: property names should use '-' instead of '_'.
> 
> > };
> > 
> > leds {
> > compatible = "gpio-leds";
> > backlight {
> > label = "omap4::backlight";
> > gpios = <_pwm 1 0>; /* PWM1 of twl6030 */
> > };
> > 
> > keypad {
> > label = "omap4::keypad";
> > gpios = <_pwm 0 0>; /* PWM0 of twl6030 */
> > };
> > };
> > 
> > The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
> > it is requested going to look something like this. I have removed the error
> > checks for now and I still don't have the code to clean up the allocated
> > memory for the created device on error, or in case the module is unloaded. 
> > We
> > should also prevent the pwm core from removal when the pwm-gpo driver is 
> > loaded.
> > We need to create the platform device for gpo-pwm, create the pdata 
> > structure
> > for it and fill it in. We also need to hand craft the pwm_lookup table so we
> > can use pwm_get() to request the PWM. I have other minor changes around this
> > to get things working when we booted with DT.
> > So the function to do the heavy lifting is something like this:
> > static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> > {
> > struct platform_device *pdev;
> > struct gpio_pwm *gpos;
> > struct gpio_pwm_pdata *pdata;
> > struct pwm_lookup *lookup;
> > char gpodev_name[15];
> > int i;
> > u32 gpio_mode = 0;
> > u32 period_ns = 0;
> > 
> > of_property_read_u32(chip->dev->of_node, "gpio-controller",
> >  _mode);
> > if (!gpio_mode)
> > return;
> > 
> > of_property_read_u32(chip->dev->of_node, "pwm,period_ns", _ns);
> > if (!period_ns) {
> > dev_err(chip->dev,
> > "period_ns is not specified for GPIO use\n");
> > return;
> > }
> 
> This property name seems ambiguous. What do you need to encode here? It
> looks like there is a specific PWM period used for the 'on' state. What
> about the 'off' state? Would different pwm outputs have different
> frequencies required for GPIO usage.
> 
> Actually, I'm a bit surprised here that a period value is needed at all.
> I would expect if a PWM is used as a GPIO then the driver would already
> know how to set it up that way.

Just to make sure we're talking about the same thing here: if a PWM is
used as GPIO the assumption is that it would be set to 0% duty-cycle
when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
The period will still need to be set here, otherwise how would the PWM
core know what the hardware even supports?

Unless you're proposing to not include that in the PWM core but rather
in individual drivers. Then I suppose the driver could choose some
sensible default.

One other problem is that some PWM devices cannot be setup to achieve a
0% or 100% duty-cycle but instead will toggle for at least one period.
This would be another argument in favour of moving the functionality to
the individual drivers, perhaps with some functionality provided by the
core to do the gpio_chip registration (a period could be passed to that
function at registration time), which will likely be the same for all
hardware that can and wants to support this feature.

Thierry


pgpAujFGxIaFt.pgp
Description: PGP signature


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-29 Thread Grant Likely
On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi  
wrote:
> Hi Grant, Lars, Thierry,
> 
> On 11/26/2012 04:46 PM, Grant Likely wrote:
> > You're effectively asking the pwm layer to behave like a gpio (which
> > is completely reasonable). Having a completely separate translation node
> > really doesn't make sense because it is entirely a software construct.
> > In fact, the way your using it is *entirely* to make the Linux driver
> > model instantiate the translation code. It has *nothing* to do with the
> > structure of the hardware. It makes complete sense that if a PWM is
> > going to be used as a GPIO, then the PWM node should conform to the GPIO
> > binding.
> 
> I understand your point around this. I might say I agree with it as well...
> I spent yesterday with prototyping and I'm not really convinced that it is a
> good approach from C code point of view. I got it working, yes.
> In essence this is what I have on top of the slightly modified gpio-pwm.c
> driver I have submitted:
> 
> DTS files:
> twl_pwm: pwm {
>   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
>   compatible = "ti,twl6030-pwm";
>   #pwm-cells = <2>;
> 
>   /* Enable GPIO us of the PWMs */
>   gpio-controller = <1>;

This line should be simply (the property shouldn't have any data):
gpio-controller;

>   #gpio-cells = <2>;
>   pwm,period_ns = <7812500>;

Nit: property names should use '-' instead of '_'.

> };
> 
> leds {
>   compatible = "gpio-leds";
>   backlight {
>   label = "omap4::backlight";
>   gpios = <_pwm 1 0>; /* PWM1 of twl6030 */
>   };
> 
>   keypad {
>   label = "omap4::keypad";
>   gpios = <_pwm 0 0>; /* PWM0 of twl6030 */
>   };
> };
> 
> The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
> it is requested going to look something like this. I have removed the error
> checks for now and I still don't have the code to clean up the allocated
> memory for the created device on error, or in case the module is unloaded. We
> should also prevent the pwm core from removal when the pwm-gpo driver is 
> loaded.
> We need to create the platform device for gpo-pwm, create the pdata structure
> for it and fill it in. We also need to hand craft the pwm_lookup table so we
> can use pwm_get() to request the PWM. I have other minor changes around this
> to get things working when we booted with DT.
> So the function to do the heavy lifting is something like this:
> static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> {
>   struct platform_device *pdev;
>   struct gpio_pwm *gpos;
>   struct gpio_pwm_pdata *pdata;
>   struct pwm_lookup *lookup;
>   char gpodev_name[15];
>   int i;
>   u32 gpio_mode = 0;
>   u32 period_ns = 0;
> 
>   of_property_read_u32(chip->dev->of_node, "gpio-controller",
>_mode);
>   if (!gpio_mode)
>   return;
> 
>   of_property_read_u32(chip->dev->of_node, "pwm,period_ns", _ns);
>   if (!period_ns) {
>   dev_err(chip->dev,
>   "period_ns is not specified for GPIO use\n");
>   return;
>   }

This property name seems ambiguous. What do you need to encode here? It
looks like there is a specific PWM period used for the 'on' state. What
about the 'off' state? Would different pwm outputs have different
frequencies required for GPIO usage.

Actually, I'm a bit surprised here that a period value is needed at all.
I would expect if a PWM is used as a GPIO then the driver would already
know how to set it up that way.

>   lookup = devm_kzalloc(chip->dev, sizeof(*lookup) * chip->npwm,
> GFP_KERNEL);
>   pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
>   gpos = devm_kzalloc(chip->dev, sizeof(*gpos) * chip->npwm,
>   GFP_KERNEL);
> 
>   pdata->gpos = gpos;
>   pdata->num_gpos = chip->npwm;
>   pdata->gpio_base = -1;
> 
>   pdev = platform_device_alloc("pwm-gpo", chip->base);
>   pdev->dev.parent = chip->dev;
> 
>   sprintf(gpodev_name, "pwm-gpo.%d", chip->base);
>   for (i = 0; i < chip->npwm; i++) {
>   struct gpio_pwm *gpo = [i];
>   struct pwm_lookup *pl = [i];
>   char con_id[15];
> 
>   sprintf(con_id, "pwm-gpo.%d", chip->base + i);
> 
>   /* prepare GPO information */
>   gpo->pwm_period_ns = period_ns;
>   gpo->name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
> 
>   /* prepare pwm lookup table */
>   pl->provider = dev_name(chip->dev);
>   pl->index = i;
>   pl->dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
>GFP_KERNEL);
>   pl->con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
>   }
> 
>   platform_device_add_data(pdev, pdata, sizeof(*pdata));

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-29 Thread Peter Ujfalusi
On 11/28/2012 08:30 PM, Thierry Reding wrote:
> I must say I'm not terribly thrilled to integrate something like this
> into the PWM subsystem.

I agree. I would not really want to add my name to something like this either...

> I wish hardware engineers wouldn't come up with such designs.

I have checked and rechecked the schematics and datasheets. Still can not
understand how anyone would come to this HW solution (when the twl4030 have at
least 4 unused GPIO line left unconnected).

> But since this seems to be a real use-case, if we want to
> support it we should try and find the "right" solution.
> 
> Reading the above sounds overly complicated. Couldn't we, instead of
> instantiating an extra driver, just register a GPIO chip if a chip wants
> to support this functionality? Where the GPIO chip's request callback
> requests the given PWM so that it cannot be used elsewhere? The
> direction_input callback would need to always fail and the set callback
> can configure the PWM either to 0% or 100% duty-cycle depending on the
> desired output value.

The original driver was doing exactly this. Basically I have added a GPO chip
(which was just a translator from PWM to GPO). In DT it looked like this:

twl_pwm: pwm {
compatible = "ti,twl4030-pwmled";
#pwm-cells = <2>;
};

pwm_gpo1: gpo1 {
compatible = "pwm-gpo";
#gpio-cells = <1>;
gpio-controller;
pwms = <_pwm 0 7812500>;
pwm-names = "nUSBHOST_PWR_EN";
};

user@1 {
compatible = "example,testuser";
gpios = <_gpo1 0>;
};

When we boot with DT the pwm_get() needs pwms phandle from the device you are
calling it in order to find the PWM in question. If we do not have DT entry
for the GPO driver, we need to build up a pwm_lookup table to ensure that the
pwm_get() will find the PWM we want to handle.

We could implement the GPIO related code under for example
drivers/pwm/pwm-as-gpo.c
But then I should merge the code from drivers/gpio/gpio-pwm.c inside.
This is not nice either since we will have a GPIO driver living under pwm/
directory and I'm not sure how this would use the pwm API when the GPO
functionality is requested.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-29 Thread Peter Ujfalusi
On 11/28/2012 08:30 PM, Thierry Reding wrote:
 I must say I'm not terribly thrilled to integrate something like this
 into the PWM subsystem.

I agree. I would not really want to add my name to something like this either...

 I wish hardware engineers wouldn't come up with such designs.

I have checked and rechecked the schematics and datasheets. Still can not
understand how anyone would come to this HW solution (when the twl4030 have at
least 4 unused GPIO line left unconnected).

 But since this seems to be a real use-case, if we want to
 support it we should try and find the right solution.
 
 Reading the above sounds overly complicated. Couldn't we, instead of
 instantiating an extra driver, just register a GPIO chip if a chip wants
 to support this functionality? Where the GPIO chip's request callback
 requests the given PWM so that it cannot be used elsewhere? The
 direction_input callback would need to always fail and the set callback
 can configure the PWM either to 0% or 100% duty-cycle depending on the
 desired output value.

The original driver was doing exactly this. Basically I have added a GPO chip
(which was just a translator from PWM to GPO). In DT it looked like this:

twl_pwm: pwm {
compatible = ti,twl4030-pwmled;
#pwm-cells = 2;
};

pwm_gpo1: gpo1 {
compatible = pwm-gpo;
#gpio-cells = 1;
gpio-controller;
pwms = twl_pwm 0 7812500;
pwm-names = nUSBHOST_PWR_EN;
};

user@1 {
compatible = example,testuser;
gpios = pwm_gpo1 0;
};

When we boot with DT the pwm_get() needs pwms phandle from the device you are
calling it in order to find the PWM in question. If we do not have DT entry
for the GPO driver, we need to build up a pwm_lookup table to ensure that the
pwm_get() will find the PWM we want to handle.

We could implement the GPIO related code under for example
drivers/pwm/pwm-as-gpo.c
But then I should merge the code from drivers/gpio/gpio-pwm.c inside.
This is not nice either since we will have a GPIO driver living under pwm/
directory and I'm not sure how this would use the pwm API when the GPO
functionality is requested.

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-29 Thread Grant Likely
On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi peter.ujfal...@ti.com 
wrote:
 Hi Grant, Lars, Thierry,
 
 On 11/26/2012 04:46 PM, Grant Likely wrote:
  You're effectively asking the pwm layer to behave like a gpio (which
  is completely reasonable). Having a completely separate translation node
  really doesn't make sense because it is entirely a software construct.
  In fact, the way your using it is *entirely* to make the Linux driver
  model instantiate the translation code. It has *nothing* to do with the
  structure of the hardware. It makes complete sense that if a PWM is
  going to be used as a GPIO, then the PWM node should conform to the GPIO
  binding.
 
 I understand your point around this. I might say I agree with it as well...
 I spent yesterday with prototyping and I'm not really convinced that it is a
 good approach from C code point of view. I got it working, yes.
 In essence this is what I have on top of the slightly modified gpio-pwm.c
 driver I have submitted:
 
 DTS files:
 twl_pwm: pwm {
   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
   compatible = ti,twl6030-pwm;
   #pwm-cells = 2;
 
   /* Enable GPIO us of the PWMs */
   gpio-controller = 1;

This line should be simply (the property shouldn't have any data):
gpio-controller;

   #gpio-cells = 2;
   pwm,period_ns = 7812500;

Nit: property names should use '-' instead of '_'.

 };
 
 leds {
   compatible = gpio-leds;
   backlight {
   label = omap4::backlight;
   gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
   };
 
   keypad {
   label = omap4::keypad;
   gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
   };
 };
 
 The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
 it is requested going to look something like this. I have removed the error
 checks for now and I still don't have the code to clean up the allocated
 memory for the created device on error, or in case the module is unloaded. We
 should also prevent the pwm core from removal when the pwm-gpo driver is 
 loaded.
 We need to create the platform device for gpo-pwm, create the pdata structure
 for it and fill it in. We also need to hand craft the pwm_lookup table so we
 can use pwm_get() to request the PWM. I have other minor changes around this
 to get things working when we booted with DT.
 So the function to do the heavy lifting is something like this:
 static void of_pwmchip_as_gpio(struct pwm_chip *chip)
 {
   struct platform_device *pdev;
   struct gpio_pwm *gpos;
   struct gpio_pwm_pdata *pdata;
   struct pwm_lookup *lookup;
   char gpodev_name[15];
   int i;
   u32 gpio_mode = 0;
   u32 period_ns = 0;
 
   of_property_read_u32(chip-dev-of_node, gpio-controller,
gpio_mode);
   if (!gpio_mode)
   return;
 
   of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
   if (!period_ns) {
   dev_err(chip-dev,
   period_ns is not specified for GPIO use\n);
   return;
   }

This property name seems ambiguous. What do you need to encode here? It
looks like there is a specific PWM period used for the 'on' state. What
about the 'off' state? Would different pwm outputs have different
frequencies required for GPIO usage.

Actually, I'm a bit surprised here that a period value is needed at all.
I would expect if a PWM is used as a GPIO then the driver would already
know how to set it up that way.

   lookup = devm_kzalloc(chip-dev, sizeof(*lookup) * chip-npwm,
 GFP_KERNEL);
   pdata = devm_kzalloc(chip-dev, sizeof(*pdata), GFP_KERNEL);
   gpos = devm_kzalloc(chip-dev, sizeof(*gpos) * chip-npwm,
   GFP_KERNEL);
 
   pdata-gpos = gpos;
   pdata-num_gpos = chip-npwm;
   pdata-gpio_base = -1;
 
   pdev = platform_device_alloc(pwm-gpo, chip-base);
   pdev-dev.parent = chip-dev;
 
   sprintf(gpodev_name, pwm-gpo.%d, chip-base);
   for (i = 0; i  chip-npwm; i++) {
   struct gpio_pwm *gpo = gpos[i];
   struct pwm_lookup *pl = lookup[i];
   char con_id[15];
 
   sprintf(con_id, pwm-gpo.%d, chip-base + i);
 
   /* prepare GPO information */
   gpo-pwm_period_ns = period_ns;
   gpo-name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
 
   /* prepare pwm lookup table */
   pl-provider = dev_name(chip-dev);
   pl-index = i;
   pl-dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
GFP_KERNEL);
   pl-con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
   }
 
   platform_device_add_data(pdev, pdata, sizeof(*pdata));
   pwm_add_table(lookup, chip-npwm);
   platform_device_add(pdev);

Ugh, yeah this isn't the way to go. Don't register a 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-29 Thread Thierry Reding
On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
 On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi peter.ujfal...@ti.com 
 wrote:
  Hi Grant, Lars, Thierry,
  
  On 11/26/2012 04:46 PM, Grant Likely wrote:
   You're effectively asking the pwm layer to behave like a gpio (which
   is completely reasonable). Having a completely separate translation node
   really doesn't make sense because it is entirely a software construct.
   In fact, the way your using it is *entirely* to make the Linux driver
   model instantiate the translation code. It has *nothing* to do with the
   structure of the hardware. It makes complete sense that if a PWM is
   going to be used as a GPIO, then the PWM node should conform to the GPIO
   binding.
  
  I understand your point around this. I might say I agree with it as well...
  I spent yesterday with prototyping and I'm not really convinced that it is a
  good approach from C code point of view. I got it working, yes.
  In essence this is what I have on top of the slightly modified gpio-pwm.c
  driver I have submitted:
  
  DTS files:
  twl_pwm: pwm {
  /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
  compatible = ti,twl6030-pwm;
  #pwm-cells = 2;
  
  /* Enable GPIO us of the PWMs */
  gpio-controller = 1;
 
 This line should be simply (the property shouldn't have any data):
   gpio-controller;
 
  #gpio-cells = 2;
  pwm,period_ns = 7812500;
 
 Nit: property names should use '-' instead of '_'.
 
  };
  
  leds {
  compatible = gpio-leds;
  backlight {
  label = omap4::backlight;
  gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
  };
  
  keypad {
  label = omap4::keypad;
  gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
  };
  };
  
  The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
  it is requested going to look something like this. I have removed the error
  checks for now and I still don't have the code to clean up the allocated
  memory for the created device on error, or in case the module is unloaded. 
  We
  should also prevent the pwm core from removal when the pwm-gpo driver is 
  loaded.
  We need to create the platform device for gpo-pwm, create the pdata 
  structure
  for it and fill it in. We also need to hand craft the pwm_lookup table so we
  can use pwm_get() to request the PWM. I have other minor changes around this
  to get things working when we booted with DT.
  So the function to do the heavy lifting is something like this:
  static void of_pwmchip_as_gpio(struct pwm_chip *chip)
  {
  struct platform_device *pdev;
  struct gpio_pwm *gpos;
  struct gpio_pwm_pdata *pdata;
  struct pwm_lookup *lookup;
  char gpodev_name[15];
  int i;
  u32 gpio_mode = 0;
  u32 period_ns = 0;
  
  of_property_read_u32(chip-dev-of_node, gpio-controller,
   gpio_mode);
  if (!gpio_mode)
  return;
  
  of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
  if (!period_ns) {
  dev_err(chip-dev,
  period_ns is not specified for GPIO use\n);
  return;
  }
 
 This property name seems ambiguous. What do you need to encode here? It
 looks like there is a specific PWM period used for the 'on' state. What
 about the 'off' state? Would different pwm outputs have different
 frequencies required for GPIO usage.
 
 Actually, I'm a bit surprised here that a period value is needed at all.
 I would expect if a PWM is used as a GPIO then the driver would already
 know how to set it up that way.

Just to make sure we're talking about the same thing here: if a PWM is
used as GPIO the assumption is that it would be set to 0% duty-cycle
when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
The period will still need to be set here, otherwise how would the PWM
core know what the hardware even supports?

Unless you're proposing to not include that in the PWM core but rather
in individual drivers. Then I suppose the driver could choose some
sensible default.

One other problem is that some PWM devices cannot be setup to achieve a
0% or 100% duty-cycle but instead will toggle for at least one period.
This would be another argument in favour of moving the functionality to
the individual drivers, perhaps with some functionality provided by the
core to do the gpio_chip registration (a period could be passed to that
function at registration time), which will likely be the same for all
hardware that can and wants to support this feature.

Thierry


pgpAujFGxIaFt.pgp
Description: PGP signature


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-28 Thread Lars-Peter Clausen
On 11/28/2012 09:54 AM, Peter Ujfalusi wrote:
> Hi Grant, Lars, Thierry,
> 
> On 11/26/2012 04:46 PM, Grant Likely wrote:
>> You're effectively asking the pwm layer to behave like a gpio (which
>> is completely reasonable). Having a completely separate translation node
>> really doesn't make sense because it is entirely a software construct.
>> In fact, the way your using it is *entirely* to make the Linux driver
>> model instantiate the translation code. It has *nothing* to do with the
>> structure of the hardware. It makes complete sense that if a PWM is
>> going to be used as a GPIO, then the PWM node should conform to the GPIO
>> binding.
> 
> I understand your point around this. I might say I agree with it as well...
> I spent yesterday with prototyping and I'm not really convinced that it is a
> good approach from C code point of view. I got it working, yes.
> In essence this is what I have on top of the slightly modified gpio-pwm.c
> driver I have submitted:
> 
> DTS files:
> twl_pwm: pwm {
>   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
>   compatible = "ti,twl6030-pwm";
>   #pwm-cells = <2>;
> 
>   /* Enable GPIO us of the PWMs */
>   gpio-controller = <1>;
>   #gpio-cells = <2>;
>   pwm,period_ns = <7812500>;
> };
> 
> leds {
>   compatible = "gpio-leds";
>   backlight {
>   label = "omap4::backlight";
>   gpios = <_pwm 1 0>; /* PWM1 of twl6030 */
>   };
> 
>   keypad {
>   label = "omap4::keypad";
>   gpios = <_pwm 0 0>; /* PWM0 of twl6030 */
>   };
> };
> 
> The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
> it is requested going to look something like this. I have removed the error
> checks for now and I still don't have the code to clean up the allocated
> memory for the created device on error, or in case the module is unloaded. We
> should also prevent the pwm core from removal when the pwm-gpo driver is 
> loaded.
> We need to create the platform device for gpo-pwm, create the pdata structure
> for it and fill it in. We also need to hand craft the pwm_lookup table so we
> can use pwm_get() to request the PWM. I have other minor changes around this
> to get things working when we booted with DT.
> So the function to do the heavy lifting is something like this:
> static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> {
>   struct platform_device *pdev;
>   struct gpio_pwm *gpos;
>   struct gpio_pwm_pdata *pdata;
>   struct pwm_lookup *lookup;
>   char gpodev_name[15];
>   int i;
>   u32 gpio_mode = 0;
>   u32 period_ns = 0;
> 
>   of_property_read_u32(chip->dev->of_node, "gpio-controller",
>_mode);
>   if (!gpio_mode)
>   return;
> 
>   of_property_read_u32(chip->dev->of_node, "pwm,period_ns", _ns);
>   if (!period_ns) {
>   dev_err(chip->dev,
>   "period_ns is not specified for GPIO use\n");
>   return;
>   }
> 
>   lookup = devm_kzalloc(chip->dev, sizeof(*lookup) * chip->npwm,
> GFP_KERNEL);
>   pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
>   gpos = devm_kzalloc(chip->dev, sizeof(*gpos) * chip->npwm,
>   GFP_KERNEL);
> 
>   pdata->gpos = gpos;
>   pdata->num_gpos = chip->npwm;
>   pdata->gpio_base = -1;
> 
>   pdev = platform_device_alloc("pwm-gpo", chip->base);
>   pdev->dev.parent = chip->dev;
> 
>   sprintf(gpodev_name, "pwm-gpo.%d", chip->base);
>   for (i = 0; i < chip->npwm; i++) {
>   struct gpio_pwm *gpo = [i];
>   struct pwm_lookup *pl = [i];
>   char con_id[15];
> 
>   sprintf(con_id, "pwm-gpo.%d", chip->base + i);
> 
>   /* prepare GPO information */
>   gpo->pwm_period_ns = period_ns;
>   gpo->name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
> 
>   /* prepare pwm lookup table */
>   pl->provider = dev_name(chip->dev);
>   pl->index = i;
>   pl->dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
>GFP_KERNEL);
>   pl->con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
>   }
> 
>   platform_device_add_data(pdev, pdata, sizeof(*pdata));
>   pwm_add_table(lookup, chip->npwm);
>   platform_device_add(pdev);
> }
> 

The whole pwm lookup table creation is a bit ugly. I wonder if we can somehow
bypass this by using pwm_request_from_chip directly in the pwm-gpo driver.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-28 Thread Thierry Reding
On Wed, Nov 28, 2012 at 09:54:57AM +0100, Peter Ujfalusi wrote:
> Hi Grant, Lars, Thierry,
> 
> On 11/26/2012 04:46 PM, Grant Likely wrote:
> > You're effectively asking the pwm layer to behave like a gpio (which
> > is completely reasonable). Having a completely separate translation node
> > really doesn't make sense because it is entirely a software construct.
> > In fact, the way your using it is *entirely* to make the Linux driver
> > model instantiate the translation code. It has *nothing* to do with the
> > structure of the hardware. It makes complete sense that if a PWM is
> > going to be used as a GPIO, then the PWM node should conform to the GPIO
> > binding.
> 
> I understand your point around this. I might say I agree with it as well...
> I spent yesterday with prototyping and I'm not really convinced that it is a
> good approach from C code point of view. I got it working, yes.
> In essence this is what I have on top of the slightly modified gpio-pwm.c
> driver I have submitted:
> 
> DTS files:
> twl_pwm: pwm {
>   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
>   compatible = "ti,twl6030-pwm";
>   #pwm-cells = <2>;
> 
>   /* Enable GPIO us of the PWMs */
>   gpio-controller = <1>;
>   #gpio-cells = <2>;
>   pwm,period_ns = <7812500>;
> };
> 
> leds {
>   compatible = "gpio-leds";
>   backlight {
>   label = "omap4::backlight";
>   gpios = <_pwm 1 0>; /* PWM1 of twl6030 */
>   };
> 
>   keypad {
>   label = "omap4::keypad";
>   gpios = <_pwm 0 0>; /* PWM0 of twl6030 */
>   };
> };
> 
> The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
> it is requested going to look something like this. I have removed the error
> checks for now and I still don't have the code to clean up the allocated
> memory for the created device on error, or in case the module is unloaded. We
> should also prevent the pwm core from removal when the pwm-gpo driver is 
> loaded.
> We need to create the platform device for gpo-pwm, create the pdata structure
> for it and fill it in. We also need to hand craft the pwm_lookup table so we
> can use pwm_get() to request the PWM. I have other minor changes around this
> to get things working when we booted with DT.
> So the function to do the heavy lifting is something like this:
> static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> {
>   struct platform_device *pdev;
>   struct gpio_pwm *gpos;
>   struct gpio_pwm_pdata *pdata;
>   struct pwm_lookup *lookup;
>   char gpodev_name[15];
>   int i;
>   u32 gpio_mode = 0;
>   u32 period_ns = 0;
> 
>   of_property_read_u32(chip->dev->of_node, "gpio-controller",
>_mode);
>   if (!gpio_mode)
>   return;
> 
>   of_property_read_u32(chip->dev->of_node, "pwm,period_ns", _ns);
>   if (!period_ns) {
>   dev_err(chip->dev,
>   "period_ns is not specified for GPIO use\n");
>   return;
>   }
> 
>   lookup = devm_kzalloc(chip->dev, sizeof(*lookup) * chip->npwm,
> GFP_KERNEL);
>   pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
>   gpos = devm_kzalloc(chip->dev, sizeof(*gpos) * chip->npwm,
>   GFP_KERNEL);
> 
>   pdata->gpos = gpos;
>   pdata->num_gpos = chip->npwm;
>   pdata->gpio_base = -1;
> 
>   pdev = platform_device_alloc("pwm-gpo", chip->base);
>   pdev->dev.parent = chip->dev;
> 
>   sprintf(gpodev_name, "pwm-gpo.%d", chip->base);
>   for (i = 0; i < chip->npwm; i++) {
>   struct gpio_pwm *gpo = [i];
>   struct pwm_lookup *pl = [i];
>   char con_id[15];
> 
>   sprintf(con_id, "pwm-gpo.%d", chip->base + i);
> 
>   /* prepare GPO information */
>   gpo->pwm_period_ns = period_ns;
>   gpo->name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
> 
>   /* prepare pwm lookup table */
>   pl->provider = dev_name(chip->dev);
>   pl->index = i;
>   pl->dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
>GFP_KERNEL);
>   pl->con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
>   }
> 
>   platform_device_add_data(pdev, pdata, sizeof(*pdata));
>   pwm_add_table(lookup, chip->npwm);
>   platform_device_add(pdev);
> }
> 
> PS: as I have said I have removed the error check just to make the code
> snippet more readable and yes we need to do some memory cleanup as well at the
> right time.
> 
> Is this something you would like to see?

I must say I'm not terribly thrilled to integrate something like this
into the PWM subsystem. I wish hardware engineers wouldn't come up with
such designs. But since this seems to be a real use-case, if we want to
support it we should try and find the "right" 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-28 Thread Peter Ujfalusi
Hi Grant, Lars, Thierry,

On 11/26/2012 04:46 PM, Grant Likely wrote:
> You're effectively asking the pwm layer to behave like a gpio (which
> is completely reasonable). Having a completely separate translation node
> really doesn't make sense because it is entirely a software construct.
> In fact, the way your using it is *entirely* to make the Linux driver
> model instantiate the translation code. It has *nothing* to do with the
> structure of the hardware. It makes complete sense that if a PWM is
> going to be used as a GPIO, then the PWM node should conform to the GPIO
> binding.

I understand your point around this. I might say I agree with it as well...
I spent yesterday with prototyping and I'm not really convinced that it is a
good approach from C code point of view. I got it working, yes.
In essence this is what I have on top of the slightly modified gpio-pwm.c
driver I have submitted:

DTS files:
twl_pwm: pwm {
/* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
compatible = "ti,twl6030-pwm";
#pwm-cells = <2>;

/* Enable GPIO us of the PWMs */
gpio-controller = <1>;
#gpio-cells = <2>;
pwm,period_ns = <7812500>;
};

leds {
compatible = "gpio-leds";
backlight {
label = "omap4::backlight";
gpios = <_pwm 1 0>; /* PWM1 of twl6030 */
};

keypad {
label = "omap4::keypad";
gpios = <_pwm 0 0>; /* PWM0 of twl6030 */
};
};

The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
it is requested going to look something like this. I have removed the error
checks for now and I still don't have the code to clean up the allocated
memory for the created device on error, or in case the module is unloaded. We
should also prevent the pwm core from removal when the pwm-gpo driver is loaded.
We need to create the platform device for gpo-pwm, create the pdata structure
for it and fill it in. We also need to hand craft the pwm_lookup table so we
can use pwm_get() to request the PWM. I have other minor changes around this
to get things working when we booted with DT.
So the function to do the heavy lifting is something like this:
static void of_pwmchip_as_gpio(struct pwm_chip *chip)
{
struct platform_device *pdev;
struct gpio_pwm *gpos;
struct gpio_pwm_pdata *pdata;
struct pwm_lookup *lookup;
char gpodev_name[15];
int i;
u32 gpio_mode = 0;
u32 period_ns = 0;

of_property_read_u32(chip->dev->of_node, "gpio-controller",
 _mode);
if (!gpio_mode)
return;

of_property_read_u32(chip->dev->of_node, "pwm,period_ns", _ns);
if (!period_ns) {
dev_err(chip->dev,
"period_ns is not specified for GPIO use\n");
return;
}

lookup = devm_kzalloc(chip->dev, sizeof(*lookup) * chip->npwm,
  GFP_KERNEL);
pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
gpos = devm_kzalloc(chip->dev, sizeof(*gpos) * chip->npwm,
GFP_KERNEL);

pdata->gpos = gpos;
pdata->num_gpos = chip->npwm;
pdata->gpio_base = -1;

pdev = platform_device_alloc("pwm-gpo", chip->base);
pdev->dev.parent = chip->dev;

sprintf(gpodev_name, "pwm-gpo.%d", chip->base);
for (i = 0; i < chip->npwm; i++) {
struct gpio_pwm *gpo = [i];
struct pwm_lookup *pl = [i];
char con_id[15];

sprintf(con_id, "pwm-gpo.%d", chip->base + i);

/* prepare GPO information */
gpo->pwm_period_ns = period_ns;
gpo->name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;

/* prepare pwm lookup table */
pl->provider = dev_name(chip->dev);
pl->index = i;
pl->dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
 GFP_KERNEL);
pl->con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
}

platform_device_add_data(pdev, pdata, sizeof(*pdata));
pwm_add_table(lookup, chip->npwm);
platform_device_add(pdev);
}

PS: as I have said I have removed the error check just to make the code
snippet more readable and yes we need to do some memory cleanup as well at the
right time.

Is this something you would like to see?

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-28 Thread Peter Ujfalusi
Hi Grant, Lars, Thierry,

On 11/26/2012 04:46 PM, Grant Likely wrote:
 You're effectively asking the pwm layer to behave like a gpio (which
 is completely reasonable). Having a completely separate translation node
 really doesn't make sense because it is entirely a software construct.
 In fact, the way your using it is *entirely* to make the Linux driver
 model instantiate the translation code. It has *nothing* to do with the
 structure of the hardware. It makes complete sense that if a PWM is
 going to be used as a GPIO, then the PWM node should conform to the GPIO
 binding.

I understand your point around this. I might say I agree with it as well...
I spent yesterday with prototyping and I'm not really convinced that it is a
good approach from C code point of view. I got it working, yes.
In essence this is what I have on top of the slightly modified gpio-pwm.c
driver I have submitted:

DTS files:
twl_pwm: pwm {
/* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
compatible = ti,twl6030-pwm;
#pwm-cells = 2;

/* Enable GPIO us of the PWMs */
gpio-controller = 1;
#gpio-cells = 2;
pwm,period_ns = 7812500;
};

leds {
compatible = gpio-leds;
backlight {
label = omap4::backlight;
gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
};

keypad {
label = omap4::keypad;
gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
};
};

The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
it is requested going to look something like this. I have removed the error
checks for now and I still don't have the code to clean up the allocated
memory for the created device on error, or in case the module is unloaded. We
should also prevent the pwm core from removal when the pwm-gpo driver is loaded.
We need to create the platform device for gpo-pwm, create the pdata structure
for it and fill it in. We also need to hand craft the pwm_lookup table so we
can use pwm_get() to request the PWM. I have other minor changes around this
to get things working when we booted with DT.
So the function to do the heavy lifting is something like this:
static void of_pwmchip_as_gpio(struct pwm_chip *chip)
{
struct platform_device *pdev;
struct gpio_pwm *gpos;
struct gpio_pwm_pdata *pdata;
struct pwm_lookup *lookup;
char gpodev_name[15];
int i;
u32 gpio_mode = 0;
u32 period_ns = 0;

of_property_read_u32(chip-dev-of_node, gpio-controller,
 gpio_mode);
if (!gpio_mode)
return;

of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
if (!period_ns) {
dev_err(chip-dev,
period_ns is not specified for GPIO use\n);
return;
}

lookup = devm_kzalloc(chip-dev, sizeof(*lookup) * chip-npwm,
  GFP_KERNEL);
pdata = devm_kzalloc(chip-dev, sizeof(*pdata), GFP_KERNEL);
gpos = devm_kzalloc(chip-dev, sizeof(*gpos) * chip-npwm,
GFP_KERNEL);

pdata-gpos = gpos;
pdata-num_gpos = chip-npwm;
pdata-gpio_base = -1;

pdev = platform_device_alloc(pwm-gpo, chip-base);
pdev-dev.parent = chip-dev;

sprintf(gpodev_name, pwm-gpo.%d, chip-base);
for (i = 0; i  chip-npwm; i++) {
struct gpio_pwm *gpo = gpos[i];
struct pwm_lookup *pl = lookup[i];
char con_id[15];

sprintf(con_id, pwm-gpo.%d, chip-base + i);

/* prepare GPO information */
gpo-pwm_period_ns = period_ns;
gpo-name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;

/* prepare pwm lookup table */
pl-provider = dev_name(chip-dev);
pl-index = i;
pl-dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
 GFP_KERNEL);
pl-con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
}

platform_device_add_data(pdev, pdata, sizeof(*pdata));
pwm_add_table(lookup, chip-npwm);
platform_device_add(pdev);
}

PS: as I have said I have removed the error check just to make the code
snippet more readable and yes we need to do some memory cleanup as well at the
right time.

Is this something you would like to see?

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-28 Thread Thierry Reding
On Wed, Nov 28, 2012 at 09:54:57AM +0100, Peter Ujfalusi wrote:
 Hi Grant, Lars, Thierry,
 
 On 11/26/2012 04:46 PM, Grant Likely wrote:
  You're effectively asking the pwm layer to behave like a gpio (which
  is completely reasonable). Having a completely separate translation node
  really doesn't make sense because it is entirely a software construct.
  In fact, the way your using it is *entirely* to make the Linux driver
  model instantiate the translation code. It has *nothing* to do with the
  structure of the hardware. It makes complete sense that if a PWM is
  going to be used as a GPIO, then the PWM node should conform to the GPIO
  binding.
 
 I understand your point around this. I might say I agree with it as well...
 I spent yesterday with prototyping and I'm not really convinced that it is a
 good approach from C code point of view. I got it working, yes.
 In essence this is what I have on top of the slightly modified gpio-pwm.c
 driver I have submitted:
 
 DTS files:
 twl_pwm: pwm {
   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
   compatible = ti,twl6030-pwm;
   #pwm-cells = 2;
 
   /* Enable GPIO us of the PWMs */
   gpio-controller = 1;
   #gpio-cells = 2;
   pwm,period_ns = 7812500;
 };
 
 leds {
   compatible = gpio-leds;
   backlight {
   label = omap4::backlight;
   gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
   };
 
   keypad {
   label = omap4::keypad;
   gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
   };
 };
 
 The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
 it is requested going to look something like this. I have removed the error
 checks for now and I still don't have the code to clean up the allocated
 memory for the created device on error, or in case the module is unloaded. We
 should also prevent the pwm core from removal when the pwm-gpo driver is 
 loaded.
 We need to create the platform device for gpo-pwm, create the pdata structure
 for it and fill it in. We also need to hand craft the pwm_lookup table so we
 can use pwm_get() to request the PWM. I have other minor changes around this
 to get things working when we booted with DT.
 So the function to do the heavy lifting is something like this:
 static void of_pwmchip_as_gpio(struct pwm_chip *chip)
 {
   struct platform_device *pdev;
   struct gpio_pwm *gpos;
   struct gpio_pwm_pdata *pdata;
   struct pwm_lookup *lookup;
   char gpodev_name[15];
   int i;
   u32 gpio_mode = 0;
   u32 period_ns = 0;
 
   of_property_read_u32(chip-dev-of_node, gpio-controller,
gpio_mode);
   if (!gpio_mode)
   return;
 
   of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
   if (!period_ns) {
   dev_err(chip-dev,
   period_ns is not specified for GPIO use\n);
   return;
   }
 
   lookup = devm_kzalloc(chip-dev, sizeof(*lookup) * chip-npwm,
 GFP_KERNEL);
   pdata = devm_kzalloc(chip-dev, sizeof(*pdata), GFP_KERNEL);
   gpos = devm_kzalloc(chip-dev, sizeof(*gpos) * chip-npwm,
   GFP_KERNEL);
 
   pdata-gpos = gpos;
   pdata-num_gpos = chip-npwm;
   pdata-gpio_base = -1;
 
   pdev = platform_device_alloc(pwm-gpo, chip-base);
   pdev-dev.parent = chip-dev;
 
   sprintf(gpodev_name, pwm-gpo.%d, chip-base);
   for (i = 0; i  chip-npwm; i++) {
   struct gpio_pwm *gpo = gpos[i];
   struct pwm_lookup *pl = lookup[i];
   char con_id[15];
 
   sprintf(con_id, pwm-gpo.%d, chip-base + i);
 
   /* prepare GPO information */
   gpo-pwm_period_ns = period_ns;
   gpo-name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
 
   /* prepare pwm lookup table */
   pl-provider = dev_name(chip-dev);
   pl-index = i;
   pl-dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
GFP_KERNEL);
   pl-con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
   }
 
   platform_device_add_data(pdev, pdata, sizeof(*pdata));
   pwm_add_table(lookup, chip-npwm);
   platform_device_add(pdev);
 }
 
 PS: as I have said I have removed the error check just to make the code
 snippet more readable and yes we need to do some memory cleanup as well at the
 right time.
 
 Is this something you would like to see?

I must say I'm not terribly thrilled to integrate something like this
into the PWM subsystem. I wish hardware engineers wouldn't come up with
such designs. But since this seems to be a real use-case, if we want to
support it we should try and find the right solution.

Reading the above sounds overly complicated. Couldn't we, instead of
instantiating an extra driver, just register a GPIO chip if a chip wants
to support this 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-28 Thread Lars-Peter Clausen
On 11/28/2012 09:54 AM, Peter Ujfalusi wrote:
 Hi Grant, Lars, Thierry,
 
 On 11/26/2012 04:46 PM, Grant Likely wrote:
 You're effectively asking the pwm layer to behave like a gpio (which
 is completely reasonable). Having a completely separate translation node
 really doesn't make sense because it is entirely a software construct.
 In fact, the way your using it is *entirely* to make the Linux driver
 model instantiate the translation code. It has *nothing* to do with the
 structure of the hardware. It makes complete sense that if a PWM is
 going to be used as a GPIO, then the PWM node should conform to the GPIO
 binding.
 
 I understand your point around this. I might say I agree with it as well...
 I spent yesterday with prototyping and I'm not really convinced that it is a
 good approach from C code point of view. I got it working, yes.
 In essence this is what I have on top of the slightly modified gpio-pwm.c
 driver I have submitted:
 
 DTS files:
 twl_pwm: pwm {
   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
   compatible = ti,twl6030-pwm;
   #pwm-cells = 2;
 
   /* Enable GPIO us of the PWMs */
   gpio-controller = 1;
   #gpio-cells = 2;
   pwm,period_ns = 7812500;
 };
 
 leds {
   compatible = gpio-leds;
   backlight {
   label = omap4::backlight;
   gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
   };
 
   keypad {
   label = omap4::keypad;
   gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
   };
 };
 
 The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
 it is requested going to look something like this. I have removed the error
 checks for now and I still don't have the code to clean up the allocated
 memory for the created device on error, or in case the module is unloaded. We
 should also prevent the pwm core from removal when the pwm-gpo driver is 
 loaded.
 We need to create the platform device for gpo-pwm, create the pdata structure
 for it and fill it in. We also need to hand craft the pwm_lookup table so we
 can use pwm_get() to request the PWM. I have other minor changes around this
 to get things working when we booted with DT.
 So the function to do the heavy lifting is something like this:
 static void of_pwmchip_as_gpio(struct pwm_chip *chip)
 {
   struct platform_device *pdev;
   struct gpio_pwm *gpos;
   struct gpio_pwm_pdata *pdata;
   struct pwm_lookup *lookup;
   char gpodev_name[15];
   int i;
   u32 gpio_mode = 0;
   u32 period_ns = 0;
 
   of_property_read_u32(chip-dev-of_node, gpio-controller,
gpio_mode);
   if (!gpio_mode)
   return;
 
   of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
   if (!period_ns) {
   dev_err(chip-dev,
   period_ns is not specified for GPIO use\n);
   return;
   }
 
   lookup = devm_kzalloc(chip-dev, sizeof(*lookup) * chip-npwm,
 GFP_KERNEL);
   pdata = devm_kzalloc(chip-dev, sizeof(*pdata), GFP_KERNEL);
   gpos = devm_kzalloc(chip-dev, sizeof(*gpos) * chip-npwm,
   GFP_KERNEL);
 
   pdata-gpos = gpos;
   pdata-num_gpos = chip-npwm;
   pdata-gpio_base = -1;
 
   pdev = platform_device_alloc(pwm-gpo, chip-base);
   pdev-dev.parent = chip-dev;
 
   sprintf(gpodev_name, pwm-gpo.%d, chip-base);
   for (i = 0; i  chip-npwm; i++) {
   struct gpio_pwm *gpo = gpos[i];
   struct pwm_lookup *pl = lookup[i];
   char con_id[15];
 
   sprintf(con_id, pwm-gpo.%d, chip-base + i);
 
   /* prepare GPO information */
   gpo-pwm_period_ns = period_ns;
   gpo-name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
 
   /* prepare pwm lookup table */
   pl-provider = dev_name(chip-dev);
   pl-index = i;
   pl-dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
GFP_KERNEL);
   pl-con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
   }
 
   platform_device_add_data(pdev, pdata, sizeof(*pdata));
   pwm_add_table(lookup, chip-npwm);
   platform_device_add(pdev);
 }
 

The whole pwm lookup table creation is a bit ugly. I wonder if we can somehow
bypass this by using pwm_request_from_chip directly in the pwm-gpo driver.

- Lars

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-26 Thread Grant Likely
On Fri, 23 Nov 2012 10:44:36 +0100, Peter Ujfalusi  
wrote:
> Hi Grant,
> 
> On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
> > Hi Grant,
> > 
> > On 11/23/2012 08:55 AM, Grant Likely wrote:
> >> Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
> >> same namespace and binding.  But that's not your fault.
> >>
> >> It's pretty horrible to have a separate translator node to convert a PWM
> >> into a GPIO (with output only of course). The gpio properties should
> >> appear directly in the PWM node itself and the translation code should
> >> be in either the pwm or the gpio core. I don't think it should look like
> >> a separate device.
> > 
> > Let me see if I understand your suggestion correctly. In the DT you suggest
> > something like this:
> > 
> > twl_pwmled: pwmled {
> > compatible = "ti,twl4030-pwmled";
> > #pwm-cells = <2>;
> > #gpio-cells = <2>;
> > gpio-controller;
> > };
> 
> After I thought about this.. Is this what we really want?
> After all what we have here is a PWM generator used to emulate a GPIO signal.
> The PWM itself can be used for driving a LED (standard LED or backlight and we
> have DT bindings for these already), vibra motor, or other things.
> If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
> include all sort of other uses of PWM at once?
> 
> IMHO it is better to keep them as separate things.
> pwm node to describe the PWM generator,
> separate nodes to describe it's uses like led, backlight, motor and gpio.

You're effectively asking the pwm layer to behave like a gpio (which
is completely reasonable). Having a completely separate translation node
really doesn't make sense because it is entirely a software construct.
In fact, the way your using it is *entirely* to make the Linux driver
model instantiate the translation code. It has *nothing* to do with the
structure of the hardware. It makes complete sense that if a PWM is
going to be used as a GPIO, then the PWM node should conform to the GPIO
binding.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-26 Thread Peter Ujfalusi
hi,

On 11/26/2012 11:30 AM, Lars-Peter Clausen wrote:
> The difference here is that the LED, backlight, etc are all different
> physical devices begin driven by the pwm pin, so it makes sense to have a
> device tree node for them, while using the pwm as gpio is just a different
> function of the same physical pin.  So in a sense the pwm controller also
> becomes a gpio controller. I like the idea of the pwm core automatically
> instantiating a pwm-gpo device if it sees a gpio-controller property in the
> pwm device devicetree node.

OK, fair enough. I will go with the plan I described in the first mail for the
GPIO use of PWM.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-26 Thread Lars-Peter Clausen
On 11/23/2012 10:44 AM, Peter Ujfalusi wrote:
> Hi Grant,
> 
> On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
>> Hi Grant,
>>
>> On 11/23/2012 08:55 AM, Grant Likely wrote:
>>> Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
>>> same namespace and binding.  But that's not your fault.
>>>
>>> It's pretty horrible to have a separate translator node to convert a PWM
>>> into a GPIO (with output only of course). The gpio properties should
>>> appear directly in the PWM node itself and the translation code should
>>> be in either the pwm or the gpio core. I don't think it should look like
>>> a separate device.
>>
>> Let me see if I understand your suggestion correctly. In the DT you suggest
>> something like this:
>>
>> twl_pwmled: pwmled {
>>  compatible = "ti,twl4030-pwmled";
>>  #pwm-cells = <2>;
>>  #gpio-cells = <2>;
>>  gpio-controller;
>> };
> 
> After I thought about this.. Is this what we really want?
> After all what we have here is a PWM generator used to emulate a GPIO signal.
> The PWM itself can be used for driving a LED (standard LED or backlight and we
> have DT bindings for these already), vibra motor, or other things.
> If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
> include all sort of other uses of PWM at once?
> 
> IMHO it is better to keep them as separate things.
> pwm node to describe the PWM generator,
> separate nodes to describe it's uses like led, backlight, motor and gpio.
> 

The difference here is that the LED, backlight, etc are all different
physical devices begin driven by the pwm pin, so it makes sense to have a
device tree node for them, while using the pwm as gpio is just a different
function of the same physical pin.  So in a sense the pwm controller also
becomes a gpio controller. I like the idea of the pwm core automatically
instantiating a pwm-gpo device if it sees a gpio-controller property in the
pwm device devicetree node.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-26 Thread Lars-Peter Clausen
On 11/23/2012 10:44 AM, Peter Ujfalusi wrote:
 Hi Grant,
 
 On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
 Hi Grant,

 On 11/23/2012 08:55 AM, Grant Likely wrote:
 Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
 same namespace and binding. grumble, mutter But that's not your fault.

 It's pretty horrible to have a separate translator node to convert a PWM
 into a GPIO (with output only of course). The gpio properties should
 appear directly in the PWM node itself and the translation code should
 be in either the pwm or the gpio core. I don't think it should look like
 a separate device.

 Let me see if I understand your suggestion correctly. In the DT you suggest
 something like this:

 twl_pwmled: pwmled {
  compatible = ti,twl4030-pwmled;
  #pwm-cells = 2;
  #gpio-cells = 2;
  gpio-controller;
 };
 
 After I thought about this.. Is this what we really want?
 After all what we have here is a PWM generator used to emulate a GPIO signal.
 The PWM itself can be used for driving a LED (standard LED or backlight and we
 have DT bindings for these already), vibra motor, or other things.
 If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
 include all sort of other uses of PWM at once?
 
 IMHO it is better to keep them as separate things.
 pwm node to describe the PWM generator,
 separate nodes to describe it's uses like led, backlight, motor and gpio.
 

The difference here is that the LED, backlight, etc are all different
physical devices begin driven by the pwm pin, so it makes sense to have a
device tree node for them, while using the pwm as gpio is just a different
function of the same physical pin.  So in a sense the pwm controller also
becomes a gpio controller. I like the idea of the pwm core automatically
instantiating a pwm-gpo device if it sees a gpio-controller property in the
pwm device devicetree node.

- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-26 Thread Peter Ujfalusi
hi,

On 11/26/2012 11:30 AM, Lars-Peter Clausen wrote:
 The difference here is that the LED, backlight, etc are all different
 physical devices begin driven by the pwm pin, so it makes sense to have a
 device tree node for them, while using the pwm as gpio is just a different
 function of the same physical pin.  So in a sense the pwm controller also
 becomes a gpio controller. I like the idea of the pwm core automatically
 instantiating a pwm-gpo device if it sees a gpio-controller property in the
 pwm device devicetree node.

OK, fair enough. I will go with the plan I described in the first mail for the
GPIO use of PWM.

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-26 Thread Grant Likely
On Fri, 23 Nov 2012 10:44:36 +0100, Peter Ujfalusi peter.ujfal...@ti.com 
wrote:
 Hi Grant,
 
 On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
  Hi Grant,
  
  On 11/23/2012 08:55 AM, Grant Likely wrote:
  Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
  same namespace and binding. grumble, mutter But that's not your fault.
 
  It's pretty horrible to have a separate translator node to convert a PWM
  into a GPIO (with output only of course). The gpio properties should
  appear directly in the PWM node itself and the translation code should
  be in either the pwm or the gpio core. I don't think it should look like
  a separate device.
  
  Let me see if I understand your suggestion correctly. In the DT you suggest
  something like this:
  
  twl_pwmled: pwmled {
  compatible = ti,twl4030-pwmled;
  #pwm-cells = 2;
  #gpio-cells = 2;
  gpio-controller;
  };
 
 After I thought about this.. Is this what we really want?
 After all what we have here is a PWM generator used to emulate a GPIO signal.
 The PWM itself can be used for driving a LED (standard LED or backlight and we
 have DT bindings for these already), vibra motor, or other things.
 If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
 include all sort of other uses of PWM at once?
 
 IMHO it is better to keep them as separate things.
 pwm node to describe the PWM generator,
 separate nodes to describe it's uses like led, backlight, motor and gpio.

You're effectively asking the pwm layer to behave like a gpio (which
is completely reasonable). Having a completely separate translation node
really doesn't make sense because it is entirely a software construct.
In fact, the way your using it is *entirely* to make the Linux driver
model instantiate the translation code. It has *nothing* to do with the
structure of the hardware. It makes complete sense that if a PWM is
going to be used as a GPIO, then the PWM node should conform to the GPIO
binding.

g.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-23 Thread Peter Ujfalusi
Hi Grant,

On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
> Hi Grant,
> 
> On 11/23/2012 08:55 AM, Grant Likely wrote:
>> Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
>> same namespace and binding.  But that's not your fault.
>>
>> It's pretty horrible to have a separate translator node to convert a PWM
>> into a GPIO (with output only of course). The gpio properties should
>> appear directly in the PWM node itself and the translation code should
>> be in either the pwm or the gpio core. I don't think it should look like
>> a separate device.
> 
> Let me see if I understand your suggestion correctly. In the DT you suggest
> something like this:
> 
> twl_pwmled: pwmled {
>   compatible = "ti,twl4030-pwmled";
>   #pwm-cells = <2>;
>   #gpio-cells = <2>;
>   gpio-controller;
> };

After I thought about this.. Is this what we really want?
After all what we have here is a PWM generator used to emulate a GPIO signal.
The PWM itself can be used for driving a LED (standard LED or backlight and we
have DT bindings for these already), vibra motor, or other things.
If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
include all sort of other uses of PWM at once?

IMHO it is better to keep them as separate things.
pwm node to describe the PWM generator,
separate nodes to describe it's uses like led, backlight, motor and gpio.

-- 
Péter

> 
> led_user {
>   compatible = "pwm-leds";
>   pwms = <_pwmled 1 7812500>; /* PWMB/LEDB from twl4030 */
>   pwm-names = "PMU_STAT LED";
> 
>   label = "beagleboard::pmu_stat";
>   max-brightness = <127>;
> };
> 
> vdd_usbhost: fixedregulator-vdd-usbhost {
>   compatible = "regulator-fixed";
>   regulator-name = "USBHOST_POWER";
>   regulator-min-microvolt = <500>;
>   regulator-max-microvolt = <500>;
>   gpio = <_pwmled 0 7812500>; /* PWMA/LEDA from twl4030 */
>   enable-active-high;
>   regulator-boot-on;
> };
> 
> With this I think this is what should happen in code level:
> - the "pwm-gpo" driver does not have of_match_table at all.
> - the driver for the "ti,twl4030-pwmled" is loaded.
> - it prepares and calls pwmchip_add() to add the PWM chip.
> - the of_pwmchip_add() will look for gpio-controller property of the node
>  - if it is found it prepares the pdata (based on the PWM chip information)
> for the "pwm-gpo" driver and registers the platform_device for it.
>  - the "pwm-gpo" driver will use:
> priv->gpio_chip.of_node = pdev->dev.parent->of_node;
> 
> In DT boot we are fine with this I think.
> 
> When it comes to legacy boot (boot without DT) I think we should still have
> the two layers to avoid big changes which would affect all existing pwm
> drivers. Something like this in the board files:
> 
> static struct pwm_lookup pwm_lookup[] = {
>   /* LEDA ->  nUSBHOST_PWR_EN */
>   PWM_LOOKUP("twl-pwmled", 0, "pwm-gpo", "nUSBHOST_PWR_EN"),
>   /* LEDB -> PMU_STAT */
>   PWM_LOOKUP("twl-pwmled", 1, "leds_pwm", "beagleboard::pmu_stat"),
> };
> 
> /* for the LED user of PWM */
> static struct led_pwm pwm_leds[] = {
>   {
>   .name   = "beagleboard::pmu_stat",
>   .max_brightness = 127,
>   .pwm_period_ns  = 7812500,
>   },
> };
> 
> static struct led_pwm_platform_data pwm_data = {
>   .num_leds   = ARRAY_SIZE(pwm_leds),
>   .leds   = pwm_leds,
> };
> 
> static struct platform_device leds_pwm = {
>   .name   = "leds_pwm",
>   .id = -1,
>   .dev= {
>   .platform_data = _data,
>   },
> };
> 
> /* for the GPIO user of PWM */
> static struct gpio_pwm pwm_gpios[] = {
>   {
>   .name   = "nUSBHOST_PWR_EN",
>   .pwm_period_ns  = 7812500,
>   },
> };
> 
> static struct gpio_pwm_pdata pwm_gpio_data = {
>   .num_gpos   = ARRAY_SIZE(pwm_gpios),
>   .gpos   = pwm_gpios,
>   .setup  = beagle_pwm_gpio_setup, /*to get the gpio base */
> };
> 
> static struct platform_device gpos_pwm = {
>   .name   = "pwm-gpo",
>   .id = -1,
>   .dev= {
>   .platform_data = _gpio_data,
>   },
> };
> 
> static int beagle_pwm_gpio_setup(struct device *dev, unsigned gpio,
>unsigned ngpio)
> {
>   beagle_usbhub_pdata.gpio = gpio; /* fixed_voltage_config struct */
> 
>   platform_device_register(_usbhub);
>   return 0;
> }
> 
> What do you think?
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-23 Thread Peter Ujfalusi
Hi Grant,

On 11/23/2012 08:55 AM, Grant Likely wrote:
> Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
> same namespace and binding.  But that's not your fault.
> 
> It's pretty horrible to have a separate translator node to convert a PWM
> into a GPIO (with output only of course). The gpio properties should
> appear directly in the PWM node itself and the translation code should
> be in either the pwm or the gpio core. I don't think it should look like
> a separate device.

Let me see if I understand your suggestion correctly. In the DT you suggest
something like this:

twl_pwmled: pwmled {
compatible = "ti,twl4030-pwmled";
#pwm-cells = <2>;
#gpio-cells = <2>;
gpio-controller;
};

led_user {
compatible = "pwm-leds";
pwms = <_pwmled 1 7812500>; /* PWMB/LEDB from twl4030 */
pwm-names = "PMU_STAT LED";

label = "beagleboard::pmu_stat";
max-brightness = <127>;
};

vdd_usbhost: fixedregulator-vdd-usbhost {
compatible = "regulator-fixed";
regulator-name = "USBHOST_POWER";
regulator-min-microvolt = <500>;
regulator-max-microvolt = <500>;
gpio = <_pwmled 0 7812500>; /* PWMA/LEDA from twl4030 */
enable-active-high;
regulator-boot-on;
};

With this I think this is what should happen in code level:
- the "pwm-gpo" driver does not have of_match_table at all.
- the driver for the "ti,twl4030-pwmled" is loaded.
- it prepares and calls pwmchip_add() to add the PWM chip.
- the of_pwmchip_add() will look for gpio-controller property of the node
 - if it is found it prepares the pdata (based on the PWM chip information)
for the "pwm-gpo" driver and registers the platform_device for it.
 - the "pwm-gpo" driver will use:
priv->gpio_chip.of_node = pdev->dev.parent->of_node;

In DT boot we are fine with this I think.

When it comes to legacy boot (boot without DT) I think we should still have
the two layers to avoid big changes which would affect all existing pwm
drivers. Something like this in the board files:

static struct pwm_lookup pwm_lookup[] = {
/* LEDA ->  nUSBHOST_PWR_EN */
PWM_LOOKUP("twl-pwmled", 0, "pwm-gpo", "nUSBHOST_PWR_EN"),
/* LEDB -> PMU_STAT */
PWM_LOOKUP("twl-pwmled", 1, "leds_pwm", "beagleboard::pmu_stat"),
};

/* for the LED user of PWM */
static struct led_pwm pwm_leds[] = {
{
.name   = "beagleboard::pmu_stat",
.max_brightness = 127,
.pwm_period_ns  = 7812500,
},
};

static struct led_pwm_platform_data pwm_data = {
.num_leds   = ARRAY_SIZE(pwm_leds),
.leds   = pwm_leds,
};

static struct platform_device leds_pwm = {
.name   = "leds_pwm",
.id = -1,
.dev= {
.platform_data = _data,
},
};

/* for the GPIO user of PWM */
static struct gpio_pwm pwm_gpios[] = {
{
.name   = "nUSBHOST_PWR_EN",
.pwm_period_ns  = 7812500,
},
};

static struct gpio_pwm_pdata pwm_gpio_data = {
.num_gpos   = ARRAY_SIZE(pwm_gpios),
.gpos   = pwm_gpios,
.setup  = beagle_pwm_gpio_setup, /*to get the gpio base */
};

static struct platform_device gpos_pwm = {
.name   = "pwm-gpo",
.id = -1,
.dev= {
.platform_data = _gpio_data,
},
};

static int beagle_pwm_gpio_setup(struct device *dev, unsigned gpio,
 unsigned ngpio)
{
beagle_usbhub_pdata.gpio = gpio; /* fixed_voltage_config struct */

platform_device_register(_usbhub);
return 0;
}

What do you think?

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-23 Thread Peter Ujfalusi
Hi Grant,

On 11/23/2012 08:55 AM, Grant Likely wrote:
 Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
 same namespace and binding. grumble, mutter But that's not your fault.
 
 It's pretty horrible to have a separate translator node to convert a PWM
 into a GPIO (with output only of course). The gpio properties should
 appear directly in the PWM node itself and the translation code should
 be in either the pwm or the gpio core. I don't think it should look like
 a separate device.

Let me see if I understand your suggestion correctly. In the DT you suggest
something like this:

twl_pwmled: pwmled {
compatible = ti,twl4030-pwmled;
#pwm-cells = 2;
#gpio-cells = 2;
gpio-controller;
};

led_user {
compatible = pwm-leds;
pwms = twl_pwmled 1 7812500; /* PWMB/LEDB from twl4030 */
pwm-names = PMU_STAT LED;

label = beagleboard::pmu_stat;
max-brightness = 127;
};

vdd_usbhost: fixedregulator-vdd-usbhost {
compatible = regulator-fixed;
regulator-name = USBHOST_POWER;
regulator-min-microvolt = 500;
regulator-max-microvolt = 500;
gpio = twl_pwmled 0 7812500; /* PWMA/LEDA from twl4030 */
enable-active-high;
regulator-boot-on;
};

With this I think this is what should happen in code level:
- the pwm-gpo driver does not have of_match_table at all.
- the driver for the ti,twl4030-pwmled is loaded.
- it prepares and calls pwmchip_add() to add the PWM chip.
- the of_pwmchip_add() will look for gpio-controller property of the node
 - if it is found it prepares the pdata (based on the PWM chip information)
for the pwm-gpo driver and registers the platform_device for it.
 - the pwm-gpo driver will use:
priv-gpio_chip.of_node = pdev-dev.parent-of_node;

In DT boot we are fine with this I think.

When it comes to legacy boot (boot without DT) I think we should still have
the two layers to avoid big changes which would affect all existing pwm
drivers. Something like this in the board files:

static struct pwm_lookup pwm_lookup[] = {
/* LEDA -  nUSBHOST_PWR_EN */
PWM_LOOKUP(twl-pwmled, 0, pwm-gpo, nUSBHOST_PWR_EN),
/* LEDB - PMU_STAT */
PWM_LOOKUP(twl-pwmled, 1, leds_pwm, beagleboard::pmu_stat),
};

/* for the LED user of PWM */
static struct led_pwm pwm_leds[] = {
{
.name   = beagleboard::pmu_stat,
.max_brightness = 127,
.pwm_period_ns  = 7812500,
},
};

static struct led_pwm_platform_data pwm_data = {
.num_leds   = ARRAY_SIZE(pwm_leds),
.leds   = pwm_leds,
};

static struct platform_device leds_pwm = {
.name   = leds_pwm,
.id = -1,
.dev= {
.platform_data = pwm_data,
},
};

/* for the GPIO user of PWM */
static struct gpio_pwm pwm_gpios[] = {
{
.name   = nUSBHOST_PWR_EN,
.pwm_period_ns  = 7812500,
},
};

static struct gpio_pwm_pdata pwm_gpio_data = {
.num_gpos   = ARRAY_SIZE(pwm_gpios),
.gpos   = pwm_gpios,
.setup  = beagle_pwm_gpio_setup, /*to get the gpio base */
};

static struct platform_device gpos_pwm = {
.name   = pwm-gpo,
.id = -1,
.dev= {
.platform_data = pwm_gpio_data,
},
};

static int beagle_pwm_gpio_setup(struct device *dev, unsigned gpio,
 unsigned ngpio)
{
beagle_usbhub_pdata.gpio = gpio; /* fixed_voltage_config struct */

platform_device_register(beagle_usbhub);
return 0;
}

What do you think?

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-23 Thread Peter Ujfalusi
Hi Grant,

On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
 Hi Grant,
 
 On 11/23/2012 08:55 AM, Grant Likely wrote:
 Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
 same namespace and binding. grumble, mutter But that's not your fault.

 It's pretty horrible to have a separate translator node to convert a PWM
 into a GPIO (with output only of course). The gpio properties should
 appear directly in the PWM node itself and the translation code should
 be in either the pwm or the gpio core. I don't think it should look like
 a separate device.
 
 Let me see if I understand your suggestion correctly. In the DT you suggest
 something like this:
 
 twl_pwmled: pwmled {
   compatible = ti,twl4030-pwmled;
   #pwm-cells = 2;
   #gpio-cells = 2;
   gpio-controller;
 };

After I thought about this.. Is this what we really want?
After all what we have here is a PWM generator used to emulate a GPIO signal.
The PWM itself can be used for driving a LED (standard LED or backlight and we
have DT bindings for these already), vibra motor, or other things.
If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
include all sort of other uses of PWM at once?

IMHO it is better to keep them as separate things.
pwm node to describe the PWM generator,
separate nodes to describe it's uses like led, backlight, motor and gpio.

-- 
Péter

 
 led_user {
   compatible = pwm-leds;
   pwms = twl_pwmled 1 7812500; /* PWMB/LEDB from twl4030 */
   pwm-names = PMU_STAT LED;
 
   label = beagleboard::pmu_stat;
   max-brightness = 127;
 };
 
 vdd_usbhost: fixedregulator-vdd-usbhost {
   compatible = regulator-fixed;
   regulator-name = USBHOST_POWER;
   regulator-min-microvolt = 500;
   regulator-max-microvolt = 500;
   gpio = twl_pwmled 0 7812500; /* PWMA/LEDA from twl4030 */
   enable-active-high;
   regulator-boot-on;
 };
 
 With this I think this is what should happen in code level:
 - the pwm-gpo driver does not have of_match_table at all.
 - the driver for the ti,twl4030-pwmled is loaded.
 - it prepares and calls pwmchip_add() to add the PWM chip.
 - the of_pwmchip_add() will look for gpio-controller property of the node
  - if it is found it prepares the pdata (based on the PWM chip information)
 for the pwm-gpo driver and registers the platform_device for it.
  - the pwm-gpo driver will use:
 priv-gpio_chip.of_node = pdev-dev.parent-of_node;
 
 In DT boot we are fine with this I think.
 
 When it comes to legacy boot (boot without DT) I think we should still have
 the two layers to avoid big changes which would affect all existing pwm
 drivers. Something like this in the board files:
 
 static struct pwm_lookup pwm_lookup[] = {
   /* LEDA -  nUSBHOST_PWR_EN */
   PWM_LOOKUP(twl-pwmled, 0, pwm-gpo, nUSBHOST_PWR_EN),
   /* LEDB - PMU_STAT */
   PWM_LOOKUP(twl-pwmled, 1, leds_pwm, beagleboard::pmu_stat),
 };
 
 /* for the LED user of PWM */
 static struct led_pwm pwm_leds[] = {
   {
   .name   = beagleboard::pmu_stat,
   .max_brightness = 127,
   .pwm_period_ns  = 7812500,
   },
 };
 
 static struct led_pwm_platform_data pwm_data = {
   .num_leds   = ARRAY_SIZE(pwm_leds),
   .leds   = pwm_leds,
 };
 
 static struct platform_device leds_pwm = {
   .name   = leds_pwm,
   .id = -1,
   .dev= {
   .platform_data = pwm_data,
   },
 };
 
 /* for the GPIO user of PWM */
 static struct gpio_pwm pwm_gpios[] = {
   {
   .name   = nUSBHOST_PWR_EN,
   .pwm_period_ns  = 7812500,
   },
 };
 
 static struct gpio_pwm_pdata pwm_gpio_data = {
   .num_gpos   = ARRAY_SIZE(pwm_gpios),
   .gpos   = pwm_gpios,
   .setup  = beagle_pwm_gpio_setup, /*to get the gpio base */
 };
 
 static struct platform_device gpos_pwm = {
   .name   = pwm-gpo,
   .id = -1,
   .dev= {
   .platform_data = pwm_gpio_data,
   },
 };
 
 static int beagle_pwm_gpio_setup(struct device *dev, unsigned gpio,
unsigned ngpio)
 {
   beagle_usbhub_pdata.gpio = gpio; /* fixed_voltage_config struct */
 
   platform_device_register(beagle_usbhub);
   return 0;
 }
 
 What do you think?
 


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-22 Thread Grant Likely
On Thu, 22 Nov 2012 14:42:03 +0100, Peter Ujfalusi  
wrote:
> There seams to be board designs using PWM generators as enable/disable 
> signals.
> For these boards we used to have custom code as hacks to deal with such a
> situations.
> With the gpio-pwm driver we can emulate the GPIO functionality using PWM
> generators via standard interfaces. The PWM will be configured to be off
> when the GPIO is off and to full duty cycle when the GPIO is requested to be
> on.
> 
> Signed-off-by: Peter Ujfalusi 
> ---
> Hi Linus,
> 
> So this is the driver I came up regarding to the issue that some boards
> (BeagleBoard for example) are using the PWM generators as enable/disable 
> signal.
> 
> On BeagleBoard the situation is like this:
> TWL4030 PWMA -> TWL4030 LEDA -> nUSBHOST_PWR_EN -> external power switch -> 
> USB
> host hub power.
> 
> So I have tested this driver on BeagleBoard:
> - Custom code to toggle the GPIO just to see if it switches correctly
> 
> - Real life usecase:
> fixed voltage regulator with GPIO control (the gpio is the gpio-pwm provided).
> ehci-omap has been modified to allow deferred probe.
> the regulator is used by ehci-omap.
> 
> All works fine.

Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
same namespace and binding.  But that's not your fault.

It's pretty horrible to have a separate translator node to convert a PWM
into a GPIO (with output only of course). The gpio properties should
appear directly in the PWM node itself and the translation code should
be in either the pwm or the gpio core. I don't think it should look like
a separate device.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-22 Thread Peter Ujfalusi
There seams to be board designs using PWM generators as enable/disable signals.
For these boards we used to have custom code as hacks to deal with such a
situations.
With the gpio-pwm driver we can emulate the GPIO functionality using PWM
generators via standard interfaces. The PWM will be configured to be off
when the GPIO is off and to full duty cycle when the GPIO is requested to be
on.

Signed-off-by: Peter Ujfalusi 
---
Hi Linus,

So this is the driver I came up regarding to the issue that some boards
(BeagleBoard for example) are using the PWM generators as enable/disable signal.

On BeagleBoard the situation is like this:
TWL4030 PWMA -> TWL4030 LEDA -> nUSBHOST_PWR_EN -> external power switch -> USB
host hub power.

So I have tested this driver on BeagleBoard:
- Custom code to toggle the GPIO just to see if it switches correctly

- Real life usecase:
fixed voltage regulator with GPIO control (the gpio is the gpio-pwm provided).
ehci-omap has been modified to allow deferred probe.
the regulator is used by ehci-omap.

All works fine.

Regards,
Peter

 .../devicetree/bindings/gpio/gpio-pwm.txt  |  29 +++
 drivers/gpio/Kconfig   |  12 +
 drivers/gpio/Makefile  |   1 +
 drivers/gpio/gpio-pwm.c| 264 +
 include/linux/platform_data/gpio-pwm.h |  16 ++
 5 files changed, 322 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pwm.txt
 create mode 100644 drivers/gpio/gpio-pwm.c
 create mode 100644 include/linux/platform_data/gpio-pwm.h

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pwm.txt 
b/Documentation/devicetree/bindings/gpio/gpio-pwm.txt
new file mode 100644
index 000..2569cd5
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-pwm.txt
@@ -0,0 +1,29 @@
+GPO driver using PWM generators
+
+Required properties:
+- compatible : "pwm-gpo"
+- #gpio-cells : Should be one.
+- gpio-controller : Marks the device node as a GPIO controller.
+- pwms : PWM property, please refer to:
+  Documentation/devicetree/bindings/pwm/pwm.txt
+- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM 
device
+
+Example:
+
+twl_pwm: pwm {
+   compatible = "ti,twl4030-pwmled";
+   #pwm-cells = <2>;
+};
+
+pwm_gpo1: gpo1 {
+   compatible = "pwm-gpo";
+   #gpio-cells = <1>;
+   gpio-controller;
+   pwms = <_pwm 0 7812500>;
+   pwm-names = "nUSBHOST_PWR_EN";
+};
+
+user@1 {
+   compatible = "example,testuser";
+   gpios = <_gpo1 0>;
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 47150f5..d68c429 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -649,4 +649,16 @@ config GPIO_MSIC
  Enable support for GPIO on intel MSIC controllers found in
  intel MID devices
 
+comment "Miscellaneous GPIO implementations"
+
+config GPIO_PWM
+   tristate "GPO emulation using PWM generators"
+   depends on PWM
+   help
+ When a signal from a PWM generator is used as enable/disable signal
+ this driver will emulate a GPIO over the PWM driver.
+ When the GPIO is requested to be on the PWM will be configured for
+ full duty cycle, when the GPIO is requested to be off the PWM will be
+ turned off.
+
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 9aeed67..f507901 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_GPIO_PCA953X)+= gpio-pca953x.o
 obj-$(CONFIG_GPIO_PCF857X) += gpio-pcf857x.o
 obj-$(CONFIG_GPIO_PCH) += gpio-pch.o
 obj-$(CONFIG_GPIO_PL061)   += gpio-pl061.o
+obj-$(CONFIG_GPIO_PWM) += gpio-pwm.o
 obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o
 obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o
diff --git a/drivers/gpio/gpio-pwm.c b/drivers/gpio/gpio-pwm.c
new file mode 100644
index 000..de38d8b
--- /dev/null
+++ b/drivers/gpio/gpio-pwm.c
@@ -0,0 +1,264 @@
+/*
+ * Simple driver to allow PWMs to be used as GPO (General Purpose Output)
+ *
+ * The PWM will be turned off completely or configured for full on based on the
+ * gpio state request.
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ * Author: Peter Ujfalusi 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 

[PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-22 Thread Peter Ujfalusi
There seams to be board designs using PWM generators as enable/disable signals.
For these boards we used to have custom code as hacks to deal with such a
situations.
With the gpio-pwm driver we can emulate the GPIO functionality using PWM
generators via standard interfaces. The PWM will be configured to be off
when the GPIO is off and to full duty cycle when the GPIO is requested to be
on.

Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
---
Hi Linus,

So this is the driver I came up regarding to the issue that some boards
(BeagleBoard for example) are using the PWM generators as enable/disable signal.

On BeagleBoard the situation is like this:
TWL4030 PWMA - TWL4030 LEDA - nUSBHOST_PWR_EN - external power switch - USB
host hub power.

So I have tested this driver on BeagleBoard:
- Custom code to toggle the GPIO just to see if it switches correctly

- Real life usecase:
fixed voltage regulator with GPIO control (the gpio is the gpio-pwm provided).
ehci-omap has been modified to allow deferred probe.
the regulator is used by ehci-omap.

All works fine.

Regards,
Peter

 .../devicetree/bindings/gpio/gpio-pwm.txt  |  29 +++
 drivers/gpio/Kconfig   |  12 +
 drivers/gpio/Makefile  |   1 +
 drivers/gpio/gpio-pwm.c| 264 +
 include/linux/platform_data/gpio-pwm.h |  16 ++
 5 files changed, 322 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pwm.txt
 create mode 100644 drivers/gpio/gpio-pwm.c
 create mode 100644 include/linux/platform_data/gpio-pwm.h

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pwm.txt 
b/Documentation/devicetree/bindings/gpio/gpio-pwm.txt
new file mode 100644
index 000..2569cd5
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-pwm.txt
@@ -0,0 +1,29 @@
+GPO driver using PWM generators
+
+Required properties:
+- compatible : pwm-gpo
+- #gpio-cells : Should be one.
+- gpio-controller : Marks the device node as a GPIO controller.
+- pwms : PWM property, please refer to:
+  Documentation/devicetree/bindings/pwm/pwm.txt
+- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM 
device
+
+Example:
+
+twl_pwm: pwm {
+   compatible = ti,twl4030-pwmled;
+   #pwm-cells = 2;
+};
+
+pwm_gpo1: gpo1 {
+   compatible = pwm-gpo;
+   #gpio-cells = 1;
+   gpio-controller;
+   pwms = twl_pwm 0 7812500;
+   pwm-names = nUSBHOST_PWR_EN;
+};
+
+user@1 {
+   compatible = example,testuser;
+   gpios = pwm_gpo1 0;
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 47150f5..d68c429 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -649,4 +649,16 @@ config GPIO_MSIC
  Enable support for GPIO on intel MSIC controllers found in
  intel MID devices
 
+comment Miscellaneous GPIO implementations
+
+config GPIO_PWM
+   tristate GPO emulation using PWM generators
+   depends on PWM
+   help
+ When a signal from a PWM generator is used as enable/disable signal
+ this driver will emulate a GPIO over the PWM driver.
+ When the GPIO is requested to be on the PWM will be configured for
+ full duty cycle, when the GPIO is requested to be off the PWM will be
+ turned off.
+
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 9aeed67..f507901 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_GPIO_PCA953X)+= gpio-pca953x.o
 obj-$(CONFIG_GPIO_PCF857X) += gpio-pcf857x.o
 obj-$(CONFIG_GPIO_PCH) += gpio-pch.o
 obj-$(CONFIG_GPIO_PL061)   += gpio-pl061.o
+obj-$(CONFIG_GPIO_PWM) += gpio-pwm.o
 obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o
 obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o
diff --git a/drivers/gpio/gpio-pwm.c b/drivers/gpio/gpio-pwm.c
new file mode 100644
index 000..de38d8b
--- /dev/null
+++ b/drivers/gpio/gpio-pwm.c
@@ -0,0 +1,264 @@
+/*
+ * Simple driver to allow PWMs to be used as GPO (General Purpose Output)
+ *
+ * The PWM will be turned off completely or configured for full on based on the
+ * gpio state request.
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ * Author: Peter Ujfalusi peter.ujfal...@ti.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free 

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

2012-11-22 Thread Grant Likely
On Thu, 22 Nov 2012 14:42:03 +0100, Peter Ujfalusi peter.ujfal...@ti.com 
wrote:
 There seams to be board designs using PWM generators as enable/disable 
 signals.
 For these boards we used to have custom code as hacks to deal with such a
 situations.
 With the gpio-pwm driver we can emulate the GPIO functionality using PWM
 generators via standard interfaces. The PWM will be configured to be off
 when the GPIO is off and to full duty cycle when the GPIO is requested to be
 on.
 
 Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
 ---
 Hi Linus,
 
 So this is the driver I came up regarding to the issue that some boards
 (BeagleBoard for example) are using the PWM generators as enable/disable 
 signal.
 
 On BeagleBoard the situation is like this:
 TWL4030 PWMA - TWL4030 LEDA - nUSBHOST_PWR_EN - external power switch - 
 USB
 host hub power.
 
 So I have tested this driver on BeagleBoard:
 - Custom code to toggle the GPIO just to see if it switches correctly
 
 - Real life usecase:
 fixed voltage regulator with GPIO control (the gpio is the gpio-pwm provided).
 ehci-omap has been modified to allow deferred probe.
 the regulator is used by ehci-omap.
 
 All works fine.

Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
same namespace and binding. grumble, mutter But that's not your fault.

It's pretty horrible to have a separate translator node to convert a PWM
into a GPIO (with output only of course). The gpio properties should
appear directly in the PWM node itself and the translation code should
be in either the pwm or the gpio core. I don't think it should look like
a separate device.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/