Re: [PATCH v4] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Guenter Roeck <linux@roeck-us.net>]

On 11/30/2015 09:26 AM, Martyn Welch wrote:
> On 30/11/15 15:55, Guenter Roeck wrote:
>> Hi Martyn,
>>
>> On 11/25/2015 09:15 AM, Martyn Welch wrote:
>>> This patch adds a driver for the Zodiac Aerospace RAVE Watchdog Procesor.
>>> This device implements a watchdog timer, accessible over I2C.
>>>
>> Mostly there, still a few nitpicks and a problem with DT properties.
>>
[ ... ]

>>> +
>>> +static int ziirave_wdt_init_duration(struct i2c_client *client, int
>>> duration)
>>> +{

Separate note: The "duration" parameter here doesn't really add value.
It just reflects the module parameter. It would only make sense if the code
to obtain the duration (either from reset_duration or from DT)
would be in a separate function, such as in

	duration = ziirave_get_duration(client);
	if (duration < 0)
		return duration;
	if (duration) {
		ret = ziirave_wdt_init_duration(client, duration);
		if (ret)
			return ret;
	}

or similar.

>>> +    int ret;
>>> +
>>> +    if (!duration) {
>>> +        /* See if the reset pulse duration is provided in an of_node */
>>> +        if (client->dev.of_node == NULL)
>>> +            return -ENODEV;
>>> +
>> Please write as "if (!client->dev.of_node)" (see checkpatch --strict).
>>
>
> ok
>
>> Also, is it really necessary to mandate DT support ? There is no mandatory
>> property to be read, so this driver would (or should) work fine on a non-DT
>> system.
>>
>
> The error doesn't cause the probe to fail. If it takes that path, it results in a message that the default value is being used and continues with that.
>
>>> +        ret = of_property_read_u32(client->dev.of_node,
>>> +                       "reset-duration-ms", &duration);
>>
>> reset-duration-ms is listed as optional property, thus it should not be
>> an error
>> if it is not provided. Can you set some default in this case ? Or do
>> nothing ?
>>
>
> It takes module parameter, DT entry or falls back to built in default.
>

Hmm, you are right. Problem with that though is that it also accepts _invalid_
parameters.

I think it would be better to only return an error if the value is really wrong,
and abort in that case. Otherwise a duration of, say, 1000, will only result
in "unable to set duration", which is likely going to be overlooked.

The current code not distinguish between "no duration provided", "bad duration",
and "duration not accepted by hardware". It seems to me that the latter two
should be fatal and not just be ignored.

Thanks,
Guenter