* [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
@ 2017-03-28 16:01 Andrey Smirnov
2017-03-28 17:07 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Andrey Smirnov @ 2017-03-28 16:01 UTC (permalink / raw)
To: Rob Herring
Cc: Andrey Smirnov, cphealy, Guenter Roeck, Andy Shevchenko,
linux-serial, linux-kernel
Convert serdev_device_write_buf's code to be able to transfer amount of
data potentially exceeding "write room" at the moment of invocation.
To support that, also add serdev_device_write_wakeup.
Drivers wanting to use full extent of serdev_device_write
functionality are expected to provide serdev_device_write_wakeup as a
sole handler of .write_wakeup event or call it as a part of driver's
custom .write_wakeup code.
Drivers wanting to retain old serdev_device_write_buf behaviour can
replace those call to calls to serdev_device_write with timeout of
0. Providing .write_wakeup handler in such case is optional.
Cc: cphealy@gmail.com
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
Changes since v1 (see [v1]):
- Make timeout to be a total(as opposed to per-iteration)
timeout
- Keep serdev_device_write_buf() as a wrapper arount
serdev_device_write() for compatibility
[v1] https://lkml.org/lkml/2017/3/20/650
drivers/tty/serdev/core.c | 36 +++++++++++++++++++++++++++++++-----
include/linux/serdev.h | 18 ++++++++++++++++--
2 files changed, 47 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index f4c6c90..c47c421 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
}
EXPORT_SYMBOL_GPL(serdev_device_close);
-int serdev_device_write_buf(struct serdev_device *serdev,
- const unsigned char *buf, size_t count)
+void serdev_device_write_wakeup(struct serdev_device *serdev)
+{
+ complete(&serdev->write_comp);
+}
+EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
+
+int serdev_device_write(struct serdev_device *serdev,
+ const unsigned char *buf, size_t count,
+ unsigned long timeout)
{
struct serdev_controller *ctrl = serdev->ctrl;
+ int ret;
- if (!ctrl || !ctrl->ops->write_buf)
+ if (!ctrl || !ctrl->ops->write_buf ||
+ (timeout && !serdev->ops->write_wakeup))
return -EINVAL;
- return ctrl->ops->write_buf(ctrl, buf, count);
+ mutex_lock(&serdev->write_lock);
+ do {
+ reinit_completion(&serdev->write_comp);
+
+ ret = ctrl->ops->write_buf(ctrl, buf, count);
+ if (ret < 0)
+ break;
+
+ buf += ret;
+ count -= ret;
+
+ } while (count &&
+ (timeout = wait_for_completion_timeout(&serdev->write_comp,
+ timeout)));
+ mutex_unlock(&serdev->write_lock);
+ return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
}
-EXPORT_SYMBOL_GPL(serdev_device_write_buf);
+EXPORT_SYMBOL_GPL(serdev_device_write);
void serdev_device_write_flush(struct serdev_device *serdev)
{
@@ -232,6 +256,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
serdev->dev.parent = &ctrl->dev;
serdev->dev.bus = &serdev_bus_type;
serdev->dev.type = &serdev_device_type;
+ init_completion(&serdev->write_comp);
+ mutex_init(&serdev->write_lock);
return serdev;
}
EXPORT_SYMBOL_GPL(serdev_device_alloc);
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 5176cdc..535566d 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -39,12 +39,17 @@ struct serdev_device_ops {
* @nr: Device number on serdev bus.
* @ctrl: serdev controller managing this device.
* @ops: Device operations.
+ * @write_comp Completion used by serdev_device_write internally
+ * @write_lock Mutext used to esure exclusive access to the bus when
+ * writing data with serdev_device_write()
*/
struct serdev_device {
struct device dev;
int nr;
struct serdev_controller *ctrl;
const struct serdev_device_ops *ops;
+ struct completion write_comp;
+ struct mutex write_lock;
};
static inline struct serdev_device *to_serdev_device(struct device *d)
@@ -186,10 +191,12 @@ int serdev_device_open(struct serdev_device *);
void serdev_device_close(struct serdev_device *);
unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
void serdev_device_set_flow_control(struct serdev_device *, bool);
-int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
+void serdev_device_write_wakeup(struct serdev_device *);
+int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, unsigned long);
void serdev_device_write_flush(struct serdev_device *);
int serdev_device_write_room(struct serdev_device *);
+
/*
* serdev device driver functions
*/
@@ -223,7 +230,8 @@ static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev
return 0;
}
static inline void serdev_device_set_flow_control(struct serdev_device *sdev, bool enable) {}
-static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsigned char *buf, size_t count)
+static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf,
+ size_t count, unsigned long timeout)
{
return -ENODEV;
}
@@ -259,4 +267,10 @@ static inline struct device *serdev_tty_port_register(struct tty_port *port,
static inline void serdev_tty_port_unregister(struct tty_port *port) {}
#endif /* CONFIG_SERIAL_DEV_CTRL_TTYPORT */
+static inline int serdev_device_write_buf(struct serdev_device *serdev,
+ const unsigned char *data, size_t count)
+{
+ return serdev_device_write(serdev, data, count, 0);
+}
+
#endif /*_LINUX_SERDEV_H */
--
2.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
2017-03-28 16:01 [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andrey Smirnov
@ 2017-03-28 17:07 ` Andy Shevchenko
2017-03-29 14:16 ` Andrey Smirnov
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2017-03-28 17:07 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel
On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Convert serdev_device_write_buf's code to be able to transfer amount of
> data potentially exceeding "write room" at the moment of invocation.
>
> To support that, also add serdev_device_write_wakeup.
>
> Drivers wanting to use full extent of serdev_device_write
> functionality are expected to provide serdev_device_write_wakeup as a
> sole handler of .write_wakeup event or call it as a part of driver's
> custom .write_wakeup code.
>
> Drivers wanting to retain old serdev_device_write_buf behaviour can
> replace those call to calls to serdev_device_write with timeout of
> 0. Providing .write_wakeup handler in such case is optional.
Some indentation would be better if, for example, 0 will be kept on
previous line.
So, what I would see if no one objects is patch series of two:
1) introduction of new API
2) removing old one.
It will benefit for easier review and any potential code anthropologist.
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
> }
> EXPORT_SYMBOL_GPL(serdev_device_close);
>
> -int serdev_device_write_buf(struct serdev_device *serdev,
> - const unsigned char *buf, size_t count)
> +void serdev_device_write_wakeup(struct serdev_device *serdev)
> +{
> + complete(&serdev->write_comp);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
> +
> +int serdev_device_write(struct serdev_device *serdev,
> + const unsigned char *buf, size_t count,
> + unsigned long timeout)
> {
> struct serdev_controller *ctrl = serdev->ctrl;
> + int ret;
>
> - if (!ctrl || !ctrl->ops->write_buf)
> + if (!ctrl || !ctrl->ops->write_buf ||
> + (timeout && !serdev->ops->write_wakeup))
> return -EINVAL;
>
> - return ctrl->ops->write_buf(ctrl, buf, count);
> + mutex_lock(&serdev->write_lock);
> + do {
> + reinit_completion(&serdev->write_comp);
> +
> + ret = ctrl->ops->write_buf(ctrl, buf, count);
> + if (ret < 0)
> + break;
> +
> + buf += ret;
Extra white spaces.
> + count -= ret;
> +
> + } while (count &&
> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
> + timeout)));
So, would it be better to support interrupts here and return a
corresponding error code to the user?
Besides that question, readability might be better if you use
temporary variable and pack above on one line:
unsigned long to = timeout;
} while (count && (to = ...(to)));
> + mutex_unlock(&serdev->write_lock);
> + return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
> }
> -EXPORT_SYMBOL_GPL(serdev_device_write_buf);
> +EXPORT_SYMBOL_GPL(serdev_device_write);
> + * @write_comp Completion used by serdev_device_write internally
Links to the functions are func()-like. Please check kernel doc howto:s.
> + * @write_lock Mutext used to esure exclusive access to the bus when
> + * writing data with serdev_device_write()
checkpatch.pl has integrated spellchecker AFAIU.
Moreover, can you try harder to make that description shorter?
> void serdev_device_write_flush(struct serdev_device *);
> int serdev_device_write_room(struct serdev_device *);
>
> +
> /*
> * serdev device driver functions
> */
This doesn't belong to the change.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
2017-03-28 17:07 ` Andy Shevchenko
@ 2017-03-29 14:16 ` Andrey Smirnov
2017-03-29 14:43 ` Andy Shevchenko
2017-03-29 21:25 ` Rob Herring
0 siblings, 2 replies; 7+ messages in thread
From: Andrey Smirnov @ 2017-03-29 14:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel
On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> Convert serdev_device_write_buf's code to be able to transfer amount of
>> data potentially exceeding "write room" at the moment of invocation.
>>
>> To support that, also add serdev_device_write_wakeup.
>>
>> Drivers wanting to use full extent of serdev_device_write
>> functionality are expected to provide serdev_device_write_wakeup as a
>> sole handler of .write_wakeup event or call it as a part of driver's
>> custom .write_wakeup code.
>>
>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>
>> replace those call to calls to serdev_device_write with timeout of
>> 0. Providing .write_wakeup handler in such case is optional.
>
> Some indentation would be better if, for example, 0 will be kept on
> previous line.
>
OK, sure.
> So, what I would see if no one objects is patch series of two:
> 1) introduction of new API
> 2) removing old one.
>
> It will benefit for easier review and any potential code anthropologist.
>
Second version of the patch preserves the old API an just
re-implements it in terms of a new one. I am not sure I see the
benefit in splitting it into two patches, but I'll leave it up to Rob
to decide.
>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>> }
>> EXPORT_SYMBOL_GPL(serdev_device_close);
>>
>> -int serdev_device_write_buf(struct serdev_device *serdev,
>> - const unsigned char *buf, size_t count)
>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>> +{
>> + complete(&serdev->write_comp);
>> +}
>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>> +
>> +int serdev_device_write(struct serdev_device *serdev,
>> + const unsigned char *buf, size_t count,
>> + unsigned long timeout)
>> {
>> struct serdev_controller *ctrl = serdev->ctrl;
>> + int ret;
>>
>> - if (!ctrl || !ctrl->ops->write_buf)
>> + if (!ctrl || !ctrl->ops->write_buf ||
>> + (timeout && !serdev->ops->write_wakeup))
>> return -EINVAL;
>>
>> - return ctrl->ops->write_buf(ctrl, buf, count);
>> + mutex_lock(&serdev->write_lock);
>> + do {
>> + reinit_completion(&serdev->write_comp);
>> +
>> + ret = ctrl->ops->write_buf(ctrl, buf, count);
>> + if (ret < 0)
>> + break;
>> +
>
>> + buf += ret;
>
> Extra white spaces.
Which is there on purpose to re-align "+=" with "-=" on the next line.
I'll remove it.
>
>> + count -= ret;
>> +
>
>> + } while (count &&
>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>> + timeout)));
>
> So, would it be better to support interrupts here and return a
> corresponding error code to the user?
>
I don't have a use-case for that and as far as I can tell, neither SPI
nor I2C slave device API offer such functionality universally, so I am
inclined to say no. Since the change from wait_for_completion to
wait_for_completion_timeout was made per Rob's request, I'd leave it
up to him to decided about this change as well.
> Besides that question, readability might be better if you use
> temporary variable and pack above on one line:
>
> unsigned long to = timeout;
>
> } while (count && (to = ...(to)));
>
Even if you shorten 'timeout' to 'to', formatted as a single line,
it'd still exceed line length limitations.
>
>
>> + mutex_unlock(&serdev->write_lock);
>> + return ret < 0 ? ret : (count ? -ETIMEDOUT : 0);
>> }
>> -EXPORT_SYMBOL_GPL(serdev_device_write_buf);
>> +EXPORT_SYMBOL_GPL(serdev_device_write);
>
>> + * @write_comp Completion used by serdev_device_write internally
>
> Links to the functions are func()-like. Please check kernel doc howto:s.
OK, will do.
>
>> + * @write_lock Mutext used to esure exclusive access to the bus when
>> + * writing data with serdev_device_write()
>
> checkpatch.pl has integrated spellchecker AFAIU.
My bad, forgot to enable it as a git hook, will fix.
> Moreover, can you try harder to make that description shorter?
>
I am all ears for suggestions alternative phrasing, otherwise, no,
that's about as hard as I try.
>> void serdev_device_write_flush(struct serdev_device *);
>> int serdev_device_write_room(struct serdev_device *);
>>
>> +
>> /*
>> * serdev device driver functions
>> */
>
> This doesn't belong to the change.
Oops, didn't notice this. Will remove.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
2017-03-29 14:16 ` Andrey Smirnov
@ 2017-03-29 14:43 ` Andy Shevchenko
2017-03-30 12:40 ` Andrey Smirnov
2017-03-29 21:25 ` Rob Herring
1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2017-03-29 14:43 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel
On Wed, Mar 29, 2017 at 5:16 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:
>> So, what I would see if no one objects is patch series of two:
>> 1) introduction of new API
>> 2) removing old one.
>>
>> It will benefit for easier review and any potential code anthropologist.
>>
>
> Second version of the patch preserves the old API an just
> re-implements it in terms of a new one. I am not sure I see the
> benefit in splitting it into two patches, but I'll leave it up to Rob
> to decide.
Sure. At least I posted benefits I see from splitting.
+ bisectability (in case we have to revert your new API by some reason
it will be easier, hope will be not the case, though...)
>>> + } while (count &&
>>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>> + timeout)));
>>
>> So, would it be better to support interrupts here and return a
>> corresponding error code to the user?
>>
>
> I don't have a use-case for that and as far as I can tell, neither SPI
> nor I2C slave device API offer such functionality universally, so I am
> inclined to say no. Since the change from wait_for_completion to
> wait_for_completion_timeout was made per Rob's request, I'd leave it
> up to him to decided about this change as well.
OK.
>> Besides that question, readability might be better if you use
>> temporary variable and pack above on one line:
>>
>> unsigned long to = timeout;
>>
>> } while (count && (to = ...(to)));
>>
>
> Even if you shorten 'timeout' to 'to', formatted as a single line,
> it'd still exceed line length limitations.
How many? If we are talking about 2-3 characters, that's okay to leave
them on one line.
>>> + * @write_lock Mutext used to esure exclusive access to the bus when
>>> + * writing data with serdev_device_write()
>>
>> checkpatch.pl has integrated spellchecker AFAIU.
>
> My bad, forgot to enable it as a git hook, will fix.
>
>> Moreover, can you try harder to make that description shorter?
>>
>
> I am all ears for suggestions alternative phrasing, otherwise, no,
> that's about as hard as I try.
First of all, "used to" is (closer) equivalent to was.
Second, Mutex is one letter longer than Lock (here is important that
is just a kind of lock).
Third, "exclusive" is implied by Mutex / Lock word.
Fourth, "access to the bus when writing data" too verbose.
So, my suggestion is (two variants):
a) "Lock to serialize bus access when writing data."
b) "Lock to serialize access when writing data with serdev_device_write()."
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
2017-03-29 14:16 ` Andrey Smirnov
2017-03-29 14:43 ` Andy Shevchenko
@ 2017-03-29 21:25 ` Rob Herring
2017-03-30 12:41 ` Andrey Smirnov
1 sibling, 1 reply; 7+ messages in thread
From: Rob Herring @ 2017-03-29 21:25 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Andy Shevchenko, Chris Healy, Guenter Roeck, linux-serial, linux-kernel
On Wed, Mar 29, 2017 at 9:16 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:
>>> Convert serdev_device_write_buf's code to be able to transfer amount of
>>> data potentially exceeding "write room" at the moment of invocation.
>>>
>>> To support that, also add serdev_device_write_wakeup.
>>>
>>> Drivers wanting to use full extent of serdev_device_write
>>> functionality are expected to provide serdev_device_write_wakeup as a
>>> sole handler of .write_wakeup event or call it as a part of driver's
>>> custom .write_wakeup code.
>>>
>>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>>
>>> replace those call to calls to serdev_device_write with timeout of
>>> 0. Providing .write_wakeup handler in such case is optional.
>>
>> Some indentation would be better if, for example, 0 will be kept on
>> previous line.
>>
>
> OK, sure.
>
>> So, what I would see if no one objects is patch series of two:
>> 1) introduction of new API
>> 2) removing old one.
>>
>> It will benefit for easier review and any potential code anthropologist.
>>
>
> Second version of the patch preserves the old API an just
> re-implements it in terms of a new one. I am not sure I see the
> benefit in splitting it into two patches, but I'll leave it up to Rob
> to decide.
I think it is fine as-is, but maybe the subject now is a bit misleading.
>>> --- a/drivers/tty/serdev/core.c
>>> +++ b/drivers/tty/serdev/core.c
>>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>>> }
>>> EXPORT_SYMBOL_GPL(serdev_device_close);
>>>
>>> -int serdev_device_write_buf(struct serdev_device *serdev,
>>> - const unsigned char *buf, size_t count)
>>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>>> +{
>>> + complete(&serdev->write_comp);
>>> +}
>>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>>> +
>>> +int serdev_device_write(struct serdev_device *serdev,
>>> + const unsigned char *buf, size_t count,
>>> + unsigned long timeout)
>>> {
>>> struct serdev_controller *ctrl = serdev->ctrl;
>>> + int ret;
>>>
>>> - if (!ctrl || !ctrl->ops->write_buf)
>>> + if (!ctrl || !ctrl->ops->write_buf ||
>>> + (timeout && !serdev->ops->write_wakeup))
>>> return -EINVAL;
>>>
>>> - return ctrl->ops->write_buf(ctrl, buf, count);
>>> + mutex_lock(&serdev->write_lock);
>>> + do {
>>> + reinit_completion(&serdev->write_comp);
>>> +
>>> + ret = ctrl->ops->write_buf(ctrl, buf, count);
>>> + if (ret < 0)
>>> + break;
>>> +
>>
>>> + buf += ret;
>>
>> Extra white spaces.
>
> Which is there on purpose to re-align "+=" with "-=" on the next line.
> I'll remove it.
>
>>
>>> + count -= ret;
>>> +
>>
>>> + } while (count &&
>>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>> + timeout)));
>>
>> So, would it be better to support interrupts here and return a
>> corresponding error code to the user?
>>
>
> I don't have a use-case for that and as far as I can tell, neither SPI
> nor I2C slave device API offer such functionality universally, so I am
> inclined to say no. Since the change from wait_for_completion to
> wait_for_completion_timeout was made per Rob's request, I'd leave it
> up to him to decided about this change as well.
Honestly, I don't know. It's added easily enough if needed later.
Rob
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
2017-03-29 14:43 ` Andy Shevchenko
@ 2017-03-30 12:40 ` Andrey Smirnov
0 siblings, 0 replies; 7+ messages in thread
From: Andrey Smirnov @ 2017-03-30 12:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel
On Wed, Mar 29, 2017 at 7:43 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 29, 2017 at 5:16 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>>> <andrew.smirnov@gmail.com> wrote:
>
>>> So, what I would see if no one objects is patch series of two:
>>> 1) introduction of new API
>>> 2) removing old one.
>>>
>>> It will benefit for easier review and any potential code anthropologist.
>>>
>>
>> Second version of the patch preserves the old API an just
>> re-implements it in terms of a new one. I am not sure I see the
>> benefit in splitting it into two patches, but I'll leave it up to Rob
>> to decide.
>
> Sure. At least I posted benefits I see from splitting.
>
> + bisectability (in case we have to revert your new API by some reason
> it will be easier, hope will be not the case, though...)
>
>>>> + } while (count &&
>>>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>>> + timeout)));
>>>
>>> So, would it be better to support interrupts here and return a
>>> corresponding error code to the user?
>>>
>>
>> I don't have a use-case for that and as far as I can tell, neither SPI
>> nor I2C slave device API offer such functionality universally, so I am
>> inclined to say no. Since the change from wait_for_completion to
>> wait_for_completion_timeout was made per Rob's request, I'd leave it
>> up to him to decided about this change as well.
>
> OK.
>
>>> Besides that question, readability might be better if you use
>>> temporary variable and pack above on one line:
>>>
>>> unsigned long to = timeout;
>>>
>>> } while (count && (to = ...(to)));
>>>
>>
>> Even if you shorten 'timeout' to 'to', formatted as a single line,
>> it'd still exceed line length limitations.
>
> How many? If we are talking about 2-3 characters, that's okay to leave
> them on one line.
Even so, at this point we are definitely in "personal preference"
territory and I'd rather avoid line wrapping.
>
>>>> + * @write_lock Mutext used to esure exclusive access to the bus when
>>>> + * writing data with serdev_device_write()
>>>
>>> checkpatch.pl has integrated spellchecker AFAIU.
>>
>> My bad, forgot to enable it as a git hook, will fix.
>>
>>> Moreover, can you try harder to make that description shorter?
>>>
>>
>> I am all ears for suggestions alternative phrasing, otherwise, no,
>> that's about as hard as I try.
>
> First of all, "used to" is (closer) equivalent to was.
> Second, Mutex is one letter longer than Lock (here is important that
> is just a kind of lock).
> Third, "exclusive" is implied by Mutex / Lock word.
> Fourth, "access to the bus when writing data" too verbose.
>
> So, my suggestion is (two variants):
> a) "Lock to serialize bus access when writing data."
> b) "Lock to serialize access when writing data with serdev_device_write()."
>
I'll use option "a". Option "b" still exceeds line limit, and I'd
rather not have that.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
2017-03-29 21:25 ` Rob Herring
@ 2017-03-30 12:41 ` Andrey Smirnov
0 siblings, 0 replies; 7+ messages in thread
From: Andrey Smirnov @ 2017-03-30 12:41 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Shevchenko, Chris Healy, Guenter Roeck, linux-serial, linux-kernel
On Wed, Mar 29, 2017 at 2:25 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Mar 29, 2017 at 9:16 AM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>>> <andrew.smirnov@gmail.com> wrote:
>>>> Convert serdev_device_write_buf's code to be able to transfer amount of
>>>> data potentially exceeding "write room" at the moment of invocation.
>>>>
>>>> To support that, also add serdev_device_write_wakeup.
>>>>
>>>> Drivers wanting to use full extent of serdev_device_write
>>>> functionality are expected to provide serdev_device_write_wakeup as a
>>>> sole handler of .write_wakeup event or call it as a part of driver's
>>>> custom .write_wakeup code.
>>>>
>>>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>>>
>>>> replace those call to calls to serdev_device_write with timeout of
>>>> 0. Providing .write_wakeup handler in such case is optional.
>>>
>>> Some indentation would be better if, for example, 0 will be kept on
>>> previous line.
>>>
>>
>> OK, sure.
>>
>>> So, what I would see if no one objects is patch series of two:
>>> 1) introduction of new API
>>> 2) removing old one.
>>>
>>> It will benefit for easier review and any potential code anthropologist.
>>>
>>
>> Second version of the patch preserves the old API an just
>> re-implements it in terms of a new one. I am not sure I see the
>> benefit in splitting it into two patches, but I'll leave it up to Rob
>> to decide.
>
> I think it is fine as-is, but maybe the subject now is a bit misleading.
>
OK, I'll modify the subject to be more representative of the change.
>>>> --- a/drivers/tty/serdev/core.c
>>>> +++ b/drivers/tty/serdev/core.c
>>>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>>>> }
>>>> EXPORT_SYMBOL_GPL(serdev_device_close);
>>>>
>>>> -int serdev_device_write_buf(struct serdev_device *serdev,
>>>> - const unsigned char *buf, size_t count)
>>>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>>>> +{
>>>> + complete(&serdev->write_comp);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>>>> +
>>>> +int serdev_device_write(struct serdev_device *serdev,
>>>> + const unsigned char *buf, size_t count,
>>>> + unsigned long timeout)
>>>> {
>>>> struct serdev_controller *ctrl = serdev->ctrl;
>>>> + int ret;
>>>>
>>>> - if (!ctrl || !ctrl->ops->write_buf)
>>>> + if (!ctrl || !ctrl->ops->write_buf ||
>>>> + (timeout && !serdev->ops->write_wakeup))
>>>> return -EINVAL;
>>>>
>>>> - return ctrl->ops->write_buf(ctrl, buf, count);
>>>> + mutex_lock(&serdev->write_lock);
>>>> + do {
>>>> + reinit_completion(&serdev->write_comp);
>>>> +
>>>> + ret = ctrl->ops->write_buf(ctrl, buf, count);
>>>> + if (ret < 0)
>>>> + break;
>>>> +
>>>
>>>> + buf += ret;
>>>
>>> Extra white spaces.
>>
>> Which is there on purpose to re-align "+=" with "-=" on the next line.
>> I'll remove it.
>>
>>>
>>>> + count -= ret;
>>>> +
>>>
>>>> + } while (count &&
>>>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>>> + timeout)));
>>>
>>> So, would it be better to support interrupts here and return a
>>> corresponding error code to the user?
>>>
>>
>> I don't have a use-case for that and as far as I can tell, neither SPI
>> nor I2C slave device API offer such functionality universally, so I am
>> inclined to say no. Since the change from wait_for_completion to
>> wait_for_completion_timeout was made per Rob's request, I'd leave it
>> up to him to decided about this change as well.
>
> Honestly, I don't know. It's added easily enough if needed later.
OK, I'll keep things as is for now.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-30 12:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-28 16:01 [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write Andrey Smirnov
2017-03-28 17:07 ` Andy Shevchenko
2017-03-29 14:16 ` Andrey Smirnov
2017-03-29 14:43 ` Andy Shevchenko
2017-03-30 12:40 ` Andrey Smirnov
2017-03-29 21:25 ` Rob Herring
2017-03-30 12:41 ` Andrey Smirnov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.