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
Subject: Re: [RFC 1/2] staging: jnx: Add Juniper connector driver
Date: Fri, 07 Oct 2016 09:25:27 -0700 [thread overview]
Message-ID: <1475857527.1945.7.camel@perches.com> (raw)
In-Reply-To: <1475853372-21997-2-git-send-email-pantelis.antoniou@konsulko.com>
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;
next prev parent reply other threads:[~2016-10-07 16:25 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-07 15:16 [RFC 0/2] Juniper DT based connector driver Pantelis Antoniou
2016-10-07 15:16 ` [RFC 1/2] staging: jnx: Add Juniper " Pantelis Antoniou
2016-10-07 16:25 ` Joe Perches [this message]
2016-10-07 18:57 ` Pantelis Antoniou
2016-10-07 15:16 ` [RFC 2/2] staging: jnx-connector: add device tree binding Pantelis Antoniou
2016-10-08 16:05 ` Rob Herring
2016-10-07 15:30 ` [RFC 0/2] Juniper DT based connector driver Greg Kroah-Hartman
2016-10-08 16:10 ` Rob Herring
2016-10-08 16:11 ` Pantelis Antoniou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1475857527.1945.7.camel@perches.com \
--to=joe@perches.com \
--cc=aronen@juniper.net \
--cc=ddevineni@juniper.net \
--cc=devel@driverdev.osuosl.org \
--cc=devicetree@vger.kernel.org \
--cc=dghosh@juniper.net \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=gvlaev@juniper.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mark.rutland@arm.com \
--cc=pantelis.antoniou@konsulko.com \
--cc=robh+dt@kernel.org \
--cc=ylu@juniper.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).