| 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: <99a84071-f737-9a8b-73fa-bcc3e0ff6a3f@roeck-us.net>
Date: Fri, 10 Sep 2021 09:21:55 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Oskar Senft <osk@...gle.com>, linux-hwmon@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Jean Delvare <jdelvare@...e.com>
Subject: Re: [PATCH] hwmon: (nct7802) Make RTD modes configurable.
On 9/10/21 6:07 AM, Oskar Senft wrote:
> This change allows the RTD modes to be configurable via device tree
> bindings. When the setting is not present via device tree, the driver
> still defaults to the previous behavior where the RTD modes are left
> alone.
>
> Signed-off-by: Oskar Senft <osk@...gle.com>
> ---
> drivers/hwmon/nct7802.c | 94 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
> index 604af2f6103a..6a6ab529bdd3 100644
> --- a/drivers/hwmon/nct7802.c
> +++ b/drivers/hwmon/nct7802.c
> @@ -51,6 +51,24 @@ static const u8 REG_VOLTAGE_LIMIT_MSB_SHIFT[2][5] = {
> #define REG_CHIP_ID 0xfe
> #define REG_VERSION_ID 0xff
>
> +/*
> + * Sensor modes according to 7.2.32 Mode Selection Register
> + */
> +#define RTD_MODE_CLOSED 0x0
> +#define RTD_MODE_CURRENT 0x1
> +#define RTD_MODE_THERMISTOR 0x2
> +#define RTD_MODE_VOLTAGE 0x3
> +#define RTD_MODE_UNDEFINED 0xf
> +
> +#define MODE_BITS_MASK 0x3
> +
> +/*
> + * Bit offsets for sensors modes in REG_MODE
> + */
> +#define MODE_OFFSET_RTD1 0
> +#define MODE_OFFSET_RTD2 2
> +#define MODE_OFFSET_RTD3 4
> +
I think the access can be defined in a macro, with the index as parameter.
to be able to use it in the temp_type_{read,store} functions.
> /*
> * Data structures and manipulation thereof
> */
> @@ -1038,7 +1056,9 @@ static const struct regmap_config nct7802_regmap_config = {
> .volatile_reg = nct7802_regmap_is_volatile,
> };
>
> -static int nct7802_init_chip(struct nct7802_data *data)
> +static int nct7802_init_chip(struct nct7802_data *data,
> + unsigned char rtd1_mode, unsigned char rtd2_mode,
> + unsigned char rtd3_mode)
> {
> int err;
>
> @@ -1052,15 +1072,57 @@ static int nct7802_init_chip(struct nct7802_data *data)
> if (err)
> return err;
>
> + /* Configure sensor modes */
> + if ((rtd1_mode & MODE_BITS_MASK) == rtd1_mode) {
This is an odd way of checking if the mode is set. Why not just
"if (rtd1_mode != RTD_MODE_UNDEFINED)" ?
> + err = regmap_update_bits(data->regmap, REG_MODE,
> + MODE_BITS_MASK << MODE_OFFSET_RTD1,
> + rtd1_mode << MODE_OFFSET_RTD1);
> + if (err)
> + return err;
> + }
> + if ((rtd2_mode & MODE_BITS_MASK) == rtd2_mode) {
> + err = regmap_update_bits(data->regmap, REG_MODE,
> + MODE_BITS_MASK << MODE_OFFSET_RTD2,
> + rtd2_mode << MODE_OFFSET_RTD2);
> + if (err)
> + return err;
> + }
> + if ((rtd3_mode & MODE_BITS_MASK) == rtd3_mode) {
> + err = regmap_update_bits(data->regmap, REG_MODE,
> + MODE_BITS_MASK << MODE_OFFSET_RTD3,
> + rtd3_mode << MODE_OFFSET_RTD3);
> + if (err)
> + return err;
> + }
This should all be handled in a single register update. Read the mode register,
do all the bit updates, then write it back as complete register if it changed.
> +
> /* Enable Vcore and VCC voltage monitoring */
> return regmap_update_bits(data->regmap, REG_VMON_ENABLE, 0x03, 0x03);
> }
>
> +static unsigned char rtd_mode_from_string(const char *value)
> +{
> + if (!strcmp(value, "closed"))
> + return RTD_MODE_CLOSED;
> + if (!strcmp(value, "current"))
> + return RTD_MODE_CURRENT;
> + if (!strcmp(value, "thermistor"))
> + return RTD_MODE_THERMISTOR;
> + if (!strcmp(value, "voltage"))
> + return RTD_MODE_VOLTAGE;
> +
> + return RTD_MODE_UNDEFINED;
> +}
> +
> static int nct7802_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> struct nct7802_data *data;
> struct device *hwmon_dev;
> + int rtd_mode_count;
> + unsigned char rtd1_mode = RTD_MODE_UNDEFINED;
> + unsigned char rtd2_mode = RTD_MODE_UNDEFINED;
> + unsigned char rtd3_mode = RTD_MODE_UNDEFINED;
> + const char *prop_value;
> int ret;
>
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> @@ -1074,7 +1136,25 @@ static int nct7802_probe(struct i2c_client *client)
> mutex_init(&data->access_lock);
> mutex_init(&data->in_alarm_lock);
>
> - ret = nct7802_init_chip(data);
> + if (dev->of_node) {
> + rtd_mode_count = of_property_count_strings(dev->of_node,
> + "nuvoton,rtd-modes");
> +
> + if (rtd_mode_count > 0)
> + if (!of_property_read_string_index(dev->of_node,
> + "nuvoton,rtd-modes", 0, &prop_value))
> + rtd1_mode = rtd_mode_from_string(prop_value);
> + if (rtd_mode_count > 1)
> + if (!of_property_read_string_index(dev->of_node,
> + "nuvoton,rtd-modes", 1, &prop_value))
> + rtd2_mode = rtd_mode_from_string(prop_value);
> + if (rtd_mode_count > 2)
> + if (!of_property_read_string_index(dev->of_node,
> + "nuvoton,rtd-modes", 2, &prop_value))
> + rtd3_mode = rtd_mode_from_string(prop_value);
> + }
> +
Better do this in nct7802_init_chip(), and pass dev as parameter.
> + ret = nct7802_init_chip(data, rtd1_mode, rtd2_mode, rtd3_mode); > if (ret < 0)
> return ret;
>
> @@ -1094,10 +1174,20 @@ static const struct i2c_device_id nct7802_idtable[] = {
> };
> MODULE_DEVICE_TABLE(i2c, nct7802_idtable);
>
> +static const struct of_device_id __maybe_unused nct7802_of_match[] = {
> + {
> + .compatible = "nuvoton,nct7802",
> + .data = 0
Unnecessary.
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, nct7802_of_match);
> +
> static struct i2c_driver nct7802_driver = {
> .class = I2C_CLASS_HWMON,
> .driver = {
> .name = DRVNAME,
> + .of_match_table = of_match_ptr(nct7802_of_match),
> },
> .detect = nct7802_detect,
> .probe_new = nct7802_probe,
>
Powered by blists - more mailing lists