From: Jonathan Cameron <jic23@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
Cosmin Tanislav <demonsingur@gmail.com>,
Jagath Jog J <jagathjog1996@gmail.com>,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, "Mutanen,
Mikko" <Mikko.Mutanen@fi.rohmeurope.com>,
"Haikola, Heikki" <Heikki.Haikola@fi.rohmeurope.com>
Subject: Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer
Date: Sun, 2 Oct 2022 12:18:57 +0100 [thread overview]
Message-ID: <20221002121857.3f7d9423@jic23-huawei> (raw)
In-Reply-To: <3eea7954-3faf-3fc9-7507-c318488c5524@gmail.com>
On Wed, 28 Sep 2022 14:14:14 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> Hi Jonathan,
>
> On 9/22/22 20:03, Jonathan Cameron wrote:
> > On Wed, 21 Sep 2022 14:45:35 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >> +
> >> +/*
> >> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
> >> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not
> >> + * available for higher sample rates. Thus the driver only supports 200 Hz and
> >> + * slower ODRs. Slowest being 0.78 Hz
> >> + */
> >> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
> >> +static IIO_CONST_ATTR(scale_available,
> >> + "598.550415 1197.10083 2394.20166 4788.40332");
> >> +
> >> +static struct attribute *kx022a_attributes[] = {
> >> + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> >> + &iio_const_attr_scale_available.dev_attr.attr,
> >
> > Use the read_avail() callback instead of doing these as attributes.
> > That makes the values available to consumer drivers...
>
> Am I correct that populating the read_avail() does not add sysfs entries
> for available scale/frequency? Eg, if I wish to expose the supported
> values via sysfs I still need these attributes? Implementing the
> read_avail() as well is not a problem though.
Need to also set the relevant bit in
info_mask_shared_by_xxx_avail in the channels for the sysfs files to be created
by calling the read_avail() callback.
When I introduced those I thought about making it mandatory to introduce them
for all the info_mask_shared_by_xxx entries and not having the extra bitmap
but that meant figuring out the relevant entries for a mass of stuff whenever
a driver was converted from the old approach like you've used here.
>
> >> +static int kx022a_turn_on_unlock(struct kx022a_data *data)
> >> +{
> >> + int ret;
> >> +
> > This is not used enough that I can see a strong reason for the
> > wrapper. Just put the two calls inline and rename the unlocked case.
>
> In my opinion the kx022a_turn_on_unlock() and kx022a_turn_off_lock() do
> simplify functions. Especially after I started using the
> iio_device_claim_direct_mode() :) Thus I will leave these for the v2 -
> please ping me again if you still want to see them removed (but I think
> the usage of iio_device_claim_direct_mode() changed this to favour the
> kx022a_turn_on_unlock() and kx022a_turn_off_lock()).
Let's see how it looks in v2.
>
> >> +static int kx022a_chip_init(struct kx022a_data *data)
> >> +{
> >> + int ret, dummy;
> >> +
> >> + /*
> >> + * Disable IRQs because if the IRQs are left on (for example by
> >> + * a shutdown which did not deactivate the accelerometer) we do
> >> + * most probably end up flooding the system with unhandled IRQs
> >> + * and get the line disabled from SOC side.
> >> + */
> >> + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);
> >
> > Unusual to do this rather than a reset. Quick look suggests there is
> > a suitable software reset (CNTL2)
>
> I switched to the software reset as you suggested. I am not really
> convinced it is a better way. It seems the software reset requires us to
> re-init the regmap cache.
Yup, though if you've provided the reset defaults that should be quick.
Jonathan
next prev parent reply other threads:[~2022-10-02 11:18 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-21 11:40 [RFC PATCH 0/5] iio: Support ROHM/Kionix kx022a Matti Vaittinen
2022-09-21 11:42 ` [RFC PATCH 1/5] regulator: Add devm helpers for get and enable Matti Vaittinen
2022-09-21 11:43 ` [RFC PATCH 2/5] " Matti Vaittinen
2022-09-21 11:45 ` [RFC PATCH 3/5] dt-bindings: iio: Add KX022A accelerometer Matti Vaittinen
2022-09-21 19:11 ` Krzysztof Kozlowski
2022-09-21 19:30 ` Vaittinen, Matti
2022-09-21 19:56 ` Krzysztof Kozlowski
2022-09-22 3:49 ` Matti Vaittinen
2022-09-21 11:45 ` [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM " Matti Vaittinen
2022-09-21 19:18 ` Vaittinen, Matti
2022-09-22 17:03 ` Jonathan Cameron
2022-09-23 6:31 ` Matti Vaittinen
2022-09-24 15:17 ` Jonathan Cameron
2022-09-26 5:02 ` Vaittinen, Matti
2022-10-02 11:11 ` Jonathan Cameron
2022-09-28 11:14 ` Matti Vaittinen
2022-09-28 14:06 ` Andy Shevchenko
2022-09-28 16:23 ` Matti Vaittinen
2022-10-02 11:14 ` Jonathan Cameron
2022-10-02 11:18 ` Jonathan Cameron [this message]
2022-10-02 14:31 ` Matti Vaittinen
2022-09-21 11:46 ` [RFC PATCH 5/5] MAINTAINERS: Add KX022A maintainer entry Matti Vaittinen
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=20221002121857.3f7d9423@jic23-huawei \
--to=jic23@kernel.org \
--cc=Heikki.Haikola@fi.rohmeurope.com \
--cc=Mikko.Mutanen@fi.rohmeurope.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=demonsingur@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jagathjog1996@gmail.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mazziesaccount@gmail.com \
--cc=nikita.yoush@cogentembedded.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).