linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Jean Delvare <khali@linux-fr.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>,
	Linux Input <linux-input@vger.kernel.org>,
	Allie Xiong <axiong@synaptics.com>,
	William Manson <wmanson@synaptics.com>,
	Peichen Chang <peichen.chang@synaptics.com>,
	Joerie de Gram <j.de.gram@gmail.com>,
	Wolfram Sang <w.sang@pengutronix.de>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Linus Walleij <linus.walleij@stericsson.com>,
	Naveen Kumar Gaddipati <naveen.gaddipati@stericsson.com>,
	Henrik Rydberg <rydberg@euromail.se>
Subject: Re: [RFC PATCH 8/17] input: RMI4 F09 Built-In Self Test
Date: Mon, 27 Aug 2012 15:07:48 -0700	[thread overview]
Message-ID: <CACRpkdaq7StwG_jg+hZCcU668=VrcUeZa7qP6R_2v9ugkkX38w@mail.gmail.com> (raw)
In-Reply-To: <1345241877-16200-9-git-send-email-cheiny@synaptics.com>

On Fri, Aug 17, 2012 at 3:17 PM, Christopher Heiny <cheiny@synaptics.com> wrote:

Put in a verbose description of what this is.

(...)
> +++ b/drivers/input/rmi4/rmi_f09.c

> +/* data specific to fn $09 that needs to be kept around */
> +struct f09_query {
> +       u8 limit_register_count;
> +       union {
> +               struct {
> +                       u8 result_register_count:3;
> +                       u8 reserved:3;
> +                       u8 internal_limits:1;
> +                       u8 host_test_enable:1;
> +               };
> +               u8 f09_bist_query1;
> +       };
> +};

__attribute__((packed));

?

> +struct f09_control {
> +       union {
> +               struct {
> +                       u8 test1_limit_low:8;
> +                       u8 test1_limit_high:8;
> +                       u8 test1_limit_diff:8;
> +               };
> +               u8 f09_control_test1[3];
> +       };
> +       union {
> +               struct {
> +                       u8 test2_limit_low:8;
> +                       u8 test2_limit_high:8;
> +                       u8 test2_limit_diff:8;
> +               };
> +               u8 f09_control_test2[3];
> +       };
> +};

__attribute__((packed));

?

> +struct f09_data {
> +       u8 test_number_control;
> +       u8 overall_bist_result;
> +       u8 test_result1;
> +       u8 test_result2;
> +       u8 transmitter_number;
> +
> +       union {
> +               struct {
> +                       u8 receiver_number:6;
> +                       u8 limit_failure_code:2;
> +               };
> +               u8 f09_bist_data2;
> +       };
> +};

__attribute__((packed));

?

> +struct f09_cmd {
> +       union {
> +               struct {
> +                       u8 run_bist:1;
> +               };
> +               u8 f09_bist_cmd0;
> +       };
> +};

__attribute__((packed));

?

(...)
> +static struct device_attribute attrs[] = {
> +       __ATTR(status, RMI_RW_ATTR,
> +                  rmi_f09_status_show, rmi_f09_status_store),
> +       __ATTR(limitRegisterCount, RMI_RO_ATTR,
> +              rmi_f09_limit_register_count_show, rmi_store_error),
> +       __ATTR(hostTestEnable, RMI_RW_ATTR,
> +              rmi_f09_host_test_enable_show, rmi_f09_host_test_enable_store),
> +       __ATTR(internalLimits, RMI_RO_ATTR,
> +              rmi_f09_internal_limits_show, rmi_store_error),
> +       __ATTR(resultRegisterCount, RMI_RO_ATTR,
> +              rmi_f09_result_register_count_show, rmi_store_error),
> +       __ATTR(overall_bist_result, RMI_RO_ATTR,
> +              rmi_f09_overall_bist_result_show, rmi_store_error),
> +       __ATTR(test_number_control, RMI_RW_ATTR,
> +              rmi_f09_test_number_control_show,
> +              rmi_f09_test_number_control_store),
> +       __ATTR(test_result1, RMI_RO_ATTR,
> +              rmi_f09_test_result1_show, rmi_store_error),
> +       __ATTR(test_result2, RMI_RO_ATTR,
> +              rmi_f09_test_result2_show, rmi_store_error),
> +       __ATTR(run_bist, RMI_RW_ATTR,
> +              rmi_f09_run_bist_show, rmi_f09_run_bist_store),
> +       __ATTR(f09_control_test1, RMI_RW_ATTR,
> +              rmi_f09_control_test1_show, rmi_f09_control_test1_store),
> +       __ATTR(f09_control_test2, RMI_RW_ATTR,
> +              rmi_f09_control_test2_show, rmi_f09_control_test2_store),
> +};

If this is *only* for tests, then for sure this should be in debugfs?

> +static int rmi_f09_alloc_memory(struct rmi_function_container *fc)
(...)
> +static void rmi_f09_free_memory(struct rmi_function_container *fc)

Why do you need separate functions for these two?

If they are only used from one place (which I suspect) then just
put the code at that site.

(...)
> +static int rmi_f09_initialize(struct rmi_function_container *fc)
> +{
> +       struct rmi_device *rmi_dev = fc->rmi_dev;
> +       struct rmi_device_platform_data *pdata;
> +       struct rmi_fn_09_data *f09 = fc->data;
> +       u16 query_base_addr;
> +       int rc;
> +
> +
> +       pdata = to_rmi_platform_data(rmi_dev);
> +       query_base_addr = fc->fd.query_base_addr;
> +
> +       /* initial all default values for f09 query here */
> +       rc = rmi_read_block(rmi_dev, query_base_addr,
> +               (u8 *)&f09->query, sizeof(f09->query));
> +       if (rc < 0) {
> +               dev_err(&fc->dev, "Failed to read query register."
> +                       " from 0x%04x\n", query_base_addr);
> +               return rc;
> +       }
> +
> +       return 0;
> +}

Similar here. Cannot this be brought into the only call site?

> +static int rmi_f09_config(struct rmi_function_container *fc)
> +{
> +       /*we do nothing here. instead reset should notify the user.*/
> +       return 0;
> +}

Make it optional and just don't define it.

> +static int rmi_f09_reset(struct rmi_function_container *fc)
> +{
> +       struct  rmi_fn_09_data  *instance_data = fc->data;
> +
> +       instance_data->status = -ECONNRESET;
> +
> +       return 0;
> +}

Dito.

(Already remarked this at the last patch.)

Yours,
Linus Walleij

  reply	other threads:[~2012-08-27 22:07 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-17 22:17 [RFC PATCH 00/11] input: Synaptics RMI4 Touchscreen Driver Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 1/17] input: RMI4 public header file and documentation Christopher Heiny
2012-08-22 19:08   ` Linus Walleij
2012-08-22 21:45     ` Dmitry Torokhov
2012-08-23  0:26       ` Christopher Heiny
2012-08-22 23:35     ` Rafael J. Wysocki
2012-08-17 22:17 ` [RFC PATCH 2/17] input: RMI4 core bus and sensor drivers Christopher Heiny
2012-08-23  8:55   ` Linus Walleij
2012-09-25 23:53     ` Christopher Heiny
2012-09-26 11:39       ` Linus Walleij
2012-08-17 22:17 ` [RFC PATCH 3/17] input: RMI4 physical layer drivers for I2C and SPI Christopher Heiny
2012-08-23 13:21   ` Linus Walleij
2012-08-17 22:17 ` [RFC PATCH 4/17] input: RMI4 configs and makefiles Christopher Heiny
2012-08-27 18:39   ` Linus Walleij
2012-08-17 22:17 ` [RFC PATCH 5/17] input: rmidev character driver for RMI4 sensors Christopher Heiny
2012-08-27 18:49   ` Linus Walleij
2012-09-05  0:26     ` Christopher Heiny
2012-09-05  8:29       ` Linus Walleij
2012-08-17 22:17 ` [RFC PATCH 6/17] input: RMI4 firmware update Christopher Heiny
2012-08-27 21:01   ` Linus Walleij
2012-08-17 22:17 ` [RFC PATCH 7/17] input: RMI4 F01 device control Christopher Heiny
2012-08-27 21:59   ` Linus Walleij
2012-08-17 22:17 ` [RFC PATCH 8/17] input: RMI4 F09 Built-In Self Test Christopher Heiny
2012-08-27 22:07   ` Linus Walleij [this message]
2012-09-05  0:21     ` Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 9/17] input: RMI4 F11 multitouch sensing Christopher Heiny
2012-08-27 22:50   ` Linus Walleij
2012-08-17 22:17 ` [RFC PATCH 10/17] input: RM4 F17 Pointing sticks Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 11/17] input: RMI4 F19 capacitive buttons Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 12/17] input: RMI4 F1A simple " Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 13/17] input: RMI4 F21 Force sensing Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 14/17] input: RMI4 F30 GPIO/LED control Christopher Heiny
2012-08-27 22:58   ` Linus Walleij
2012-09-05  0:28     ` Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 15/17] input: RMI4 F34 device reflash Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 16/17] input: RMI4 F41 Active pen 2D input Christopher Heiny
2012-08-17 22:17 ` [RFC PATCH 17/17] input: RMI4 F54 analog data reporting Christopher Heiny
2012-08-27 23:01   ` Linus Walleij
2012-09-05  0:38     ` Christopher Heiny
2012-08-22 12:50 ` [RFC PATCH 00/11] input: Synaptics RMI4 Touchscreen Driver Linus Walleij
2012-08-22 21:29   ` Christopher Heiny
2012-08-27 23:20   ` Christopher Heiny
2012-08-28  0:12     ` Linus Walleij
2012-08-27 23:05 ` Linus Walleij

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='CACRpkdaq7StwG_jg+hZCcU668=VrcUeZa7qP6R_2v9ugkkX38w@mail.gmail.com' \
    --to=linus.walleij@linaro.org \
    --cc=axiong@synaptics.com \
    --cc=cheiny@synaptics.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=j.de.gram@gmail.com \
    --cc=khali@linux-fr.org \
    --cc=linus.walleij@stericsson.com \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=naveen.gaddipati@stericsson.com \
    --cc=peichen.chang@synaptics.com \
    --cc=rydberg@euromail.se \
    --cc=w.sang@pengutronix.de \
    --cc=wmanson@synaptics.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).