Subject: Re: [RFC 1/2] staging: jnx: Add Juniper connector driver
Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\))
Content-Type: text/plain; charset=utf-8
From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
In-Reply-To: <1475857527.1945.7.camel@perches.com>
Date: Fri, 7 Oct 2016 21:57:45 +0300
Cc: Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
        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
Message-Id: <B39C2234-7E92-4067-B403-F93CC07CC062@konsulko.com>
References: <1475853372-21997-1-git-send-email-pantelis.antoniou@konsulko.com> <1475853372-21997-2-git-send-email-pantelis.antoniou@konsulko.com> <1475857527.1945.7.camel@perches.com>
To: Joe Perches <joe@perches.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 3852
Lines: 133

Hi Joe,

> On Oct 7, 2016, at 19:25 , Joe Perches <joe@perches.com> wrote:
> 
> 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
> 

Yes, good idea; this structure sorta grew organically.

>> +	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?
> 

Yeah, it is weird; it works but its risky.

I’ll change it.

>> +/*
>> + * 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;
> 	}
> 

It does look better.

> 	dev_err(...);
> 	return -EINVAL;
> 
> 


Regards

— Pantelis