Hi Joe,
> On Oct 7, 2016, at 19:25 , Joe Perches <joe@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
