Re: [PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 11/5/19 11:46 AM, Ahmad Fatoum wrote:
> Hi,
> 
> On 11/5/19 11:40 AM, Sascha Hauer wrote:
>> When we just started the watchdog we actually know that it is running,
>> so we could support the parameter for all watchdogs once it's started.

I'll do this.

>> Casting p->value to (struct watchdog *) seems wrong. However, this
>> function shouldn't be needed, see below.

It's well-defined, but ye, it feels a bit wrong.

>>> +
>>>  static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
>>>  {
>>>  	wd->dev.parent = wd->hwdev;
>>> @@ -162,6 +184,15 @@ int watchdog_register(struct watchdog *wd)
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
>>> +		p = dev_add_param(&wd->dev, "running", NULL,
>>> +				  watchdog_get_running, PARAM_FLAG_RO);
>>> +		if (IS_ERR(p))
>>> +			return PTR_ERR(p);
>>> +
>>> +		p->value = (char *)wd;
>>> +	}
>>
>> How about adding this parameter unconditionally, with "unknown" as value
>> when WDOG_HW_RUNNING_SUPPORTED is not set. You can use
>> dev_add_param_enum() here.

Thinking about it, I'd rather not use dev_add_param_enum here. I would like
to leave the driver-side API not too different from Linux, i.e. status bits
that can be set. If I use dev_add_param_enum, I'll have to add a new struct
member for every new status bit. I can respin the patch with an unconditional
dev_add_param though.

> 
> Sounds good.
> 
>>
>> Sascha
>>
> 


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox