Message-ID: <1475857527.1945.7.camel@perches.com>
Subject: Re: [RFC 1/2] staging: jnx: Add Juniper connector driver
From: Joe Perches <joe@perches.com>
To: Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
        Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>
Cc: Frank Rowand <frowand.list@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alon Ronen <aronen@juniper.net>, Debjit Ghosh <dghosh@juniper.net>,
        Dhruva Diveneni <ddevineni@juniper.net>,
        Georgi Vlaev <gvlaev@juniper.net>, Guenter Roeck <linux@roeck-us.net>,
        Yu-Lin Lu <ylu@juniper.net>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org
Date: Fri, 07 Oct 2016 09:25:27 -0700
In-Reply-To: <1475853372-21997-2-git-send-email-pantelis.antoniou@konsulko.com>
References: <1475853372-21997-1-git-send-email-pantelis.antoniou@konsulko.com>
         <1475853372-21997-2-git-send-email-pantelis.antoniou@konsulko.com>
Content-Type: text/plain; charset="ISO-8859-1"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3466
Lines: 111

On Fri, 2016-10-07 at 18:16 +0300, Pantelis Antoniou wrote:
> diff --git a/drivers/staging/jnx/jnx-connector.c b/drivers/staging/jnx/jnx-connector.c
[]
> +struct jnx_conn_data {
> +	struct device *dev;		/* parent (platform) device */
> +	const char *name[NUM_OVERLAYS];	/* overlay file names */
> +	bool enabled;			/* true if can handle interrupts */
> +	bool poweron;			/* true if assumed to be powered on */

maybe use pahole and remove some of the wasteful padding

> +	int attention_button;		/* attention button gpio pin */
> +	bool have_attention_button;	/* true if attention button exists */
> +	unsigned long attention_button_holdtime;/* button hold time, jiffies */
> +	bool attention_ignore;		/* true if handled by user space */
> +	int power_enable;		/* power enable gpio pin */
> +	bool auto_enable;		/* true if board should auto-enable */
> +	struct jnx_i2c_power_seq pon;	/* power-on sequence */
[]
> +	u32 gpio_flags;
> +	u16 assembly_id;
> +	int slot;			/* slot number */
> +	int type;			/* card type */
> +	bool static_assembly_id;	/* true if assembly_id is static */
> +	bool assembly_id_valid;		/* true if assembly_id is valid */
> +	int adapter;			/* parent i2c adapter number */
[]
> +	struct mutex mutex;		/* mutex to protect state changes */
> +	bool synchronous;		/* true if state changes are ok */
> +	struct mutex fdt_mutex;		/* mutex to protect fdt accesses */
[]
> +	bool standby_to_master;		/* standby:master_ev processing */
> +};
[]
> +/*
> + * jnx_conn_insert_ideeprom()
> + *   Inserts ideeprom with a parent from OF prop
> + */
> +static int jnx_conn_insert_ideeprom(struct jnx_conn_data *data,
> +				    struct i2c_adapter *adap,
> +				    struct device_node *node,
> +				    struct i2c_board_info *info)
> +{
> +	struct device *dev = data->dev;
> +	struct i2c_adapter *parent = NULL;
> +	struct i2c_client *client;
> +	struct device_node *anode;
> +	struct at24_platform_data at24_pdata = {
> +		.byte_len = 256,
> +		.page_size = 4,
> +		.setup = jnx_conn_at24_callback,
> +		.context = data,
> +	};
> +
> +	info->platform_data = &at24_pdata;

Assigning a temporary address through a pointer argument?
Isn't there a better way?

> +/*
> + * jnx_conn_verify_overlay()
> + *
> + * Verify if overlay is compatible with this board/slot
> + */
> +static int jnx_conn_verify_overlay(struct jnx_conn_data *data,
> +				   struct device_node *np)
> +{
[]
> +	ret = of_property_read_u32(np, "type", &var);
> +	if (ret) {
> +		dev_err(dev, "Missing type property\n");
> +		return ret;
> +	}
> +	if (var != data->type) {
> +		dev_err(dev, "Wrong type: Expected %d, got %d\n",
> +			data->type, var);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * 'assembly-ids' property must exist, and one of its entries must match
> +	 * the card assembly id
> +	 */
> +	assembly_ids = of_get_property(np, "assembly-ids", &size);
> +	if (!assembly_ids || size < sizeof(u32)) {
> +		dev_err(dev, "Bad assembly-ids property\n");
> +		return -EINVAL;
> +	}
> +	ret = -EINVAL;
> +	for (i = 0; i < size / sizeof(u32); i++) {
> +		if (be32_to_cpu(assembly_ids[i]) == data->assembly_id) {
> +			ret = 0;
> +			break;
> +		}
> +	}
> +	if (ret) {
> +		dev_err(dev, "Assembly ID 0x%x not supported by overlay\n",
> +			data->assembly_id);
> +		return ret;
> +	}

Given all the direct returns above here, perhaps

	for (i = 0; i < size / sizeof(u32); i++) {
		if (be32_to_cpu(assembly_ids[i]) == data->assembly_id)
			return 0;
	}

	dev_err(...);
	return -EINVAL;