Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support
Hi
On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban
wrote:
>
> On Mi, 2018-11-28 at 16:15 -0200, Giuliano Belinassi wrote:
> > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> > pin. This commit adds suppport for the 'gain' and 'filter' pin.
> >
> > Signed-off-by: Giuliano Belinassi
> > ---
> > Changes in v2:
> > - Now this patch is part of the patchset that aims to remove ad7780
> > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2
> > - Also, now it reads voltage and filter values from the userspace
> > instead of gpio pin states.
>
> Hello,
> Please see bellow.
>
> >
> > drivers/staging/iio/adc/ad7780.c | 78 --
> > include/linux/iio/adc/ad_sigma_delta.h | 5 ++
> > 2 files changed, 79 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index c4a85789c2db..05979a79fda3 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -39,6 +39,12 @@
> > #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > #define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 |
> > AD7170_PAT2)
> >
> > +#define AD7780_GAIN_GPIO 0
> > +#define AD7780_FILTER_GPIO 1
> > +
> > +#define AD7780_GAIN_MIDPOINT 64
> > +#define AD7780_FILTER_MIDPOINT 13350
> > +
> > struct ad7780_chip_info {
> > struct iio_chan_specchannel;
> > unsigned intpattern_mask;
> > @@ -50,6 +56,8 @@ struct ad7780_state {
> > const struct ad7780_chip_info *chip_info;
> > struct regulator*reg;
> > struct gpio_desc*powerdown_gpio;
> > + struct gpio_desc*gain_gpio;
> > + struct gpio_desc*filter_gpio;
> > unsigned intgain;
> >
> > struct ad_sigma_delta sd;
> > @@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev
> > *indio_dev,
> > return -EINVAL;
> > }
> >
> > +static int ad7780_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val,
> > + int val2,
> > + long m)
> > +{
> > + struct ad7780_state *st = iio_priv(indio_dev);
> > + const struct ad7780_chip_info *chip_info = st->chip_info;
> > + int uvref, gain;
> > + unsigned int full_scale;
> > +
> > + if (!chip_info->is_ad778x)
> > + return 0;
> > +
> > + switch (m) {
> > + case IIO_CHAN_INFO_SCALE:
> > + if (val != 0)
> > + return -EINVAL;
> > +
> > + uvref = regulator_get_voltage(st->reg);
>
> regulator_get_voltage() has already been called in the probe function and
> the result is stored in st->int_vref_mv.
This was removed in commit 9eae69ddbc4717a0bd702eddac76c7848773bf71
because the value was not being updated. But I agree if the vref
voltage is not going to change at all after the initialization, then
this value should be kept in memory.
> My suggestion would be to use a local vref variable declared as unsigned
> int. It is my fault that I haven't explained correctly in the previous
> email, but you need to multiply vref_mv with 100LL in order to get the
> right precision: vref = st->int_vref_mv * 100LL. Afterwards you will be
> able to perform the divisions.
Thanks for this info! :-)
Shouldn't we store this in uV (microVolts)? This will yield a more
accurate result after the multiplication.
> > +
> > + if (uvref < 0)
> > + return uvref;
> > +
> > + full_scale = 1 << (chip_info->channel.scan_type.realbits
> > - 1);
> > + gain = DIV_ROUND_CLOSEST(uvref, full_scale);
> > + gain = DIV_ROUND_CLOSEST(gain, val2);
> > +
> > + gpiod_set_value(st->gain_gpio, gain <
> > AD7780_GAIN_MIDPOINT ? 0 : 1);
>
> Once the gain is set, you can store it in st->gain variable.
Yes, we forgot it.
>
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + if (val2 != 0)
> > + return -EINVAL;
> > +
> > + gpiod_set_value(st->filter_gpio, val <
> > AD7780_FILTER_MIDPOINT ? 0 : 1);
>
> This is probably fine, although I am not a big fan of the ternary operator.
> A simple if else statement would do. However, I don't feel strongly about
> it, so feel free to disagree.
>
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> >unsigned int raw_sample)
> > {
> > struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> > const struct ad7780_chip_info *chip_info = st->chip_info;
> > + int val;
> >
> > if ((raw_sample & AD7780_ERR) ||
> > ((raw_sample & chip_info->pattern_mask) != chip_info-
> > >pattern))
> > return -EIO;
> >
> > if (ch
Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits
Hi
>While I agree that it looks nicer to indent all these to the same level,
>you also need to think about the fact that the kernel git repo is already
>pretty big as-is, so it's a good idea if a patch adds as much code/semantic
>value [as possible] with as little change [as possible] and with as good of
>readability [as possible].
Understood. But can't someone else submit another patch fixing these
indentation issues? That would be another commit and more stuff to the
repository.
On Thu, Nov 8, 2018 at 11:51 AM Ardelean, Alexandru
wrote:
>
> On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > Previously, all pattern_masks and patterns in the chip_info table were
> > hardcoded. Now they are generated using the PAT macros, as described in
> > the datasheets.
>
> One comment about indentation/whitespace.
>
> Rest looks good.
>
> Alex
>
> >
> > Signed-off-by: Giuliano Belinassi
> > ---
> > Changes in v2:
> > - Added the PATTERN and PATTERN_MASK macros
> > - Update BIT macros alignment to match with PATTERN
> > - Generate pattern mask with PATTERN_MASK macros.
> >
> > drivers/staging/iio/adc/ad7780.c | 40 +++-
> > 1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 9ec2b002539e..ff8e3b2d0efc 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -22,14 +22,22 @@
> > #include
> > #include
> >
> > -#define AD7780_RDY BIT(7)
> > -#define AD7780_FILTERBIT(6)
> > -#define AD7780_ERR BIT(5)
> > -#define AD7780_ID1 BIT(4)
> > -#define AD7780_ID0 BIT(3)
> > -#define AD7780_GAIN BIT(2)
> > -#define AD7780_PAT1 BIT(1)
> > -#define AD7780_PAT0 BIT(0)
> > +#define AD7780_RDY BIT(7)
> > +#define AD7780_FILTERBIT(6)
> > +#define AD7780_ERR BIT(5)
> > +#define AD7780_ID1 BIT(4)
> > +#define AD7780_ID0 BIT(3)
> > +#define AD7780_GAIN BIT(2)
> > +#define AD7780_PAT1 BIT(1)
> > +#define AD7780_PAT0 BIT(0)
>
> Changing indentation here doesn't add much value; it's mostly
> patch/whitespace noise.
>
> While I agree that it looks nicer to indent all these to the same level,
> you also need to think about the fact that the kernel git repo is already
> pretty big as-is, so it's a good idea if a patch adds as much code/semantic
> value [as possible] with as little change [as possible] and with as good of
> readability [as possible].
> [ Kind of sounds like a balance act between the 3 things ].
>
>
> > +
> > +#define AD7780_PATTERN (AD7780_PAT0)
> > +#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1)
> > +
> > +#define AD7170_PAT2 BIT(2)
> > +
> > +#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > +#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> >
> > struct ad7780_chip_info {
> > struct iio_chan_specchannel;
> > @@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info
> > ad7780_sigma_delta_info = {
> > static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > [ID_AD7170] = {
> > .channel = AD7780_CHANNEL(12, 24),
> > - .pattern = 0x5,
> > - .pattern_mask = 0x7,
> > + .pattern = AD7170_PATTERN,
> > + .pattern_mask = AD7170_PATTERN_MASK,
> > .is_ad778x = false,
> > },
> > [ID_AD7171] = {
> > .channel = AD7780_CHANNEL(16, 24),
> > - .pattern = 0x5,
> > - .pattern_mask = 0x7,
> > + .pattern = AD7170_PATTERN,
> > + .pattern_mask = AD7170_PATTERN_MASK,
> > .is_ad778x = false,
> > },
> > [ID_AD7780] = {
> > .channel = AD7780_CHANNEL(24, 32),
> > - .pattern = 0x1,
> > - .pattern_mask = 0x3,
> > + .pattern = AD7780_PATTERN,
> > + .pattern_mask = AD7780_PATTERN_MASK,
> > .is_ad778x = true,
> > },
> > [ID_AD7781] = {
> > .channel = AD7780_CHANNEL(20, 32),
> > - .pattern = 0x1,
> > - .pattern_mask = 0x3,
> > + .pattern = AD7780_PATTERN,
> > + .pattern_mask = AD7780_PATTERN_MASK,
> > .is_ad778x = true,
> > },
> > };
>
> --
> You received this message because you are subscribed to the Google Groups
> "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-usp+unsubscr...@googlegroups.com.
> To post to this group, send email to kernel-...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kernel-usp/43efc182fc7ab9aa1d2f1ca798e27d85c7132e1f.camel%40analog.com.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de
Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update
> Just some random though. Instead of introducing extra level of indentation you
> can simply check whether is_ad778x is asserted and simply return.
I agree that the patch would be smaller if I do that, but is it really
an issue? If yes, then I will update the patch with this change
> Any reason for setting this explicitly? That's going to be false anyway
It seems clearer to me :-)
On Thu, Nov 8, 2018 at 4:26 PM Tomasz Duszynski wrote:
>
> Hi Giuliano,
>
> Comment inline.
>
> On 11/8/18 2:03 PM, Giuliano Belinassi wrote:
> > Only the ad778x have the 'gain' status bit. Check it before updating
> > through a new variable is_ad778x in chip_info.
> >
> > Signed-off-by: Giuliano Belinassi
> > ---
> > Changes in v2:
> > - Squashed is_ad778x declaration commit with the ad778x checkage
> > - Changed is_ad778x type to bool
> >
> > drivers/staging/iio/adc/ad7780.c | 15 +++
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 91e016d534ed..9ec2b002539e 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> > struct iio_chan_specchannel;
> > unsigned intpattern_mask;
> > unsigned intpattern;
> > + boolis_ad778x;
> > };
> >
> > struct ad7780_state {
> > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> > ad_sigma_delta *sigma_delta,
> > ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> > return -EIO;
> >
> > - if (raw_sample & AD7780_GAIN)
> > - st->gain = 1;
> > - else
> > - st->gain = 128;
> > + if (chip_info->is_ad778x) {
> > + if (raw_sample & AD7780_GAIN)
> > + st->gain = 1;
> > + else
> > + st->gain = 128;
> > + }
>
> Just some random though. Instead of introducing extra level of indentation you
> can simply check whether is_ad778x is asserted and simply return.
>
> >
> > return 0;
> > }
> > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> > ad7780_chip_info_tbl[] = {
> > .channel = AD7780_CHANNEL(12, 24),
> > .pattern = 0x5,
> > .pattern_mask = 0x7,
> > + .is_ad778x = false,
>
> Any reason for setting this explicitly? That's going to be false anyway.
>
> > },
> > [ID_AD7171] = {
> > .channel = AD7780_CHANNEL(16, 24),
> > .pattern = 0x5,
> > .pattern_mask = 0x7,
> > + .is_ad778x = false,
> > },
> > [ID_AD7780] = {
> > .channel = AD7780_CHANNEL(24, 32),
> > .pattern = 0x1,
> > .pattern_mask = 0x3,
> > + .is_ad778x = true,
> > },
> > [ID_AD7781] = {
> > .channel = AD7780_CHANNEL(20, 32),
> > .pattern = 0x1,
> > .pattern_mask = 0x3,
> > + .is_ad778x = true,
> > },
> > };
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-usp+unsubscr...@googlegroups.com.
> To post to this group, send email to kernel-...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kernel-usp/55b5de74-a607-94b9-8c85-40658e38fbb5%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: ad7280a: Lines should not end with a '(' - style
>(There is a linux-...@googlegroups.com mailing list >that bounces when I reply, so I removed it from the >cc list) Sorry. I think this may be because my HTML mode in gmail was enabled. > I wrote that to encourage you to do more than > what checkpatch says. > I just moved code around and reduced duplication. > There are many similar opportunities for code > refactoring in staging. Thank you :-) I will put effort to improve these points. > You could test what I wrote, add a good commit > message with a subject like: > > [PATCH V2] staging: iio: ad7280a: Refactor > > with a commit message that describes the changes and > perhaps shows the object size difference using > > $ size I will do that. :-) > Are you doing this for a class assignment? Yes, I am. I am into a group that aims to contribute to opensource projects and we chose the Linux kernel. Our mentor suggested us to contribute to the linux-iio Thank you On Tue, Oct 16, 2018 at 8:08 PM Joe Perches wrote: > > (There is a linux-...@googlegroups.com mailing list > that bounces when I reply, so I removed it from the > cc list) > > On Tue, 2018-10-16 at 19:48 -0300, Giuliano Belinassi wrote: > > Hello, > > Thank you for your review :-). > > Sorry, but I am a newbie on this, and now I am confused about my next > > step. Should I make a v2 based on your changes, or do you want to submit > > your changes? > > I wrote that to encourage you to do more than > what checkpatch says. > > I just moved code around and reduced duplication. > > There are many similar opportunities for code > refactoring in staging. > > You could test what I wrote, add a good commit > message with a subject like: > > [PATCH V2] staging: iio: ad7280a: Refactor > > with a commit message that describes the changes and > perhaps shows the object size difference using > > $ size > > Maybe add a Suggested-by: tag if it pleases you, but > what I did is trivial and I think it's unnecessary. > > Are you doing this for a class assignment? > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
