Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Christian Eggers <ceggers@arri.de>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Jonathan Cameron <jic23@kernel.org>,
	"Hartmut Knaack" <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	"Peter Meerwald-Stadler" <pmeerw@pmeerw.net>,
	linux-iio <linux-iio@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 2/2] iio: light: as73211: New driver
Date: Fri, 31 Jul 2020 12:52:22 +0200
Message-ID: <2706267.JtmGt7LAV2@n95hx1g2> (raw)
In-Reply-To: <CAHp75VdDCnQLh0Qts8hsgLBy5TqibOKAYSeFxuV69XLroRBOEg@mail.gmail.com>

Hi Andy,

thanks for further review. If nobody else sends comments, I'll
publish the next version tonight. Maybe we could clarify the questions
below in time.

Best regards
Christian


> W=1 (not V=1) runs kernel doc validation script.
without V=1, I get nothing. Neither excess nor missing members
are reported on my system.


On Friday, 31 July 2020, 09:34:02 CEST, Andy Shevchenko wrote:
> On Fri, Jul 31, 2020 at 10:03 AM Christian Eggers <ceggers@arri.de> wrote:
> > +static const int as73211_samp_freq_avail[] = { 1024000, 2048000, 4096000,
> > 8192000 };
> This looks related to the below mentioned 1.024MHz.
> 
> Perhaps add a definition above and comment here?
> 
> #define AS73211_BASE_FREQ_1024KHZ   1024000
added similar define in v5. The array looks like the following now

static const int as73211_samp_freq_avail[] = {
	AS73211_SAMPLE_FREQ_BASE,
	AS73211_SAMPLE_FREQ_BASE * 2,
	AS73211_SAMPLE_FREQ_BASE * 4,
	AS73211_SAMPLE_FREQ_BASE * 8
};


> > +/* integration time in units of 1024 clock cycles */
> 
> Unify this with below one. Or the other way around, i.o.w. join one of
> them into the other.
> 
> > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data
> > *data) +{
> > +       /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */
> > +       return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1));
> > +}
I'm not sure, whether this is possible. as73211_integration_time_1024cyc()
returns the current setting from hardware. as73211_integration_time_us()
calculates the resulting time. But as73211_integration_time_us() is also
called in as73211_integration_time_calc_avail() inside the loop.

> > +       unsigned int time_us = as73211_integration_time_us(data,
> > +                                                          as73211_integration_time_1024cyc(data));
> One line?
checkpatch complains... ignore?


> > +               int reg_bits, freq_kHz = val / 1000 /* HZ_PER_KHZ */;  /*
> > 1024, 2048, ... */ +
> > +               /* val must be 1024 * 2^x */
> > +               if (val < 0 || (freq_kHz * 1000 /* HZ_PER_KHZ */) != val
> > ||
> > +                               !is_power_of_2(freq_kHz) || val2)
> > +                       return -EINVAL;
> 
> Please, define HZ_PER_KHZ locally. It will really help when we move
> these definitions to a global level.
ok

> 
> ...
> 
> > +               /* gain can be calculated from CREG1 as 2^(13 -
> > CREG1_GAIN) */ +               reg_bits = 13 - ilog2(val);
> 
> 13 is the second time in the code. Deserves a descriptive definition.
I'm unsure how to solve this. Possible values for gain:

CREG1[7:4]  | gain
-----------------------------
0           | 2048x
1           | 1024x
2           |  512x
...         |  ...
13          |    1x

#define AS73211_CREG1_GAIN_1_NON_SHIFTED 13  // this define is CREG1 related, but not shifted to the right position

static unsigned int as73211_gain(struct as73211_data *data)
{
	/* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
	return BIT(AS73211_CREG1_GAIN_1_NON_SHIFTED - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
}

---- or ----

#define AS73211_CREG1_GAIN_1 FIELD_PREP(AS73211_CREG1_GAIN_MASK, 13)

static unsigned int as73211_gain(struct as73211_data *data)
{
	/* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
	return BIT(FIELD_GET(AS73211_CREG1_GAIN_MASK, AS73211_CREG1_GAIN_1) - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
}


> > +       indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +       if (indio_dev == NULL)
> 
> if (!indio_dev)
> 
> > +               return -ENOMEM;
> 
> ...
> 
> > +       indio_dev->dev.parent = dev;
> 
> Doesn't IIO core do this for you?
devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc().
I already looked around, but I didn't find. And after debugging
v5.4, devm_iio_device_alloc() definitely doesn't do it.

> > +       ret = devm_iio_device_register(dev, indio_dev);
> > +       if (ret < 0)
> > +               goto powerdown;
> > +
> > +       return 0;
> > 
> > +powerdown:
> > +       as73211_power(indio_dev, false);
> > +       return ret;
> 
> devm_*() is tricky. Here you broke ordering heavily. So, consider to
> add this under devm_add_action_or_reset().
Sorry, my mistake! I already felt that something may be wrong here...




  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-31  7:01 [PATCH v4 0/2] iio: light: Support AMS AS73211 digital XYZ sensor Christian Eggers
2020-07-31  7:01 ` [PATCH v4 1/2] dt-bindings: iio: light: add AMS AS73211 support Christian Eggers
2020-07-31 22:45   ` Rob Herring
2020-07-31  7:01 ` [PATCH v4 2/2] iio: light: as73211: New driver Christian Eggers
2020-07-31  7:34   ` Andy Shevchenko
2020-07-31 10:52     ` Christian Eggers [this message]
2020-07-31 15:41       ` Andy Shevchenko
2020-07-31 16:19         ` Jonathan Cameron
2020-08-01 15:29   ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2706267.JtmGt7LAV2@n95hx1g2 \
    --to=ceggers@arri.de \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmeerw@pmeerw.net \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git