[PATCH linux dev-4.10] drivers/watchdog: Add user space access to aspeed ext signal and reset

[Christopher Bostic]

On 5/23/17 5:49 PM, Andrew Jeffery wrote:
> On Tue, 2017-05-23 at 16:28 -0500, Christopher Bostic wrote:
>> Add sysfs files for 'ext signal' mode and 'system reset' mode within
>> the aspeed watchdog device driver for wdt1 and wdt2
> This commit message explains what the patch does, not why we need it.
> The code change already describes what it does, so can you please
> instead outline in the commit message what the motivation is (or
> potential motivations are). Ultimately what I'm interested in is
> whether this should be a platform design decision (and so should be
> managed via devicetree), or userspace policy, which is what we have
> here.
>
> Separately, given it's fairly generic and assuming the motivation is
> reasonable, I'd suggest also sending it upstream.

Hi Andrew,

Will change the 'what' to 'why' as per your recommendations.

> Andrew
>
>>> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
>> + wdt->ctrl |= WDT_CTRL_WDT_EXT;
>>> +		writel(wdt->ctrl, wdt->base + WDT_CTRL);
>>> +	} else if (!strncmp(buf, "off", strlen("off"))) {
>>> +		wdt->ctrl &= ~WDT_CTRL_WDT_EXT;
>>> +		writel(wdt->ctrl, wdt->base + WDT_CTRL);
>>> +	} else {
>>> +		dev_warn(dev, "Unknown reset system mode command: %s\n", buf);
>>> +		return -EINVAL;
>>> +	}
>> +
>>> +	return count;
>> +}
>> +
>> +static ssize_t aspeed_wdt_ext_signal_show(struct device *dev,
>>> +					  struct device_attribute *attr,
>>> +					  char *buf)
>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct aspeed_wdt *wdt = platform_get_drvdata(pdev);
>>> +	int ret;
>> +
>>> +	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_WDT_EXT)
>>> +		ret = sprintf(buf, "on\n");
>>> +	else
>>> +		ret = sprintf(buf, "off\n");
>> +
>>> +	return ret;
>> +}
>> +
>> +static DEVICE_ATTR(ext_signal, 0644,
>> +		   aspeed_wdt_ext_signal_show, aspeed_wdt_ext_signal_store);
> The on/off implementations for show/store callbacks are very similar,
> only differentiated by the bit that we're flipping. We could almost cut
> the code in half by parameterising a function with the bit of interest
> and invoking it appropriately in the callback handlers. Thoughts?

True, will optimize this.

Thanks,
Chris

>
> Andrew
>
>> +
>> +static struct attribute *aspeed_wdt_attrs[] = {
>>> +	&dev_attr_reset_system.attr,
>>> +	&dev_attr_ext_signal.attr,
>>> +	NULL
>> +};
>> +ATTRIBUTE_GROUPS(aspeed_wdt);
>> +
>>   static int aspeed_wdt_probe(struct platform_device *pdev)
>>   {
>>>   	struct aspeed_wdt *wdt;
>> @@ -169,7 +256,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>>>   	wdt->wdd.ops = &aspeed_wdt_ops;
>>>   	wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS;
>>>   	wdt->wdd.parent = &pdev->dev;
>> -
>>> +	wdt->wdd.groups = aspeed_wdt_groups;
>>>   	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
>>>   	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
>>