Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Christian Eggers <ceggers@arri.de>,
	Rob Herring <robh+dt@kernel.org>,
	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 17:19:55 +0100
Message-ID: <20200731171955.00000942@Huawei.com> (raw)
In-Reply-To: <CAHp75VdSNXWCVVgX+8BCC5iWjO14KMUCNrYvZyFfez-fFerQsA@mail.gmail.com>

On Fri, 31 Jul 2020 18:41:47 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jul 31, 2020 at 1:52 PM Christian Eggers <ceggers@arri.de> wrote:
> 
> > > 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.  
> 
> It's strange.
> 
> ...
> 
> > > 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,  
> 
> ' * 1'
> 
> >         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.  
> 
> What I meant is solely comments to be joined, not the code.
> 
> ...
> 
       unsigned int time_us = as73211_integration_time_us(data, as73211_integration_time_1024cyc(data));  
> > > One line?  
> 
> > checkpatch complains... ignore?  
> 
> Hmm... is it over 100? Or you are using some old tools to work with
> the kernel...

It's over a 100... (about 103 by the handy scale at the top of my email client :)

> 
> ...
> 
> > > > +               /* 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));
> > }  
> 
> This way (w/o _NON_SHIFTED suffix) if both 13:s in the code are of the
> same meaning.
> 
> ...
> 
> > > > +       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.  
> 
> Why are you talking about v5.4? We are in v5.8 cycle contributing to v5.9.

This will be 5.10 now.  5.9 cycle for new stuff via IIO effectively closed
last week. 

> 
> Recently IIO gained some features among which I think the one that
> assigns parent devices.
> 
yup. This should be in linux-next now for the coming merge window (which probably opens on Sunday).

Note that we have lots of time to tidy up loose ends for this one as it will only sit
in my tree for next few weeks if I do pick it up.

Jonathan



  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
2020-07-31 15:41       ` Andy Shevchenko
2020-07-31 16:19         ` Jonathan Cameron [this message]
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=20200731171955.00000942@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=ceggers@arri.de \
    --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