Re: [PATCH v4 2/3] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Vaittinen, Matti]

Good Morning Jonathan,

On 10/23/22 14:43, Jonathan Cameron wrote:
> On Fri, 21 Oct 2022 14:22:49 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> 
>> KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor features
>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
>> tap/motion detection, wake-up & back-to-sleep events, four acceleration
>> ranges (2, 4, 8 and 16g), and probably some other cool features.
>>
>> Add support for the basic accelerometer features such as getting the
>> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
>> using the WMI IRQ).
>>
>> Important things to be added include the double-tap, motion
>> detection and wake-up as well as the runtime power management.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> 
> Hi Matti,
> 
> A few things from me on this read through to add to those last few comments
> from Andy.
> 
> Thanks,
> 
> Jonathan
> 
> 
>> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
>> new file mode 100644
>> index 000000000000..6510f8d62b85
>> --- /dev/null
>> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
>> @@ -0,0 +1,51 @@
> 
>> +
>> +MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
>> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS(KIONIX_ACCEL);
> Not sure why I didn't noticed this before, but after various debates we
> ended up prefixing namespaces to limit them to IIO. Also, there are other kionix
> drivers already and may be more in future. Better to limit this to scope of this
> driver,

Prefixing with IIO_ is all fine. And I was thinking of using the KX022A 
but ended up with KIONIX as I am pretty sure this same driver will be 
extended to cover the old 022 (without A) and probably also 122 (at 
least)...

...After that being said - I have been doing this same naming by 
part-name with ROHM PMIC drivers. As an example, first IC I was working 
with after jumping from Nokia to ROHM was BD71837. After the driver was 
written I encountered BD71847 - which was almost identical but just 
omitted few BUCKs. So, I renamed driver to BD718x7 and added the support.

As you can probably guess, this was not the end of the story. Next came 
BD71850 - which is pretty much the BD71847 with different OTP. I think I 
used some defines with BD718XX_...

...and later I did drivers for BD71815, BD71827, BD71828 ... - which all 
were completely different from the ones supported by bd718x7 driver.

What I learned was - either use the exact part name in file/defines 
(even when supporting many ICs) or use completely abstract name with no 
part name. It seems to be impossible to do correct wildcards unless you 
can dictate the IC naming :)

> 
> IIO_KX022A >
> seems like a reaonsable choice.
> 

Long babbling just to say: "Yes".
We can use IIO_KX022A even if we support many ICs. I don't plan to 
change the file/function/define naming when adding support to new ICs :)


>> +int kx022a_probe_internal(struct device *dev)
>> +{
>> +	static const char * const regulator_names[] = {"io-vdd", "vdd"};
>> +	struct iio_trigger *indio_trig;
>> +	struct fwnode_handle *fwnode;
>> +	struct kx022a_data *data;
>> +	struct regmap *regmap;
>> +	unsigned int chip_id;
>> +	struct iio_dev *idev;
>> +	int ret, irq;
>> +	char *name;
>> +
>> +	regmap = dev_get_regmap(dev, NULL);
>> +	if (!regmap) {
>> +		dev_err(dev, "no regmap\n");
>> +		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 and
>> +	 * 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)
>> +		return dev_err_probe(dev, ret, "Failed to access sensor\n");
>> +
>> +	if (chip_id != KX022A_ID) {
>> +		dev_err(dev, "unsupported device 0x%x\n", chip_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	fwnode = dev_fwnode(dev);
>> +	if (!fwnode)
>> +		return -ENODEV;
>> +
>> +	irq = fwnode_irq_get_byname(fwnode, "INT1");
>> +	if (irq > 0) {
>> +		data->inc_reg = KX022A_REG_INC1;
>> +		data->ien_reg = KX022A_REG_INC4;
>> +
>> +		if (fwnode_irq_get_byname(fwnode, "INT2") > 0)
>> +			dev_warn(dev, "Only one IRQ supported\n");
> 
> Why?  If you get the both, only the first one will be used by the driver.
> Not really worth warning about the lack of features...

My thinking regarding developing new device went along the lines:

Precondition: The HW (and data-sheet) explain how there is two INT pins.
1. Board designer reads the data-sheet and uses both INT pins.
2. SW engineer finds the driver and reads the DT-binding description.
3. SW engineer writes the DT-description and hopes everything "just 
works". (Amount of hope is probably inversely proportional to the amount 
of experience XD).
4) SW engineer gives a first go to the sensor SW and notices everything 
does not just magically work.

I think there is important difference between following options:
4a) Log shows print "Only one IRQ supported" from the kx022a driver
4b) Log shows no errors.

The amount of work potentially avoided by the check here is probably 
again inversely proportional to the SW engineer's experience - but I 
think it may be significant. OTOH, the amount of problems caused by this 
check is (in my opinion) negligible. Well, the additional print string 
does eat up few bytes of space - and I know people/systems that do not 
like this. I am still tempted to say that those systems can opt to 
disable the config for this driver though.

Well, this is not a point I would like to start fighting over though. If 
this email does not convince the audience about usefulness of the print 
- then I'll just drop it from the v5 :)

>> +	ret = devm_iio_device_register(data->dev, idev);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret,
>> +				     "Unable to register iio device\n");
>> +
>> +	/*
>> +	 * No need to check for NULL. request_threadedI_irq() defaults to
> Why I?
> 

NoI_idea(). I'll fix this, thanks! :)

>> +	 * dev_name() should the alloc fail.
>> +	 */
>> +	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
>> +			      dev_name(data->dev));
>> +
>> +	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
>> +					&kx022a_irq_thread_handler,
>> +					IRQF_ONESHOT, name, idev);
> 
> You are requesting the irq 'after' exposing the userspace interfaces.
> Technically that potentially introduces a race as we might in theory successfully
> trigger an interrupt before requesting it.
> 
> Obviously that would be challenging to pull off given likely short window, but
> good to close it anyway!  Rule of thumb is that devm_iio_device_register()
> is always the last thing to call in problem.  The very rare exceptions need
> a comment to explain why they are there...
> 
> Note that the devm_iio_trigger_register() also exposes the userspace part of the
> trigger so if you allow that to be used by other drivers you'd need the irq registration
> before that as well.
> 

Thanks! I did overlook this. In a few cases handling the IRQs use 
subsystem facilities established during driver registration. In those 
cases the dependencies and the race goes the other way around. Thus it 
is almost a habit for me to register IRQs as last thing and purge them 
as first thing.

I'll cook-up the v5 soon-ish :)

Yours
	-- Matti Vaittinen

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~