Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376659 > unrolled thread

Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 12:30 +0200
    Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data Lee Jones <lee.jones@linaro.org> - 2016-04-15 14:40 +0200
      Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data Thierry Reding <thierry.reding@gmail.com> - 2016-04-15 16:30 +0200
        Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data Lee Jones <lee.jones@linaro.org> - 2016-04-15 16:40 +0200
    Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data Lee Jones <lee.jones@linaro.org> - 2016-04-15 15:20 +0200
      Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data Thierry Reding <thierry.reding@gmail.com> - 2016-04-15 16:30 +0200

#1376659 — Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Multipart message — attachments visible in raw view] — view raw

On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
[...]
> +struct sti_cpt_data {
> +	u32 snapshot[3];
> +	int index;
> +	int gpio;

On a side-note, this should probably use struct gpio_desc * instead of
an integer along with the gpiod_*() APIs for the GPIO handling.

> +	struct mutex lock;
> +	wait_queue_head_t wait;
> +};
> +
>  struct sti_pwm_compat_data {
>  	const struct reg_field *reg_fields;
> -	unsigned int num_chan;
> +	unsigned int pwm_num_chan;
> +	unsigned int cpt_num_chan;
>  	unsigned int max_pwm_cnt;
>  	unsigned int max_prescale;
>  };
> @@ -77,6 +90,7 @@ struct sti_pwm_chip {
>  	struct clk *cpt_clk;
>  	struct regmap *regmap;
>  	struct sti_pwm_compat_data *cdata;
> +	struct sti_cpt_data *cpt_data[STI_MAX_CPT_CHANS];

The PWM subsystem allows chip-specific data to be associated with each
PWM device. I'd prefer if the driver used it rather than homebrew some-
thing similar. See pwm_set_chip_data() and pwm_get_chip_data().

> @@ -389,6 +411,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	for (chan = 0; chan < cdata->cpt_num_chan; chan++) {
> +		struct sti_cpt_data *data;
> +
> +		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +		if (!data)
> +			return -ENOMEM;
> +
> +		init_waitqueue_head(&data->wait);
> +		mutex_init(&data->lock);
> +		data->gpio = of_get_named_gpio(np, "capture-gpios", chan);
> +		pc->cpt_data[chan] = data;

Converting to per-PWM data should be as simple as turning this last line
into:

	pwm_set_chip_data(pc->chip.pwms[chan], data);

Also I don't see any cleanup for this data in the driver. The memory for
the per-PWM data should be freed by devm_*() infrastructure, but how
will the GPIO be released?

Thierry

[toc] | [next] | [standalone]

#1379790

On Tue, 12 Apr 2016, Thierry Reding wrote:

> On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> [...]
> > +struct sti_cpt_data {
> > +	u32 snapshot[3];
> > +	int index;
> > +	int gpio;
> 
> On a side-note, this should probably use struct gpio_desc * instead of
> an integer along with the gpiod_*() APIs for the GPIO handling.

Why would you need to do that?

of_get_named_gpio() does all that for you.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1379856

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 01:39:41PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> > [...]
> > > +struct sti_cpt_data {
> > > +	u32 snapshot[3];
> > > +	int index;
> > > +	int gpio;
> > 
> > On a side-note, this should probably use struct gpio_desc * instead of
> > an integer along with the gpiod_*() APIs for the GPIO handling.
> 
> Why would you need to do that?
> 
> of_get_named_gpio() does all that for you.

Use the of_get_named_gpio*d*() function instead. My understanding is
that referring to GPIOs by integer is deprecated and should not be used
in new code.

Thierry

[toc] | [prev] | [next] | [standalone]

#1379874

On Fri, 15 Apr 2016, Thierry Reding wrote:

> On Fri, Apr 15, 2016 at 01:39:41PM +0100, Lee Jones wrote:
> > On Tue, 12 Apr 2016, Thierry Reding wrote:
> > 
> > > On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> > > [...]
> > > > +struct sti_cpt_data {
> > > > +	u32 snapshot[3];
> > > > +	int index;
> > > > +	int gpio;
> > > 
> > > On a side-note, this should probably use struct gpio_desc * instead of
> > > an integer along with the gpiod_*() APIs for the GPIO handling.
> > 
> > Why would you need to do that?
> > 
> > of_get_named_gpio() does all that for you.
> 
> Use the of_get_named_gpio*d*() function instead. My understanding is
> that referring to GPIOs by integer is deprecated and should not be used
> in new code.

I've since conducted some research and have now converted over to
gpiod.  Thanks for the pointer.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1379806

On Tue, 12 Apr 2016, Thierry Reding wrote:
> On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> [...]
> > +struct sti_cpt_data {
> > +	u32 snapshot[3];
> > +	int index;
> > +	int gpio;

[...]

> > +
> > +		init_waitqueue_head(&data->wait);
> > +		mutex_init(&data->lock);
> > +		data->gpio = of_get_named_gpio(np, "capture-gpios", chan);
> > +		pc->cpt_data[chan] = data;
> 
> Converting to per-PWM data should be as simple as turning this last line
> into:
> 
> 	pwm_set_chip_data(pc->chip.pwms[chan], data);
> 
> Also I don't see any cleanup for this data in the driver. The memory for
> the per-PWM data should be freed by devm_*() infrastructure, but how
> will the GPIO be released?

There is no reason to release a GPIO.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1379870

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 02:11:46PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> > On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> > [...]
> > > +struct sti_cpt_data {
> > > +	u32 snapshot[3];
> > > +	int index;
> > > +	int gpio;
> 
> [...]
> 
> > > +
> > > +		init_waitqueue_head(&data->wait);
> > > +		mutex_init(&data->lock);
> > > +		data->gpio = of_get_named_gpio(np, "capture-gpios", chan);
> > > +		pc->cpt_data[chan] = data;
> > 
> > Converting to per-PWM data should be as simple as turning this last line
> > into:
> > 
> > 	pwm_set_chip_data(pc->chip.pwms[chan], data);
> > 
> > Also I don't see any cleanup for this data in the driver. The memory for
> > the per-PWM data should be freed by devm_*() infrastructure, but how
> > will the GPIO be released?
> 
> There is no reason to release a GPIO.

Why? If you request it why would you not need to release it?

Thierry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web