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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
