Re: [PATCH] Staging: gdm724x: LTE: Fix trailing open parentheses.

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Quytelda Kahja <quytelda@tamalin.org>
Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org,
	jonathankim@gctsemi.com, linux-kernel@vger.kernel.org,
	deanahn@gctsemi.com
Subject: Re: [PATCH] Staging: gdm724x: LTE: Fix trailing open parentheses.
Date: Wed, 21 Feb 2018 13:43:06 +0300	[thread overview]
Message-ID: <20180221104306.zq7fdbgzjmfdrqju@mwanda> (raw)
In-Reply-To: <20180221102017.17211-1-quytelda@tamalin.org>

This patch is fine.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

But I have a some comments for later.

On Wed, Feb 21, 2018 at 02:20:17AM -0800, Quytelda Kahja wrote:
> @@ -509,8 +511,9 @@ static struct net_device_stats *gdm_lte_stats(struct net_device *dev)
>  
>  static int gdm_lte_event_send(struct net_device *dev, char *buf, int len)
>  {
> -	struct nic *nic = netdev_priv(dev);
> +	struct phy_dev *phy_dev = ((struct nic *)netdev_priv(dev))->phy_dev;
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is a bit unsightly.  Better to make it two assignments:

	struct nic *nic = netdev_priv(dev);
	struct phy_dev *phy_dev = nic->phy_dev;

>  	struct hci_packet *hci = (struct hci_packet *)buf;
> +	int length;
>  	int idx;
>  	int ret;
>  
> @@ -518,11 +521,9 @@ static int gdm_lte_event_send(struct net_device *dev, char *buf, int len)
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	return netlink_send(lte_event.sock, idx, 0, buf,
> -			    gdm_dev16_to_cpu(
> -				    nic->phy_dev->get_endian(
> -					    nic->phy_dev->priv_dev), hci->len)
> -			    + HCI_HEADER_SIZE);
> +	length = gdm_dev16_to_cpu(phy_dev->get_endian(phy_dev->priv_dev),
> +				  hci->len) + HCI_HEADER_SIZE;
> +	return netlink_send(lte_event.sock, idx, 0, buf, length);

It would be nicer to store:

	struct gdm_endian *ed = phy_dev->get_endian(phy_dev->priv_dev);

at the start of the function as well.  Then this looks like:

	length = gdm_dev16_to_cpu(ed, hci->len) + HCI_HEADER_SIZE;
	netlink_send(lte_event.sock, idx, 0, buf, length);

The endian information doesn't need to be a struct any more since
we remove ed->host_ed in 77e8a50149a2 ("staging: gdm724x: Remove test
for host endian").  We should change gdm_dev16_to_cpu() to just take
dev_ed.

>  }
>  
>  static void gdm_lte_event_rcv(struct net_device *dev, u16 type,
> @@ -731,15 +732,13 @@ static void gdm_lte_pdn_table(struct net_device *dev, char *buf, int len)
>  	struct hci_pdn_table_ind *pdn_table = (struct hci_pdn_table_ind *)buf;
>  
>  	if (pdn_table->activate) {
> +		struct gdm_endian *ed;
> +
>  		nic->pdn_table.activate = pdn_table->activate;
> -		nic->pdn_table.dft_eps_id = gdm_dev32_to_cpu(
> -						nic->phy_dev->get_endian(
> -							nic->phy_dev->priv_dev),
> -						pdn_table->dft_eps_id);
> -		nic->pdn_table.nic_type = gdm_dev32_to_cpu(
> -						nic->phy_dev->get_endian(
> -							nic->phy_dev->priv_dev),
> -						pdn_table->nic_type);
> +
> +		ed = nic->phy_dev->get_endian(nic->phy_dev->priv_dev);
> +		nic->pdn_table.dft_eps_id = gdm_dev32_to_cpu(ed, pdn_table->dft_eps_id);
> +		nic->pdn_table.nic_type = gdm_dev32_to_cpu(ed, pdn_table->nic_type);
>  
>  		netdev_info(dev, "pdn activated, nic_type=0x%x\n",
>  			    nic->pdn_table.nic_type);

Use the same three initial variables here:

	struct nic *nic = netdev_priv(dev);
	struct phy_dev *phy_dev = nic->phy_dev;
	struct gdm_endian *ed = phy_dev->get_endian(nic->phy_dev->priv_dev);

Then reverse the ->active test:

	if (!pdn_table->activate) {
		memset(&nic->pdn_table, 0, sizeof(struct pdn_table));
		netdev_info(dev, "pdn deactivated\n");
		return;
	}

Then pull everything in a tab:

	nic->pdn_table.activate = pdn_table->activate;
	nic->pdn_table.dft_eps_id = gdm_dev32_to_cpu(ed, pdn_table->dft_eps_id);
	nic->pdn_table.nic_type = gdm_dev32_to_cpu(ed, pdn_table->nic_type);

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel