Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1497236 > unrolled thread

[RFC 0/2] Juniper DT based connector driver

Back to article view | Back to linux.kernel

Contents

  [RFC 0/2] Juniper DT based connector driver Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-10-07 17:30 +0200
    Re: [RFC 0/2] Juniper DT based connector driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-10-07 17:40 +0200
    Re: [RFC 1/2] staging: jnx: Add Juniper connector driver Joe Perches <joe@perches.com> - 2016-10-07 18:30 +0200
      Re: [RFC 1/2] staging: jnx: Add Juniper connector driver Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-10-07 21:00 +0200
    Re: [RFC 0/2] Juniper DT based connector driver Rob Herring <robh+dt@kernel.org> - 2016-10-08 18:20 +0200
      Re: [RFC 0/2] Juniper DT based connector driver Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-10-08 18:20 +0200

#1497236 — [RFC 0/2] Juniper DT based connector driver

Introduce a Juniper PTX router series DT overlay based
connector driver.

This is submitted as an RFC since some OF infrastructure
patches (like changeset helpers etc) are not yet mainlined.

The Juniper series of routers comprise of both x86 and powerpc
platforms that contain similar hardware components necessitating
common support methods.

Note that this is the first submission and we expect things to be
moved around as required.

This patchset is against mainline as of today: v4.8-9431-g3477d16
and is dependent on the "Juniper prerequisites",
"Juniper infrastructure" and "Juniper PCI methods" patchsets
sent earlier.

Guenter Roeck (2):
  staging: jnx: Add Juniper connector driver
  staging: jnx-connector: add device tree binding

 .../devicetree/bindings/jnx/jnx-connector          |   59 +
 drivers/staging/jnx/Kconfig                        |   15 +
 drivers/staging/jnx/Makefile                       |    1 +
 drivers/staging/jnx/jnx-connector.c                | 2172 ++++++++++++++++++++
 4 files changed, 2247 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jnx/jnx-connector
 create mode 100644 drivers/staging/jnx/jnx-connector.c

-- 
1.9.1

[toc] | [next] | [standalone]

#1497266

On Fri, Oct 07, 2016 at 06:16:10PM +0300, Pantelis Antoniou wrote:
> Introduce a Juniper PTX router series DT overlay based
> connector driver.
> 
> This is submitted as an RFC since some OF infrastructure
> patches (like changeset helpers etc) are not yet mainlined.

staging drivers almost always need to at least build (or if not, have a
good reason why not.)

So don't post code until people can at least test-build it, otherwise no
one will care about it.  Would you review such code?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1497318 — Re: [RFC 1/2] staging: jnx: Add Juniper connector driver

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;

[toc] | [prev] | [next] | [standalone]

#1497476 — Re: [RFC 1/2] staging: jnx: Add Juniper connector driver

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

[toc] | [prev] | [next] | [standalone]

#1497770

On Fri, Oct 7, 2016 at 10:16 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Introduce a Juniper PTX router series DT overlay based
> connector driver.
>
> This is submitted as an RFC since some OF infrastructure
> patches (like changeset helpers etc) are not yet mainlined.

Because you haven't sent them out again. There were only minor issues.
Someone else said recently they were going to refresh them, but I
haven't seen them.

Rob

[toc] | [prev] | [next] | [standalone]

#1497772

Hi Rob,

> On Oct 8, 2016, at 19:10 , Rob Herring <robh+dt@kernel.org> wrote:
> 
> On Fri, Oct 7, 2016 at 10:16 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Introduce a Juniper PTX router series DT overlay based
>> connector driver.
>> 
>> This is submitted as an RFC since some OF infrastructure
>> patches (like changeset helpers etc) are not yet mainlined.
> 
> Because you haven't sent them out again. There were only minor issues.
> Someone else said recently they were going to refresh them, but I
> haven't seen them.
> 

Yes, they will go out right after ELCE.

> Rob

Regards

— Pantelis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web