Re: Re: [PATCH V5.1] serial/uart/8250: Add tunable RX interrupt trigger I/F of FIFO buffers

[Yoshihiro YUNOMAE]

Hi Alan,

Thank you for your review.

(2014/04/15 19:10), One Thousand Gnomes wrote:
> > +static int convert_val2rxtrig(struct uart_8250_port *up, unsigned char val)
> > +{
> > +	const struct serial8250_config *conf_type = &uart_config[up->port.type];
> > +	int i;
> > +
> > +	if (!conf_type->rx_trig_byte[FCR_RX_TRIG_BITS(UART_FCR_R_TRIG_00)]) {
> > +		pr_info("Not support RX-trigger setting for this serial %s\n",
> > +			conf_type->name);
> > +		return -EOPNOTSUPP;
>
> We don't want the pr_info here - otherwise users can flood the logs. The
> err code is sufficient.

OK, I'll delete this pr_info().

> > +	for (i = 1; i < FCR_RX_TRIG_MAX_STATE; i++) {
> > +		if (val < conf_type->rx_trig_byte[i])
> > +			/* Use the nearest lower value */
> > +			return (--i) << FCR_RX_TRIG_OFFSET;
>
> Odd case here - your 16654 entry has a minimum of 8 so if I pass 1 then
> it'll return -1 ? I guess for that case the best we can do is to set 8.

No, in that case, this function returns 0.

Note that the "i" starts from 1, so the minimal value is 0.
Actually convert_val2rxtrig() returns the index of rx_trig_byte, and
when val == 1, it returns 0 and caller gets rx_trig_byte[0] == 8.

> Other trivial thing to tweak - you have "uaser" in spot instead of "user".

Oops, I'll fix it.

Thanks,
Yoshihiro YUNOMAE

> With those two fixed it looks good to me.
>
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@xxxxxxxxxxx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/