Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 27 Nov 2020 00:42:47 +0900
From:   Dongjin Kim <tobetter@gmail.com>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
        Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: (pwm-fan) add fan pwm1_enable attribute
Message-ID: <20201126154247.GA24875@paju>
References: <20201125163242.GA1264232@paju>
 <20201126020536.GE111386@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201126020536.GE111386@roeck-us.net>
Precedence: bulk

On Wed, Nov 25, 2020 at 06:05:36PM -0800, Guenter Roeck wrote:
> On Thu, Nov 26, 2020 at 01:32:42AM +0900, Dongjin Kim wrote:
> > This patch adds to new attribute 'pwm1_enable' to change the fan speed
> > control method as documented in 'Documentation/hwmon/sysfs-interface'.
> > 
> > Signed-off-by: Dongjin Kim <tobetter@gmail.com>
> 
> The new attribute needs to be documented in
> Documentation/hwmon/pwm-fan.rst, with supported values.
> 
> > ---
> >  drivers/hwmon/pwm-fan.c | 52 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 46 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index 1f63807c0399..834275309421 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -39,6 +39,7 @@ struct pwm_fan_ctx {
> >  	unsigned int pwm_fan_max_state;
> >  	unsigned int *pwm_fan_cooling_levels;
> >  	struct thermal_cooling_device *cdev;
> > +	int enable;
> >  };
> >  
> >  /* This handler assumes self resetting edge triggered interrupt. */
> > @@ -76,6 +77,10 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> >  	struct pwm_state state = { };
> >  
> >  	mutex_lock(&ctx->lock);
> > +
> > +	if (ctx->enable == 0)
> > +		pwm = MAX_PWM;
> > +
> >  	if (ctx->pwm_value == pwm)
> >  		goto exit_set_pwm_err;
> >  
> > @@ -137,11 +142,42 @@ static ssize_t rpm_show(struct device *dev,
> >  	return sprintf(buf, "%u\n", ctx->rpm);
> >  }
> >  
> > +static ssize_t enable_store(struct device *dev,
> > +		struct device_attribute *attr,
> > +		const char *buf, size_t count)
> > +{
> > +	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > +	int err;
> > +	unsigned long val;
> > +
> > +	err = kstrtoul(buf, 10, &val);
> > +	if (err)
> > +		return err;
> > +
> 
> 'val' must be validated and only accept permitted values.
Sure.
> 
> > +	mutex_lock(&ctx->lock);
> > +	ctx->enable = val;
> > +	mutex_unlock(&ctx->lock);
> > +
> > +	err = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[ctx->pwm_fan_state]);
> > +
> > +	return err ? err : count;
> > +}
> > +
> > +static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%u\n", ctx->enable);
> > +}
> > +
> >  static SENSOR_DEVICE_ATTR_RW(pwm1, pwm, 0);
> > +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, enable, 0);
> >  static SENSOR_DEVICE_ATTR_RO(fan1_input, rpm, 0);
> >  
> >  static struct attribute *pwm_fan_attrs[] = {
> >  	&sensor_dev_attr_pwm1.dev_attr.attr,
> > +	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
> >  	&sensor_dev_attr_fan1_input.dev_attr.attr,
> >  	NULL,
> >  };
> > @@ -153,7 +189,7 @@ static umode_t pwm_fan_attrs_visible(struct kobject *kobj, struct attribute *a,
> >  	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> >  
> >  	/* Hide fan_input in case no interrupt is available  */
> > -	if (n == 1 && ctx->irq <= 0)
> > +	if (n == 2 && ctx->irq <= 0)
> >  		return 0;
> >  
> >  	return a->mode;
> > @@ -200,7 +236,7 @@ static int
> >  pwm_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> >  {
> >  	struct pwm_fan_ctx *ctx = cdev->devdata;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	if (!ctx || (state > ctx->pwm_fan_max_state))
> >  		return -EINVAL;
> > @@ -208,10 +244,12 @@ pwm_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> >  	if (state == ctx->pwm_fan_state)
> >  		return 0;
> >  
> > -	ret = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
> > -	if (ret) {
> > -		dev_err(&cdev->device, "Cannot set pwm!\n");
> > -		return ret;
> > +	if (ctx->enable >= 2) {
> 
> What is "automatic" here ? I don't see how this driver or the underlying
> pwm controller/chip would support automatic fan speed control. This is
> completely independent of thermal control: thermal device support does _not_
> imply or suggest "automatic" fan speed control from a pwm chip perspective.
> 
My understanding of 'automatic' is to set the fan speed by a thermal device
with the trip points, it changes the cooling state and change the fan speed.
Please correct me if I am wrong.

> This makes me also very concerned about this attribute in the first please:
> All it does, in its current implementation, is to disable thermal device 
> control. That is not the idea here, and it doesn't make any sense to me.
> On the contrary, all I can find is that it is dangerous.
> 
I don't intend to change the thermal control itself in 'manual' mode, but does
not change the fan speed with given cooling state. In 'automatic' mode, the
fan speed will be changed by the thermal device.

> Please describe in detail what you think this attribute is supposed to
> accomplish, and why you think it is needed or even makes sense. Overriding
> thermal control doesn't make sense to me: If you want that, just disable
> thermal control, or don't register this device as cooling device in the
> first place.  Other than that, the whole driver implies manual pwm control.
> That means pwm can already be "disabled" by writing a 0 pwm value. An
> additional attributes doesn't add value and only makes the driver ABI
> unnecessarily complex. So please prepare a detailed rationale to convince
> me otherwise.
>
I am using this driver to run a fan on ARM SBC and thought 'fancontrol' can
help to map the fan speed with pwm value if necessary instead of using the pwm
values in a device tree by writing '1' to 'pwm1_enable'. When this driver is
not registered as a cooling device, as you suggested, OS should provide the
default fancontrol config to run fully in manual mode if my understanding is
correct. Based on this what I am thinking is that OS images for ARM SBC should
have a fan control service or a tool if it allows user to change the fan speed.
But this could be very various for all SBC, I would prefer to provide the
default cooling device in the device tree to prevent the absence of fan
control due to the missing or wrong config.

> We can have a separate discussion if disabling a pwm controller using the
> hwmon ABI should disable that pwm controller from a thermal (cooling) device
> perspective. If so, we'll need to think about the implications. We can not
> just disable thermal cooling device support without telling the thermal
> subsystem that this means of temperature control doesn't work anymore.
> That should, however, be a completely separate discussion, independent
> of this driver, and it should include thermal subsystem maintainers
> and the thermal subsystem mailing list.
Ok, thank you for your advice and review. :)

Dongjin.
> 
> Thanks,
> Guenter
> 
> > +		ret = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
> > +		if (ret) {
> > +			dev_err(&cdev->device, "Cannot set pwm!\n");
> > +			return ret;
> > +		}
> >  	}
> >  
> >  	ctx->pwm_fan_state = state;
> > @@ -298,6 +336,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
> >  	if (IS_ERR(ctx->pwm))
> >  		return dev_err_probe(dev, PTR_ERR(ctx->pwm), "Could not get PWM\n");
> >  
> > +	ctx->enable = 2;
> > +
> >  	platform_set_drvdata(pdev, ctx);
> >  
> >  	ctx->irq = platform_get_irq_optional(pdev, 0);
> > -- 
> > 2.25.1
> >