From: Johan Hovold <johan@kernel.org>
To: Gioh Kim <gi-oh.kim@profitbricks.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org, elder@kernel.org,
greybus-dev@lists.linaro.org, devel@driverdev.osuosl.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] staging: greybus: fix "line over 80 characters" coding style issues
Date: Mon, 13 Feb 2017 11:41:25 +0100 [thread overview]
Message-ID: <20170213104125.GB27275@localhost> (raw)
In-Reply-To: <1486657812-7948-1-git-send-email-gi-oh.kim@profitbricks.com>
On Thu, Feb 09, 2017 at 05:30:11PM +0100, Gioh Kim wrote:
> This patch fixes only obvious lines.
> There are still more issues.
Please make this commit message self-contained (e.g. mention what you
are doing here and why).
> Signed-off-by: Gioh Kim <gi-oh.kim@profitbricks.com>
> ---
> drivers/staging/greybus/arche-apb-ctrl.c | 5 +++-
> drivers/staging/greybus/arche-platform.c | 43 +++++++++++++++++++++-----------
> drivers/staging/greybus/audio_codec.c | 5 +++-
> drivers/staging/greybus/bootrom.c | 13 ++++++----
> drivers/staging/greybus/es2.c | 3 ++-
> drivers/staging/greybus/fw-download.c | 6 +++--
> drivers/staging/greybus/gbphy.c | 3 ++-
> drivers/staging/greybus/gpio.c | 3 ++-
> drivers/staging/greybus/svc.c | 26 +++++++++++--------
> drivers/staging/greybus/uart.c | 8 +++---
> drivers/staging/greybus/vibrator.c | 4 ++-
> 11 files changed, 77 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
> index 17fa290..02243b4 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -168,7 +168,10 @@ static int standby_boot_seq(struct platform_device *pdev)
> if (apb->init_disabled)
> return 0;
>
> - /* Even if it is in OFF state, then we do not want to change the state */
> + /*
> + * Even if it is in OFF state,
> + * then we do not want to change the state
> + */
Odd comment, but still don't break the line after the comma, break
around 80 cols.
But in this case I think the comment should instead be shortened to:
/* Even if in OFF state we do not want to change the state. */
> if (apb->state == ARCHE_PLATFORM_STATE_STANDBY ||
> apb->state == ARCHE_PLATFORM_STATE_OFF)
> return 0;
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index 338c2d3..aac1145 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -312,9 +312,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> arche_pdata->wake_detect_start = jiffies;
> /*
> - * In the begining, when wake/detect goes low (first time), we assume
> - * it is meant for coldboot and set the flag. If wake/detect line stays low
> - * beyond 30msec, then it is coldboot else fallback to standby boot.
> + * In the begining, when wake/detect goes low
> + * (first time), we assume it is meant for coldboot
> + * and set the flag. If wake/detect line stays low
> + * beyond 30msec, then it is coldboot else fallback
> + * to standby boot.
> */
> arche_platform_set_wake_detect_state(arche_pdata,
> WD_STATE_BOOT_INIT);
> @@ -330,7 +332,8 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> /*
> * Requires arche_pdata->platform_state_mutex to be held
> */
> -static int arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
> +static int
> +arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
> {
> int ret;
>
> @@ -364,7 +367,8 @@ static int arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdat
> /*
> * Requires arche_pdata->platform_state_mutex to be held
> */
> -static int arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
> +static int
> +arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
> {
> int ret;
>
> @@ -398,7 +402,8 @@ static int arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_p
> /*
> * Requires arche_pdata->platform_state_mutex to be held
> */
> -static void arche_platform_poweroff_seq(struct arche_platform_drvdata *arche_pdata)
> +static void
> +arche_platform_poweroff_seq(struct arche_platform_drvdata *arche_pdata)
> {
> unsigned long flags;
>
> @@ -561,14 +566,17 @@ static int arche_platform_probe(struct platform_device *pdev)
> struct device_node *np = dev->of_node;
> int ret;
>
> - arche_pdata = devm_kzalloc(&pdev->dev, sizeof(*arche_pdata), GFP_KERNEL);
> + arche_pdata = devm_kzalloc(&pdev->dev, sizeof(*arche_pdata),
> + GFP_KERNEL);
> if (!arche_pdata)
> return -ENOMEM;
>
> /* setup svc reset gpio */
> arche_pdata->is_reset_act_hi = of_property_read_bool(np,
> "svc,reset-active-high");
> - arche_pdata->svc_reset_gpio = of_get_named_gpio(np, "svc,reset-gpio", 0);
> + arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> + "svc,reset-gpio",
> + 0);
> if (arche_pdata->svc_reset_gpio < 0) {
> dev_err(dev, "failed to get reset-gpio\n");
> return arche_pdata->svc_reset_gpio;
> @@ -610,7 +618,8 @@ static int arche_platform_probe(struct platform_device *pdev)
> dev_err(dev, "failed to get svc clock-req gpio\n");
> return arche_pdata->svc_refclk_req;
> }
> - ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req, "svc-clk-req");
> + ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
> + "svc-clk-req");
> if (ret) {
> dev_err(dev, "failed to request svc-clk-req gpio: %d\n", ret);
> return ret;
> @@ -634,13 +643,16 @@ static int arche_platform_probe(struct platform_device *pdev)
> arche_pdata->num_apbs = of_get_child_count(np);
> dev_dbg(dev, "Number of APB's available - %d\n", arche_pdata->num_apbs);
>
> - arche_pdata->wake_detect_gpio = of_get_named_gpio(np, "svc,wake-detect-gpio", 0);
> + arche_pdata->wake_detect_gpio = of_get_named_gpio(np,
> + "svc,wake-detect-gpio",
> + 0);
> if (arche_pdata->wake_detect_gpio < 0) {
> dev_err(dev, "failed to get wake detect gpio\n");
> return arche_pdata->wake_detect_gpio;
> }
>
> - ret = devm_gpio_request(dev, arche_pdata->wake_detect_gpio, "wake detect");
> + ret = devm_gpio_request(dev, arche_pdata->wake_detect_gpio,
> + "wake detect");
> if (ret) {
> dev_err(dev, "Failed requesting wake_detect gpio %d\n",
> arche_pdata->wake_detect_gpio);
> @@ -658,10 +670,11 @@ static int arche_platform_probe(struct platform_device *pdev)
> gpio_to_irq(arche_pdata->wake_detect_gpio);
>
> ret = devm_request_threaded_irq(dev, arche_pdata->wake_detect_irq,
> - arche_platform_wd_irq,
> - arche_platform_wd_irq_thread,
> - IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> - dev_name(dev), arche_pdata);
> + arche_platform_wd_irq,
> + arche_platform_wd_irq_thread,
> + IRQF_TRIGGER_FALLING |
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + dev_name(dev), arche_pdata);
Just leave this unchanged for now.
> if (ret) {
> dev_err(dev, "failed to request wake detect IRQ %d\n", ret);
> return ret;
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index 30941f9..25c8bb4 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -838,7 +838,10 @@ int gbaudio_register_module(struct gbaudio_module_info *module)
> snd_soc_dapm_link_component_dai_widgets(codec->card,
> &codec->dapm);
> #ifdef CONFIG_SND_JACK
> - /* register jack devices for this module from codec->jack_list */
> + /*
> + * register jack devices for this module
> + * from codec->jack_list
> + */
> list_for_each_entry(jack, &codec->jack_list, list) {
> if ((jack == &module->headset_jack)
> || (jack == &module->button_jack))
> diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
> index 5f90721..06df0ce 100644
> --- a/drivers/staging/greybus/bootrom.c
> +++ b/drivers/staging/greybus/bootrom.c
> @@ -53,7 +53,8 @@ static void free_firmware(struct gb_bootrom *bootrom)
> static void gb_bootrom_timedout(struct work_struct *work)
> {
> struct delayed_work *dwork = to_delayed_work(work);
> - struct gb_bootrom *bootrom = container_of(dwork, struct gb_bootrom, dwork);
> + struct gb_bootrom *bootrom = container_of(dwork,
> + struct gb_bootrom, dwork);
Leave this unchanged as well.
> struct device *dev = &bootrom->connection->bundle->dev;
> const char *reason;
>
> @@ -187,7 +188,8 @@ static int find_firmware(struct gb_bootrom *bootrom, u8 stage)
> static int gb_bootrom_firmware_size_request(struct gb_operation *op)
> {
> struct gb_bootrom *bootrom = gb_connection_get_data(op->connection);
> - struct gb_bootrom_firmware_size_request *size_request = op->request->payload;
> + struct gb_bootrom_firmware_size_request *size_request =
> + op->request->payload;
Don't break declarations this way if it can be avoided. Leave unchanged
for now.
> struct gb_bootrom_firmware_size_response *size_response;
> struct device *dev = &op->connection->bundle->dev;
> int ret;
> @@ -220,7 +222,8 @@ static int gb_bootrom_firmware_size_request(struct gb_operation *op)
> size_response = op->response->payload;
> size_response->size = cpu_to_le32(bootrom->fw->size);
>
> - dev_dbg(dev, "%s: firmware size %d bytes\n", __func__, size_response->size);
> + dev_dbg(dev, "%s: firmware size %d bytes\n",
> + __func__, size_response->size);
>
> unlock:
> mutex_unlock(&bootrom->mutex);
> @@ -287,8 +290,8 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
> firmware_response = op->response->payload;
> memcpy(firmware_response->data, fw->data + offset, size);
>
> - dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n", offset,
> - size);
> + dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n",
> + offset, size);
>
> unlock:
> mutex_unlock(&bootrom->mutex);
> diff --git a/drivers/staging/greybus/es2.c b/drivers/staging/greybus/es2.c
> index 93afd93..0eba059 100644
> --- a/drivers/staging/greybus/es2.c
> +++ b/drivers/staging/greybus/es2.c
> @@ -1085,7 +1085,8 @@ static void apb_log_get(struct es2_ap_dev *es2, char *buf)
> retval = usb_control_msg(es2->usb_dev,
> usb_rcvctrlpipe(es2->usb_dev, 0),
> GB_APB_REQUEST_LOG,
> - USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
> + USB_DIR_IN | USB_TYPE_VENDOR |
> + USB_RECIP_INTERFACE,
Leave unchanged.
> 0x00, 0x00,
> buf,
> APB1_LOG_MSG_SIZE,
> diff --git a/drivers/staging/greybus/fw-download.c b/drivers/staging/greybus/fw-download.c
> index 2d72468..8a1a413 100644
> --- a/drivers/staging/greybus/fw-download.c
> +++ b/drivers/staging/greybus/fw-download.c
> @@ -130,7 +130,8 @@ static void free_firmware(struct fw_download *fw_download,
> static void fw_request_timedout(struct work_struct *work)
> {
> struct delayed_work *dwork = to_delayed_work(work);
> - struct fw_request *fw_req = container_of(dwork, struct fw_request, dwork);
> + struct fw_request *fw_req = container_of(dwork,
> + struct fw_request, dwork);
Drop this too.
> struct fw_download *fw_download = fw_req->fw_download;
>
> dev_err(fw_download->parent,
> @@ -239,7 +240,8 @@ static int fw_download_find_firmware(struct gb_operation *op)
> tag = (const char *)request->firmware_tag;
>
> /* firmware_tag must be null-terminated */
> - if (strnlen(tag, GB_FIRMWARE_TAG_MAX_SIZE) == GB_FIRMWARE_TAG_MAX_SIZE) {
> + if (strnlen(tag, GB_FIRMWARE_TAG_MAX_SIZE) ==
> + GB_FIRMWARE_TAG_MAX_SIZE) {
Drop.
> dev_err(fw_download->parent,
> "firmware-tag is not null-terminated\n");
> return -EINVAL;
> diff --git a/drivers/staging/greybus/gbphy.c b/drivers/staging/greybus/gbphy.c
> index bcde7c9..64a1eb9 100644
> --- a/drivers/staging/greybus/gbphy.c
> +++ b/drivers/staging/greybus/gbphy.c
> @@ -104,7 +104,8 @@ static int gbphy_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
> }
>
> static const struct gbphy_device_id *
> -gbphy_dev_match_id(struct gbphy_device *gbphy_dev, struct gbphy_driver *gbphy_drv)
> +gbphy_dev_match_id(struct gbphy_device *gbphy_dev,
> + struct gbphy_driver *gbphy_drv)
> {
> const struct gbphy_device_id *id = gbphy_drv->id_table;
>
> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> index 558550c..0eabfe1 100644
> --- a/drivers/staging/greybus/gpio.c
> +++ b/drivers/staging/greybus/gpio.c
> @@ -557,7 +557,8 @@ static void gb_gpio_irqchip_remove(struct gb_gpio_controller *ggc)
> /* Remove all IRQ mappings and delete the domain */
> if (ggc->irqdomain) {
> for (offset = 0; offset < (ggc->line_max + 1); offset++)
> - irq_dispose_mapping(irq_find_mapping(ggc->irqdomain, offset));
> + irq_dispose_mapping(irq_find_mapping(ggc->irqdomain,
> + offset));
> irq_domain_remove(ggc->irqdomain);
> }
>
> diff --git a/drivers/staging/greybus/svc.c b/drivers/staging/greybus/svc.c
> index be151a6..5549db2 100644
> --- a/drivers/staging/greybus/svc.c
> +++ b/drivers/staging/greybus/svc.c
> @@ -678,7 +678,8 @@ static int gb_svc_version_request(struct gb_operation *op)
> static ssize_t pwr_debugfs_voltage_read(struct file *file, char __user *buf,
> size_t len, loff_t *offset)
> {
> - struct svc_debugfs_pwrmon_rail *pwrmon_rails = file_inode(file)->i_private;
> + struct svc_debugfs_pwrmon_rail *pwrmon_rails =
> + file_inode(file)->i_private;
Drop.
> struct gb_svc *svc = pwrmon_rails->svc;
> int ret, desc;
> u32 value;
> @@ -701,7 +702,8 @@ static ssize_t pwr_debugfs_voltage_read(struct file *file, char __user *buf,
> static ssize_t pwr_debugfs_current_read(struct file *file, char __user *buf,
> size_t len, loff_t *offset)
> {
> - struct svc_debugfs_pwrmon_rail *pwrmon_rails = file_inode(file)->i_private;
> + struct svc_debugfs_pwrmon_rail *pwrmon_rails =
> + file_inode(file)->i_private;
Drop.
> struct gb_svc *svc = pwrmon_rails->svc;
> int ret, desc;
> u32 value;
> @@ -724,7 +726,8 @@ static ssize_t pwr_debugfs_current_read(struct file *file, char __user *buf,
> static ssize_t pwr_debugfs_power_read(struct file *file, char __user *buf,
> size_t len, loff_t *offset)
> {
> - struct svc_debugfs_pwrmon_rail *pwrmon_rails = file_inode(file)->i_private;
> + struct svc_debugfs_pwrmon_rail *pwrmon_rails =
> + file_inode(file)->i_private;
Drop.
> struct gb_svc *svc = pwrmon_rails->svc;
> int ret, desc;
> u32 value;
> @@ -924,14 +927,15 @@ static void gb_svc_process_hello_deferred(struct gb_operation *operation)
> * Power Mode Changes is resolved.
> */
> ret = gb_svc_intf_set_power_mode(svc, svc->ap_intf_id,
> - GB_SVC_UNIPRO_HS_SERIES_A,
> - GB_SVC_UNIPRO_SLOW_AUTO_MODE,
> - 2, 1,
> - GB_SVC_SMALL_AMPLITUDE, GB_SVC_NO_DE_EMPHASIS,
> - GB_SVC_UNIPRO_SLOW_AUTO_MODE,
> - 2, 1,
> - 0, 0,
> - NULL, NULL);
> + GB_SVC_UNIPRO_HS_SERIES_A,
> + GB_SVC_UNIPRO_SLOW_AUTO_MODE,
> + 2, 1,
> + GB_SVC_SMALL_AMPLITUDE,
> + GB_SVC_NO_DE_EMPHASIS,
> + GB_SVC_UNIPRO_SLOW_AUTO_MODE,
> + 2, 1,
> + 0, 0,
> + NULL, NULL);
>
> if (ret)
> dev_warn(&svc->dev,
> diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
> index 6d39f4a..6177b9c 100644
> --- a/drivers/staging/greybus/uart.c
> +++ b/drivers/staging/greybus/uart.c
> @@ -630,8 +630,9 @@ static int get_serial_info(struct gb_tty *gb_tty,
> tmp.xmit_fifo_size = 16;
> tmp.baud_base = 9600;
> tmp.close_delay = gb_tty->port.close_delay / 10;
> - tmp.closing_wait = gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
> - ASYNC_CLOSING_WAIT_NONE : gb_tty->port.closing_wait / 10;
> + tmp.closing_wait =
> + gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
> + ASYNC_CLOSING_WAIT_NONE : gb_tty->port.closing_wait / 10;
Leave as is for now, this should be rewritten without the ternary
operator.
> if (copy_to_user(info, &tmp, sizeof(tmp)))
> return -EFAULT;
> @@ -1000,7 +1001,8 @@ static int gb_tty_init(void)
> gb_tty_driver->subtype = SERIAL_TYPE_NORMAL;
> gb_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> gb_tty_driver->init_termios = tty_std_termios;
> - gb_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL;
> + gb_tty_driver->init_termios.c_cflag = B9600 | CS8 |
> + CREAD | HUPCL | CLOCAL;
Leave as is.
> tty_set_operations(gb_tty_driver, &gb_ops);
>
> retval = tty_register_driver(gb_tty_driver);
> diff --git a/drivers/staging/greybus/vibrator.c b/drivers/staging/greybus/vibrator.c
> index 4ba0e16..77a2365 100644
> --- a/drivers/staging/greybus/vibrator.c
> +++ b/drivers/staging/greybus/vibrator.c
> @@ -70,7 +70,9 @@ static void gb_vibrator_worker(struct work_struct *work)
> {
> struct delayed_work *delayed_work = to_delayed_work(work);
> struct gb_vibrator_device *vib =
> - container_of(delayed_work, struct gb_vibrator_device, delayed_work);
> + container_of(delayed_work,
> + struct gb_vibrator_device,
> + delayed_work);
Split declaration and initialisation here instead.
>
> turn_off(vib);
> }
Johan
prev parent reply other threads:[~2017-02-13 10:41 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-09 16:30 [PATCH 1/2] staging: greybus: fix "line over 80 characters" coding style issues Gioh Kim
2017-02-09 16:30 ` [PATCH 2/2] staging: greybus: fix symbolic permission " Gioh Kim
2017-02-13 10:12 ` Johan Hovold
2017-02-13 10:41 ` Johan Hovold [this message]
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=20170213104125.GB27275@localhost \
--to=johan@kernel.org \
--cc=devel@driverdev.osuosl.org \
--cc=elder@kernel.org \
--cc=gi-oh.kim@profitbricks.com \
--cc=gregkh@linuxfoundation.org \
--cc=greybus-dev@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.