From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755264AbcJGS57 (ORCPT ); Fri, 7 Oct 2016 14:57:59 -0400 Received: from mail-wm0-f53.google.com ([74.125.82.53]:37241 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753430AbcJGS5u (ORCPT ); Fri, 7 Oct 2016 14:57:50 -0400 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 In-Reply-To: <1475857527.1945.7.camel@perches.com> Date: Fri, 7 Oct 2016 21:57:45 +0300 Cc: Rob Herring , Mark Rutland , Frank Rowand , Greg Kroah-Hartman , Alon Ronen , Debjit Ghosh , Dhruva Diveneni , Georgi Vlaev , Guenter Roeck , Yu-Lin Lu , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org Message-Id: 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 X-Mailer: Apple Mail (2.3124) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id u97IwJdl027338 Hi Joe, > On Oct 7, 2016, at 19:25 , Joe Perches 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