[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20251004145526.13224aaf@jic23-huawei>
Date: Sat, 4 Oct 2025 14:55:26 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Frank Li <Frank.Li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>, Miquel Raynal
<miquel.raynal@...tlin.com>, David Lechner <dlechner@...libre.com>, Nuno
Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org,
imx@...ts.linux.dev, linux-iio@...r.kernel.org, Carlos Song
<carlos.song@....com>
Subject: Re: [PATCH v3 5/5] iio: magnetometer: Add mmc5633 sensor
On Tue, 30 Sep 2025 15:34:24 -0400
Frank Li <Frank.Li@....com> wrote:
> Add mmc5633 sensor basic support.
> - Support read 20 bits X/Y/Z magnetic.
> - Support I3C HDR mode to send start measurememt command.
> - Support I3C HDR mode to read all sensors data by one command.
>
> Co-developed-by: Carlos Song <carlos.song@....com>
> Signed-off-by: Carlos Song <carlos.song@....com>
> Signed-off-by: Frank Li <Frank.Li@....com>
Hi Frank,
A few more minor things inline.
Thanks,
Jonathan
> ---
> Change in v3
> - remove mmc5633_hw_set
> - make -> Make
> - change indention for mmc5633_samp_freq
> - use u8 arrary to handle dword data
> - get_unaligned_be16() to get raw data
> - add helper function to check if i3c support hdr
> - use read_avail() callback
>
> change in v2
> - new patch
> diff --git a/drivers/iio/magnetometer/mmc5633.c b/drivers/iio/magnetometer/mmc5633.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..bcd79ab6053d50026961f7cf9da296c30c720399
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mmc5633.c
> @@ -0,0 +1,534 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MMC5633 - MEMSIC 3-axis Magnetic Sensor
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + * Copyright (c) 2025, NXP
> + *
> + * IIO driver for MMC5633, base on mmc35240.c
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/i3c/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/unaligned.h>
> +
> +#define MMC5633_REG_XOUT_L 0x00
> +#define MMC5633_REG_XOUT_H 0x01
> +#define MMC5633_REG_YOUT_L 0x02
> +#define MMC5633_REG_YOUT_H 0x03
> +#define MMC5633_REG_ZOUT_L 0x04
> +#define MMC5633_REG_ZOUT_H 0x05
> +#define MMC5633_REG_XOUT_2 0x06
> +#define MMC5633_REG_YOUT_2 0x07
> +#define MMC5633_REG_ZOUT_2 0x08
> +
> +#define MMC5633_REG_STATUS1 0x18
> +#define MMC5633_REG_STATUS0 0x19
> +#define MMC5633_REG_CTRL0 0x1b
> +#define MMC5633_REG_CTRL1 0x1c
> +#define MMC5633_REG_CTRL2 0x1d
> +
> +#define MMC5633_REG_ID 0x39
> +
> +#define MMC5633_STATUS1_MEAS_M_DONE_BIT BIT(6)
> +
> +#define MMC5633_CTRL0_CMM_FREQ_EN BIT(7)
> +#define MMC5633_CTRL0_AUTO_ST_EN BIT(6)
> +#define MMC5633_CTRL0_AUTO_SR_EN BIT(5)
> +#define MMC5633_CTRL0_RESET BIT(4)
> +#define MMC5633_CTRL0_SET BIT(3)
> +#define MMC5633_CTRL0_MEAS_T BIT(1)
> +#define MMC5633_CTRL0_MEAS_M BIT(0)
> +
> +#define MMC5633_CTRL1_BW0_BIT BIT(0)
> +#define MMC5633_CTRL1_BW1_BIT BIT(1)
Drop these and
> +
> +#define MMC5633_CTRL1_BW_MASK (MMC5633_CTRL1_BW0_BIT | \
> + MMC5633_CTRL1_BW1_BIT)
use GENMASK(1, 0) here
> +
> +#define MMC5633_WAIT_CHARGE_PUMP 50000 /* us */
Name them so the unit is in the define and then you don't need
the comment + we can see the unit where they are used.
*PUMP_USECS for example
> +#define MMC5633_WAIT_SET_RESET 1000 /* us */
> +
> +#define MMC5633_HDR_CTRL0_MEAS_M 0x01
> +#define MMC5633_HDR_CTRL0_MEAS_T 0x03
> +#define MMC5633_HDR_CTRL0_SET 0X05
> +#define MMC5633_HDR_CTRL0_RESET 0x07
> +
> +static const struct {
> + int val;
> + int val2;
> +} mmc5633_samp_freq[] = {
> + {1, 200000},
> + {2, 0},
> + {3, 500000},
> + {6, 600000},
My preference for style in IIO is
{ 1, 200000 },
{ 2, 0 },
etc.
> +};
> +static bool mmc5633_is_support_hdr(struct mmc5633_data *data)
> +{
> + if (!data->i3cdev)
> + return false;
> +
> + return !!(i3c_device_get_supported_xfer_mode(data->i3cdev) & BIT(I3C_HDR_DDR));
The !! isn't needed given assigning an integer to the bool will have same effect.
> +}
> +static const struct of_device_id mmc5633_of_match[] = {
> + { .compatible = "memsic,mmc5633", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, mmc5633_of_match);
> +
> +static const struct i2c_device_id mmc5633_i2c_id[] = {
> + { "mmc5633" },
> + {}
{ }
Both for consistency and because I'm more broadly trying to standardize
on that formatting for IIO.
> +};
> +MODULE_DEVICE_TABLE(i2c, mmc5633_i2c_id);
> +
> +static struct i2c_driver mmc5633_i2c_driver = {
> + .driver = {
> + .name = "mmc5633_i2c",
> + .of_match_table = mmc5633_of_match,
> + .pm = pm_sleep_ptr(&mmc5633_pm_ops),
> + },
> + .probe = mmc5633_i2c_probe,
> + .id_table = mmc5633_i2c_id,
Mixture of tabs and spacing here. I'd just use a single space
before the = and don't try to align them.
> +};
Powered by blists - more mailing lists