Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162969AbbKTWum (ORCPT ); Fri, 20 Nov 2015 17:50:42 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:53454 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760099AbbKTWul (ORCPT ); Fri, 20 Nov 2015 17:50:41 -0500 Date: Fri, 20 Nov 2015 14:50:40 -0800 From: Darren Hart To: Azael Avalos Cc: platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, Fabian Koester Subject: Re: [PATCH v2 2/2] toshiba_acpi: Add WWAN RFKill support Message-ID: <20151120225040.GB7413@malice.jf.intel.com> References: <1447948165-2343-1-git-send-email-coproscefalo@gmail.com> <1447948165-2343-3-git-send-email-coproscefalo@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447948165-2343-3-git-send-email-coproscefalo@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4553 Lines: 175 On Thu, Nov 19, 2015 at 08:49:25AM -0700, Azael Avalos wrote: Hi Azael, > A previuos patch added WWAN support to the driver, allowing to query > and set the device status. > > This patch adds RFKill support for the recently introduced WWAN device, > making use of the WWAN and *wireless_status functions to query the > killswitch and (de)activate the device accordingly to its status. > > Signed-off-by: Fabian Koester So this is Fabian's code which he sent to you and you are submitting on his behalf? > Signed-off-by: Azael Avalos A couple minor nits below. > --- > drivers/platform/x86/toshiba_acpi.c | 79 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c > index 60d1ad9..d1315c5 100644 > --- a/drivers/platform/x86/toshiba_acpi.c > +++ b/drivers/platform/x86/toshiba_acpi.c > @@ -51,6 +51,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -174,6 +175,7 @@ struct toshiba_acpi_dev { > struct led_classdev kbd_led; > struct led_classdev eco_led; > struct miscdevice miscdev; > + struct rfkill *wwan_rfk; > > int force_fan; > int last_key_event; > @@ -2330,6 +2332,67 @@ static const struct file_operations toshiba_acpi_fops = { > }; > > /* > + * WWAN RFKill handlers > + */ > +static int toshiba_acpi_wwan_set_block(void *data, bool blocked) > +{ > + struct toshiba_acpi_dev *dev = data; > + int ret; > + > + ret = toshiba_wireless_status(dev); > + if (ret) > + return ret; > + > + if (!dev->killswitch) > + return 0; > + > + return toshiba_wwan_set(dev, blocked ? 0 : 1); You can avoid the ternary operation with binary output and just invert the bool. !blocked > +} > + > +static void toshiba_acpi_wwan_poll(struct rfkill *rfkill, void *data) > +{ > + struct toshiba_acpi_dev *dev = data; > + > + if (toshiba_wireless_status(dev)) > + return; > + > + rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch); > +} > + > +static const struct rfkill_ops wwan_rfk_ops = { > + .set_block = toshiba_acpi_wwan_set_block, > + .poll = toshiba_acpi_wwan_poll, > +}; > + > +static int toshiba_acpi_setup_wwan_rfkill(struct toshiba_acpi_dev *dev) > +{ > + int ret = toshiba_wireless_status(dev); > + > + if (ret) > + return ret; > + > + dev->wwan_rfk = rfkill_alloc("Toshiba WWAN", > + &dev->acpi_dev->dev, > + RFKILL_TYPE_WWAN, > + &wwan_rfk_ops, > + dev); > + if (!dev->wwan_rfk) { > + pr_err("Unable to allocate WWAN rfkill device\n"); > + return -ENOMEM; > + } > + > + rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch); > + > + ret = rfkill_register(dev->wwan_rfk); > + if (ret) { > + pr_err("Unable to register WWAN rfkill device\n"); > + rfkill_destroy(dev->wwan_rfk); > + } > + > + return ret; > +} > + > +/* > * Hotkeys > */ > static int toshiba_acpi_enable_hotkeys(struct toshiba_acpi_dev *dev) > @@ -2688,6 +2751,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev) > if (dev->eco_led_registered) > led_classdev_unregister(&dev->eco_led); > > + if (dev->wwan_rfk) { > + rfkill_unregister(dev->wwan_rfk); > + rfkill_destroy(dev->wwan_rfk); > + } > + > if (toshiba_acpi) > toshiba_acpi = NULL; > > @@ -2827,6 +2895,8 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev) > dev->fan_supported = !ret; > > toshiba_wwan_available(dev); > + if (dev->wwan_supported) > + toshiba_acpi_setup_wwan_rfkill(dev); > > print_supported_features(dev); > > @@ -2930,6 +3000,15 @@ static int toshiba_acpi_resume(struct device *device) > pr_info("Unable to re-enable hotkeys\n"); > } > > + if (dev->wwan_rfk) { > + int error = toshiba_wireless_status(dev); > + > + if (error) > + return error; For consistency, please use ret. You can just initialize it to 0 outside the if block, use it inside, and return it outside as well. We don't buy much by declaring inside the if block on a resume function. > + > + rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch); > + } > + > return 0; > } > #endif > -- > 2.6.2 > > -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/