| Message ID | 20250324185744.2421462-1-you@example.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (max6639) : Allow setting target RPM | expand |
On 3/24/25 11:57, Your Name wrote: > From: Naresh Solanki <naresh.solanki@9elements.com> > > Currently, during startup, the fan is set to its maximum RPM by default, > which may not be suitable for all use cases. > This patch introduces support for specifying a target RPM via the Device > Tree property "target-rpm". > > Changes: > - Added `target_rpm` field to `max6639_data` structure to store the > target RPM for each fan channel. > - Modified `max6639_probe_child_from_dt()` to read the `"target-rpm"` > property from the Device Tree and set `target_rpm` accordingly. > - Updated `max6639_init_client()` to use `target_rpm` to compute the > initial PWM duty cycle instead of defaulting to full speed (120/120). > > Behavior: > - If `"target-rpm"` is specified, the fan speed is set accordingly. > - If `"target-rpm"` is not specified, the previous behavior (full speed > at startup) is retained. > Unless I am missing something, that is not really correct. See below. > This allows better control over fan speed during system initialization. > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > --- > drivers/hwmon/max6639.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c > index 32b4d54b2076..ca8a8f58d133 100644 > --- a/drivers/hwmon/max6639.c > +++ b/drivers/hwmon/max6639.c > @@ -80,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 */ > + u32 target_rpm[MAX6639_NUM_CHANNELS]; > > /* Optional regulator for FAN supply */ > struct regulator *reg; > @@ -560,8 +561,14 @@ static int max6639_probe_child_from_dt(struct i2c_client *client, > } > target_rpm[] is 0 here. > err = of_property_read_u32(child, "max-rpm", &val); > - if (!err) > + if (!err) { > data->rpm_range[i] = rpm_range_to_reg(val); > + data->target_rpm[i] = val; > + } If there is no max-rpm property, or if there is no devicetree support, target_rpm[i] is still 0. > + > + err = of_property_read_u32(child, "target-rpm", &val); > + if (!err) > + data->target_rpm[i] = val; If there is neither max-rpm nor target-rpm, target_rpm[i] is still 0. > > return 0; > } > @@ -573,6 +580,7 @@ static int max6639_init_client(struct i2c_client *client, > const struct device_node *np = dev->of_node; > struct device_node *child; > int i, err; > + u8 target_duty; > > /* Reset chip to default values, see below for GCONFIG setup */ > err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR); > @@ -639,8 +647,9 @@ static int max6639_init_client(struct i2c_client *client, > if (err) > return err; > > - /* PWM 120/120 (i.e. 100%) */ > - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), 120); > + /* Set PWM based on target RPM if specified */ > + target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; If there is no devicetree support, or if neither max-rpm nor target-rpm are provided, target_duty will be 0, and the fans will stop. Maybe my interpretation is wrong, but I think both target_rpm[] and rpm_range[] will need to be initialized. Also, it seems to me that there will need to be an upper bound for target_rpm[]; without it, it is possible that target_duty > 120, which would probably not be a good idea. Guenter > + err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), target_duty); > if (err) > return err; > } > > base-commit: 2115cbeec8a3ccc69e3b7ecdf97b4472b0829cfc
Hi Guenter, On Tue, 25 Mar 2025 at 05:00, Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/24/25 11:57, Your Name wrote: > > From: Naresh Solanki <naresh.solanki@9elements.com> > > > > Currently, during startup, the fan is set to its maximum RPM by default, > > which may not be suitable for all use cases. > > This patch introduces support for specifying a target RPM via the Device > > Tree property "target-rpm". > > > > Changes: > > - Added `target_rpm` field to `max6639_data` structure to store the > > target RPM for each fan channel. > > - Modified `max6639_probe_child_from_dt()` to read the `"target-rpm"` > > property from the Device Tree and set `target_rpm` accordingly. > > - Updated `max6639_init_client()` to use `target_rpm` to compute the > > initial PWM duty cycle instead of defaulting to full speed (120/120). > > > > Behavior: > > - If `"target-rpm"` is specified, the fan speed is set accordingly. > > - If `"target-rpm"` is not specified, the previous behavior (full speed > > at startup) is retained. > > > > Unless I am missing something, that is not really correct. See below. > > > This allows better control over fan speed during system initialization. > > > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > > --- > > drivers/hwmon/max6639.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c > > index 32b4d54b2076..ca8a8f58d133 100644 > > --- a/drivers/hwmon/max6639.c > > +++ b/drivers/hwmon/max6639.c > > @@ -80,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 */ > > + u32 target_rpm[MAX6639_NUM_CHANNELS]; > > > > /* Optional regulator for FAN supply */ > > struct regulator *reg; > > @@ -560,8 +561,14 @@ static int max6639_probe_child_from_dt(struct i2c_client *client, > > } > > > > target_rpm[] is 0 here. > > > err = of_property_read_u32(child, "max-rpm", &val); > > - if (!err) > > + if (!err) { > > data->rpm_range[i] = rpm_range_to_reg(val); > > + data->target_rpm[i] = val; > > + } > > If there is no max-rpm property, or if there is no devicetree support, > target_rpm[i] is still 0. > > > + > > + err = of_property_read_u32(child, "target-rpm", &val); > > + if (!err) > > + data->target_rpm[i] = val; > > If there is neither max-rpm nor target-rpm, target_rpm[i] is still 0. > > > > > return 0; > > } > > @@ -573,6 +580,7 @@ static int max6639_init_client(struct i2c_client *client, > > const struct device_node *np = dev->of_node; > > struct device_node *child; > > int i, err; > > + u8 target_duty; > > > > /* Reset chip to default values, see below for GCONFIG setup */ > > err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR); > > @@ -639,8 +647,9 @@ static int max6639_init_client(struct i2c_client *client, > > if (err) > > return err; > > > > - /* PWM 120/120 (i.e. 100%) */ > > - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), 120); > > + /* Set PWM based on target RPM if specified */ > > + target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; > > If there is no devicetree support, or if neither max-rpm nor target-rpm are > provided, target_duty will be 0, and the fans will stop. > > Maybe my interpretation is wrong, but I think both target_rpm[] and rpm_range[] > will need to be initialized. Also, it seems to me that there will need to be an > upper bound for target_rpm[]; without it, it is possible that target_duty > 120, > which would probably not be a good idea. Yes you're right. I missed it in my analysis. Here is the logic that would address: target_rpm = 120; /* Set PWM based on target RPM if specified */ if (data->target_rpm[i] != 0 && data->target_rpm[i] <= rpm_ranges[data->rpm_range[i]]) { target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; } Please let me know your thoughts & suggestions. Regards, Naresh > > Guenter > > > + err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), target_duty); > > if (err) > > return err; > > } > > > > base-commit: 2115cbeec8a3ccc69e3b7ecdf97b4472b0829cfc >
On 3/26/25 09:36, Naresh Solanki wrote: > Hi Guenter, > > On Tue, 25 Mar 2025 at 05:00, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 3/24/25 11:57, Your Name wrote: >>> From: Naresh Solanki <naresh.solanki@9elements.com> >>> >>> Currently, during startup, the fan is set to its maximum RPM by default, >>> which may not be suitable for all use cases. >>> This patch introduces support for specifying a target RPM via the Device >>> Tree property "target-rpm". >>> >>> Changes: >>> - Added `target_rpm` field to `max6639_data` structure to store the >>> target RPM for each fan channel. >>> - Modified `max6639_probe_child_from_dt()` to read the `"target-rpm"` >>> property from the Device Tree and set `target_rpm` accordingly. >>> - Updated `max6639_init_client()` to use `target_rpm` to compute the >>> initial PWM duty cycle instead of defaulting to full speed (120/120). >>> >>> Behavior: >>> - If `"target-rpm"` is specified, the fan speed is set accordingly. >>> - If `"target-rpm"` is not specified, the previous behavior (full speed >>> at startup) is retained. >>> >> >> Unless I am missing something, that is not really correct. See below. >> >>> This allows better control over fan speed during system initialization. >>> >>> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> >>> --- >>> drivers/hwmon/max6639.c | 15 ++++++++++++--- >>> 1 file changed, 12 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c >>> index 32b4d54b2076..ca8a8f58d133 100644 >>> --- a/drivers/hwmon/max6639.c >>> +++ b/drivers/hwmon/max6639.c >>> @@ -80,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 */ >>> + u32 target_rpm[MAX6639_NUM_CHANNELS]; >>> >>> /* Optional regulator for FAN supply */ >>> struct regulator *reg; >>> @@ -560,8 +561,14 @@ static int max6639_probe_child_from_dt(struct i2c_client *client, >>> } >>> >> >> target_rpm[] is 0 here. >> >>> err = of_property_read_u32(child, "max-rpm", &val); >>> - if (!err) >>> + if (!err) { >>> data->rpm_range[i] = rpm_range_to_reg(val); >>> + data->target_rpm[i] = val; >>> + } >> >> If there is no max-rpm property, or if there is no devicetree support, >> target_rpm[i] is still 0. >> >>> + >>> + err = of_property_read_u32(child, "target-rpm", &val); >>> + if (!err) >>> + data->target_rpm[i] = val; >> >> If there is neither max-rpm nor target-rpm, target_rpm[i] is still 0. >> >>> >>> return 0; >>> } >>> @@ -573,6 +580,7 @@ static int max6639_init_client(struct i2c_client *client, >>> const struct device_node *np = dev->of_node; >>> struct device_node *child; >>> int i, err; >>> + u8 target_duty; >>> >>> /* Reset chip to default values, see below for GCONFIG setup */ >>> err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR); >>> @@ -639,8 +647,9 @@ static int max6639_init_client(struct i2c_client *client, >>> if (err) >>> return err; >>> >>> - /* PWM 120/120 (i.e. 100%) */ >>> - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), 120); >>> + /* Set PWM based on target RPM if specified */ >>> + target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; >> >> If there is no devicetree support, or if neither max-rpm nor target-rpm are >> provided, target_duty will be 0, and the fans will stop. >> >> Maybe my interpretation is wrong, but I think both target_rpm[] and rpm_range[] >> will need to be initialized. Also, it seems to me that there will need to be an >> upper bound for target_rpm[]; without it, it is possible that target_duty > 120, >> which would probably not be a good idea. > Yes you're right. I missed it in my analysis. > > Here is the logic that would address: > target_rpm = 120; > /* Set PWM based on target RPM if specified */ > if (data->target_rpm[i] != 0 && > data->target_rpm[i] <= rpm_ranges[data->rpm_range[i]]) { > > target_duty = 120 * data->target_rpm[i] / > rpm_ranges[data->rpm_range[i]]; > } > > Please let me know your thoughts & suggestions. > I would prefer if target_rpm[] and rpm_range[] were pre-initialized with default values in the probe function. That would avoid runtime checks. Thanks, Guenter
Hi Guenter, On Wed, 26 Mar 2025 at 22:19, Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/26/25 09:36, Naresh Solanki wrote: > > Hi Guenter, > > > > On Tue, 25 Mar 2025 at 05:00, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 3/24/25 11:57, Your Name wrote: > >>> From: Naresh Solanki <naresh.solanki@9elements.com> > >>> > >>> Currently, during startup, the fan is set to its maximum RPM by default, > >>> which may not be suitable for all use cases. > >>> This patch introduces support for specifying a target RPM via the Device > >>> Tree property "target-rpm". > >>> > >>> Changes: > >>> - Added `target_rpm` field to `max6639_data` structure to store the > >>> target RPM for each fan channel. > >>> - Modified `max6639_probe_child_from_dt()` to read the `"target-rpm"` > >>> property from the Device Tree and set `target_rpm` accordingly. > >>> - Updated `max6639_init_client()` to use `target_rpm` to compute the > >>> initial PWM duty cycle instead of defaulting to full speed (120/120). > >>> > >>> Behavior: > >>> - If `"target-rpm"` is specified, the fan speed is set accordingly. > >>> - If `"target-rpm"` is not specified, the previous behavior (full speed > >>> at startup) is retained. > >>> > >> > >> Unless I am missing something, that is not really correct. See below. > >> > >>> This allows better control over fan speed during system initialization. > >>> > >>> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > >>> --- > >>> drivers/hwmon/max6639.c | 15 ++++++++++++--- > >>> 1 file changed, 12 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c > >>> index 32b4d54b2076..ca8a8f58d133 100644 > >>> --- a/drivers/hwmon/max6639.c > >>> +++ b/drivers/hwmon/max6639.c > >>> @@ -80,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 */ > >>> + u32 target_rpm[MAX6639_NUM_CHANNELS]; > >>> > >>> /* Optional regulator for FAN supply */ > >>> struct regulator *reg; > >>> @@ -560,8 +561,14 @@ static int max6639_probe_child_from_dt(struct i2c_client *client, > >>> } > >>> > >> > >> target_rpm[] is 0 here. > >> > >>> err = of_property_read_u32(child, "max-rpm", &val); > >>> - if (!err) > >>> + if (!err) { > >>> data->rpm_range[i] = rpm_range_to_reg(val); > >>> + data->target_rpm[i] = val; > >>> + } > >> > >> If there is no max-rpm property, or if there is no devicetree support, > >> target_rpm[i] is still 0. > >> > >>> + > >>> + err = of_property_read_u32(child, "target-rpm", &val); > >>> + if (!err) > >>> + data->target_rpm[i] = val; > >> > >> If there is neither max-rpm nor target-rpm, target_rpm[i] is still 0. > >> > >>> > >>> return 0; > >>> } > >>> @@ -573,6 +580,7 @@ static int max6639_init_client(struct i2c_client *client, > >>> const struct device_node *np = dev->of_node; > >>> struct device_node *child; > >>> int i, err; > >>> + u8 target_duty; > >>> > >>> /* Reset chip to default values, see below for GCONFIG setup */ > >>> err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR); > >>> @@ -639,8 +647,9 @@ static int max6639_init_client(struct i2c_client *client, > >>> if (err) > >>> return err; > >>> > >>> - /* PWM 120/120 (i.e. 100%) */ > >>> - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), 120); > >>> + /* Set PWM based on target RPM if specified */ > >>> + target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; > >> > >> If there is no devicetree support, or if neither max-rpm nor target-rpm are > >> provided, target_duty will be 0, and the fans will stop. > >> > >> Maybe my interpretation is wrong, but I think both target_rpm[] and rpm_range[] > >> will need to be initialized. Also, it seems to me that there will need to be an > >> upper bound for target_rpm[]; without it, it is possible that target_duty > 120, > >> which would probably not be a good idea. > > Yes you're right. I missed it in my analysis. > > > > Here is the logic that would address: > > target_rpm = 120; > > /* Set PWM based on target RPM if specified */ > > if (data->target_rpm[i] != 0 && > > data->target_rpm[i] <= rpm_ranges[data->rpm_range[i]]) { > > > > target_duty = 120 * data->target_rpm[i] / > > rpm_ranges[data->rpm_range[i]]; > > } > > > > Please let me know your thoughts & suggestions. > > > > I would prefer if target_rpm[] and rpm_range[] were pre-initialized with default > values in the probe function. That would avoid runtime checks. rpm_range is pre-initialized to 4000 RPM [1] I can also init target_rpm[] to 4000 RPM as default along with above init. But still there might be a case wherein DT doesn't provide max-rpm but target-rpm is set to greater than 4000 RPM Thus there will be a need to check to cover this kind of scenario. Please let me know your thoughts & will implement that. [1]https://kernel.googlesource.com/pub/scm/linux/kernel/git/groeck/linux-staging/+/hwmon-next/drivers/hwmon/max6639.c#586 Regards, Naresh > > Thanks, > Guenter >
On 3/26/25 09:59, Naresh Solanki wrote: > Hi Guenter, > > On Wed, 26 Mar 2025 at 22:19, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 3/26/25 09:36, Naresh Solanki wrote: >>> Hi Guenter, >>> >>> On Tue, 25 Mar 2025 at 05:00, Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On 3/24/25 11:57, Your Name wrote: >>>>> From: Naresh Solanki <naresh.solanki@9elements.com> >>>>> >>>>> Currently, during startup, the fan is set to its maximum RPM by default, >>>>> which may not be suitable for all use cases. >>>>> This patch introduces support for specifying a target RPM via the Device >>>>> Tree property "target-rpm". >>>>> >>>>> Changes: >>>>> - Added `target_rpm` field to `max6639_data` structure to store the >>>>> target RPM for each fan channel. >>>>> - Modified `max6639_probe_child_from_dt()` to read the `"target-rpm"` >>>>> property from the Device Tree and set `target_rpm` accordingly. >>>>> - Updated `max6639_init_client()` to use `target_rpm` to compute the >>>>> initial PWM duty cycle instead of defaulting to full speed (120/120). >>>>> >>>>> Behavior: >>>>> - If `"target-rpm"` is specified, the fan speed is set accordingly. >>>>> - If `"target-rpm"` is not specified, the previous behavior (full speed >>>>> at startup) is retained. >>>>> >>>> >>>> Unless I am missing something, that is not really correct. See below. >>>> >>>>> This allows better control over fan speed during system initialization. >>>>> >>>>> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> >>>>> --- >>>>> drivers/hwmon/max6639.c | 15 ++++++++++++--- >>>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c >>>>> index 32b4d54b2076..ca8a8f58d133 100644 >>>>> --- a/drivers/hwmon/max6639.c >>>>> +++ b/drivers/hwmon/max6639.c >>>>> @@ -80,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 */ >>>>> + u32 target_rpm[MAX6639_NUM_CHANNELS]; >>>>> >>>>> /* Optional regulator for FAN supply */ >>>>> struct regulator *reg; >>>>> @@ -560,8 +561,14 @@ static int max6639_probe_child_from_dt(struct i2c_client *client, >>>>> } >>>>> >>>> >>>> target_rpm[] is 0 here. >>>> >>>>> err = of_property_read_u32(child, "max-rpm", &val); >>>>> - if (!err) >>>>> + if (!err) { >>>>> data->rpm_range[i] = rpm_range_to_reg(val); >>>>> + data->target_rpm[i] = val; >>>>> + } >>>> >>>> If there is no max-rpm property, or if there is no devicetree support, >>>> target_rpm[i] is still 0. >>>> >>>>> + >>>>> + err = of_property_read_u32(child, "target-rpm", &val); >>>>> + if (!err) >>>>> + data->target_rpm[i] = val; >>>> >>>> If there is neither max-rpm nor target-rpm, target_rpm[i] is still 0. >>>> >>>>> >>>>> return 0; >>>>> } >>>>> @@ -573,6 +580,7 @@ static int max6639_init_client(struct i2c_client *client, >>>>> const struct device_node *np = dev->of_node; >>>>> struct device_node *child; >>>>> int i, err; >>>>> + u8 target_duty; >>>>> >>>>> /* Reset chip to default values, see below for GCONFIG setup */ >>>>> err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR); >>>>> @@ -639,8 +647,9 @@ static int max6639_init_client(struct i2c_client *client, >>>>> if (err) >>>>> return err; >>>>> >>>>> - /* PWM 120/120 (i.e. 100%) */ >>>>> - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), 120); >>>>> + /* Set PWM based on target RPM if specified */ >>>>> + target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; >>>> >>>> If there is no devicetree support, or if neither max-rpm nor target-rpm are >>>> provided, target_duty will be 0, and the fans will stop. >>>> >>>> Maybe my interpretation is wrong, but I think both target_rpm[] and rpm_range[] >>>> will need to be initialized. Also, it seems to me that there will need to be an >>>> upper bound for target_rpm[]; without it, it is possible that target_duty > 120, >>>> which would probably not be a good idea. >>> Yes you're right. I missed it in my analysis. >>> >>> Here is the logic that would address: >>> target_rpm = 120; >>> /* Set PWM based on target RPM if specified */ >>> if (data->target_rpm[i] != 0 && >>> data->target_rpm[i] <= rpm_ranges[data->rpm_range[i]]) { >>> >>> target_duty = 120 * data->target_rpm[i] / >>> rpm_ranges[data->rpm_range[i]]; >>> } >>> >>> Please let me know your thoughts & suggestions. >>> >> >> I would prefer if target_rpm[] and rpm_range[] were pre-initialized with default >> values in the probe function. That would avoid runtime checks. > rpm_range is pre-initialized to 4000 RPM [1] > I can also init target_rpm[] to 4000 RPM as default along with above init. > Yes. > But still there might be a case wherein DT doesn't provide max-rpm but > target-rpm is set to greater than 4000 RPM > Thus there will be a need to check to cover this kind of scenario. > > Please let me know your thoughts & will implement that. > You'll need to validate target_rpm against max-rpm either way, to make sure that target_rpm <= max_rpm. Thanks, Guenter
Hi Guenter, On Wed, 26 Mar 2025 at 23:12, Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/26/25 09:59, Naresh Solanki wrote: > > Hi Guenter, > > > > On Wed, 26 Mar 2025 at 22:19, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 3/26/25 09:36, Naresh Solanki wrote: > >>> Hi Guenter, > >>> > >>> On Tue, 25 Mar 2025 at 05:00, Guenter Roeck <linux@roeck-us.net> wrote: > >>>> > >>>> On 3/24/25 11:57, Your Name wrote: > >>>>> From: Naresh Solanki <naresh.solanki@9elements.com> > >>>>> > >>>>> Currently, during startup, the fan is set to its maximum RPM by default, > >>>>> which may not be suitable for all use cases. > >>>>> This patch introduces support for specifying a target RPM via the Device > >>>>> Tree property "target-rpm". > >>>>> > >>>>> Changes: > >>>>> - Added `target_rpm` field to `max6639_data` structure to store the > >>>>> target RPM for each fan channel. > >>>>> - Modified `max6639_probe_child_from_dt()` to read the `"target-rpm"` > >>>>> property from the Device Tree and set `target_rpm` accordingly. > >>>>> - Updated `max6639_init_client()` to use `target_rpm` to compute the > >>>>> initial PWM duty cycle instead of defaulting to full speed (120/120). > >>>>> > >>>>> Behavior: > >>>>> - If `"target-rpm"` is specified, the fan speed is set accordingly. > >>>>> - If `"target-rpm"` is not specified, the previous behavior (full speed > >>>>> at startup) is retained. > >>>>> > >>>> > >>>> Unless I am missing something, that is not really correct. See below. > >>>> > >>>>> This allows better control over fan speed during system initialization. > >>>>> > >>>>> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > >>>>> --- > >>>>> drivers/hwmon/max6639.c | 15 ++++++++++++--- > >>>>> 1 file changed, 12 insertions(+), 3 deletions(-) > >>>>> > >>>>> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c > >>>>> index 32b4d54b2076..ca8a8f58d133 100644 > >>>>> --- a/drivers/hwmon/max6639.c > >>>>> +++ b/drivers/hwmon/max6639.c > >>>>> @@ -80,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 */ > >>>>> + u32 target_rpm[MAX6639_NUM_CHANNELS]; > >>>>> > >>>>> /* Optional regulator for FAN supply */ > >>>>> struct regulator *reg; > >>>>> @@ -560,8 +561,14 @@ static int max6639_probe_child_from_dt(struct i2c_client *client, > >>>>> } > >>>>> > >>>> > >>>> target_rpm[] is 0 here. > >>>> > >>>>> err = of_property_read_u32(child, "max-rpm", &val); > >>>>> - if (!err) > >>>>> + if (!err) { > >>>>> data->rpm_range[i] = rpm_range_to_reg(val); > >>>>> + data->target_rpm[i] = val; > >>>>> + } > >>>> > >>>> If there is no max-rpm property, or if there is no devicetree support, > >>>> target_rpm[i] is still 0. > >>>> > >>>>> + > >>>>> + err = of_property_read_u32(child, "target-rpm", &val); > >>>>> + if (!err) > >>>>> + data->target_rpm[i] = val; > >>>> > >>>> If there is neither max-rpm nor target-rpm, target_rpm[i] is still 0. > >>>> > >>>>> > >>>>> return 0; > >>>>> } > >>>>> @@ -573,6 +580,7 @@ static int max6639_init_client(struct i2c_client *client, > >>>>> const struct device_node *np = dev->of_node; > >>>>> struct device_node *child; > >>>>> int i, err; > >>>>> + u8 target_duty; > >>>>> > >>>>> /* Reset chip to default values, see below for GCONFIG setup */ > >>>>> err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR); > >>>>> @@ -639,8 +647,9 @@ static int max6639_init_client(struct i2c_client *client, > >>>>> if (err) > >>>>> return err; > >>>>> > >>>>> - /* PWM 120/120 (i.e. 100%) */ > >>>>> - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), 120); > >>>>> + /* Set PWM based on target RPM if specified */ > >>>>> + target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; > >>>> > >>>> If there is no devicetree support, or if neither max-rpm nor target-rpm are > >>>> provided, target_duty will be 0, and the fans will stop. > >>>> > >>>> Maybe my interpretation is wrong, but I think both target_rpm[] and rpm_range[] > >>>> will need to be initialized. Also, it seems to me that there will need to be an > >>>> upper bound for target_rpm[]; without it, it is possible that target_duty > 120, > >>>> which would probably not be a good idea. > >>> Yes you're right. I missed it in my analysis. > >>> > >>> Here is the logic that would address: > >>> target_rpm = 120; > >>> /* Set PWM based on target RPM if specified */ > >>> if (data->target_rpm[i] != 0 && > >>> data->target_rpm[i] <= rpm_ranges[data->rpm_range[i]]) { > >>> > >>> target_duty = 120 * data->target_rpm[i] / > >>> rpm_ranges[data->rpm_range[i]]; > >>> } > >>> > >>> Please let me know your thoughts & suggestions. > >>> > >> > >> I would prefer if target_rpm[] and rpm_range[] were pre-initialized with default > >> values in the probe function. That would avoid runtime checks. > > rpm_range is pre-initialized to 4000 RPM [1] > > I can also init target_rpm[] to 4000 RPM as default along with above init. > > > Yes. > > > But still there might be a case wherein DT doesn't provide max-rpm but > > target-rpm is set to greater than 4000 RPM > > Thus there will be a need to check to cover this kind of scenario. > > > > Please let me know your thoughts & will implement that. > > > > You'll need to validate target_rpm against max-rpm either way, > to make sure that target_rpm <= max_rpm. Will update in V2 accordingly. Thanks for your inputs. Regards, Naresh > > Thanks, > Guenter >
diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c index 32b4d54b2076..ca8a8f58d133 100644 --- a/drivers/hwmon/max6639.c +++ b/drivers/hwmon/max6639.c @@ -80,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 */ + u32 target_rpm[MAX6639_NUM_CHANNELS]; /* Optional regulator for FAN supply */ struct regulator *reg; @@ -560,8 +561,14 @@ static int max6639_probe_child_from_dt(struct i2c_client *client, } err = of_property_read_u32(child, "max-rpm", &val); - if (!err) + if (!err) { data->rpm_range[i] = rpm_range_to_reg(val); + data->target_rpm[i] = val; + } + + err = of_property_read_u32(child, "target-rpm", &val); + if (!err) + data->target_rpm[i] = val; return 0; } @@ -573,6 +580,7 @@ static int max6639_init_client(struct i2c_client *client, const struct device_node *np = dev->of_node; struct device_node *child; int i, err; + u8 target_duty; /* Reset chip to default values, see below for GCONFIG setup */ err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, MAX6639_GCONFIG_POR); @@ -639,8 +647,9 @@ static int max6639_init_client(struct i2c_client *client, if (err) return err; - /* PWM 120/120 (i.e. 100%) */ - err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), 120); + /* Set PWM based on target RPM if specified */ + target_duty = 120 * data->target_rpm[i] / rpm_ranges[data->rpm_range[i]]; + err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), target_duty); if (err) return err; }