2014-02-26 17:37 GMT+01:00 Alexander Shiyan <shc_work@xxxxxxx>:
> Hello Richard.
>
> Среда, 26 февраля 2014, 17:19 +01:00 от Richard Genoud <richard.genoud@xxxxxxxxx>:
>> This patch add some helpers to control modem lines (CTS/RTS/DSR...) via
>> GPIO.
>> This will be useful for many boards which have a serial controller that
>> only handle CTS/RTS pins (or even just RX/TX).
>>
>> Signed-off-by: Richard Genoud <richard.genoud@xxxxxxxxx>
>
> Better, but I found few bugs... :)
> ...
>> +Modem control lines via GPIO
>> +----------------------------
>> +
>> +Some helpers are provided in order to set/get modem control lines via GPIO.
>> +
>> +mctrl_gpio_init(dev, idx):
>
> I think we should indicate variable types used in these functions.
In the whole file, the variable type are not indicated. So, for
consistency, I think it's better like that.
...
>> +void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
>> +{
>> + enum mctrl_gpio_idx i;
>> +
>> + if (IS_ERR_OR_NULL(gpios))
>> + return;
>
> No. IS_ERR_OR_NULL() is not valid here. This is allocated by kzalloc(),
> so you should check valid pointer only, ie (!gpios).
> Same in other places.
You're right !
>
>> + if (err) {
>> + dev_err(dev, "Unable to set direction for %s GPIO",
>> + mctrl_gpios_desc[i].name);
>> + devm_gpiod_put(dev, gpios->gpio[i]);
>> + gpios->gpio[i] = NULL;
>> + }
>> + }
>
> Let's use dev_dbg().
Why dbg ? I agree that _err may be a bit strong, we could use
dev_warn() instead.
But as it really should not happen, I'm a bit reluctant to set it as debug.
Or have you something else in mind ?
> Thanks.
> ---
Thanks for reviewing !
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
