Re: [PATCH RFC] Add Microchip KSZ8895 DSA driver

[Pavel Machek]

Hi!

> +static void ksz8895_set_prio_queue(struct ksz_device *dev, int port, int queue)
> +{
> +	u8 hi;
> +	u8 lo;
> +
> +	/* Number of queues can only be 1, 2, or 4. */
> +	switch (queue) {
> +	case 4:
> +	case 3:
> +		queue = PORT_QUEUE_SPLIT_4;
> +		break;
> +	case 2:
> +		queue = PORT_QUEUE_SPLIT_2;
> +		break;
> +	default:
> +		queue = PORT_QUEUE_SPLIT_1;
> +	}
> +	ksz_pread8(dev, port, REG_PORT_CTRL_0, &lo);
> +	ksz_pread8(dev, port, P_DROP_TAG_CTRL, &hi);
> +	lo &= ~PORT_QUEUE_SPLIT_L;
> +	if (queue & PORT_QUEUE_SPLIT_2)
> +		lo |= PORT_QUEUE_SPLIT_L;
> +	hi &= ~PORT_QUEUE_SPLIT_H;
> +	if (queue & PORT_QUEUE_SPLIT_4)
> +		hi |= PORT_QUEUE_SPLIT_H;
> +	ksz_pwrite8(dev, port, REG_PORT_CTRL_0, lo);
> +	ksz_pwrite8(dev, port, P_DROP_TAG_CTRL, hi);
> +
> +	/* Default is port based for egress rate limit. */
> +	if (queue != PORT_QUEUE_SPLIT_1)
> +		ksz_cfg(dev, REG_SW_CTRL_19, SW_OUT_RATE_LIMIT_QUEUE_BASED,
> +			true);
> +}

This is same as the other driver, right? Same comments apply here, and
please find a way to make it shared.


> +static void ksz8895_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *cnt)
> +{
> +	u32 data;
> +	u16 ctrl_addr;
> +	u8 check;
> +	int loop;
> +
> +	ctrl_addr = addr + SWITCH_COUNTER_NUM * port;
> +	ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
> +
> +	mutex_lock(&dev->alu_mutex);
> +	ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> +
> +	/* It is almost guaranteed to always read the valid bit because of
> +	 * slow SPI speed.
> +	 */
> +	for (loop = 2; loop > 0; loop--) {
> +		ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> +
> +		if (check & MIB_COUNTER_VALID) {
> +			ksz_read32(dev, REG_IND_DATA_LO, &data);
> +			if (check & MIB_COUNTER_OVERFLOW)
> +				*cnt += MIB_COUNTER_VALUE + 1;
> +			*cnt += data & MIB_COUNTER_VALUE;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dev->alu_mutex);

Again, same function, same review comments.

> +static void ksz8895_r_table(struct ksz_device *dev, int table, u16 addr,
> +			    u64 *data)
> +{
> +	u16 ctrl_addr;
> +
> +	ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
> +
> +	mutex_lock(&dev->alu_mutex);
> +	ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> +	ksz_get(dev, REG_IND_DATA_HI, data, sizeof(u64));
> +	mutex_unlock(&dev->alu_mutex);
> +	*data = be64_to_cpu(*data);
> +}

I've seen this before; this is duplicated code, it does not make sense
to review.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature