Re: [PATCH V17 3/3] Input: new da7280 haptic driver
On 7/23/20 10:47 PM, Roy Im wrote: > OK, thanks for your comments. To be clearer, I would like to update as below > if you agree > > depends on INPUT && I2C > ... > The haptics can be controlled by PWM or GPIO > with I2C communication. Yes, that sounds good. Thanks. -- ~Randy
RE: [PATCH V17 3/3] Input: new da7280 haptic driver
Friday, July 24, 2020 11:57 AM, Randy Dunlap wrote > On 7/23/20 6:54 PM, Roy Im wrote: > > On Fri, July 24, 2020 5:51 AM, Randy Dunlap wrote > >> On 7/23/20 8:01 AM, Roy Im wrote: > >>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > >>> index 362e8a0..06dc5a3 100644 > >>> --- a/drivers/input/misc/Kconfig > >>> +++ b/drivers/input/misc/Kconfig > >>> @@ -869,4 +869,17 @@ config INPUT_STPMIC1_ONKEY > >>> To compile this driver as a module, choose M here: the > >>> module will be called stpmic1_onkey. > >>> > >>> +config INPUT_DA7280_HAPTICS > >>> + tristate "Dialog Semiconductor DA7280 haptics support" > >>> + depends on INPUT && I2C > >>> + select INPUT_FF_MEMLESS > >>> + select REGMAP_I2C > >>> + help > >>> + Say Y to enable support for the Dialog DA7280 haptics driver. > >>> + The haptics can be controlled by I2C communication, > >>> + or by PWM input, or by GPI. > >> > >> Is thatGPIO. > >> ? > > The Haptics can be working by GPI(if see from the haptic device), but from > > the Host it is GPO. Do you think the GPIO is > correct? > > To me it needs to represent what services/interfaces/facilities are used by > this driver that are provided by the Linux kernel. > If it uses Linux GPIO services, then it should say GPIO -- although I don't > see it using any Linux GPIO services. OK, let me change to GPIO. > > >> > >> Can the haptics be controlled only by PWM or only by GPI(O)? > >> > >> Just curious: why is I2C required to build the driver if a user is only > >> controlling the device by PWM or GPI? > > > > I2C is required to control registers and it can be triggered by I2C or PWM > > or GPI(controlled by host outside this driver), > so PWM and GPI are optional. > > With your comments, I think it's better to remove below lines(//remove) to > > avoid confusion and add PWM as below if > you agree. > > // remove > > The haptics can be controlled by I2C communication, > > or by PWM input, or by GPI. > > // update, adding || PWM > > depends on (INPUT && I2C) || PWM > > Since provides stubs for when CONFIG_PWM is not enabled, it > appears that "depends on PWM" is not > required. > > I'll leave it up to you. I was just trying to understand better. > It may be that no changes are needed. OK, thanks for your comments. To be clearer, I would like to update as below if you agree depends on INPUT && I2C ... The haptics can be controlled by PWM or GPIO with I2C communication. > > > thanks. > -- > ~Randy Kind regards, Roy
Re: [PATCH V17 3/3] Input: new da7280 haptic driver
On 7/23/20 6:54 PM, Roy Im wrote: > On Fri, July 24, 2020 5:51 AM, Randy Dunlap wrote >> On 7/23/20 8:01 AM, Roy Im wrote: >>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig >>> index 362e8a0..06dc5a3 100644 >>> --- a/drivers/input/misc/Kconfig >>> +++ b/drivers/input/misc/Kconfig >>> @@ -869,4 +869,17 @@ config INPUT_STPMIC1_ONKEY >>> To compile this driver as a module, choose M here: the >>> module will be called stpmic1_onkey. >>> >>> +config INPUT_DA7280_HAPTICS >>> + tristate "Dialog Semiconductor DA7280 haptics support" >>> + depends on INPUT && I2C >>> + select INPUT_FF_MEMLESS >>> + select REGMAP_I2C >>> + help >>> + Say Y to enable support for the Dialog DA7280 haptics driver. >>> + The haptics can be controlled by I2C communication, >>> + or by PWM input, or by GPI. >> >>Is thatGPIO. >> ? > The Haptics can be working by GPI(if see from the haptic device), but from > the Host it is GPO. Do you think the GPIO is correct? To me it needs to represent what services/interfaces/facilities are used by this driver that are provided by the Linux kernel. If it uses Linux GPIO services, then it should say GPIO -- although I don't see it using any Linux GPIO services. >> >> Can the haptics be controlled only by PWM or only by GPI(O)? >> >> Just curious: why is I2C required to build the driver if a user is only >> controlling the device by PWM or GPI? > > I2C is required to control registers and it can be triggered by I2C or PWM or > GPI(controlled by host outside this driver), so PWM and GPI are optional. > With your comments, I think it's better to remove below lines(//remove) to > avoid confusion and add PWM as below if you agree. > // remove > The haptics can be controlled by I2C communication, > or by PWM input, or by GPI. > // update, adding || PWM > depends on (INPUT && I2C) || PWM Since provides stubs for when CONFIG_PWM is not enabled, it appears that "depends on PWM" is not required. I'll leave it up to you. I was just trying to understand better. It may be that no changes are needed. thanks. -- ~Randy
RE: [PATCH V17 3/3] Input: new da7280 haptic driver
On Fri July 24, 2020 5:55 AM, Randy Dunlap wrote:
> On 7/23/20 8:01 AM, Roy Im wrote:
> > Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with multiple
> > mode and integrated waveform memory and wideband support.
> > It communicates via an I2C bus to the device.
> >
> > Reviewed-by: Jes Sorensen .
> >
> > Signed-off-by: Roy Im
> >
> > ---
> >
> >
> > drivers/input/misc/Kconfig | 13 +
> > drivers/input/misc/Makefile |1 +
> > drivers/input/misc/da7280.c | 1840
> > +++
> > 3 files changed, 1854 insertions(+)
> > create mode 100644 drivers/input/misc/da7280.c
> >
>
> > diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
> > new file mode 100644 index 000..6e3ead5
> > --- /dev/null
> > +++ b/drivers/input/misc/da7280.c
> > @@ -0,0 +1,1840 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * DA7280 Haptic device driver
> > + *
> > + * Copyright (c) 2020 Dialog Semiconductor.
> > + * Author: Roy Im */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
>
> ...
>
>
> > +static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool
> > +enabled) {
> > + struct pwm_state state;
> > + u64 period_mag_multi;
> > + int error;
> > +
> > + if (!haptics->gain && enabled) {
> > + dev_err(haptics->dev,
> > + "Please set the gain first for the pwm mode\n");
> > + return -EINVAL;
> > + }
> > +
> > + pwm_get_state(haptics->pwm_dev, &state);
> > + state.enabled = enabled;
> > + if (enabled) {
> > + period_mag_multi = (u64)state.period * haptics->gain;
> > + period_mag_multi >>= MAX_MAGNITUDE_SHIFT;
> > +
> > + /* The interpretation of duty cycle depends on the acc_en,
> > +* it should be between 50% and 100% for acc_en = 0.
> > +* See datasheet 'PWM mode' section.
> > +*/
>
> from coding-style.rst:
>
> /*
>* This is the preferred style for multi-line
>* comments in the Linux kernel source code.
>* Please use it consistently.
>*
>* Description: A column of asterisks on the left side,
>* with beginning and ending almost-blank lines.
>*/
>
> (except for networking code)
>
> Please fix multiple locations.
OK, I will fix them.
>
>
> thanks.
> --
> ~Randy
Kind regards,
Roy
RE: [PATCH V17 3/3] Input: new da7280 haptic driver
On Fri, July 24, 2020 5:51 AM, Randy Dunlap wrote > On 7/23/20 8:01 AM, Roy Im wrote: > > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > > index 362e8a0..06dc5a3 100644 > > --- a/drivers/input/misc/Kconfig > > +++ b/drivers/input/misc/Kconfig > > @@ -869,4 +869,17 @@ config INPUT_STPMIC1_ONKEY > > To compile this driver as a module, choose M here: the > > module will be called stpmic1_onkey. > > > > +config INPUT_DA7280_HAPTICS > > + tristate "Dialog Semiconductor DA7280 haptics support" > > + depends on INPUT && I2C > > + select INPUT_FF_MEMLESS > > + select REGMAP_I2C > > + help > > + Say Y to enable support for the Dialog DA7280 haptics driver. > > + The haptics can be controlled by I2C communication, > > + or by PWM input, or by GPI. > > Is thatGPIO. > ? The Haptics can be working by GPI(if see from the haptic device), but from the Host it is GPO. Do you think the GPIO is correct? > > Can the haptics be controlled only by PWM or only by GPI(O)? > > Just curious: why is I2C required to build the driver if a user is only > controlling the device by PWM or GPI? I2C is required to control registers and it can be triggered by I2C or PWM or GPI(controlled by host outside this driver), so PWM and GPI are optional. With your comments, I think it's better to remove below lines(//remove) to avoid confusion and add PWM as below if you agree. // remove The haptics can be controlled by I2C communication, or by PWM input, or by GPI. // update, adding || PWM depends on (INPUT && I2C) || PWM > > > > + > > + To compile this driver as a module, choose M here: the > > + module will be called da7280. > > + > > endif > > thanks. > -- > ~Randy Kind regards, Roy
Re: [PATCH V17 3/3] Input: new da7280 haptic driver
On 7/23/20 8:01 AM, Roy Im wrote:
> Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with
> multiple mode and integrated waveform memory and wideband support.
> It communicates via an I2C bus to the device.
>
> Reviewed-by: Jes Sorensen .
>
> Signed-off-by: Roy Im
>
> ---
>
>
> drivers/input/misc/Kconfig | 13 +
> drivers/input/misc/Makefile |1 +
> drivers/input/misc/da7280.c | 1840
> +++
> 3 files changed, 1854 insertions(+)
> create mode 100644 drivers/input/misc/da7280.c
>
> diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
> new file mode 100644
> index 000..6e3ead5
> --- /dev/null
> +++ b/drivers/input/misc/da7280.c
> @@ -0,0 +1,1840 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * DA7280 Haptic device driver
> + *
> + * Copyright (c) 2020 Dialog Semiconductor.
> + * Author: Roy Im
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
...
> +static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool enabled)
> +{
> + struct pwm_state state;
> + u64 period_mag_multi;
> + int error;
> +
> + if (!haptics->gain && enabled) {
> + dev_err(haptics->dev,
> + "Please set the gain first for the pwm mode\n");
> + return -EINVAL;
> + }
> +
> + pwm_get_state(haptics->pwm_dev, &state);
> + state.enabled = enabled;
> + if (enabled) {
> + period_mag_multi = (u64)state.period * haptics->gain;
> + period_mag_multi >>= MAX_MAGNITUDE_SHIFT;
> +
> + /* The interpretation of duty cycle depends on the acc_en,
> + * it should be between 50% and 100% for acc_en = 0.
> + * See datasheet 'PWM mode' section.
> + */
from coding-style.rst:
/*
* This is the preferred style for multi-line
* comments in the Linux kernel source code.
* Please use it consistently.
*
* Description: A column of asterisks on the left side,
* with beginning and ending almost-blank lines.
*/
(except for networking code)
Please fix multiple locations.
thanks.
--
~Randy
Re: [PATCH V17 3/3] Input: new da7280 haptic driver
On 7/23/20 8:01 AM, Roy Im wrote: > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > index 362e8a0..06dc5a3 100644 > --- a/drivers/input/misc/Kconfig > +++ b/drivers/input/misc/Kconfig > @@ -869,4 +869,17 @@ config INPUT_STPMIC1_ONKEY > To compile this driver as a module, choose M here: the > module will be called stpmic1_onkey. > > +config INPUT_DA7280_HAPTICS > + tristate "Dialog Semiconductor DA7280 haptics support" > + depends on INPUT && I2C > + select INPUT_FF_MEMLESS > + select REGMAP_I2C > + help > + Say Y to enable support for the Dialog DA7280 haptics driver. > + The haptics can be controlled by I2C communication, > + or by PWM input, or by GPI. Is thatGPIO. ? Can the haptics be controlled only by PWM or only by GPI(O)? Just curious: why is I2C required to build the driver if a user is only controlling the device by PWM or GPI? > + > + To compile this driver as a module, choose M here: the > + module will be called da7280. > + > endif thanks. -- ~Randy
