| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <73cf8ac8-932b-44cf-88b8-6a175bcff8cb@kernel.org>
Date: Thu, 18 Jul 2024 14:35:36 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jianping.Shen@...bosch.com, jic23@...nel.org, lars@...afoo.de,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
dima.fedrau@...il.com, marcelo.schmitt1@...il.com,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Christian.Lorenz3@...bosch.com,
Ulrike.Frauendorf@...bosch.com, Kai.Dolde@...bosch.com
Subject: Re: [PATCH] drivers: Bosch SMI240 IMU Driver
On 18/07/2024 14:24, Jianping.Shen@...bosch.com wrote:
> From: "Shen Jianping (ME-SE/EAD2)" <she2rt@...C-0008DVM.rt.de.bosch.com>
>
> Add Bosch SMI240 IMU IIO Driver to iio-for-6.10b
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
> Signed-off-by: Shen Jianping (ME-SE/EAD2) <she2rt@...C-0008DVM.rt.de.bosch.com>
> ---
> .../bindings/iio/imu/bosch,smi240.yaml | 45 +
Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.
> drivers/iio/imu/Kconfig | 2 +
> drivers/iio/imu/Makefile | 1 +
> drivers/iio/imu/smi240/Kconfig | 30 +
> drivers/iio/imu/smi240/Makefile | 8 +
> drivers/iio/imu/smi240/smi240.h | 31 +
> drivers/iio/imu/smi240/smi240_core.c | 814 ++++++++++++++++++
> drivers/iio/imu/smi240/smi240_spi.c | 153 ++++
> 8 files changed, 1084 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
> create mode 100644 drivers/iio/imu/smi240/Kconfig
> create mode 100644 drivers/iio/imu/smi240/Makefile
> create mode 100644 drivers/iio/imu/smi240/smi240.h
> create mode 100644 drivers/iio/imu/smi240/smi240_core.c
> create mode 100644 drivers/iio/imu/smi240/smi240_spi.c
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
> new file mode 100644
> index 00000000000..972819cacff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bosch,smi240.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/imu/bosch,smi240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: BOSCH SMI240
> +
> +maintainers:
> + - unknown
You need person responsible for this. Otherwise why would we care?
> +
> +description: |
> + Inertial Measurement Unit with Accelerometer, Gyroscope
> + https://www.bosch-semiconductors.com/mems-sensors/highly-automated-driving/smi240/
> +
> +properties:
> + compatible:
> + const: BOSCH,SMI240
Nope, no uppercase. Do you see any posting like that?
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - spi-max-frequency
Why?
> + - reg
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + // Example
Drop
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + smi240@0 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "BOSCH,SMI240";
> + spi-max-frequency = <10000000>;
> + reg = <0>;
Please order the properties according to DTS coding style:
https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node
...
> +
> +#ifndef _SMI240_H
> +#define _SMI240_H
> +
> +#define SENSOR_NAME "SMI240"
> +#define DRIVER_VERSION "1.0.0"
No, drop.
> +
> +#define SET_BITS(reg_var, bitname, val) \
> + (((reg_var) & ~(bitname##_MASK)) | \
> + (((val) << bitname##_POS) & bitname##_MASK))
Drop, use generic macros
> +
> +#define GET_BITS(reg_var, bitname) \
> + (((reg_var) & (bitname##_MASK)) >> (bitname##_POS))
Drop, use generic macros
> +
> +struct smi240_device {
> + uint16_t accel_filter_freq;
> + uint16_t anglvel_filter_freq;
> + uint16_t sign_of_channels;
> + uint8_t bite_reps;
> + int8_t (*xfer)(uint32_t request, uint32_t *data);
> +};
...
> +
> +int smi240_probe(struct device *dev, struct smi240_device *smi240_dev)
> +{
> + int ret;
> + int16_t response;
> + struct iio_dev *indio_dev;
> +
> + ret = smi240_get_regs(SMI240_CHIP_ID_REG, &response, 1, 0, smi240_dev);
> + if (ret) {
> + pr_err("Read chip id failed.");
Uhu... drivers use dev_err. This applies everywhere.
> + return ret;
> + }
> +
> + if (response == SMI240_CHIP_ID) {
> + pr_info("SMI240 Chip ID: 0x%04x", response);
Drop
> + } else {
> + pr_err("Unexpected Chip ID for SMI240: 0x%04x", response);
> + return -ENODEV;
> + }
> +
> + ret = smi240_soft_reset(smi240_dev);
> + if (ret) {
> + pr_err("Soft Reset failed.");
> + return ret;
> + }
> +
> + ret = smi240_init(smi240_dev);
> + if (ret)
> + return ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, 0);
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + iio_device_set_drvdata(indio_dev, smi240_dev);
> + dev_set_drvdata(dev, indio_dev);
> +
> + indio_dev->dev.parent = dev;
> + indio_dev->channels = smi240_channels;
> + indio_dev->num_channels = ARRAY_SIZE(smi240_channels);
> + indio_dev->name = SENSOR_NAME;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &smi240_info;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret) {
> + dev_err(dev, "Register IIO device failed");
> + goto exit_failure;
> + }
> +
> + return ret;
> +
> +exit_failure:
> + smi240_remove(dev);
> + return ret;
> +}
> +
> +int smi240_remove(struct device *dev)
> +{
> + dev_info(dev, "unregister SMI240");
Drop
> + return 0;
> +}
> diff --git a/drivers/iio/imu/smi240/smi240_spi.c b/drivers/iio/imu/smi240/smi240_spi.c
> new file mode 100644
> index 00000000000..2be6320eaba
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_spi.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2024 Robert Bosch GmbH.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <generated/uapi/linux/version.h>
> +
> +#include "smi240.h"
> +
> +#define SMI240_SPI_MAX_BUFFER_SIZE 32
> +
> +static uint8_t *rx_buf;
> +static uint8_t *tx_buf;
No, drop
> +static struct spi_device *smi240_spi_dev;
> +static struct smi240_device smi240_dev;
No, drop all. No file-scope variables. How do you support two devices?
> +
> +static int8_t smi240_spi_transfer(uint32_t request, uint32_t *response)
> +{
> + int8_t ret;
> + struct spi_message msg;
> + struct spi_transfer xfer = {
> + .tx_buf = tx_buf, .rx_buf = rx_buf, .len = 4,
> + //.bits_per_word = 32,
> + };
> +
> + if (smi240_spi_dev == NULL)
> + return -ENODEV;
> +
> + tx_buf[0] = request >> 24;
> + tx_buf[1] = request >> 16;
> + tx_buf[2] = request >> 8;
> + tx_buf[3] = request;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> + ret = spi_sync(smi240_spi_dev, &msg);
> +
> + if (ret)
> + return ret;
> +
> + if (response != NULL)
> + *response = (rx_buf[0] << 24) | (rx_buf[1] << 16) |
> + (rx_buf[2] << 8) | rx_buf[3];
> +
> + return ret;
> +}
> +
> +static int smi240_spi_probe(struct spi_device *device)
This is never called "device"
> +{
> + int err;
> +
> + device->bits_per_word = 8;
> + device->max_speed_hz = 10000000;
> + device->mode = SPI_MODE_0;
> +
> + err = spi_setup(device);
> + if (err < 0) {
> + pr_err("spi_setup err!\n");
Oh, no need to scream.
> + return err;
> + }
> +
> + if (rx_buf == NULL)
WTF?
> + rx_buf = kmalloc(4, GFP_KERNEL);
What the hell is 4?
> + if (!rx_buf)
> + return -ENOMEM;
> +
> + if (tx_buf == NULL)
> + tx_buf = kmalloc(4, GFP_KERNEL);
Why none of these are devm?
> + if (!tx_buf)
> + return -ENOMEM;
> +
> + smi240_spi_dev = device;
> +
> + err = smi240_probe(&device->dev, &smi240_dev);
> + if (err) {
> + kfree(rx_buf);
> + rx_buf = NULL;
> + kfree(tx_buf);
> + tx_buf = NULL;
> + smi240_spi_dev = NULL;
What are these? What error handling is this? Sorry, this code is
absolutely ugly. Why this cannot be devm?
> + dev_err(&device->dev,
> + "Bosch Sensor Device %s initialization failed %d",
> + SENSOR_NAME, err);
> + } else
> + dev_info(&device->dev, "Bosch Sensor Device %s initialized",
> + SENSOR_NAME);
No drop
> +
> + return err;
> +}
> +
> +static void smi240_spi_remove(struct spi_device *device)
> +{
> + if (rx_buf != NULL) {
??? Drop, useless
> + kfree(rx_buf);
> + rx_buf = NULL;
Why? What the heck is this?
> + }
> +
> + if (tx_buf != NULL) {
> + kfree(tx_buf);
> + tx_buf = NULL;
> + }
> + smi240_remove(&device->dev);
> +}
> +
> +static const struct spi_device_id smi240_id[] = { { SENSOR_NAME, 0 }, {} };
> +
> +MODULE_DEVICE_TABLE(spi, smi240_id);
> +
> +static const struct of_device_id smi240_of_match[] = {
> + {
> + .compatible = SENSOR_NAME,
No clue what's that, but that's a strong NAK.
There is no such code. NOWWHERE. Please do not invent your own, Bosch,
style.
> + },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(of, smi240_of_match);
> +
> +static struct spi_driver smi240_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
Don't upstream some ancient code.
This contribution lacks basic review and does not follow basic
guidelines. Please carefully read submitting patches and coding style
before posting.
Be sure your driver has no W=1 warnings, nothing from
coccinelle/coccicheck, smatch and sparse.
Also, you shou;d have zero warnigns from checkpatch --strict
> + .name = SENSOR_NAME,
> + .of_match_table = smi240_of_match,
> + },
> + .id_table = smi240_id,
> + .probe = smi240_spi_probe,
> + .remove = smi240_spi_remove,
> +};
> +
> +static int __init smi240_module_init(void)
> +{
> + int err = 0;
> +
> + smi240_dev.xfer = smi240_spi_transfer;
> +
> + err |= spi_register_driver(&smi240_driver);
> + return err;
Drop err, useless. Just return.
> +}
> +
> +static void __exit smi240_module_exit(void)
> +{
> + spi_unregister_driver(&smi240_driver);
> +}
> +
> +module_init(smi240_module_init);
> +module_exit(smi240_module_exit);
> +
> +MODULE_DESCRIPTION("SMI240 IMU SENSOR DRIVER");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_VERSION(DRIVER_VERSION);
No, drop.
Best regards,
Krzysztof
Powered by blists - more mailing lists