| 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: <349543e5-21b2-4725-9b33-1fcb4ae124f6@roeck-us.net>
Date: Fri, 28 Jun 2024 08:00:41 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Naresh Solanki <naresh.solanki@...ements.com>,
Jean Delvare <jdelvare@...e.com>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH] hwmon: (max6639) : Add DT support
On 6/28/24 04:54, Naresh Solanki wrote:
> Remove platform data & add devicetree support.
>
Unless I am missing something, this doesn't just add devicetree support,
it actually _mandates_ devicetree support. There are no defaults.
That is not acceptable.
More comments inline.
> Signed-off-by: Naresh Solanki <naresh.solanki@...ements.com>
> ---
> drivers/hwmon/max6639.c | 99 ++++++++++++++++++++-------
> include/linux/platform_data/max6639.h | 15 ----
> 2 files changed, 73 insertions(+), 41 deletions(-)
> delete mode 100644 include/linux/platform_data/max6639.h
>
> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> index f54720d3d2ce..9ae7aecb0737 100644
> --- a/drivers/hwmon/max6639.c
> +++ b/drivers/hwmon/max6639.c
> @@ -19,7 +19,6 @@
> #include <linux/hwmon-sysfs.h>
> #include <linux/err.h>
> #include <linux/mutex.h>
> -#include <linux/platform_data/max6639.h>
> #include <linux/regmap.h>
> #include <linux/util_macros.h>
>
> @@ -81,6 +80,7 @@ struct max6639_data {
> /* Register values initialized only once */
> u8 ppr[MAX6639_NUM_CHANNELS]; /* Pulses per rotation 0..3 for 1..4 ppr */
> u8 rpm_range[MAX6639_NUM_CHANNELS]; /* Index in above rpm_ranges table */
> + bool fan_enable[MAX6639_NUM_CHANNELS];
>
> /* Optional regulator for FAN supply */
> struct regulator *reg;
> @@ -276,6 +276,11 @@ static int max6639_write_fan(struct device *dev, u32 attr, int channel,
>
> static umode_t max6639_fan_is_visible(const void *_data, u32 attr, int channel)
> {
> + struct max6639_data *data = (struct max6639_data *)_data;
> +
> + if (!data->fan_enable[channel])
> + return 0;
> +
> switch (attr) {
> case hwmon_fan_input:
> case hwmon_fan_fault:
> @@ -372,6 +377,11 @@ static int max6639_write_pwm(struct device *dev, u32 attr, int channel,
>
> static umode_t max6639_pwm_is_visible(const void *_data, u32 attr, int channel)
> {
> + struct max6639_data *data = (struct max6639_data *)_data;
> +
> + if (!data->fan_enable[channel])
> + return 0;
> +
> switch (attr) {
> case hwmon_pwm_input:
> case hwmon_pwm_freq:
> @@ -544,43 +554,85 @@ static int rpm_range_to_reg(int range)
> int i;
>
> for (i = 0; i < ARRAY_SIZE(rpm_ranges); i++) {
> - if (rpm_ranges[i] == range)
> + if (range <= rpm_ranges[i])
What does this change have to do with adding devicetree support,
why is it done, and what is its impact ?
So far the assumption was that only valid values would be accepted by
the function, returning a default if there was no match. The incoming
data is from devicetree, where the range should be well defined and
accurate. With that in mind, I don't see the point of accepting values
outside the supported ranges.
Flexible data makes sense for sysfs attributes, where we can not
expect users to know acceptable values. For those, it is acceptable
and even desirable to find a closest match. However, that does not apply
to data obtained from devicetree.
> return i;
> }
>
> return 1; /* default: 4000 RPM */
> }
>
> +static int max6639_probe_child_from_dt(struct i2c_client *client,
> + struct device_node *child,
> + struct max6639_data *data)
> +
> +{
> + struct device *dev = &client->dev;
> + u32 i, val;
> + int err;
> +
> + err = of_property_read_u32(child, "reg", &i);
> + if (err) {
> + dev_err(dev, "missing reg property of %pOFn\n", child);
> + return err;
> + }
> +
> + if (i > 1) {
> + dev_err(dev, "Invalid fan index reg %d\n", i);
> + return -EINVAL;
> + }
> +
> + data->fan_enable[i] = true;
> +
> + err = of_property_read_u32(child, "pulses-per-revolution", &val);
> +
> + if (!err) {
> + if (val < 0 || val > 5) {
Accepting 0 seems wrong. Also, val is u32 and will never be < 0.
> + dev_err(dev, "invalid pulses-per-revolution %d of %pOFn\n", val, child);
> + return -EINVAL;
> + }
> + data->ppr[i] = val;
> + }
> +
> + err = of_property_read_u32(child, "max-rpm", &val);
> +
> + if (!err)
> + data->rpm_range[i] = rpm_range_to_reg(val);
> +
> + return 0;
> +}
> +
> static int max6639_init_client(struct i2c_client *client,
> struct max6639_data *data)
> {
> - struct max6639_platform_data *max6639_info =
> - dev_get_platdata(&client->dev);
> - int i;
> - int rpm_range = 1; /* default: 4000 RPM */
> - int err, ppr;
> + struct device *dev = &client->dev;
> + const struct device_node *np = dev->of_node;
> + struct device_node *child;
> + int i, err;
>
> /* Reset chip to default values, see below for GCONFIG setup */
> err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR);
> if (err)
> return err;
>
> - /* Fans pulse per revolution is 2 by default */
> - if (max6639_info && max6639_info->ppr > 0 &&
> - max6639_info->ppr < 5)
> - ppr = max6639_info->ppr;
> - else
> - ppr = 2;
> -
> - data->ppr[0] = ppr;
> - data->ppr[1] = ppr;
As mentioned above, this may work for your use case, but it won't work
for existing users of this driver.
> + for_each_child_of_node(np, child) {
> + if (strcmp(child->name, "fan"))
> + continue;
>
> - if (max6639_info)
> - rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
> - data->rpm_range[0] = rpm_range;
> - data->rpm_range[1] = rpm_range;
> + err = max6639_probe_child_from_dt(client, child, data);
> + if (err) {
> + of_node_put(child);
> + return err;
> + }
> + }
>
> for (i = 0; i < MAX6639_NUM_CHANNELS; i++) {
> + if (!data->fan_enable[i])
> + err = regmap_set_bits(data->regmap, MAX6639_REG_OUTPUT_MASK, BIT(1 - i));
> + else
> + err = regmap_clear_bits(data->regmap, MAX6639_REG_OUTPUT_MASK, BIT(1 - i));
> + if (err)
> + return err;
> +
> /* Set Fan pulse per revolution */
> err = max6639_set_ppr(data, i, data->ppr[i]);
> if (err)
> @@ -593,12 +645,7 @@ static int max6639_init_client(struct i2c_client *client,
> return err;
>
> /* Fans PWM polarity high by default */
> - if (max6639_info) {
> - if (max6639_info->pwm_polarity == 0)
> - err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), 0x00);
> - else
> - err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), 0x02);
> - }
> + err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), 0x00);
> if (err)
> return err;
>
> diff --git a/include/linux/platform_data/max6639.h b/include/linux/platform_data/max6639.h
> deleted file mode 100644
> index 65bfdb4fdc15..000000000000
> --- a/include/linux/platform_data/max6639.h
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _LINUX_MAX6639_H
> -#define _LINUX_MAX6639_H
> -
> -#include <linux/types.h>
> -
> -/* platform data for the MAX6639 temperature sensor and fan control */
> -
> -struct max6639_platform_data {
> - bool pwm_polarity; /* Polarity low (0) or high (1, default) */
> - int ppr; /* Pulses per rotation 1..4 (default == 2) */
> - int rpm_range; /* 2000, 4000 (default), 8000 or 16000 */
> -};
> -
> -#endif /* _LINUX_MAX6639_H */
>
> base-commit: 52c1e818d66bfed276bd371f9e7947be4055af87
Powered by blists - more mailing lists