From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>,
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: Wed, 28 Sep 2022 14:14:14 +0300 [thread overview]
Message-ID: <3eea7954-3faf-3fc9-7507-c318488c5524@gmail.com> (raw)
In-Reply-To: <20220922180339.30138141@jic23-huawei>
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.
>> +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()).
>> +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. Well, I don't think it is a bid geal though -
just something worth noticing I guess.
>> +
>> +int kx022a_probe_internal(struct device *dev, int irq)
>> +{
>> + static const char * const regulator_names[] = {"io_vdd", "vdd"};
>> + struct iio_trigger *indio_trig;
>> + struct kx022a_data *data;
>> + struct regmap *regmap;
>> + unsigned int chip_id;
>> + struct iio_dev *idev;
>> + int ret;
>> +
>> + if (WARN_ON(!dev))
>> + return -ENODEV;
>> +
>> + regmap = dev_get_regmap(dev, NULL);
>> + if (!regmap) {
>> + dev_err(dev, "no regmap\n");
>
> Use dev_err_probe() for all dev_err() stuff in probe paths.
> It ends up cleaner and we don't care about the tiny overhead
> of checking for deferred.
This one bothers me a bit. It just does not feel correct to pass -EINVAL
for the dev_err_probe() so the dev_err_probe() can check if -EINVAL !=
-EPROBE_DEFER. I do understand perfectly well the consistent use of
dev_err_probe() for all cases where we get an error-code from a function
and return it - but using dev_err_probe() when we hard-code the return
value in code calling the dev_err_probe() does not feel like "the right
thing to do" (tm).
Eg, I agree that
return dev_err_probe(dev, ret, "bar");
is nice even if we know the function that gave us the "ret" never
requests defer (as that can change some day).
However, I don't like issuing:
return dev_err_probe(dev, -EINVAL, "bar");
Well, please let me know if you think the dev_err_probe() should be used
even in cases where we hard code the return to something...
For v2 I do change the other prints (like the one about failed regmap
read below).
>
>> +
>> + return -EINVAL;
>> + }
>> +
>> + idev = devm_iio_device_alloc(dev, sizeof(*data));
>> + if (!idev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(idev);
>> +
>> + /*
>> + * VDD is the analog and digital domain voltage supply
>> + * IO_VDD is the digital I/O voltage supply
>> + */
>> + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
>> + regulator_names);
>> + if (ret && ret != -ENODEV)
>> + return dev_err_probe(dev, ret, "failed to enable regulator\n");
>> +
>> + ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
>> + if (ret) {
>> + dev_err(dev, "Failed to access sensor\n");
Yours,
-- Matti Vaittinen
--
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
next prev parent reply other threads:[~2022-09-28 11:15 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 [this message]
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
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=3eea7954-3faf-3fc9-7507-c318488c5524@gmail.com \
--to=mazziesaccount@gmail.com \
--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=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.