| Message ID | 1447664207-24370-6-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
W dniu 16.11.2015 o 17:56, Boris Brezillon pisze: > Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field. > Doing that will ease adaptation of the PWM framework to support atomic > update. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > Patch generated with the following coccinelle script: > > --->8--- > virtual patch > > @@ > struct pwm_device *p; > expression e; > @@ > ( > -(p)->polarity = e; > +pwm_set_polarity((p), e); > | > -(p)->polarity > +pwm_get_polarity((p)) > | > -(p)->period = e; > +pwm_set_period((p), e); > | > -(p)->period > +pwm_get_period((p)) > | > -(p)->duty_cycle = e; > +pwm_set_duty_cycle((p), e); > | > -(p)->duty_cycle > +pwm_get_duty_cycle((p)) > ) > --->8--- > --- > drivers/input/misc/max77693-haptic.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c > index 6d96bff..a038fb3 100644 > --- a/drivers/input/misc/max77693-haptic.c > +++ b/drivers/input/misc/max77693-haptic.c > @@ -70,10 +70,11 @@ struct max77693_haptic { > > static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic) > { > - int delta = (haptic->pwm_dev->period + haptic->pwm_duty) / 2; > + int delta = (pwm_get_period((haptic->pwm_dev)) + haptic->pwm_duty) / 2; Double parentheses over argument are not needed so just: pwm_get_period(haptic->pwm_dev) + ... Beside that patch looks good, so with removing parentheses here and below: Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Best regards, Krzysztof > int error; > > - error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period); > + error = pwm_config(haptic->pwm_dev, delta, > + pwm_get_period((haptic->pwm_dev))); > if (error) { > dev_err(haptic->dev, "failed to configure pwm: %d\n", error); > return error; > @@ -245,7 +246,7 @@ static int max77693_haptic_play_effect(struct input_dev *dev, void *data, > * The formula to convert magnitude to pwm_duty as follows: > * - pwm_duty = (magnitude * pwm_period) / MAX_MAGNITUDE(0xFFFF) > */ > - period_mag_multi = (u64)haptic->pwm_dev->period * haptic->magnitude; > + period_mag_multi = (u64)pwm_get_period((haptic->pwm_dev)) * haptic->magnitude; > haptic->pwm_duty = (unsigned int)(period_mag_multi >> > MAX_MAGNITUDE_SHIFT); > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof, On Mon, 16 Nov 2015 22:10:40 +0900 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > W dniu 16.11.2015 o 17:56, Boris Brezillon pisze: > > Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field. > > Doing that will ease adaptation of the PWM framework to support atomic > > update. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > Patch generated with the following coccinelle script: > > > > --->8--- > > virtual patch > > > > @@ > > struct pwm_device *p; > > expression e; > > @@ > > ( > > -(p)->polarity = e; > > +pwm_set_polarity((p), e); > > | > > -(p)->polarity > > +pwm_get_polarity((p)) > > | > > -(p)->period = e; > > +pwm_set_period((p), e); > > | > > -(p)->period > > +pwm_get_period((p)) > > | > > -(p)->duty_cycle = e; > > +pwm_set_duty_cycle((p), e); > > | > > -(p)->duty_cycle > > +pwm_get_duty_cycle((p)) > > ) > > --->8--- > > --- > > drivers/input/misc/max77693-haptic.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c > > index 6d96bff..a038fb3 100644 > > --- a/drivers/input/misc/max77693-haptic.c > > +++ b/drivers/input/misc/max77693-haptic.c > > @@ -70,10 +70,11 @@ struct max77693_haptic { > > > > static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic) > > { > > - int delta = (haptic->pwm_dev->period + haptic->pwm_duty) / 2; > > + int delta = (pwm_get_period((haptic->pwm_dev)) + haptic->pwm_duty) / 2; > > Double parentheses over argument are not needed so just: > pwm_get_period(haptic->pwm_dev) + ... Actually it was generated with coccinelle, hence I didn't fix existing coding style issues, but I have no problem fixing them. Thanks, Boris > > Beside that patch looks good, so with removing parentheses here and below: > > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > Best regards, > Krzysztof > > > int error; > > > > - error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period); > > + error = pwm_config(haptic->pwm_dev, delta, > > + pwm_get_period((haptic->pwm_dev))); > > if (error) { > > dev_err(haptic->dev, "failed to configure pwm: %d\n", error); > > return error; > > @@ -245,7 +246,7 @@ static int max77693_haptic_play_effect(struct input_dev *dev, void *data, > > * The formula to convert magnitude to pwm_duty as follows: > > * - pwm_duty = (magnitude * pwm_period) / MAX_MAGNITUDE(0xFFFF) > > */ > > - period_mag_multi = (u64)haptic->pwm_dev->period * haptic->magnitude; > > + period_mag_multi = (u64)pwm_get_period((haptic->pwm_dev)) * haptic->magnitude; > > haptic->pwm_duty = (unsigned int)(period_mag_multi >> > > MAX_MAGNITUDE_SHIFT); > > > > >
On 11/16/2015 05:55 AM, Boris Brezillon wrote: > Hi Krzysztof, > > On Mon, 16 Nov 2015 22:10:40 +0900 > Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > >> W dniu 16.11.2015 o 17:56, Boris Brezillon pisze: >>> Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field. >>> Doing that will ease adaptation of the PWM framework to support atomic >>> update. >>> >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>> --- >>> Patch generated with the following coccinelle script: >>> >>> --->8--- >>> virtual patch >>> >>> @@ >>> struct pwm_device *p; >>> expression e; >>> @@ >>> ( >>> -(p)->polarity = e; >>> +pwm_set_polarity((p), e); >>> | >>> -(p)->polarity >>> +pwm_get_polarity((p)) >>> | >>> -(p)->period = e; >>> +pwm_set_period((p), e); >>> | >>> -(p)->period >>> +pwm_get_period((p)) >>> | >>> -(p)->duty_cycle = e; >>> +pwm_set_duty_cycle((p), e); >>> | >>> -(p)->duty_cycle >>> +pwm_get_duty_cycle((p)) >>> ) >>> --->8--- >>> --- >>> drivers/input/misc/max77693-haptic.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c >>> index 6d96bff..a038fb3 100644 >>> --- a/drivers/input/misc/max77693-haptic.c >>> +++ b/drivers/input/misc/max77693-haptic.c >>> @@ -70,10 +70,11 @@ struct max77693_haptic { >>> >>> static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic) >>> { >>> - int delta = (haptic->pwm_dev->period + haptic->pwm_duty) / 2; >>> + int delta = (pwm_get_period((haptic->pwm_dev)) + haptic->pwm_duty) / 2; >> >> Double parentheses over argument are not needed so just: >> pwm_get_period(haptic->pwm_dev) + ... > > Actually it was generated with coccinelle, hence I didn't fix existing > coding style issues, but I have no problem fixing them. > There was no existing coding style issue. Your coccinelle script introduces it. You might want to consider updating your script and remove the unnecessary (( )) from it. Guenter > Thanks, > > Boris > >> >> Beside that patch looks good, so with removing parentheses here and below: >> >> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> >> Best regards, >> Krzysztof >> >>> int error; >>> >>> - error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period); >>> + error = pwm_config(haptic->pwm_dev, delta, >>> + pwm_get_period((haptic->pwm_dev))); >>> if (error) { >>> dev_err(haptic->dev, "failed to configure pwm: %d\n", error); >>> return error; >>> @@ -245,7 +246,7 @@ static int max77693_haptic_play_effect(struct input_dev *dev, void *data, >>> * The formula to convert magnitude to pwm_duty as follows: >>> * - pwm_duty = (magnitude * pwm_period) / MAX_MAGNITUDE(0xFFFF) >>> */ >>> - period_mag_multi = (u64)haptic->pwm_dev->period * haptic->magnitude; >>> + period_mag_multi = (u64)pwm_get_period((haptic->pwm_dev)) * haptic->magnitude; >>> haptic->pwm_duty = (unsigned int)(period_mag_multi >> >>> MAX_MAGNITUDE_SHIFT); >>> >>> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 16 Nov 2015 07:55:33 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On 11/16/2015 05:55 AM, Boris Brezillon wrote: > > Hi Krzysztof, > > > > On Mon, 16 Nov 2015 22:10:40 +0900 > > Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > > >> W dniu 16.11.2015 o 17:56, Boris Brezillon pisze: > >>> Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field. > >>> Doing that will ease adaptation of the PWM framework to support atomic > >>> update. > >>> > >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > >>> --- > >>> Patch generated with the following coccinelle script: > >>> > >>> --->8--- > >>> virtual patch > >>> > >>> @@ > >>> struct pwm_device *p; > >>> expression e; > >>> @@ > >>> ( > >>> -(p)->polarity = e; > >>> +pwm_set_polarity((p), e); > >>> | > >>> -(p)->polarity > >>> +pwm_get_polarity((p)) > >>> | > >>> -(p)->period = e; > >>> +pwm_set_period((p), e); > >>> | > >>> -(p)->period > >>> +pwm_get_period((p)) > >>> | > >>> -(p)->duty_cycle = e; > >>> +pwm_set_duty_cycle((p), e); > >>> | > >>> -(p)->duty_cycle > >>> +pwm_get_duty_cycle((p)) > >>> ) > >>> --->8--- > >>> --- > >>> drivers/input/misc/max77693-haptic.c | 7 ++++--- > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c > >>> index 6d96bff..a038fb3 100644 > >>> --- a/drivers/input/misc/max77693-haptic.c > >>> +++ b/drivers/input/misc/max77693-haptic.c > >>> @@ -70,10 +70,11 @@ struct max77693_haptic { > >>> > >>> static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic) > >>> { > >>> - int delta = (haptic->pwm_dev->period + haptic->pwm_duty) / 2; > >>> + int delta = (pwm_get_period((haptic->pwm_dev)) + haptic->pwm_duty) / 2; > >> > >> Double parentheses over argument are not needed so just: > >> pwm_get_period(haptic->pwm_dev) + ... > > > > Actually it was generated with coccinelle, hence I didn't fix existing > > coding style issues, but I have no problem fixing them. > > > There was no existing coding style issue. Your coccinelle script introduces it. > You might want to consider updating your script and remove the unnecessary (( )) > from it. My bad, you are right: my script is buggy. I'll fix that. Thanks, Boris
On Mon, Nov 16, 2015 at 09:56:28AM +0100, Boris Brezillon wrote: > Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field. > Doing that will ease adaptation of the PWM framework to support atomic > update. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> Could you please tag input-related bits with "Input: driver - ... " instead of "misc: ... "? Thanks!
diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c index 6d96bff..a038fb3 100644 --- a/drivers/input/misc/max77693-haptic.c +++ b/drivers/input/misc/max77693-haptic.c @@ -70,10 +70,11 @@ struct max77693_haptic { static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic) { - int delta = (haptic->pwm_dev->period + haptic->pwm_duty) / 2; + int delta = (pwm_get_period((haptic->pwm_dev)) + haptic->pwm_duty) / 2; int error; - error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period); + error = pwm_config(haptic->pwm_dev, delta, + pwm_get_period((haptic->pwm_dev))); if (error) { dev_err(haptic->dev, "failed to configure pwm: %d\n", error); return error; @@ -245,7 +246,7 @@ static int max77693_haptic_play_effect(struct input_dev *dev, void *data, * The formula to convert magnitude to pwm_duty as follows: * - pwm_duty = (magnitude * pwm_period) / MAX_MAGNITUDE(0xFFFF) */ - period_mag_multi = (u64)haptic->pwm_dev->period * haptic->magnitude; + period_mag_multi = (u64)pwm_get_period((haptic->pwm_dev)) * haptic->magnitude; haptic->pwm_duty = (unsigned int)(period_mag_multi >> MAX_MAGNITUDE_SHIFT);
Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field. Doing that will ease adaptation of the PWM framework to support atomic update. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- Patch generated with the following coccinelle script: --->8--- virtual patch @@ struct pwm_device *p; expression e; @@ ( -(p)->polarity = e; +pwm_set_polarity((p), e); | -(p)->polarity +pwm_get_polarity((p)) | -(p)->period = e; +pwm_set_period((p), e); | -(p)->period +pwm_get_period((p)) | -(p)->duty_cycle = e; +pwm_set_duty_cycle((p), e); | -(p)->duty_cycle +pwm_get_duty_cycle((p)) ) --->8--- --- drivers/input/misc/max77693-haptic.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)