Hi Guenter,
Sorry for the late reply,
On Fri, 28 Jun 2024 at 20:30, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> 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.
I agree with you. It is best to have some defaults & then overwrite
based on DT properties.
But in that case we would have to assume that all fans are enabled
irrespective of their hardware connections in the schematics(example
fan_enable).
I'm not sure if that is fine. But if you feel that is alright then
I'll rewrite the driver to assume
everything is enabled with default values.
Later based on DT properties, we just overwrite defaults to align with hardware.
For commit message I'll update it as :
hwmon: (max6639) : Configure based on DT property
Remove platform data & initialize driver with defaults & overwrite based on
DT properties.
>
> More comments inline.
>
> > Signed-off-by: Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>
> > ---
> > 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.
Agree. Will remove this change.
>
> > 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.
Ack.
Will make variable 'i' as int.
Also will update if check with:
if (val < 1 || val > 5) {
>
> > + 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.
Somewhere I was with the assumption that there is no active use of the driver
as there is no reference to this driver.
That is why I felt these changes will be good.
i.e., DT property would indicate if Fan0 or Fan1 should be enabled or not
instead of assuming that they are always enabled.
I would need your input here to proceed further.
Regards,
Naresh
>
> > + 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
>
