Messages in this thread | | | Date | Wed, 28 Sep 2022 14:14:14 +0300 | Subject | Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer | From | Matti Vaittinen <> |
| |
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! ~~
| |