Re: ping [PATCH v3] WAN: Adding support for Lantiq PEF2256 E1 chipset (FALC56)

[Joe Perches <joe@perches.com>]

On Sun, 2014-01-19 at 19:07 +0100, Christophe Leroy wrote:
> Pinging this watch as we got no feedback since 22 Nov, although we have taken
> into account reviews from v1 and v2.
> 
> The patch adds WAN support for Lantiq FALC56 - PEF2256 E1 Chipset.

trivia:

> diff -urN a/drivers/net/wan/pef2256.c b/drivers/net/wan/pef2256.c
[]
> +static void config_hdlc_timeslot(struct pef2256_dev_priv *priv, int ts)
> +{
> +	static struct {
> +		u32 ttr;
> +		u32 rtr;
> +	} regs[] = {
> +		{ TTR1, RTR1 },
> +		{ TTR2, RTR2 },
> +		{ TTR3, RTR3 },
> +		{ TTR4, RTR4 },
> +	};

const

> +	int cfg_bit = 1 << (31 - ts);
> +	int reg_bit = 1 << (7 - (ts % 8));
> +	int j = ts / 8;

looks endian specific

> +
> +	if (j >= 4)
> +		return;
> +
> +	if (priv->Tx_TS & cfg_bit)
> +		pef2256_s8(priv, regs[j].ttr, 1 << reg_bit);
> +
> +	if (priv->Rx_TS & cfg_bit)
> +		pef2256_s8(priv, regs[j].rtr, 1 << reg_bit);
> +}

> +static void init_falc(struct pef2256_dev_priv *priv)
> +{

a lot of the below looks like it should use
switch/case blocks.

> +	/* Clocking rate for FALC56 */
> +
> +	/* Nothing to do for clocking rate 2M  */
> +
> +	/* clocking rate 4M  */
> +	if (priv->clock_rate == CLOCK_RATE_4M)
> +		pef2256_s8(priv, SIC1, SIC1_SSC0);
> +
> +	/* clocking rate 8M  */
> +	if (priv->clock_rate == CLOCK_RATE_8M)
> +		pef2256_s8(priv, SIC1, SIC1_SSC1);
> +
> +	/* clocking rate 16M  */
> +	if (priv->clock_rate == CLOCK_RATE_16M) {
> +		pef2256_s8(priv, SIC1, SIC1_SSC0);
> +		pef2256_s8(priv, SIC1, SIC1_SSC1);
> +	}
> +
> +	/* data rate for FALC56 */
> +
> +	/* Nothing to do for data rate 2M on the system data bus */
> +
> +	/* data rate 4M on the system data bus */
> +	if (priv->data_rate == DATA_RATE_4M)
> +		pef2256_s8(priv, FMR1, FMR1_SSD0);
> +
> +	/* data rate 8M on the system data bus */
> +	if (priv->data_rate == DATA_RATE_8M)
> +		pef2256_s8(priv, SIC1, SIC1_SSD1);
> +
> +	/* data rate 16M on the system data bus */
> +	if (priv->data_rate == DATA_RATE_16M) {
> +		pef2256_s8(priv, FMR1, FMR1_SSD0);
> +		pef2256_s8(priv, SIC1, SIC1_SSD1);
> +	}
> +
> +	/* channel phase for FALC56 */
> +
> +	/* Nothing to do for channel phase 1 */
> +
> +	if (priv->channel_phase == CHANNEL_PHASE_2)
> +		pef2256_s8(priv, SIC2, SIC2_SICS0);
> +
> +	if (priv->channel_phase == CHANNEL_PHASE_3)
> +		pef2256_s8(priv, SIC2, SIC2_SICS1);
> +
> +	if (priv->channel_phase == CHANNEL_PHASE_4) {
> +		pef2256_s8(priv, SIC2, SIC2_SICS0);
> +		pef2256_s8(priv, SIC2, SIC2_SICS1);
> +	}
> +
> +	if (priv->channel_phase == CHANNEL_PHASE_5)
> +		pef2256_s8(priv, SIC2, SIC2_SICS2);
> +
> +	if (priv->channel_phase == CHANNEL_PHASE_6) {
> +		pef2256_s8(priv, SIC2, SIC2_SICS0);
> +		pef2256_s8(priv, SIC2, SIC2_SICS2);
> +	}
> +
> +	if (priv->channel_phase == CHANNEL_PHASE_7) {
> +		pef2256_s8(priv, SIC2, SIC2_SICS1);
> +		pef2256_s8(priv, SIC2, SIC2_SICS2);
> +	}
> +
> +	if (priv->channel_phase == CHANNEL_PHASE_8) {
> +		pef2256_s8(priv, SIC2, SIC2_SICS0);
> +		pef2256_s8(priv, SIC2, SIC2_SICS1);
> +		pef2256_s8(priv, SIC2, SIC2_SICS2);
> +	}

> +static ssize_t fs_attr_mode_store(struct device *dev,
> +			struct device_attribute *attr,  const char *buf,
> +			size_t count)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct pef2256_dev_priv *priv = dev_to_hdlc(ndev)->priv;
> +	long int value;
> +	int ret = kstrtol(buf, 10, &value);
> +	int reconfigure = (value != priv->mode);

Ugly test and set before determining if the previous
function was successful.

> +	if (value != MASTER_MODE && value != SLAVE_MODE)
> +		ret = -EINVAL;
> +
> +	if (ret < 0)
> +		netdev_info(ndev, "Invalid mode (0 or 1 expected\n");
> +	else {
> +		priv->mode = value;
> +		if (reconfigure && netif_carrier_ok(ndev))
> +			init_falc(priv);
> +	}
> +
> +	return strnlen(buf, count);

odd that you set ret and then don't use it.


> +static ssize_t fs_attr_Tx_TS_store(struct device *dev,
> +			struct device_attribute *attr,  const char *buf,
> +			size_t count)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct pef2256_dev_priv *priv = dev_to_hdlc(ndev)->priv;
> +	unsigned long value;
> +	int ret = kstrtoul(buf, 16, (long int *)&value);

unportable cast

> +	int reconfigure = (value != priv->mode);

again with the test/set before determining function success.