* Re: [PATCH 11/14] HID: sony: remove hid_output_raw_report calls
From: Benjamin Tissoires @ 2014-02-13 15:46 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4TjAdiMMPmqX9N__3Bqf2SVu2sV6_P-kOZ0dvZHwXDWwQ@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:47 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> We can not directly change the underlying struct hid_ll_driver here
>> as we did for hdev->hid_output_raw_report.
>> So allocate a struct ll_driver, and replace the old one when removing
>> the device.
>> To get a fully functional driver, we must also split the function
>> sixaxis_usb_output_raw_report() in 2, one regarding output_report, and
>> the other for raw_request.
>
> Sorry, i cannot follow here. Could you explain why this is needed? And
Well, as mentioned in the original comments, the sixaxis has two problem:
- when you send an output report, you should call hid_hw_raw_request
instead (though there is an interrupt output endpoint)
- when you send a raw_request, you should drop the reportID in the
buffer you are sending, but keep it in the SET_REPORT call... (quite
annoying).
Splitting this in two (it was all implemented in hid_output_report)
allows to transparently use the hid_hw_call, not matter who calls
what.
> I also don't think you're supposed to change the ll_driver unlocked.
> The real ll_driver might access it in any way. I don't think we do it
> right now, but it might happen.
Yeah, I am not a big fan of this either. My other concern regarding
that is the dependency against usb, while it could be a uHID device.
>
> Why can't we introduce QUIRK flags that make HID core drop the
> report_id for outgoing messages?
I did not want to come with because it would make the bright new
ll_transport interface ugly, but this may be the only way to have
something generic and drop the usb dependency and the races that may
appears (plus the ugly alloc/free).
Cheers,
Benjamin
>
> Thanks
> David
>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-sony.c | 75 ++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 52 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index d980943..dbbcd0c8 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -507,6 +507,8 @@ struct sony_sc {
>> struct work_struct state_worker;
>> struct power_supply battery;
>>
>> + struct hid_ll_driver *prev_ll_driver;
>> +
>> #ifdef CONFIG_SONY_FF
>> __u8 left;
>> __u8 right;
>> @@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
>>
>> /*
>> * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
>> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
>> + * like it should according to usbhid/hid-core.c::usbhid_output_report()
>> * so we need to override that forcing HID Output Reports on the Control EP.
>> - *
>> - * There is also another issue about HID Output Reports via USB, the Sixaxis
>> - * does not want the report_id as part of the data packet, so we have to
>> - * discard buf[0] when sending the actual control message, even for numbered
>> - * reports, humpf!
>> */
>> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
>> - size_t count, unsigned char report_type)
>> +static int sixaxis_usb_output_report(struct hid_device *hid, __u8 *buf,
>> + size_t count)
>> +{
>> + return hid_hw_raw_request(hid, buf[0], buf, count, HID_OUTPUT_REPORT,
>> + HID_REQ_SET_REPORT);
>> +}
>> +
>> +/*
>> + * There is also another issue about the SET_REPORT command via USB,
>> + * the Sixaxis does not want the report_id as part of the data packet, so
>> + * we have to discard buf[0] when sending the actual control message, even
>> + * for numbered reports, humpf!
>> + */
>> +static int sixaxis_usb_raw_request(struct hid_device *hid,
>> + unsigned char reportnum, __u8 *buf,
>> + size_t len, unsigned char rtype, int reqtype)
>> {
>> struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>> struct usb_device *dev = interface_to_usbdev(intf);
>> struct usb_host_interface *interface = intf->cur_altsetting;
>> - int report_id = buf[0];
>> int ret;
>> + u8 usb_dir;
>> + unsigned int usb_pipe;
>>
>> - if (report_type == HID_OUTPUT_REPORT) {
>> + if (reqtype == HID_REQ_SET_REPORT) {
>> /* Don't send the Report ID */
>> buf++;
>> - count--;
>> + len--;
>> + usb_dir = USB_DIR_OUT;
>> + usb_pipe = usb_sndctrlpipe(dev, 0);
>> + } else {
>> + usb_dir = USB_DIR_IN;
>> + usb_pipe = usb_rcvctrlpipe(dev, 0);
>> }
>>
>> - ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
>> - HID_REQ_SET_REPORT,
>> - USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> - ((report_type + 1) << 8) | report_id,
>> - interface->desc.bInterfaceNumber, buf, count,
>> + ret = usb_control_msg(dev, usb_pipe, reqtype,
>> + usb_dir | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> + ((rtype + 1) << 8) | reportnum,
>> + interface->desc.bInterfaceNumber, buf, len,
>> USB_CTRL_SET_TIMEOUT);
>>
>> - /* Count also the Report ID, in case of an Output report. */
>> - if (ret > 0 && report_type == HID_OUTPUT_REPORT)
>> + /* Count also the Report ID. */
>> + if (ret > 0 && reqtype == HID_REQ_SET_REPORT)
>> ret++;
>>
>> return ret;
>> @@ -1047,7 +1063,7 @@ static void sixaxis_state_worker(struct work_struct *work)
>> buf[10] |= sc->led_state[2] << 3;
>> buf[10] |= sc->led_state[3] << 4;
>>
>> - hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>> + hid_hw_output_report(sc->hdev, buf, sizeof(buf));
>> }
>>
>> static void dualshock4_state_worker(struct work_struct *work)
>> @@ -1254,6 +1270,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> unsigned long quirks = id->driver_data;
>> struct sony_sc *sc;
>> unsigned int connect_mask = HID_CONNECT_DEFAULT;
>> + struct hid_ll_driver *ll_driver;
>>
>> sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
>> if (sc == NULL) {
>> @@ -1285,13 +1302,20 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> }
>>
>> if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
>> - hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
>> + ll_driver = devm_kzalloc(&hdev->dev, sizeof(*ll_driver),
>> + GFP_KERNEL);
>> + if (ll_driver == NULL)
>> + return -ENOMEM;
>> + sc->prev_ll_driver = hdev->ll_driver;
>> + *ll_driver = *hdev->ll_driver;
>> + hdev->ll_driver = ll_driver;
>> + ll_driver->output_report = sixaxis_usb_output_report;
>> + ll_driver->raw_request = sixaxis_usb_raw_request;
>> ret = sixaxis_set_operational_usb(hdev);
>> INIT_WORK(&sc->state_worker, sixaxis_state_worker);
>> - }
>> - else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
>> + } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
>> ret = sixaxis_set_operational_bt(hdev);
>> - else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>> + } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>> /* Report 5 (31 bytes) is used to send data to the controller via USB */
>> ret = sony_set_output_report(sc, 0x05, 248);
>> if (ret < 0)
>> @@ -1339,6 +1363,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> err_close:
>> hid_hw_close(hdev);
>> err_stop:
>> + if (sc->prev_ll_driver)
>> + hdev->ll_driver = sc->prev_ll_driver;
>> if (sc->quirks & SONY_LED_SUPPORT)
>> sony_leds_remove(hdev);
>> if (sc->quirks & SONY_BATTERY_SUPPORT)
>> @@ -1351,6 +1377,9 @@ static void sony_remove(struct hid_device *hdev)
>> {
>> struct sony_sc *sc = hid_get_drvdata(hdev);
>>
>> + if (sc->prev_ll_driver)
>> + hdev->ll_driver = sc->prev_ll_driver;
>> +
>> if (sc->quirks & SONY_LED_SUPPORT)
>> sony_leds_remove(hdev);
>>
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 07/14] HID: input: hid-input remove hid_output_raw_report call
From: Benjamin Tissoires @ 2014-02-13 15:38 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4QvC936A_r6GPgO_gikpXGrD6UwBdBEcwUBwtjxW9iw6Q@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:35 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> hid_output_raw_report() is not a ll_driver callback and should not be used.
>> To keep the same code path than before, we are forced to play with the
>> different hid_hw_* calls: if the usb or i2c device does not support
>> direct output reports, then we will rely on the SET_REPORT HID call.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-input.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index eb00a5b..6b7bdca 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1153,7 +1153,7 @@ static void hidinput_led_worker(struct work_struct *work)
>> led_work);
>> struct hid_field *field;
>> struct hid_report *report;
>> - int len;
>> + int len, ret;
>> __u8 *buf;
>>
>> field = hidinput_get_led_field(hid);
>> @@ -1187,7 +1187,10 @@ static void hidinput_led_worker(struct work_struct *work)
>>
>> hid_output_report(report, buf);
>> /* synchronous output report */
>> - hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
>> + ret = hid_hw_output_report(hid, buf, len);
>> + if (ret == -ENOSYS)
>> + hid_hw_raw_request(hid, buf[0], buf, len, HID_OUTPUT_REPORT,
>> + HID_REQ_SET_REPORT);
>
> Does HID core always set the report-id in buf[0]? Even if none are
> used? I know the incoming data may lack the report-id, but I always
> thought we do the same for outgoing if it's implicit?
oh, yes, you are right. The HID spec says: "The meaning of the request
fields for the Set_Report request is the same as for the Get_Report
request, however the data direction is reversed and the Report Data is
sent from host to device. "
So this is wrong... We should use (report->id) instead of buf[0].
Will fix in v2.
>
> I also already see devices with broken OUTPUT_REPORTs.. I guess at
> some point we have to introduce a quirk-flag to choose between both
> calls. But lets wait for that to happen, maybe we're lucky.
>
>> kfree(buf);
>> }
>>
>> @@ -1266,7 +1269,8 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>> }
>>
>> input_set_drvdata(input_dev, hid);
>> - if (hid->ll_driver->request || hid->hid_output_raw_report)
>> + if (hid->ll_driver->request || hid->ll_driver->output_report ||
>> + hid->ll_driver->raw_request)
>
> Isn't raw_request mandatory? So we could remove that whole if() thing here.
Currently, it's not (see hid_hw_raw_request). However, all the
upstream hid transport drivers implement it.
We can make it mandatory, but we should check it while adding the
device in hid_add_device.
will do for v2 too.
Cheers,
Benjamin
>
> Thanks
> David
>
>> input_dev->event = hidinput_input_event;
>> input_dev->open = hidinput_open;
>> input_dev->close = hidinput_close;
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 03/14] HID: core: implement generic .request()
From: Benjamin Tissoires @ 2014-02-13 15:21 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4TjLJUts20nVYE3WT5FXu_eswKrM0CLwQAu25df3GEDig@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> .request() can be emulated through .raw_request()
>> we can implement this emulation in hid-core, and make .request
>> not mandatory for transport layer drivers.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/hid.h | 5 ++++-
>> 2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 18fe49b..f36778a 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>> }
>> EXPORT_SYMBOL_GPL(hid_output_report);
>>
>> +static int hid_report_len(struct hid_report *report)
>> +{
>> + return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>
> Just for clarity, this is equivalent to the following, right?
>
> return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;
yes, it should (at least that's what I understand too :)
>
> I always have to read that shifting code twice to get it.. Maybe add a
> comment explaining the different entries here.
good idea.
>
>> +}
>> +
>> /*
>> * Allocator for buffer that is going to be passed to hid_output_report()
>> */
>> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
>> * of implement() working on 8 byte chunks
>> */
>>
>> - int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>> + int len = hid_report_len(report);
>>
>> return kmalloc(len, flags);
>> }
>> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>> return report;
>> }
>>
>> +/*
>> + * Implement a generic .request() callback, using .raw_request()
>> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
>> + */
>> +void __hid_request(struct hid_device *hid, struct hid_report *report,
>> + int reqtype)
>> +{
>> + char *buf;
>> + int ret;
>> + int len;
>> +
>> + if (!hid->ll_driver->raw_request)
>> + return;
>> +
>> + buf = hid_alloc_report_buf(report, GFP_KERNEL);
>> + if (!buf)
>> + return;
>> +
>> + len = hid_report_len(report);
actually, after sending the patches, I was wondering if we should use
the +7 in hid_report_len.
"len" is used in .raw_request(), and the +7 was only for the implement(), right?
So maybe a device can reject this because the size of the report is too big...
Jiri, David, any ideas?
Cheers,
Benjamin
>> +
>> + if (reqtype == HID_REQ_SET_REPORT)
>> + hid_output_report(report, buf);
>> +
>> + ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
>> + report->type, reqtype);
>> + if (ret < 0) {
>> + dbg_hid("unable to complete request: %d\n", ret);
>> + goto out;
>> + }
>> +
>> + if (reqtype == HID_REQ_GET_REPORT)
>> + hid_input_report(hid, report->type, buf, ret, 0);
>> +
>> +out:
>> + kfree(buf);
>> +}
>> +EXPORT_SYMBOL_GPL(__hid_request);
>> +
>
> Looks good to me.
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>> int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>> int interrupt)
>> {
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index a837ede..09fbbd7 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>> unsigned int hidinput_count_leds(struct hid_device *hid);
>> __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>> void hid_output_report(struct hid_report *report, __u8 *data);
>> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>> u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>> struct hid_device *hid_allocate_device(void);
>> struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
>> struct hid_report *report, int reqtype)
>> {
>> if (hdev->ll_driver->request)
>> - hdev->ll_driver->request(hdev, report, reqtype);
>> + return hdev->ll_driver->request(hdev, report, reqtype);
>> +
>> + __hid_request(hdev, report, reqtype);
>> }
>>
>> /**
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 12/14] HID: hidraw: replace hid_output_raw_report() calls by appropriates ones
From: Benjamin Tissoires @ 2014-02-13 15:16 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4RvnReiak20x2HPeZ6evYzDuVnLtbT9-WNeZR9yQbeHXg@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Remove hid_output_raw_report() call as it is not a ll_driver callbacj,
>> and switch to the hid_hw_* implementation. USB-HID used to fallback
>> into SET_REPORT when there were no output interrupt endpoint,
>> so emulating this if hid_hw_output_report() returns -ENOSYS.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hidraw.c | 22 +++++++++++++++++-----
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
>> index f8708c9..704718b 100644
>> --- a/drivers/hid/hidraw.c
>> +++ b/drivers/hid/hidraw.c
>> @@ -123,10 +123,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>>
>> dev = hidraw_table[minor]->hid;
>>
>> - if (!dev->hid_output_raw_report) {
>> - ret = -ENODEV;
>> - goto out;
>> - }
>> + if (!dev->ll_driver->raw_request || !dev->ll_driver->output_report)
>> + return -ENODEV;
>
> You still have a "goto out;" above (not visible in the patch). The
> error paths look quite ugly now. I'd prefer consistency here, so
> either keep the jump or fix the error-path above, too. Otherwise looks
> good.
ok, will use "goto out" in the v2 then.
Cheers,
Benjamin
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>>
>> if (count > HID_MAX_BUFFER_SIZE) {
>> hid_warn(dev, "pid %d passed too large report\n",
>> @@ -153,7 +151,21 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>> goto out_free;
>> }
>>
>> - ret = hid_output_raw_report(dev, buf, count, report_type);
>> + if (report_type == HID_OUTPUT_REPORT) {
>> + ret = hid_hw_output_report(dev, buf, count);
>> + /*
>> + * compatibility with old implementation of USB-HID and I2C-HID:
>> + * if the device does not support receiving output reports,
>> + * on an interrupt endpoint, then fallback to the SET_REPORT
>> + * HID command.
>> + */
>> + if (ret != -ENOSYS)
>> + goto out_free;
>> + }
>> +
>> + ret = hid_hw_raw_request(dev, buf[0], buf, count, report_type,
>> + HID_REQ_SET_REPORT);
>> +
>> out_free:
>> kfree(buf);
>> out:
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 05/14] HID: i2c-hid: use generic .request() implementation
From: Benjamin Tissoires @ 2014-02-13 15:14 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4SYvLAhH3WqGNCZBrxwd8RiwL-wH90tZP9Je1doDxvhXQ@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:30 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Having our own .request() implementation does not give anything,
>> so use the generic binding.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
>> 1 file changed, 31 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 07a054a..925fb8d 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -632,36 +632,6 @@ static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
>> }
>> }
>>
>> -static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
>> - int reqtype)
>> -{
>> - struct i2c_client *client = hid->driver_data;
>> - char *buf;
>> - int ret;
>> - int len = i2c_hid_get_report_length(rep) - 2;
>> -
>> - buf = kzalloc(len, GFP_KERNEL);
>
> Haven't you recently fixed this to use hid_alloc_buffer()? Anyhow,
yes, it has been fixed. But Jiri carried it through a *-upstream
branch, which is not merged into his for-next currently (or at the
time I sent the patch). hopefully the merge will be easy, or I can
wait for it to be in for-next before resending a smoother v2.
Cheers,
Benjamin
> patch obviously looks good:
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>> - if (!buf)
>> - return;
>> -
>> - switch (reqtype) {
>> - case HID_REQ_GET_REPORT:
>> - ret = i2c_hid_get_raw_report(hid, rep->id, buf, len, rep->type);
>> - if (ret < 0)
>> - dev_err(&client->dev, "%s: unable to get report: %d\n",
>> - __func__, ret);
>> - else
>> - hid_input_report(hid, rep->type, buf, ret, 0);
>> - break;
>> - case HID_REQ_SET_REPORT:
>> - hid_output_report(rep, buf);
>> - i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
>> - break;
>> - }
>> -
>> - kfree(buf);
>> -}
>> -
>> static int i2c_hid_parse(struct hid_device *hid)
>> {
>> struct i2c_client *client = hid->driver_data;
>> @@ -817,7 +787,6 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
>> .open = i2c_hid_open,
>> .close = i2c_hid_close,
>> .power = i2c_hid_power,
>> - .request = i2c_hid_request,
>> .output_report = i2c_hid_output_report,
>> .raw_request = i2c_hid_raw_request,
>> };
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH v2 0/9] HID: spring cleanup v2
From: Benjamin Tissoires @ 2014-02-13 15:11 UTC (permalink / raw)
To: David Herrmann, Nestor Lopez Casado
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4RvavkaPvz8vozUvgOs6fSmYNR=2=sn76aFFCbR5caaBA@mail.gmail.com>
Hi David,
On Wed, Feb 12, 2014 at 5:13 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Feb 5, 2014 at 10:33 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Hi guys,
>>
>> well, here comes the promised v2 of the ll_transport cleanup.
>>
>> As I said, I removed patches which need some more work, and kept only the
>> trivial ones. I also added David's documentation, which gives us a net
>> difference of +210 lines of code :(
>> Let's say that we still have a net worth of -106 lines of actual code :)
>>
>> Cheers,
>> Benjamin
>>
>> Changes since v1:
>> - removed uhid, i2c-hid patches
>> - removed the previous 11/11 (move hid_output_raw_report to hid_ll_driver)
>> - hid-logitech-dj: use hid_hw_raw_request instead of hid_output_report (2/9)
>> - add documentation - I removed the hid_input_event() doc (9/9)
>>
>> Benjamin Tissoires (9):
>> HID: add inliners for ll_driver transport-layer callbacks
>> HID: logitech-dj: remove hidinput_input_event
>
> Apart from this logitech-dj patch, which I feel uncomfortable with
> reviewing if I don't have the hardware, this series looks really good.
> I think all patches already carry my r-b, otherwise feel free to add
> them.
Thanks a lot for the review!
I tested the logitech one with a TK820 which has a LED on the
capslock, and it's working fine. Still adding Nestor in the loop if he
manages to find some time to review it.
Nestor, could you have a look at 2/9
(https://patchwork.kernel.org/patch/3591381/)?
Cheers,
Benjamin
>
> Thanks a lot for the cleanup!
> David
>
>> HID: HIDp: remove hidp_hidinput_event
>> HID: remove hidinput_input_event handler
>> HID: HIDp: remove duplicated coded
>> HID: usbhid: remove duplicated code
>> HID: remove hid_get_raw_report in struct hid_device
>> HID: introduce helper to access hid_output_raw_report()
>> HID: Add HID transport driver documentation
>>
>> Documentation/hid/hid-transport.txt | 316 ++++++++++++++++++++++++++++++++++++
>> drivers/hid/hid-input.c | 12 +-
>> drivers/hid/hid-lg.c | 6 +-
>> drivers/hid/hid-logitech-dj.c | 106 +++++-------
>> drivers/hid/hid-magicmouse.c | 2 +-
>> drivers/hid/hid-sony.c | 9 +-
>> drivers/hid/hid-thingm.c | 4 +-
>> drivers/hid/hid-wacom.c | 16 +-
>> drivers/hid/hid-wiimote-core.c | 2 +-
>> drivers/hid/hidraw.c | 9 +-
>> drivers/hid/i2c-hid/i2c-hid.c | 1 -
>> drivers/hid/uhid.c | 1 -
>> drivers/hid/usbhid/hid-core.c | 65 ++------
>> include/linux/hid.h | 68 +++++++-
>> net/bluetooth/hidp/core.c | 115 ++-----------
>> 15 files changed, 471 insertions(+), 261 deletions(-)
>> create mode 100644 Documentation/hid/hid-transport.txt
>>
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 2/2] gpio: adp5588 - add support for gpio names
From: Linus Walleij @ 2014-02-13 12:53 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Michael Hennerich, Dmitry Torokhov, dtor, Alexandre Courbot,
linux-gpio@vger.kernel.org, Linux Input
In-Reply-To: <1392053045-12121-2-git-send-email-jeff.dagenais@gmail.com>
On Mon, Feb 10, 2014 at 6:24 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
> which is already found in the common header for adp5588
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Patch applied with Michael's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 1/2] gpio: adp5588 - use "unsigned" for the setup and teardown callbacks
From: Linus Walleij @ 2014-02-13 12:52 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Michael Hennerich, Dmitry Torokhov, dtor, Alexandre Courbot,
linux-gpio@vger.kernel.org, Linux Input
In-Reply-To: <1392053045-12121-1-git-send-email-jeff.dagenais@gmail.com>
On Mon, Feb 10, 2014 at 6:24 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
> to comply with the rest of the GPIO drivers.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Patch applied with Michael's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 1/2] gpio: adp5588: get value from data out when dir is out
From: Linus Walleij @ 2014-02-13 12:50 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Michael Hennerich, Dmitry Torokhov, dtor, Alexandre Courbot,
linux-gpio@vger.kernel.org, Linux Input
In-Reply-To: <1392051929-32290-1-git-send-email-jeff.dagenais@gmail.com>
On Mon, Feb 10, 2014 at 6:05 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
> As discussed here: http://ez.analog.com/message/35852,
> the 5587 revC and 5588 revB spec sheets contain a mistake
> in the GPIO_DAT_STATx register description.
>
> According to R.Shnell at ADI, as well as my own
> observations, it should read:
> "GPIO data status (shows GPIO state when read for inputs)".
>
> This commit changes the get value function accordingly.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Patch applied with Michael's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 3/4] input: sirfsoc-onkey - use dev_get_drvdata instead of platform_get_drvdata
From: Barry Song @ 2014-02-13 7:47 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, DL-SHA-WorkGroupLinux, Xianglong Du,
Barry Song
In-Reply-To: <20140213072459.GB23392@core.coreip.homeip.net>
2014-02-13 15:24 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Thu, Feb 13, 2014 at 02:40:23PM +0800, Barry Song wrote:
>> From: Xianglong Du <Xianglong.Du@csr.com>
>>
>> In resume entry, use dev_get_drvdata() instead of to_platform_device(dev) +
>> platform_get_drvdata(pdev).
>>
>> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>> drivers/input/misc/sirfsoc-onkey.c | 3 +--
>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
>> index 097c10a..8e45bf11 100644
>> --- a/drivers/input/misc/sirfsoc-onkey.c
>> +++ b/drivers/input/misc/sirfsoc-onkey.c
>> @@ -157,8 +157,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
>> #ifdef CONFIG_PM_SLEEP
>> static int sirfsoc_pwrc_resume(struct device *dev)
>> {
>> - struct platform_device *pdev = to_platform_device(dev);
>> - struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
>> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(dev);
>
> I believe we should use accessors matching the object type. Currently
> dev_get_drvdata and platform_get_drvdata return the same object, but
> they do not have to.
>
> IOW I prefer the original code.
i did see many commits in kernel which did same jobs with this one. e.g:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a1216394e620d0dfbb03c712ae3210e7b77c9e11
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bab8b563ef08455440badca7fe79b2c700bd1b75
...
in my understand, the changed context is a general pm_ops to general
"driver" and not a legacy suspend/resume in "platform_driver" bound
with pdev, so in the context, we are caring about "device" more than
"pdev".
how do you think if i do a more change in probe() with this by:
- platform_set_drvdata(pdev, pwrcdrv);
+ dev_set_drvdata(&pdev->dev, pwrcdrv);
would this make everything look fine?
>
> Thanks.
>
> --
> Dmitry
-barry
^ permalink raw reply
* Re: [PATCH v2] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-13 7:30 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, DL-SHA-WorkGroupLinux, Xianglong Du,
Rongjun Ying, Barry Song
In-Reply-To: <20140213072339.GA23392@core.coreip.homeip.net>
2014-02-13 15:23 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>
> On Thu, Feb 13, 2014 at 02:38:56PM +0800, Barry Song wrote:
> > From: Xianglong Du <Xianglong.Du@csr.com>
> >
> > this patch adds a delayed_work to detect the untouch of onkey since HW will
> > not generate interrupt for it.
> >
> > at the same time, we move the KEY event to POWER instead of SUSPEND, which
> > will be suitable for both Android and Linux. Userspace PowerManager Daemon
> > will decide to suspend or shutdown based on how long we have touched onkey
> >
> > Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
> > Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---
> > -v2:
> > avoid the race of reschedule the work in remove;
> > fix the typo about reporting KEY_POWER;
> >
> > drivers/input/misc/sirfsoc-onkey.c | 60 +++++++++++++++++++++++++++---------
> > 1 files changed, 45 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> > index e8897c3..4d13903 100644
> > --- a/drivers/input/misc/sirfsoc-onkey.c
> > +++ b/drivers/input/misc/sirfsoc-onkey.c
> > @@ -13,16 +13,45 @@
> > #include <linux/input.h>
> > #include <linux/rtc/sirfsoc_rtciobrg.h>
> > #include <linux/of.h>
> > +#include <linux/workqueue.h>
> >
> > struct sirfsoc_pwrc_drvdata {
> > u32 pwrc_base;
> > struct input_dev *input;
> > + int irq;
> > + struct delayed_work work;
> > };
> >
> > #define PWRC_ON_KEY_BIT (1 << 0)
> >
> > #define PWRC_INT_STATUS 0xc
> > #define PWRC_INT_MASK 0x10
> > +#define PWRC_PIN_STATUS 0x14
> > +#define PWRC_KEY_DETECT_UP_TIME 20 /* ms*/
> > +
> > +static inline int sirfsoc_pwrc_is_on_key_down(
> > + struct sirfsoc_pwrc_drvdata *pwrcdrv)
> > +{
> > + int state = sirfsoc_rtc_iobrg_readl(
> > + pwrcdrv->pwrc_base + PWRC_PIN_STATUS)
> > + & PWRC_ON_KEY_BIT;
> > + return !state; /* ON_KEY is active low */
> > +}
> > +
> > +static void sirfsoc_pwrc_report_event(struct work_struct *work)
> > +{
> > + struct sirfsoc_pwrc_drvdata *pwrcdrv =
> > + container_of((struct delayed_work *)work,
> > + struct sirfsoc_pwrc_drvdata, work);
> > +
> > + if (!sirfsoc_pwrc_is_on_key_down(pwrcdrv)) {
> > + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 0);
> > + input_sync(pwrcdrv->input);
> > + } else {
> > + schedule_delayed_work(&pwrcdrv->work,
> > + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
> > + }
> > +}
> >
> > static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> > {
> > @@ -34,17 +63,11 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> > sirfsoc_rtc_iobrg_writel(int_status & ~PWRC_ON_KEY_BIT,
> > pwrcdrv->pwrc_base + PWRC_INT_STATUS);
> >
> > - /*
> > - * For a typical Linux system, we report KEY_SUSPEND to trigger apm-power.c
> > - * to queue a SUSPEND APM event
> > - */
> > - input_event(pwrcdrv->input, EV_PWR, KEY_SUSPEND, 1);
> > - input_sync(pwrcdrv->input);
> >
> > - /*
> > - * Todo: report KEY_POWER event for Android platforms, Android PowerManager
> > - * will handle the suspend and powerdown/hibernation
> > - */
> > + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 1);
> > + input_sync(pwrcdrv->input);
> > + schedule_delayed_work(&pwrcdrv->work,
> > + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
> >
> > return IRQ_HANDLED;
> > }
> > @@ -59,7 +82,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> > {
> > struct device_node *np = pdev->dev.of_node;
> > struct sirfsoc_pwrc_drvdata *pwrcdrv;
> > - int irq;
> > int error;
> >
> > pwrcdrv = devm_kzalloc(&pdev->dev, sizeof(struct sirfsoc_pwrc_drvdata),
> > @@ -86,15 +108,17 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >
> > pwrcdrv->input->name = "sirfsoc pwrckey";
> > pwrcdrv->input->phys = "pwrc/input0";
> > - pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
> > + pwrcdrv->input->evbit[0] = BIT_MASK(EV_KEY);
> >
> > - irq = platform_get_irq(pdev, 0);
> > - error = devm_request_irq(&pdev->dev, irq,
> > + INIT_DELAYED_WORK(&pwrcdrv->work, sirfsoc_pwrc_report_event);
> > +
> > + pwrcdrv->irq = platform_get_irq(pdev, 0);
> > + error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
> > sirfsoc_pwrc_isr, IRQF_SHARED,
> > "sirfsoc_pwrc_int", pwrcdrv);
> > if (error) {
> > dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
> > - irq, error);
> > + pwrcdrv->irq, error);
> > return error;
> > }
>
> From this point on you should ensure that work is cancelled in error
> unwinding path, similarly to sirfsoc_pwrc_probe().
>
> I think custom devm action is way to go - it follows the devm model
> instead of going against it with forced devm_free_irq().
yes. in case probe fails.
>
> Thanks.
>
> --
> Dmitry
-barry
^ permalink raw reply
* Re: [PATCH 3/4] input: sirfsoc-onkey - use dev_get_drvdata instead of platform_get_drvdata
From: Dmitry Torokhov @ 2014-02-13 7:24 UTC (permalink / raw)
To: Barry Song; +Cc: linux-input, workgroup.linux, Xianglong Du, Barry Song
In-Reply-To: <1392273624-22920-4-git-send-email-21cnbao@gmail.com>
On Thu, Feb 13, 2014 at 02:40:23PM +0800, Barry Song wrote:
> From: Xianglong Du <Xianglong.Du@csr.com>
>
> In resume entry, use dev_get_drvdata() instead of to_platform_device(dev) +
> platform_get_drvdata(pdev).
>
> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
> drivers/input/misc/sirfsoc-onkey.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> index 097c10a..8e45bf11 100644
> --- a/drivers/input/misc/sirfsoc-onkey.c
> +++ b/drivers/input/misc/sirfsoc-onkey.c
> @@ -157,8 +157,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
> #ifdef CONFIG_PM_SLEEP
> static int sirfsoc_pwrc_resume(struct device *dev)
> {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(dev);
I believe we should use accessors matching the object type. Currently
dev_get_drvdata and platform_get_drvdata return the same object, but
they do not have to.
IOW I prefer the original code.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Dmitry Torokhov @ 2014-02-13 7:23 UTC (permalink / raw)
To: Barry Song
Cc: linux-input, workgroup.linux, Xianglong Du, Rongjun Ying,
Barry Song
In-Reply-To: <1392273536-22848-1-git-send-email-21cnbao@gmail.com>
On Thu, Feb 13, 2014 at 02:38:56PM +0800, Barry Song wrote:
> From: Xianglong Du <Xianglong.Du@csr.com>
>
> this patch adds a delayed_work to detect the untouch of onkey since HW will
> not generate interrupt for it.
>
> at the same time, we move the KEY event to POWER instead of SUSPEND, which
> will be suitable for both Android and Linux. Userspace PowerManager Daemon
> will decide to suspend or shutdown based on how long we have touched onkey
>
> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
> -v2:
> avoid the race of reschedule the work in remove;
> fix the typo about reporting KEY_POWER;
>
> drivers/input/misc/sirfsoc-onkey.c | 60 +++++++++++++++++++++++++++---------
> 1 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> index e8897c3..4d13903 100644
> --- a/drivers/input/misc/sirfsoc-onkey.c
> +++ b/drivers/input/misc/sirfsoc-onkey.c
> @@ -13,16 +13,45 @@
> #include <linux/input.h>
> #include <linux/rtc/sirfsoc_rtciobrg.h>
> #include <linux/of.h>
> +#include <linux/workqueue.h>
>
> struct sirfsoc_pwrc_drvdata {
> u32 pwrc_base;
> struct input_dev *input;
> + int irq;
> + struct delayed_work work;
> };
>
> #define PWRC_ON_KEY_BIT (1 << 0)
>
> #define PWRC_INT_STATUS 0xc
> #define PWRC_INT_MASK 0x10
> +#define PWRC_PIN_STATUS 0x14
> +#define PWRC_KEY_DETECT_UP_TIME 20 /* ms*/
> +
> +static inline int sirfsoc_pwrc_is_on_key_down(
> + struct sirfsoc_pwrc_drvdata *pwrcdrv)
> +{
> + int state = sirfsoc_rtc_iobrg_readl(
> + pwrcdrv->pwrc_base + PWRC_PIN_STATUS)
> + & PWRC_ON_KEY_BIT;
> + return !state; /* ON_KEY is active low */
> +}
> +
> +static void sirfsoc_pwrc_report_event(struct work_struct *work)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv =
> + container_of((struct delayed_work *)work,
> + struct sirfsoc_pwrc_drvdata, work);
> +
> + if (!sirfsoc_pwrc_is_on_key_down(pwrcdrv)) {
> + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 0);
> + input_sync(pwrcdrv->input);
> + } else {
> + schedule_delayed_work(&pwrcdrv->work,
> + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
> + }
> +}
>
> static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> {
> @@ -34,17 +63,11 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> sirfsoc_rtc_iobrg_writel(int_status & ~PWRC_ON_KEY_BIT,
> pwrcdrv->pwrc_base + PWRC_INT_STATUS);
>
> - /*
> - * For a typical Linux system, we report KEY_SUSPEND to trigger apm-power.c
> - * to queue a SUSPEND APM event
> - */
> - input_event(pwrcdrv->input, EV_PWR, KEY_SUSPEND, 1);
> - input_sync(pwrcdrv->input);
>
> - /*
> - * Todo: report KEY_POWER event for Android platforms, Android PowerManager
> - * will handle the suspend and powerdown/hibernation
> - */
> + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 1);
> + input_sync(pwrcdrv->input);
> + schedule_delayed_work(&pwrcdrv->work,
> + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
>
> return IRQ_HANDLED;
> }
> @@ -59,7 +82,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct sirfsoc_pwrc_drvdata *pwrcdrv;
> - int irq;
> int error;
>
> pwrcdrv = devm_kzalloc(&pdev->dev, sizeof(struct sirfsoc_pwrc_drvdata),
> @@ -86,15 +108,17 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>
> pwrcdrv->input->name = "sirfsoc pwrckey";
> pwrcdrv->input->phys = "pwrc/input0";
> - pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
> + pwrcdrv->input->evbit[0] = BIT_MASK(EV_KEY);
>
> - irq = platform_get_irq(pdev, 0);
> - error = devm_request_irq(&pdev->dev, irq,
> + INIT_DELAYED_WORK(&pwrcdrv->work, sirfsoc_pwrc_report_event);
> +
> + pwrcdrv->irq = platform_get_irq(pdev, 0);
> + error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
> sirfsoc_pwrc_isr, IRQF_SHARED,
> "sirfsoc_pwrc_int", pwrcdrv);
> if (error) {
> dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
> - irq, error);
> + pwrcdrv->irq, error);
> return error;
> }
>From this point on you should ensure that work is cancelled in error
unwinding path, similarly to sirfsoc_pwrc_probe().
I think custom devm action is way to go - it follows the devm model
instead of going against it with forced devm_free_irq().
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-13 6:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
DL-SHA-WorkGroupLinux, Xianglong Du, Rongjun Ying, Barry Song
In-Reply-To: <892ec8be-638e-48d6-9e7d-5175694a7eaf@email.android.com>
2014-02-13 11:41 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On February 12, 2014 6:32:03 PM PST, Barry Song <21cnbao@gmail.com> wrote:
>>2014-02-13 7:11 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>>> Hi Barry,
>>>
>>> On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote:
>>>>
>>>> static int sirfsoc_pwrc_remove(struct platform_device *pdev)
>>>> {
>>>> + struct sirfsoc_pwrc_drvdata *pwrcdrv =
>>dev_get_drvdata(&pdev->dev);
>>>> +
>>>> device_init_wakeup(&pdev->dev, 0);
>>>>
>>>> + cancel_delayed_work_sync(&pwrcdrv->work);
>>>> +
>>>
>>> This is racy: interrupt is freed later and can schedule work again.
>>
>>thanks, Dmitry. i will do a manual devm_free_irq() before cancelling
>>the work and before devres removes the resources.
>
> Another option would be to use devm custom action to ensure that work is canceled after freeing IRQ.
yes. you did a great job to have a devm_add_action() API.
>
>
> --
> Dmitry
-barry
^ permalink raw reply
* [PATCH 4/4] input: sirfsoc-onkey - update copyright years to 2014
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Barry Song <Baohua.Song@csr.com>
Happy the year of horse, 2014.
,((((^`\
(((( (6 \
,((((( , \
,,,_ ,((((( /"._ ,`,
((((\\ ,... ,(((( / `-.-'
))) ;' `"'"'""(((( (
((( / ((( \
)) | |
(( | . ' |
)) \ _ ' `t ,.')
( | y;- -,-""'"-.\ \/
) / ./ ) / `\ \
|./ ( ( / /'
|| \\ //'|
|| \\ _//'||
|| )) |_/ ||
\_\ |_/ ||
`'" \_\
`'"
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 8e45bf11..1a11207 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -1,7 +1,8 @@
/*
* Power key driver for SiRF PrimaII
*
- * Copyright (c) 2013 Cambridge Silicon Radio Limited, a CSR plc group company.
+ * Copyright (c) 2013 - 2014 Cambridge Silicon Radio Limited, a CSR plc group
+ * company.
*
* Licensed under GPLv2 or later.
*/
--
1.7.5.4
^ permalink raw reply related
* [PATCH 3/4] input: sirfsoc-onkey - use dev_get_drvdata instead of platform_get_drvdata
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Xianglong Du, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Xianglong Du <Xianglong.Du@csr.com>
In resume entry, use dev_get_drvdata() instead of to_platform_device(dev) +
platform_get_drvdata(pdev).
Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 097c10a..8e45bf11 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -157,8 +157,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
#ifdef CONFIG_PM_SLEEP
static int sirfsoc_pwrc_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
+ struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(dev);
/*
* Do not mask pwrc interrupt as we want pwrc work as a wakeup source
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/4] input: sirfsoc-onkey - namespace pwrc_resume function
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Xianglong Du, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Xianglong Du <Xianglong.Du@csr.com>
this function lost namespace, this patch fixes it.
Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 4d54744..097c10a 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -155,7 +155,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-static int pwrc_resume(struct device *dev)
+static int sirfsoc_pwrc_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
@@ -173,7 +173,7 @@ static int pwrc_resume(struct device *dev)
}
#endif
-static SIMPLE_DEV_PM_OPS(sirfsoc_pwrc_pm_ops, NULL, pwrc_resume);
+static SIMPLE_DEV_PM_OPS(sirfsoc_pwrc_pm_ops, NULL, sirfsoc_pwrc_resume);
static struct platform_driver sirfsoc_pwrc_driver = {
.probe = sirfsoc_pwrc_probe,
--
1.7.5.4
^ permalink raw reply related
* [PATCH 1/4] input: sirfsoc-onkey - drop the IRQF_SHARED flag
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Barry Song <Baohua.Song@csr.com>
since the irq handler always return IRQ_HANDLED, it means this irq is not
a shared IRQ at all. or at least, the SW is not self-consistent now.
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 4d13903..4d54744 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -114,7 +114,7 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
pwrcdrv->irq = platform_get_irq(pdev, 0);
error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
- sirfsoc_pwrc_isr, IRQF_SHARED,
+ sirfsoc_pwrc_isr, 0,
"sirfsoc_pwrc_int", pwrcdrv);
if (error) {
dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
--
1.7.5.4
^ permalink raw reply related
* [PATCH 0/4] input: sirfsoc-onkey - a bundle of minor clean or fix
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Barry Song
From: Barry Song <Baohua.Song@csr.com>
Hi Dmitry,
this patchset depends on Xianglong's
"[PATCH v2] input: sirfsoc-onkey - report onkey untouch event by
detecting pin status"
Barry Song (2):
input: sirfsoc-onkey - drop the IRQF_SHARED flag
input: sirfsoc-onkey - update copyright years to 2014
Xianglong Du (2):
input: sirfsoc-onkey - namespace pwrc_resume function
input: sirfsoc-onkey - use dev_get_drvdata instead of
platform_get_drvdata
drivers/input/misc/sirfsoc-onkey.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
--
1.7.5.4
^ permalink raw reply
* [PATCH v2] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-13 6:38 UTC (permalink / raw)
To: dmitry.torokhov, linux-input
Cc: workgroup.linux, Xianglong Du, Rongjun Ying, Barry Song
From: Xianglong Du <Xianglong.Du@csr.com>
this patch adds a delayed_work to detect the untouch of onkey since HW will
not generate interrupt for it.
at the same time, we move the KEY event to POWER instead of SUSPEND, which
will be suitable for both Android and Linux. Userspace PowerManager Daemon
will decide to suspend or shutdown based on how long we have touched onkey
Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
-v2:
avoid the race of reschedule the work in remove;
fix the typo about reporting KEY_POWER;
drivers/input/misc/sirfsoc-onkey.c | 60 +++++++++++++++++++++++++++---------
1 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index e8897c3..4d13903 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -13,16 +13,45 @@
#include <linux/input.h>
#include <linux/rtc/sirfsoc_rtciobrg.h>
#include <linux/of.h>
+#include <linux/workqueue.h>
struct sirfsoc_pwrc_drvdata {
u32 pwrc_base;
struct input_dev *input;
+ int irq;
+ struct delayed_work work;
};
#define PWRC_ON_KEY_BIT (1 << 0)
#define PWRC_INT_STATUS 0xc
#define PWRC_INT_MASK 0x10
+#define PWRC_PIN_STATUS 0x14
+#define PWRC_KEY_DETECT_UP_TIME 20 /* ms*/
+
+static inline int sirfsoc_pwrc_is_on_key_down(
+ struct sirfsoc_pwrc_drvdata *pwrcdrv)
+{
+ int state = sirfsoc_rtc_iobrg_readl(
+ pwrcdrv->pwrc_base + PWRC_PIN_STATUS)
+ & PWRC_ON_KEY_BIT;
+ return !state; /* ON_KEY is active low */
+}
+
+static void sirfsoc_pwrc_report_event(struct work_struct *work)
+{
+ struct sirfsoc_pwrc_drvdata *pwrcdrv =
+ container_of((struct delayed_work *)work,
+ struct sirfsoc_pwrc_drvdata, work);
+
+ if (!sirfsoc_pwrc_is_on_key_down(pwrcdrv)) {
+ input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 0);
+ input_sync(pwrcdrv->input);
+ } else {
+ schedule_delayed_work(&pwrcdrv->work,
+ msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
+ }
+}
static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
{
@@ -34,17 +63,11 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
sirfsoc_rtc_iobrg_writel(int_status & ~PWRC_ON_KEY_BIT,
pwrcdrv->pwrc_base + PWRC_INT_STATUS);
- /*
- * For a typical Linux system, we report KEY_SUSPEND to trigger apm-power.c
- * to queue a SUSPEND APM event
- */
- input_event(pwrcdrv->input, EV_PWR, KEY_SUSPEND, 1);
- input_sync(pwrcdrv->input);
- /*
- * Todo: report KEY_POWER event for Android platforms, Android PowerManager
- * will handle the suspend and powerdown/hibernation
- */
+ input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 1);
+ input_sync(pwrcdrv->input);
+ schedule_delayed_work(&pwrcdrv->work,
+ msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
return IRQ_HANDLED;
}
@@ -59,7 +82,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct sirfsoc_pwrc_drvdata *pwrcdrv;
- int irq;
int error;
pwrcdrv = devm_kzalloc(&pdev->dev, sizeof(struct sirfsoc_pwrc_drvdata),
@@ -86,15 +108,17 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
pwrcdrv->input->name = "sirfsoc pwrckey";
pwrcdrv->input->phys = "pwrc/input0";
- pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
+ pwrcdrv->input->evbit[0] = BIT_MASK(EV_KEY);
- irq = platform_get_irq(pdev, 0);
- error = devm_request_irq(&pdev->dev, irq,
+ INIT_DELAYED_WORK(&pwrcdrv->work, sirfsoc_pwrc_report_event);
+
+ pwrcdrv->irq = platform_get_irq(pdev, 0);
+ error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
sirfsoc_pwrc_isr, IRQF_SHARED,
"sirfsoc_pwrc_int", pwrcdrv);
if (error) {
dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
- irq, error);
+ pwrcdrv->irq, error);
return error;
}
@@ -119,8 +143,14 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
static int sirfsoc_pwrc_remove(struct platform_device *pdev)
{
+ struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(&pdev->dev);
+
device_init_wakeup(&pdev->dev, 0);
+ devm_free_irq(&pdev->dev, pwrcdrv->irq, pwrcdrv);
+
+ cancel_delayed_work_sync(&pwrcdrv->work);
+
return 0;
}
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH 01/15] Input: synaptics-rmi4 - fix checkpatch.pl, sparse and GCC warnings
From: Dmitry Torokhov @ 2014-02-13 6:36 UTC (permalink / raw)
To: Christopher Heiny; +Cc: Courtney Cavin, linux-input
In-Reply-To: <52F2E709.5060007@synaptics.com>
On Wed, Feb 05, 2014 at 05:36:09PM -0800, Christopher Heiny wrote:
> On 02/05/2014 05:09 PM, Dmitry Torokhov wrote:
> >On Tue, Feb 04, 2014 at 03:08:12PM -0800, Christopher Heiny wrote:
> >>>On 01/23/2014 04:00 PM, Courtney Cavin wrote:
> >>>> >Cc: Christopher Heiny<cheiny@synaptics.com>
> >>>> >Cc: Dmitry Torokhov<dmitry.torokhov@gmail.com>
> >>>> >Signed-off-by: Courtney Cavin<courtney.cavin@sonymobile.com>
> >>>> >---
> >>>> > drivers/input/rmi4/rmi_bus.c | 4 ++--
> >>>> > drivers/input/rmi4/rmi_bus.h | 2 +-
> >>>> > drivers/input/rmi4/rmi_driver.c | 17 ++++++++++++-----
> >>>> > drivers/input/rmi4/rmi_f11.c | 4 +++-
> >>>> > 4 files changed, 18 insertions(+), 9 deletions(-)
> >>>> >
> >>>> >diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> >>>> >index 96a76e7..8a939f3 100644
> >>>> >--- a/drivers/input/rmi4/rmi_bus.c
> >>>> >+++ b/drivers/input/rmi4/rmi_bus.c
> >>>> >@@ -37,7 +37,7 @@ static void rmi_release_device(struct device *dev)
> >>>> > kfree(rmi_dev);
> >>>> > }
> >>>> >
> >>>> >-struct device_type rmi_device_type = {
> >>>> >+static struct device_type rmi_device_type = {
> >>>> > .name = "rmi_sensor",
> >>>> > .release = rmi_release_device,
> >>>> > };
> >>>
> >>>This struct is used by diagnostic modules to identify sensor
> >>>devices, so it cannot be static.
> >
> >Then we need to declare it somewhere or provide an accessor function.
>
> Currently it's in a header not included in the patches. We'll move
> it to rmi_bus.h.
Hmm, we do have rmi_is_physical_device() to identify whether it is a
sensor or a function, so I believe we should mark all structures static
to avoid anyone poking at them.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Dmitry Torokhov @ 2014-02-13 6:15 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires, David Herrmann, Jiri Kosina, Courtney Cavin
In-Reply-To: <52FC2E68.1060709@synaptics.com>
On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote:
> Input: synaptics-rmi4 - Use put_device() and device_type.release()
> to free storage.
>
> From: Christopher Heiny <cheiny@synaptics.com>
>
> For rmi_sensor and rmi_function device_types, use put_device() and
> the assocated device_type.release() function to clean up related
> structures and storage in the correct and safe order.
>
> Allocate irq_mask as part of struct rmi_function.
>
> Delete unused rmi_driver_irq_get_mask() function.
>
> Suggested-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>
> ---
>
> drivers/input/rmi4/rmi_bus.c | 17 ++++++++-------
> drivers/input/rmi4/rmi_bus.h | 2 +-
> drivers/input/rmi4/rmi_driver.c | 46
> +++++------------------------------------
> 3 files changed, 15 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 96a76e7..6342b45 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -34,6 +34,7 @@ static struct dentry *rmi_debugfs_root;
> static void rmi_release_device(struct device *dev)
> {
> struct rmi_device *rmi_dev = to_rmi_device(dev);
> +
> kfree(rmi_dev);
> }
>
> @@ -94,8 +95,7 @@ int rmi_register_transport_device(struct
> rmi_transport_dev *xport)
> return -EINVAL;
> }
>
> - rmi_dev = devm_kzalloc(xport->dev,
> - sizeof(struct rmi_device), GFP_KERNEL);
> + rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
> if (!rmi_dev)
> return -ENOMEM;
>
> @@ -112,8 +112,10 @@ int rmi_register_transport_device(struct
> rmi_transport_dev *xport)
> rmi_physical_setup_debugfs(rmi_dev);
>
> error = device_register(&rmi_dev->dev);
> - if (error)
> + if (error) {
> + put_device(&rmi_dev->dev);
We also need to tear down debugfs here.
> return error;
> + }
>
> dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
> pdata->sensor_name, dev_name(&rmi_dev->dev));
> @@ -142,6 +144,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
> static void rmi_release_function(struct device *dev)
> {
> struct rmi_function *fn = to_rmi_function(dev);
> +
> kfree(fn);
> }
>
> @@ -240,16 +243,14 @@ int rmi_register_function(struct rmi_function *fn)
> dev_err(&rmi_dev->dev,
> "Failed device_register function device %s\n",
> dev_name(&fn->dev));
> - goto error_exit;
> + rmi_function_teardown_debugfs(fn);
> + put_device(&fn->dev);
This is not proper place to free structures since
rmi_register_function() was not the one that allocated them.
OK, I split the rmi_driver_irq_get_mask() removal into a separate patch
and applied it, this leaves us with the patch below (BTW, your mailed
damaged - line wrapped - the patch so I had to reconstruct it).
I switched from using device_register/device_unregister to
device_initialize/device_add/device_del/put_device as it allows better
control over error unwinding and destroying the objects which is
important if you want to delete debugfs entries once all fucntion
handler have been unregister but before our memory is gone.
Thanks.
--
Dmitry
Input: synaptics-rmi4 - use put_device() to free devices
From: Christopher Heiny <cheiny@synaptics.com>
For rmi_sensor and rmi_function device_types, use put_device() and
the associated device_type->release() function to clean up related
structures and storage in the correct and safe order.
Allocate irq_mask as part of struct rmi_function.
Suggested-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_bus.c | 30 +++++++++++++++++++++---------
drivers/input/rmi4/rmi_bus.h | 7 ++++---
drivers/input/rmi4/rmi_driver.c | 25 +++++++------------------
3 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..7efe7ed 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -34,6 +34,7 @@ static struct dentry *rmi_debugfs_root;
static void rmi_release_device(struct device *dev)
{
struct rmi_device *rmi_dev = to_rmi_device(dev);
+
kfree(rmi_dev);
}
@@ -94,11 +95,12 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
return -EINVAL;
}
- rmi_dev = devm_kzalloc(xport->dev,
- sizeof(struct rmi_device), GFP_KERNEL);
+ rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
if (!rmi_dev)
return -ENOMEM;
+ device_initialize(&rmi_dev->dev);
+
rmi_dev->xport = xport;
rmi_dev->number = atomic_inc_return(&transport_device_count) - 1;
@@ -111,14 +113,19 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
rmi_physical_setup_debugfs(rmi_dev);
- error = device_register(&rmi_dev->dev);
+ error = device_add(&rmi_dev->dev);
if (error)
- return error;
+ goto err_put_device;
dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
pdata->sensor_name, dev_name(&rmi_dev->dev));
return 0;
+
+err_put_device:
+ rmi_physical_teardown_debugfs(rmi_dev);
+ put_device(&rmi_dev->dev);
+ return error;
}
EXPORT_SYMBOL_GPL(rmi_register_transport_device);
@@ -131,8 +138,9 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
{
struct rmi_device *rmi_dev = xport->rmi_dev;
+ device_del(&rmi_dev->dev);
rmi_physical_teardown_debugfs(rmi_dev);
- device_unregister(&rmi_dev->dev);
+ put_device(&rmi_dev->dev);
}
EXPORT_SYMBOL(rmi_unregister_transport_device);
@@ -142,6 +150,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
static void rmi_release_function(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
+
kfree(fn);
}
@@ -226,6 +235,8 @@ int rmi_register_function(struct rmi_function *fn)
struct rmi_device *rmi_dev = fn->rmi_dev;
int error;
+ device_initialize(&fn->dev);
+
dev_set_name(&fn->dev, "%s.fn%02x",
dev_name(&rmi_dev->dev), fn->fd.function_number);
@@ -235,27 +246,28 @@ int rmi_register_function(struct rmi_function *fn)
rmi_function_setup_debugfs(fn);
- error = device_register(&fn->dev);
+ error = device_add(&fn->dev);
if (error) {
dev_err(&rmi_dev->dev,
"Failed device_register function device %s\n",
dev_name(&fn->dev));
- goto error_exit;
+ goto err_teardown_debugfs;
}
dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);
return 0;
-error_exit:
+err_teardown_debugfs:
rmi_function_teardown_debugfs(fn);
return error;
}
void rmi_unregister_function(struct rmi_function *fn)
{
+ device_del(&fn->dev);
rmi_function_teardown_debugfs(fn);
- device_unregister(&fn->dev);
+ put_device(&fn->dev);
}
/**
diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 5ad94c6..1672b05 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -45,14 +45,15 @@ struct rmi_function {
struct rmi_function_descriptor fd;
struct rmi_device *rmi_dev;
struct device dev;
- int num_of_irqs;
- int irq_pos;
- unsigned long *irq_mask;
struct list_head node;
#ifdef CONFIG_RMI4_DEBUG
struct dentry *debugfs_root;
#endif
+
+ unsigned int num_of_irqs;
+ unsigned int irq_pos;
+ unsigned long irq_mask[];
};
#define to_rmi_function(d) container_of(d, struct rmi_function, dev)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 473efbc..788343a 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -185,7 +185,6 @@ static void rmi_free_function_list(struct rmi_device *rmi_dev)
list_for_each_entry_safe_reverse(fn, tmp,
&data->function_list, node) {
list_del(&fn->node);
- kfree(fn->irq_mask);
rmi_unregister_function(fn);
}
}
@@ -617,7 +616,7 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
}
static int rmi_create_function(struct rmi_device *rmi_dev,
- void *ctx, const struct pdt_entry *pdt)
+ void *ctx, const struct pdt_entry *pdt)
{
struct device *dev = &rmi_dev->dev;
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
@@ -630,7 +629,9 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
dev_dbg(dev, "Initializing F%02X for %s.\n",
pdt->function_number, pdata->sensor_name);
- fn = kzalloc(sizeof(struct rmi_function), GFP_KERNEL);
+ fn = kzalloc(sizeof(struct rmi_function) +
+ BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
+ GFP_KERNEL);
if (!fn) {
dev_err(dev, "Failed to allocate memory for F%02X\n",
pdt->function_number);
@@ -646,22 +647,12 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
fn->irq_pos = *current_irq_count;
*current_irq_count += fn->num_of_irqs;
- fn->irq_mask = kzalloc(
- BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
- GFP_KERNEL);
- if (!fn->irq_mask) {
- dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
- __func__, pdt->function_number);
- error = -ENOMEM;
- goto err_free_mem;
- }
-
for (i = 0; i < fn->num_of_irqs; i++)
set_bit(fn->irq_pos + i, fn->irq_mask);
error = rmi_register_function(fn);
if (error)
- goto err_free_irq_mask;
+ goto err_put_fn;
if (pdt->function_number == 0x01)
data->f01_container = fn;
@@ -670,10 +661,8 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
return RMI_SCAN_CONTINUE;
-err_free_irq_mask:
- kfree(fn->irq_mask);
-err_free_mem:
- kfree(fn);
+err_put_fn:
+ put_device(&fn->dev);
return error;
}
^ permalink raw reply related
* [PATCH 10/11] Input: synaptics-rmi4 - make accessor for platform data return const pointer
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
When accessing platform data of RMI device let's make sure we do not
accidentally change data that may be shared by returning const pointer.
Also switch to an inline function instead of macro to ensure type safety.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_bus.h | 7 ++++++-
drivers/input/rmi4/rmi_driver.c | 8 ++++----
drivers/input/rmi4/rmi_f01.c | 2 +-
drivers/input/rmi4/rmi_f11.c | 2 +-
4 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 8d47f52..02a2af8 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -224,7 +224,12 @@ struct rmi_device {
};
#define to_rmi_device(d) container_of(d, struct rmi_device, dev)
-#define to_rmi_platform_data(d) ((d)->xport->dev->platform_data)
+
+static inline const struct rmi_device_platform_data *
+rmi_get_platform_data(struct rmi_device *d)
+{
+ return dev_get_platdata(d->xport->dev);
+}
bool rmi_is_physical_device(struct device *dev);
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 736cfbd..473efbc 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -598,7 +598,7 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
u16 cmd_addr = pdt->page_start + pdt->command_base_addr;
u8 cmd_buf = RMI_DEVICE_RESET_CMD;
const struct rmi_device_platform_data *pdata =
- to_rmi_platform_data(rmi_dev);
+ rmi_get_platform_data(rmi_dev);
error = rmi_write_block(rmi_dev, cmd_addr, &cmd_buf, 1);
if (error) {
@@ -621,7 +621,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
{
struct device *dev = &rmi_dev->dev;
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
int *current_irq_count = ctx;
struct rmi_function *fn;
int i;
@@ -745,7 +745,7 @@ static int rmi_driver_remove(struct device *dev)
struct rmi_device *rmi_dev = to_rmi_device(dev);
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
const struct rmi_device_platform_data *pdata =
- to_rmi_platform_data(rmi_dev);
+ rmi_get_platform_data(rmi_dev);
disable_sensor(rmi_dev);
rmi_free_function_list(rmi_dev);
@@ -781,7 +781,7 @@ static int rmi_driver_probe(struct device *dev)
rmi_driver = to_rmi_driver(dev->driver);
rmi_dev->driver = rmi_driver;
- pdata = to_rmi_platform_data(rmi_dev);
+ pdata = rmi_get_platform_data(rmi_dev);
data = devm_kzalloc(dev, sizeof(struct rmi_driver_data), GFP_KERNEL);
if (!data) {
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 0e21004..36fcf8d 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -184,7 +184,7 @@ static int rmi_f01_probe(struct rmi_function *fn)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- const struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
struct f01_data *f01;
size_t f01_size;
int error;
diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
index 2aa7d17..5072437 100644
--- a/drivers/input/rmi4/rmi_f11.c
+++ b/drivers/input/rmi4/rmi_f11.c
@@ -1176,7 +1176,7 @@ static int rmi_f11_initialize(struct rmi_function *fn)
u16 control_base_addr;
u16 max_x_pos, max_y_pos, temp;
int rc;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
struct f11_2d_sensor *sensor;
u8 buf;
--
1.8.5.3
^ permalink raw reply related
* [PATCH 09/11] Input: synaptics-rmi4 - consolidate memory allocations in F01
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
Let's allocate interrupt mask together with the main structure and combine
rmi_f01_alloc_memory, rmi_f01_initialize and rmi_f01_probe into single
function.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 86 ++++++++++++++++----------------------------
1 file changed, 30 insertions(+), 56 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index a2f05bc..0e21004 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -130,37 +130,15 @@ struct f01_data {
u16 doze_interval_addr;
u16 wakeup_threshold_addr;
u16 doze_holdoff_addr;
- int irq_count;
- int num_of_irq_regs;
#ifdef CONFIG_PM_SLEEP
bool suspended;
bool old_nosleep;
#endif
-};
-
-static int rmi_f01_alloc_memory(struct rmi_function *fn,
- int num_of_irq_regs)
-{
- struct f01_data *f01;
- f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
- if (!f01) {
- dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
- return -ENOMEM;
- }
-
- f01->device_control.interrupt_enable = devm_kzalloc(&fn->dev,
- sizeof(u8) * (num_of_irq_regs),
- GFP_KERNEL);
- if (!f01->device_control.interrupt_enable) {
- dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
- return -ENOMEM;
- }
- fn->data = f01;
-
- return 0;
-}
+ unsigned int num_of_irq_regs;
+ u8 interrupt_enable[];
+};
static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
u16 query_base_addr,
@@ -202,16 +180,28 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
return 0;
}
-static int rmi_f01_initialize(struct rmi_function *fn)
+static int rmi_f01_probe(struct rmi_function *fn)
{
- u8 temp;
- int error;
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- struct f01_data *f01 = fn->data;
const struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ struct f01_data *f01;
+ size_t f01_size;
+ int error;
u16 ctrl_base_addr = fn->fd.control_base_addr;
u8 device_status;
+ u8 temp;
+
+ f01_size = sizeof(struct f01_data) +
+ sizeof(u8) * driver_data->num_of_irq_regs;
+ f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
+ if (!f01) {
+ dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
+ return -ENOMEM;
+ }
+
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+ f01->device_control.interrupt_enable = f01->interrupt_enable;
/*
* Set the configured bit and (optionally) other important stuff
@@ -257,12 +247,11 @@ static int rmi_f01_initialize(struct rmi_function *fn)
return error;
}
- f01->irq_count = driver_data->irq_count;
- f01->num_of_irq_regs = driver_data->num_of_irq_regs;
- ctrl_base_addr += sizeof(u8);
-
+ /* Advance to interrupt control registers */
+ ctrl_base_addr++;
f01->interrupt_enable_addr = ctrl_base_addr;
- error = rmi_read_block(rmi_dev, ctrl_base_addr,
+
+ error = rmi_read_block(rmi_dev, f01->interrupt_enable_addr,
f01->device_control.interrupt_enable,
sizeof(u8) * (f01->num_of_irq_regs));
if (error) {
@@ -272,9 +261,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
return error;
}
- ctrl_base_addr += f01->num_of_irq_regs;
-
- /* dummy read in order to clear irqs */
+ /* Dummy read in order to clear irqs */
error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
if (error < 0) {
dev_err(&fn->dev, "Failed to read Interrupt Status.\n");
@@ -287,11 +274,13 @@ static int rmi_f01_initialize(struct rmi_function *fn)
dev_err(&fn->dev, "Failed to read F01 properties.\n");
return error;
}
+
dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
- f01->properties.manufacturer_id == 1 ?
- "Synaptics" : "unknown",
+ f01->properties.manufacturer_id == 1 ? "Synaptics" : "unknown",
f01->properties.product_id);
+ ctrl_base_addr += f01->num_of_irq_regs;
+
/* read control register */
if (f01->properties.has_adjustable_doze) {
f01->doze_interval_addr = ctrl_base_addr;
@@ -365,6 +354,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
return -EINVAL;
}
+ fn->data = f01;
+
return 0;
}
@@ -424,23 +415,6 @@ static int rmi_f01_config(struct rmi_function *fn)
return 0;
}
-static int rmi_f01_probe(struct rmi_function *fn)
-{
- struct rmi_driver_data *driver_data =
- dev_get_drvdata(&fn->rmi_dev->dev);
- int error;
-
- error = rmi_f01_alloc_memory(fn, driver_data->num_of_irq_regs);
- if (error)
- return error;
-
- error = rmi_f01_initialize(fn);
- if (error)
- return error;
-
- return 0;
-}
-
#ifdef CONFIG_PM_SLEEP
static int rmi_f01_suspend(struct device *dev)
{
--
1.8.5.3
^ permalink raw reply related
* [PATCH 07/11] Input: synaptics-rmi4 - rename instances of f01_data from data to f01
From: Dmitry Torokhov @ 2014-02-13 5:27 UTC (permalink / raw)
To: Christopher Heiny
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
We have too many "data"s: f01_data, driver_data, pdata, etc. Let's
untangle it a bit.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 135 ++++++++++++++++++++++---------------------
1 file changed, 68 insertions(+), 67 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 1e49ab4..1219e0c 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -208,7 +208,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
u16 ctrl_base_addr;
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
u8 device_status;
@@ -218,8 +218,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
*/
ctrl_base_addr = fn->fd.control_base_addr;
error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 control.\n");
return error;
@@ -228,10 +228,10 @@ static int rmi_f01_initialize(struct rmi_function *fn)
case RMI_F01_NOSLEEP_DEFAULT:
break;
case RMI_F01_NOSLEEP_OFF:
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
break;
case RMI_F01_NOSLEEP_ON:
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
break;
}
@@ -240,38 +240,38 @@ static int rmi_f01_initialize(struct rmi_function *fn)
* reboot without power cycle. If so, clear it so the sensor
* is certain to function.
*/
- if ((data->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
+ if ((f01->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
RMI_SLEEP_MODE_NORMAL) {
dev_warn(&fn->dev,
"WARNING: Non-zero sleep mode found. Clearing...\n");
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
}
- data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev, "Failed to write F01 control.\n");
return error;
}
- data->irq_count = driver_data->irq_count;
- data->num_of_irq_regs = driver_data->num_of_irq_regs;
+ f01->irq_count = driver_data->irq_count;
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
ctrl_base_addr += sizeof(u8);
- data->interrupt_enable_addr = ctrl_base_addr;
+ f01->interrupt_enable_addr = ctrl_base_addr;
error = rmi_read_block(rmi_dev, ctrl_base_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * (data->num_of_irq_regs));
+ f01->device_control.interrupt_enable,
+ sizeof(u8) * (f01->num_of_irq_regs));
if (error < 0) {
dev_err(&fn->dev,
"Failed to read F01 control interrupt enable register.\n");
return error;
}
- ctrl_base_addr += data->num_of_irq_regs;
+ ctrl_base_addr += f01->num_of_irq_regs;
/* dummy read in order to clear irqs */
error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
@@ -281,42 +281,42 @@ static int rmi_f01_initialize(struct rmi_function *fn)
}
error = rmi_f01_read_properties(rmi_dev, fn->fd.query_base_addr,
- &data->properties);
+ &f01->properties);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 properties.\n");
return error;
}
dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
- data->properties.manufacturer_id == 1 ?
+ f01->properties.manufacturer_id == 1 ?
"Synaptics" : "unknown",
- data->properties.product_id);
+ f01->properties.product_id);
/* read control register */
- if (data->properties.has_adjustable_doze) {
- data->doze_interval_addr = ctrl_base_addr;
+ if (f01->properties.has_adjustable_doze) {
+ f01->doze_interval_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_interval) {
- data->device_control.doze_interval =
+ f01->device_control.doze_interval =
pdata->power_management.doze_interval;
} else {
- error = rmi_read(rmi_dev, data->doze_interval_addr,
- &data->device_control.doze_interval);
+ error = rmi_read(rmi_dev, f01->doze_interval_addr,
+ &f01->device_control.doze_interval);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
return error;
}
}
- data->wakeup_threshold_addr = ctrl_base_addr;
+ f01->wakeup_threshold_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.wakeup_threshold) {
- data->device_control.wakeup_threshold =
+ f01->device_control.wakeup_threshold =
pdata->power_management.wakeup_threshold;
} else {
- error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
- &data->device_control.wakeup_threshold);
+ error = rmi_read(rmi_dev, f01->wakeup_threshold_addr,
+ &f01->device_control.wakeup_threshold);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
return error;
@@ -324,19 +324,19 @@ static int rmi_f01_initialize(struct rmi_function *fn)
}
}
- if (data->properties.has_lts)
+ if (f01->properties.has_lts)
ctrl_base_addr++;
- if (data->properties.has_adjustable_doze_holdoff) {
- data->doze_holdoff_addr = ctrl_base_addr;
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ f01->doze_holdoff_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_holdoff) {
- data->device_control.doze_holdoff =
+ f01->device_control.doze_holdoff =
pdata->power_management.doze_holdoff;
} else {
- error = rmi_read(rmi_dev, data->doze_holdoff_addr,
- &data->device_control.doze_holdoff);
+ error = rmi_read(rmi_dev, f01->doze_holdoff_addr,
+ &f01->device_control.doze_holdoff);
if (error < 0) {
dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
return error;
@@ -363,28 +363,28 @@ static int rmi_f01_initialize(struct rmi_function *fn)
static int rmi_f01_config(struct rmi_function *fn)
{
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int retval;
retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write device_control.reg.\n");
return retval;
}
- retval = rmi_write_block(fn->rmi_dev, data->interrupt_enable_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * data->num_of_irq_regs);
-
+ retval = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
+ f01->device_control.interrupt_enable,
+ sizeof(u8) * f01->num_of_irq_regs);
if (retval < 0) {
dev_err(&fn->dev, "Failed to write interrupt enable.\n");
return retval;
}
- if (data->properties.has_adjustable_doze) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
- &data->device_control.doze_interval,
+
+ if (f01->properties.has_adjustable_doze) {
+ retval = rmi_write_block(fn->rmi_dev, f01->doze_interval_addr,
+ &f01->device_control.doze_interval,
sizeof(u8));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write doze interval.\n");
@@ -392,8 +392,8 @@ static int rmi_f01_config(struct rmi_function *fn)
}
retval = rmi_write_block(fn->rmi_dev,
- data->wakeup_threshold_addr,
- &data->device_control.wakeup_threshold,
+ f01->wakeup_threshold_addr,
+ &f01->device_control.wakeup_threshold,
sizeof(u8));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
@@ -401,15 +401,16 @@ static int rmi_f01_config(struct rmi_function *fn)
}
}
- if (data->properties.has_adjustable_doze_holdoff) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_holdoff_addr,
- &data->device_control.doze_holdoff,
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ retval = rmi_write_block(fn->rmi_dev, f01->doze_holdoff_addr,
+ &f01->device_control.doze_holdoff,
sizeof(u8));
if (retval < 0) {
dev_err(&fn->dev, "Failed to write doze holdoff.\n");
return retval;
}
}
+
return 0;
}
@@ -435,28 +436,28 @@ static int rmi_f01_suspend(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- data->old_nosleep = data->device_control.ctrl0 &
+ f01->old_nosleep = f01->device_control.ctrl0 &
RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
error = rmi_write_block(rmi_dev,
fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
error);
- if (data->old_nosleep)
- data->device_control.ctrl0 |=
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |=
RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
return error;
}
@@ -467,18 +468,18 @@ static int rmi_f01_resume(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- if (data->old_nosleep)
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
+ &f01->device_control.ctrl0,
+ sizeof(f01->device_control.ctrl0));
if (error < 0) {
dev_err(&fn->dev,
"Failed to restore normal operation. Code: %d.\n",
--
1.8.5.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox