On 18/07/2024 14:24, Jianping.Shen@xxxxxxxxxxxx wrote: > From: "Shen Jianping (ME-SE/EAD2)" <she2rt@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> > --- > .../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
