Re: [PATCH] gpio: New driver for GPO emulation using PWM generators
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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/