* [PATCH 0/2] serdev API extension @ 2017-03-14 13:48 Andrey Smirnov 2017-03-14 13:48 ` [PATCH 1/2] serdev: Add serdev_device_write subroutine Andrey Smirnov 2017-03-14 13:48 ` [PATCH 2/2] serdev: Add minimal bus locking API Andrey Smirnov 0 siblings, 2 replies; 13+ messages in thread From: Andrey Smirnov @ 2017-03-14 13:48 UTC (permalink / raw) To: Rob Herring Cc: Andrey Smirnov, cphealy, Guenter Roeck, linux-serial, linux-kernel Rob, These patches are a continuation of discussion about adding functionality to serdev API we had off-list. Being added are a function to do a blocking write capable of dispatching multiple calls to write_buf() and some very minimalistic bus locking API. Any feedback is appreciated. Thanks, Andrey Smirnov Andrey Smirnov (2): serdev: Add serdev_device_write subroutine serdev: Add minimal bus locking API drivers/tty/serdev/core.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/serdev.h | 34 ++++++++++++++++++++++++++++------ 2 files changed, 66 insertions(+), 6 deletions(-) -- 2.9.3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-14 13:48 [PATCH 0/2] serdev API extension Andrey Smirnov @ 2017-03-14 13:48 ` Andrey Smirnov 2017-03-14 23:17 ` Andy Shevchenko 2017-03-15 14:18 ` Rob Herring 2017-03-14 13:48 ` [PATCH 2/2] serdev: Add minimal bus locking API Andrey Smirnov 1 sibling, 2 replies; 13+ messages in thread From: Andrey Smirnov @ 2017-03-14 13:48 UTC (permalink / raw) To: Rob Herring Cc: Andrey Smirnov, cphealy, Guenter Roeck, linux-serial, linux-kernel Add serdev_device_write() which is a blocking call allowing to transfer arbitraty amount of data (potentially exceeding amount that serdev_device_write_buf can process in a single call) Cc: cphealy@gmail.com Cc: Guenter Roeck <linux@roeck-us.net> Cc: linux-serial@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/serdev.h | 23 +++++++++++++++++------ 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c index f4c6c90..759e834 100644 --- a/drivers/tty/serdev/core.c +++ b/drivers/tty/serdev/core.c @@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev, } EXPORT_SYMBOL_GPL(serdev_device_write_buf); +int serdev_device_write(struct serdev_device *serdev, + const unsigned char *buf, size_t count) +{ + int ret = count; + + if (serdev->ops->write_wakeup) + return -EINVAL; + + mutex_lock(&serdev->write_lock); + + for (;;) { + size_t chunk; + + reinit_completion(&serdev->write_wakeup); + + chunk = serdev_device_write_buf(serdev, buf, count); + if (chunk < 0) { + ret = chunk; + goto done; + } + + buf += chunk; + count -= chunk; + + if (!count) + break; + + wait_for_completion(&serdev->write_wakeup); + } +done: + mutex_unlock(&serdev->write_lock); + return ret; +} +EXPORT_SYMBOL_GPL(serdev_device_write); + void serdev_device_write_flush(struct serdev_device *serdev) { struct serdev_controller *ctrl = serdev->ctrl; @@ -232,6 +267,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_wakeup); + 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..8f7aa35 100644 --- a/include/linux/serdev.h +++ b/include/linux/serdev.h @@ -35,16 +35,19 @@ struct serdev_device_ops { /** * struct serdev_device - Basic representation of an serdev device - * @dev: Driver model representation of the device. - * @nr: Device number on serdev bus. - * @ctrl: serdev controller managing this device. - * @ops: Device operations. + * @dev: Driver model representation of the device. + * @nr: Device number on serdev bus. + * @ctrl: serdev controller managing this device. + * @ops: Device operations. + * @write_wakeup Completion used by serdev_device_write internally */ struct serdev_device { struct device dev; int nr; struct serdev_controller *ctrl; const struct serdev_device_ops *ops; + struct completion write_wakeup; + struct mutex write_lock; }; static inline struct serdev_device *to_serdev_device(struct device *d) @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl { struct serdev_device *serdev = ctrl->serdev; - if (!serdev || !serdev->ops->write_wakeup) + if (!serdev) return; - serdev->ops->write_wakeup(serdev); + if (serdev->ops->write_wakeup) + serdev->ops->write_wakeup(serdev); + else + complete(&serdev->write_wakeup); } static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl, @@ -187,6 +193,7 @@ 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); +int serdev_device_write(struct serdev_device *serdev, const unsigned char *buf, size_t count); void serdev_device_write_flush(struct serdev_device *); int serdev_device_write_room(struct serdev_device *); @@ -227,6 +234,10 @@ static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsi { return -ENODEV; } +static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf, size_t count) +{ + return -ENODEV; +} static inline void serdev_device_write_flush(struct serdev_device *sdev) {} static inline int serdev_device_write_room(struct serdev_device *sdev) { -- 2.9.3 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-14 13:48 ` [PATCH 1/2] serdev: Add serdev_device_write subroutine Andrey Smirnov @ 2017-03-14 23:17 ` Andy Shevchenko 2017-03-16 14:23 ` Andrey Smirnov 2017-03-15 14:18 ` Rob Herring 1 sibling, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2017-03-14 23:17 UTC (permalink / raw) To: Andrey Smirnov Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > Add serdev_device_write() which is a blocking call allowing to transfer > arbitraty amount of data (potentially exceeding amount that > serdev_device_write_buf can process in a single call) > +int serdev_device_write(struct serdev_device *serdev, > + const unsigned char *buf, size_t count) > +{ > + int ret = count; If count by some reason bigger than INT_MAX... > + > + if (serdev->ops->write_wakeup) > + return -EINVAL; > + > + mutex_lock(&serdev->write_lock); > + > + for (;;) { > + size_t chunk; > + > + reinit_completion(&serdev->write_wakeup); > + > + chunk = serdev_device_write_buf(serdev, buf, count); > + if (chunk < 0) { This will never happen. What kind of test did you try? > + ret = chunk; > + goto done; > + } > + > + buf += chunk; > + count -= chunk; > + > + if (!count) What is supposed to be returned? Initial count? Does it make any sense? > + break; Perhaps you need to refactor this function. > + > + wait_for_completion(&serdev->write_wakeup); > + } > +done: It would be nice to have a suffix, like done_unlock: But I'm pretty sure if you refactor the code in a smart way you will not need it. > + mutex_unlock(&serdev->write_lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(serdev_device_write); > /** > * struct serdev_device - Basic representation of an serdev device > - * @dev: Driver model representation of the device. > - * @nr: Device number on serdev bus. > - * @ctrl: serdev controller managing this device. > - * @ops: Device operations. > + * @dev: Driver model representation of the device. > + * @nr: Device number on serdev bus. > + * @ctrl: serdev controller managing this device. > + * @ops: Device operations. Does it make sense to shift? I would think of shorter field names instead. > + * @write_wakeup Completion used by serdev_device_write internally Colon is missed. Another filed is missed. > */ > struct serdev_device { > struct device dev; > int nr; > struct serdev_controller *ctrl; > const struct serdev_device_ops *ops; > + struct completion write_wakeup; > + struct mutex write_lock; > }; > > static inline struct serdev_device *to_serdev_device(struct device *d) > @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl > { > struct serdev_device *serdev = ctrl->serdev; > > - if (!serdev || !serdev->ops->write_wakeup) > + if (!serdev) > return; > > - serdev->ops->write_wakeup(serdev); > + if (serdev->ops->write_wakeup) > + serdev->ops->write_wakeup(serdev); > + else > + complete(&serdev->write_wakeup); By the way does this changes the possible context of application (atomic / non-atomic)? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-14 23:17 ` Andy Shevchenko @ 2017-03-16 14:23 ` Andrey Smirnov 2017-03-16 15:09 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Andrey Smirnov @ 2017-03-16 14:23 UTC (permalink / raw) To: Andy Shevchenko Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Tue, Mar 14, 2017 at 4:17 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov > <andrew.smirnov@gmail.com> wrote: >> Add serdev_device_write() which is a blocking call allowing to transfer >> arbitraty amount of data (potentially exceeding amount that >> serdev_device_write_buf can process in a single call) > >> +int serdev_device_write(struct serdev_device *serdev, >> + const unsigned char *buf, size_t count) >> +{ > >> + int ret = count; > > If count by some reason bigger than INT_MAX... > True, I was merely copying type signature of serdev_device_write_buf, which would have the same problem. I can add an appropriate check to the code, but at the same time, I'd love to know why it wasn't a concern in the latter function. >> + >> + if (serdev->ops->write_wakeup) >> + return -EINVAL; >> + >> + mutex_lock(&serdev->write_lock); >> + >> + for (;;) { >> + size_t chunk; >> + >> + reinit_completion(&serdev->write_wakeup); >> + >> + chunk = serdev_device_write_buf(serdev, buf, count); > > >> + if (chunk < 0) { > > This will never happen. What kind of test did you try? > None of my code using that function ever hit that condition, OTOH, I am not sure why I should ignore the API's signed return type, which I assume is so in order to facilitate negative error returns. My thinking is that even if in the current codebase is incapable of ever returning negative result today, that doesn't mean it won't ever be, and IMHO the chances of that are no different from the chances of someone passing 'count' that is bigger than INT_MAX. Given that I'd rather keep the check until the type signature of serdev_device_write_buf changes. >> + ret = chunk; >> + goto done; >> + } > > >> + >> + buf += chunk; >> + count -= chunk; >> + > >> + if (!count) > > What is supposed to be returned? Initial count? Does it make any sense? > That part of this code is not very well thought out. I think returning number of bytes successfully written would make more sense. I'll change it to do that in v2, but I am open to suggestions. >> + break; > > Perhaps you need to refactor this function. > >> + >> + wait_for_completion(&serdev->write_wakeup); >> + } > >> +done: > > It would be nice to have a suffix, like > > done_unlock: > > > But I'm pretty sure if you refactor the code in a smart way you will > not need it. > Yeah, I agree, it should be possible to make that loop more concise. I'll give it a try in v2. >> + mutex_unlock(&serdev->write_lock); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(serdev_device_write); > >> /** >> * struct serdev_device - Basic representation of an serdev device >> - * @dev: Driver model representation of the device. >> - * @nr: Device number on serdev bus. >> - * @ctrl: serdev controller managing this device. >> - * @ops: Device operations. >> + * @dev: Driver model representation of the device. >> + * @nr: Device number on serdev bus. >> + * @ctrl: serdev controller managing this device. >> + * @ops: Device operations. > > Does it make sense to shift? I would think of shorter field names instead. I can see how that might make the diff look nice, but OTOH, I'd rather not limit myself to only ~4 letters. > >> + * @write_wakeup Completion used by serdev_device_write internally > > Colon is missed. > > Another filed is missed. Yep, thank you, will fix in v2. > >> */ >> struct serdev_device { >> struct device dev; >> int nr; >> struct serdev_controller *ctrl; >> const struct serdev_device_ops *ops; >> + struct completion write_wakeup; >> + struct mutex write_lock; >> }; >> >> static inline struct serdev_device *to_serdev_device(struct device *d) >> @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl >> { >> struct serdev_device *serdev = ctrl->serdev; >> >> - if (!serdev || !serdev->ops->write_wakeup) >> + if (!serdev) >> return; >> >> - serdev->ops->write_wakeup(serdev); >> + if (serdev->ops->write_wakeup) >> + serdev->ops->write_wakeup(serdev); >> + else >> + complete(&serdev->write_wakeup); > > By the way does this changes the possible context of application > (atomic / non-atomic)? It should be possible to call complete() in atomic context, so I don't think it does. > > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-16 14:23 ` Andrey Smirnov @ 2017-03-16 15:09 ` Andy Shevchenko 2017-03-17 13:33 ` Andrey Smirnov 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2017-03-16 15:09 UTC (permalink / raw) To: Andrey Smirnov Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Thu, Mar 16, 2017 at 4:23 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > On Tue, Mar 14, 2017 at 4:17 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov >> <andrew.smirnov@gmail.com> wrote: >>> +int serdev_device_write(struct serdev_device *serdev, >>> + const unsigned char *buf, size_t count) >>> +{ >> >>> + int ret = count; >> >> If count by some reason bigger than INT_MAX... > True, I was merely copying type signature of serdev_device_write_buf, > which would have the same problem. I can add an appropriate check to > the code, but at the same time, I'd love to know why it wasn't a > concern in the latter function. Perhaps we may survive without changing it, just document that count should not be bigger than INT_MAX. OTOH it's counter intuitive to have size_t type which is used as int. >>> + for (;;) { >>> + size_t chunk; >>> + >>> + reinit_completion(&serdev->write_wakeup); >>> + >>> + chunk = serdev_device_write_buf(serdev, buf, count); >> >> >>> + if (chunk < 0) { >> >> This will never happen. What kind of test did you try? > None of my code using that function ever hit that condition, OTOH, I > am not sure why I should ignore the API's signed return type, which I > assume is so in order to facilitate negative error returns. My > thinking is that even if in the current codebase is incapable of ever > returning negative result today, that doesn't mean it won't ever be, > and IMHO the chances of that are no different from the chances of > someone passing 'count' that is bigger than INT_MAX. Given that I'd > rather keep the check until the type signature of > serdev_device_write_buf changes. You missed the point size_t is *unsigned* type! >>> + if (!count) >> >> What is supposed to be returned? Initial count? Does it make any sense? > I'll change it to do that in v2, but I am open to suggestions. Since function lacks of description (or I missed it?) I am out of knowledge what you are trying to achieve here. > I can see how that might make the diff look nice, but OTOH, I'd rather > not limit myself to only ~4 letters. Up to you and Rob. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-16 15:09 ` Andy Shevchenko @ 2017-03-17 13:33 ` Andrey Smirnov 0 siblings, 0 replies; 13+ messages in thread From: Andrey Smirnov @ 2017-03-17 13:33 UTC (permalink / raw) To: Andy Shevchenko Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Thu, Mar 16, 2017 at 8:09 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Mar 16, 2017 at 4:23 PM, Andrey Smirnov > <andrew.smirnov@gmail.com> wrote: >> On Tue, Mar 14, 2017 at 4:17 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >>> On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov >>> <andrew.smirnov@gmail.com> wrote: > >>>> +int serdev_device_write(struct serdev_device *serdev, >>>> + const unsigned char *buf, size_t count) >>>> +{ >>> >>>> + int ret = count; >>> >>> If count by some reason bigger than INT_MAX... > >> True, I was merely copying type signature of serdev_device_write_buf, >> which would have the same problem. I can add an appropriate check to >> the code, but at the same time, I'd love to know why it wasn't a >> concern in the latter function. > > Perhaps we may survive without changing it, just document that count > should not be bigger than INT_MAX. > OTOH it's counter intuitive to have size_t type which is used as int. > >>>> + for (;;) { >>>> + size_t chunk; >>>> + >>>> + reinit_completion(&serdev->write_wakeup); >>>> + >>>> + chunk = serdev_device_write_buf(serdev, buf, count); >>> >>> >>>> + if (chunk < 0) { >>> >>> This will never happen. What kind of test did you try? > >> None of my code using that function ever hit that condition, OTOH, I >> am not sure why I should ignore the API's signed return type, which I >> assume is so in order to facilitate negative error returns. My >> thinking is that even if in the current codebase is incapable of ever >> returning negative result today, that doesn't mean it won't ever be, >> and IMHO the chances of that are no different from the chances of >> someone passing 'count' that is bigger than INT_MAX. Given that I'd >> rather keep the check until the type signature of >> serdev_device_write_buf changes. > > You missed the point > > size_t is *unsigned* type! > Ah! I did miss the point! Yes, that's a bug for sure, will fix in v2. Good catch, thank you! >>>> + if (!count) >>> >>> What is supposed to be returned? Initial count? Does it make any sense? > >> I'll change it to do that in v2, but I am open to suggestions. > > Since function lacks of description (or I missed it?) I am out of > knowledge what you are trying to achieve here. > There's a description of what I had in mind for this function in commit message. It does need to be documented in the code, I agree. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-14 13:48 ` [PATCH 1/2] serdev: Add serdev_device_write subroutine Andrey Smirnov 2017-03-14 23:17 ` Andy Shevchenko @ 2017-03-15 14:18 ` Rob Herring 2017-03-16 13:33 ` Andrey Smirnov 1 sibling, 1 reply; 13+ messages in thread From: Rob Herring @ 2017-03-15 14:18 UTC (permalink / raw) To: Andrey Smirnov; +Cc: Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > Add serdev_device_write() which is a blocking call allowing to transfer > arbitraty amount of data (potentially exceeding amount that > serdev_device_write_buf can process in a single call) > > Cc: cphealy@gmail.com > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: linux-serial@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/serdev.h | 23 +++++++++++++++++------ > 2 files changed, 54 insertions(+), 6 deletions(-) > > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c > index f4c6c90..759e834 100644 > --- a/drivers/tty/serdev/core.c > +++ b/drivers/tty/serdev/core.c > @@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev, > } > EXPORT_SYMBOL_GPL(serdev_device_write_buf); > > +int serdev_device_write(struct serdev_device *serdev, > + const unsigned char *buf, size_t count) _write vs. _write_buf are not all that clear what the difference is. but I don't have a better name. Perhaps a timeout param is needed? This could never complete if CTS remains deasserted. > +{ > + int ret = count; > + > + if (serdev->ops->write_wakeup) > + return -EINVAL; > + > + mutex_lock(&serdev->write_lock); > + > + for (;;) { > + size_t chunk; > + > + reinit_completion(&serdev->write_wakeup); Perhaps write_comp instead as write_wakeup is already a function name. > + > + chunk = serdev_device_write_buf(serdev, buf, count); > + if (chunk < 0) { > + ret = chunk; > + goto done; > + } > + > + buf += chunk; > + count -= chunk; > + > + if (!count) > + break; > + > + wait_for_completion(&serdev->write_wakeup); > + } > +done: > + mutex_unlock(&serdev->write_lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(serdev_device_write); > + > void serdev_device_write_flush(struct serdev_device *serdev) > { > struct serdev_controller *ctrl = serdev->ctrl; > @@ -232,6 +267,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_wakeup); > + 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..8f7aa35 100644 > --- a/include/linux/serdev.h > +++ b/include/linux/serdev.h > @@ -35,16 +35,19 @@ struct serdev_device_ops { > > /** > * struct serdev_device - Basic representation of an serdev device > - * @dev: Driver model representation of the device. > - * @nr: Device number on serdev bus. > - * @ctrl: serdev controller managing this device. > - * @ops: Device operations. > + * @dev: Driver model representation of the device. > + * @nr: Device number on serdev bus. > + * @ctrl: serdev controller managing this device. > + * @ops: Device operations. > + * @write_wakeup Completion used by serdev_device_write internally > */ > struct serdev_device { > struct device dev; > int nr; > struct serdev_controller *ctrl; > const struct serdev_device_ops *ops; > + struct completion write_wakeup; > + struct mutex write_lock; > }; > > static inline struct serdev_device *to_serdev_device(struct device *d) > @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl > { > struct serdev_device *serdev = ctrl->serdev; > > - if (!serdev || !serdev->ops->write_wakeup) > + if (!serdev) > return; > > - serdev->ops->write_wakeup(serdev); > + if (serdev->ops->write_wakeup) > + serdev->ops->write_wakeup(serdev); > + else > + complete(&serdev->write_wakeup); > } > > static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl, > @@ -187,6 +193,7 @@ 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); > +int serdev_device_write(struct serdev_device *serdev, const unsigned char *buf, size_t count); Drop the param names to be consistent with the rest of the declarations. > void serdev_device_write_flush(struct serdev_device *); > int serdev_device_write_room(struct serdev_device *); > > @@ -227,6 +234,10 @@ static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsi > { > return -ENODEV; > } > +static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf, size_t count) > +{ > + return -ENODEV; > +} > static inline void serdev_device_write_flush(struct serdev_device *sdev) {} > static inline int serdev_device_write_room(struct serdev_device *sdev) > { > -- > 2.9.3 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-15 14:18 ` Rob Herring @ 2017-03-16 13:33 ` Andrey Smirnov 2017-03-17 16:31 ` Rob Herring 0 siblings, 1 reply; 13+ messages in thread From: Andrey Smirnov @ 2017-03-16 13:33 UTC (permalink / raw) To: Rob Herring; +Cc: Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Wed, Mar 15, 2017 at 7:18 AM, Rob Herring <robh@kernel.org> wrote: > On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov > <andrew.smirnov@gmail.com> wrote: >> Add serdev_device_write() which is a blocking call allowing to transfer >> arbitraty amount of data (potentially exceeding amount that >> serdev_device_write_buf can process in a single call) >> >> Cc: cphealy@gmail.com >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: linux-serial@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> --- >> drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++ >> include/linux/serdev.h | 23 +++++++++++++++++------ >> 2 files changed, 54 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c >> index f4c6c90..759e834 100644 >> --- a/drivers/tty/serdev/core.c >> +++ b/drivers/tty/serdev/core.c >> @@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev, >> } >> EXPORT_SYMBOL_GPL(serdev_device_write_buf); >> >> +int serdev_device_write(struct serdev_device *serdev, >> + const unsigned char *buf, size_t count) > > _write vs. _write_buf are not all that clear what the difference is. > but I don't have a better name. > serdev_device_send? > Perhaps a timeout param is needed? This could never complete if CTS > remains deasserted. Yeah, I think it is a good idea, I'll add that in v2. > >> +{ >> + int ret = count; >> + >> + if (serdev->ops->write_wakeup) >> + return -EINVAL; >> + >> + mutex_lock(&serdev->write_lock); >> + >> + for (;;) { >> + size_t chunk; >> + >> + reinit_completion(&serdev->write_wakeup); > > Perhaps write_comp instead as write_wakeup is already a function name. > OK, will do. >> + >> + chunk = serdev_device_write_buf(serdev, buf, count); >> + if (chunk < 0) { >> + ret = chunk; >> + goto done; >> + } >> + >> + buf += chunk; >> + count -= chunk; >> + >> + if (!count) >> + break; >> + >> + wait_for_completion(&serdev->write_wakeup); >> + } >> +done: >> + mutex_unlock(&serdev->write_lock); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(serdev_device_write); >> + >> void serdev_device_write_flush(struct serdev_device *serdev) >> { >> struct serdev_controller *ctrl = serdev->ctrl; >> @@ -232,6 +267,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_wakeup); >> + 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..8f7aa35 100644 >> --- a/include/linux/serdev.h >> +++ b/include/linux/serdev.h >> @@ -35,16 +35,19 @@ struct serdev_device_ops { >> >> /** >> * struct serdev_device - Basic representation of an serdev device >> - * @dev: Driver model representation of the device. >> - * @nr: Device number on serdev bus. >> - * @ctrl: serdev controller managing this device. >> - * @ops: Device operations. >> + * @dev: Driver model representation of the device. >> + * @nr: Device number on serdev bus. >> + * @ctrl: serdev controller managing this device. >> + * @ops: Device operations. >> + * @write_wakeup Completion used by serdev_device_write internally >> */ >> struct serdev_device { >> struct device dev; >> int nr; >> struct serdev_controller *ctrl; >> const struct serdev_device_ops *ops; >> + struct completion write_wakeup; >> + struct mutex write_lock; >> }; >> >> static inline struct serdev_device *to_serdev_device(struct device *d) >> @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl >> { >> struct serdev_device *serdev = ctrl->serdev; >> >> - if (!serdev || !serdev->ops->write_wakeup) >> + if (!serdev) >> return; >> >> - serdev->ops->write_wakeup(serdev); >> + if (serdev->ops->write_wakeup) >> + serdev->ops->write_wakeup(serdev); >> + else >> + complete(&serdev->write_wakeup); >> } >> >> static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl, >> @@ -187,6 +193,7 @@ 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); >> +int serdev_device_write(struct serdev_device *serdev, const unsigned char *buf, size_t count); > > Drop the param names to be consistent with the rest of the declarations. OK, will do. > >> void serdev_device_write_flush(struct serdev_device *); >> int serdev_device_write_room(struct serdev_device *); >> >> @@ -227,6 +234,10 @@ static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsi >> { >> return -ENODEV; >> } >> +static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf, size_t count) >> +{ >> + return -ENODEV; >> +} >> static inline void serdev_device_write_flush(struct serdev_device *sdev) {} >> static inline int serdev_device_write_room(struct serdev_device *sdev) >> { >> -- >> 2.9.3 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine 2017-03-16 13:33 ` Andrey Smirnov @ 2017-03-17 16:31 ` Rob Herring 0 siblings, 0 replies; 13+ messages in thread From: Rob Herring @ 2017-03-17 16:31 UTC (permalink / raw) To: Andrey Smirnov; +Cc: Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Thu, Mar 16, 2017 at 8:33 AM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > On Wed, Mar 15, 2017 at 7:18 AM, Rob Herring <robh@kernel.org> wrote: >> On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov >> <andrew.smirnov@gmail.com> wrote: >>> Add serdev_device_write() which is a blocking call allowing to transfer >>> arbitraty amount of data (potentially exceeding amount that >>> serdev_device_write_buf can process in a single call) >>> >>> Cc: cphealy@gmail.com >>> Cc: Guenter Roeck <linux@roeck-us.net> >>> Cc: linux-serial@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >>> --- >>> drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++ >>> include/linux/serdev.h | 23 +++++++++++++++++------ >>> 2 files changed, 54 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c >>> index f4c6c90..759e834 100644 >>> --- a/drivers/tty/serdev/core.c >>> +++ b/drivers/tty/serdev/core.c >>> @@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev, >>> } >>> EXPORT_SYMBOL_GPL(serdev_device_write_buf); >>> >>> +int serdev_device_write(struct serdev_device *serdev, >>> + const unsigned char *buf, size_t count) >> >> _write vs. _write_buf are not all that clear what the difference is. >> but I don't have a better name. >> > > serdev_device_send? send vs. write_buf? Still not that clear. >> Perhaps a timeout param is needed? This could never complete if CTS >> remains deasserted. > > Yeah, I think it is a good idea, I'll add that in v2. Maybe just combine the 2 functions and a timeout of 0 follows the original behavior of serdev_device_write_buf. I'm not sure if we could handle write_wakeup in that case though. Rob ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] serdev: Add minimal bus locking API 2017-03-14 13:48 [PATCH 0/2] serdev API extension Andrey Smirnov 2017-03-14 13:48 ` [PATCH 1/2] serdev: Add serdev_device_write subroutine Andrey Smirnov @ 2017-03-14 13:48 ` Andrey Smirnov 2017-03-14 23:20 ` Andy Shevchenko 2017-03-15 14:25 ` Rob Herring 1 sibling, 2 replies; 13+ messages in thread From: Andrey Smirnov @ 2017-03-14 13:48 UTC (permalink / raw) To: Rob Herring Cc: Andrey Smirnov, cphealy, Guenter Roeck, linux-serial, linux-kernel Add minimal bus locking API which is useful for serial devices that implement request-reply protocol Cc: cphealy@gmail.com Cc: Guenter Roeck <linux@roeck-us.net> Cc: linux-serial@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- drivers/tty/serdev/core.c | 1 + include/linux/serdev.h | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c index 759e834..20ca231 100644 --- a/drivers/tty/serdev/core.c +++ b/drivers/tty/serdev/core.c @@ -269,6 +269,7 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl) serdev->dev.type = &serdev_device_type; init_completion(&serdev->write_wakeup); mutex_init(&serdev->write_lock); + mutex_init(&serdev->bus_lock); return serdev; } EXPORT_SYMBOL_GPL(serdev_device_alloc); diff --git a/include/linux/serdev.h b/include/linux/serdev.h index 8f7aa35..6b73a79 100644 --- a/include/linux/serdev.h +++ b/include/linux/serdev.h @@ -48,6 +48,7 @@ struct serdev_device { const struct serdev_device_ops *ops; struct completion write_wakeup; struct mutex write_lock; + struct mutex bus_lock; }; static inline struct serdev_device *to_serdev_device(struct device *d) @@ -55,6 +56,16 @@ static inline struct serdev_device *to_serdev_device(struct device *d) return container_of(d, struct serdev_device, dev); } +static inline void serdev_device_bus_lock(struct serdev_device *serdev) +{ + mutex_lock(&serdev->bus_lock); +} + +static inline void serdev_device_bus_unlock(struct serdev_device *serdev) +{ + mutex_unlock(&serdev->bus_lock); +} + /** * struct serdev_device_driver - serdev slave device driver * @driver: serdev device drivers should initialize name field of this -- 2.9.3 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] serdev: Add minimal bus locking API 2017-03-14 13:48 ` [PATCH 2/2] serdev: Add minimal bus locking API Andrey Smirnov @ 2017-03-14 23:20 ` Andy Shevchenko 2017-03-16 14:37 ` Andrey Smirnov 2017-03-15 14:25 ` Rob Herring 1 sibling, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2017-03-14 23:20 UTC (permalink / raw) To: Andrey Smirnov Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > Add minimal bus locking API which is useful for serial devices that > implement request-reply protocol Can you put an example here? I'm not sure mutex is needed at all. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] serdev: Add minimal bus locking API 2017-03-14 23:20 ` Andy Shevchenko @ 2017-03-16 14:37 ` Andrey Smirnov 0 siblings, 0 replies; 13+ messages in thread From: Andrey Smirnov @ 2017-03-16 14:37 UTC (permalink / raw) To: Andy Shevchenko Cc: Rob Herring, Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Tue, Mar 14, 2017 at 4:20 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov > <andrew.smirnov@gmail.com> wrote: >> Add minimal bus locking API which is useful for serial devices that >> implement request-reply protocol > > Can you put an example here? > > I'm not sure mutex is needed at all. My use case is a "supervisory" microcontroller connected to SoC via UART various aspects of which are exposed via MFD. I saw this kind of "device design pattern" a number of times in my career, so I thought and abstraction to help dealing with cases like that might be useful. However, since Rob mentioned that API's expectation is that any such locking is driver's responsibility, I'll drop this patch. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] serdev: Add minimal bus locking API 2017-03-14 13:48 ` [PATCH 2/2] serdev: Add minimal bus locking API Andrey Smirnov 2017-03-14 23:20 ` Andy Shevchenko @ 2017-03-15 14:25 ` Rob Herring 1 sibling, 0 replies; 13+ messages in thread From: Rob Herring @ 2017-03-15 14:25 UTC (permalink / raw) To: Andrey Smirnov; +Cc: Chris Healy, Guenter Roeck, linux-serial, linux-kernel On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > Add minimal bus locking API which is useful for serial devices that > implement request-reply protocol It's assumed that there's a single client, so I think the client drivers should manage any locking they need. Maybe that changes if we have multiple slave devices using RS-485. Rob ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-03-17 16:32 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-14 13:48 [PATCH 0/2] serdev API extension Andrey Smirnov 2017-03-14 13:48 ` [PATCH 1/2] serdev: Add serdev_device_write subroutine Andrey Smirnov 2017-03-14 23:17 ` Andy Shevchenko 2017-03-16 14:23 ` Andrey Smirnov 2017-03-16 15:09 ` Andy Shevchenko 2017-03-17 13:33 ` Andrey Smirnov 2017-03-15 14:18 ` Rob Herring 2017-03-16 13:33 ` Andrey Smirnov 2017-03-17 16:31 ` Rob Herring 2017-03-14 13:48 ` [PATCH 2/2] serdev: Add minimal bus locking API Andrey Smirnov 2017-03-14 23:20 ` Andy Shevchenko 2017-03-16 14:37 ` Andrey Smirnov 2017-03-15 14:25 ` Rob Herring
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.