Messages in this thread | | | From | Andrey Smirnov <> | Date | Tue, 25 Jul 2017 05:37:40 -0700 | Subject | Re: [PATCH v3 1/2] platform: Add driver for RAVE Supervisory Processor |
| |
On Tue, Jul 25, 2017 at 3:25 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jul 24, 2017 at 10:53 PM, Andrey Smirnov > <andrew.smirnov@gmail.com> wrote: >> On Mon, Jul 24, 2017 at 10:25 AM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >>> On Mon, Jul 24, 2017 at 6:09 PM, Andrey Smirnov >>> <andrew.smirnov@gmail.com> wrote: > >>> Field descriptions are supposed to be _short_. > >>>> + * @part_number_firmware: >>>> + * @part_number_bootloader: >>>> + * @reset_reason: >>>> + * @copper_rev_rmb: >>>> + * @copper_rev_deb: >>>> + * @silicon_devid: >>>> + * @silicon_devrev: >>>> + * @copper_mod_rmb: >>>> + * @copper_mod_deb: >>> >>> ??? > >> That's my interpretation of you advice to describe those fields in >> detailed comment below. > > Put there short descriptions and explain them in details (if you want > to) below. Don't leave them blank. >
OK.
>>>> +int devm_rave_sp_register_event_notifier(struct device *dev, >>>> + struct notifier_block *nb) >>>> +{ >>>> + struct rave_sp *sp = dev_get_drvdata(dev->parent); >>>> + struct notifier_block **rcnb; >>>> + int ret; >>>> + >>>> + rcnb = devres_alloc(rave_sp_unregister_event_notifier, >>>> + sizeof(*rcnb), GFP_KERNEL); >>>> + if (!rcnb) >>>> + return -ENOMEM; >>>> + >>>> + ret = blocking_notifier_chain_register(&sp->event_notifier_list, nb); >>>> + if (!ret) { >>>> + *rcnb = nb; >>>> + devres_add(dev, rcnb); >>>> + } else { >>>> + devres_free(rcnb); >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> +EXPORT_SYMBOL_GPL(devm_rave_sp_register_event_notifier); >>> >>> Did I miss >>> >>> EXPORT_SYMBOL_GPL(devm_rave_sp_unregister_event_notifier); >>> >>> ? >> >> No, you did not, as I mentioned in my reply for v2 to you, there's no >> use-case for having that function, there's only one MFD-cell driver >> that calls devm_rave_sp_register_event_notifier() and it does so as >> the last step of its probe(), so there's not going to be any users of >> devm_rave_sp_unregister_event_notifier(). > > Ok. > >>>> +static const char *devm_rave_sp_version(struct device *dev, const char *buf) >>>> +{ >>>> + /* >>>> + * NOTE: Ther format string below uses %02d to display u16 >>>> + * intentionally for the sake of backwards compatibility with >>>> + * legacy software. >>>> + */ >>>> + return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n", >>>> + buf[0], le16_to_cpup((const __le16 *)&buf[1]), >>>> + buf[3], buf[4], buf[5]); >>>> +} >>> >>> One more question about le16_to_cpup() use. Is the variable in the >>> buffer guaranteed to be always in little endian format? >>> Okay, looks like it's protocol which is little endian. Ok. >>> >>> By the way, here it might be needed to call get_unaligned(). >>> >> >> Sure, I'll add that just in case. > > He-h, "just in case" is not good enough :-) I would like you > understand why that might be needed. > > When you run on platforms that have issues with unaligned access you > might get weird behaviour. To prevent such we have helpers. >
I understand the purpose of get_unaligned(). The reason I say "just in case" is because such platforms don't really exist in practice. All of the devices that ship with RAVE SP MCUs on-board are ARMv7A based CPUs which should be capable of performing unaligned access.
>>>> +int rave_sp_exec(struct rave_sp *sp, >>>> + void *__data, size_t data_size, >>>> + void *reply_data, size_t reply_data_size) >>>> +{ >>>> + int ret = 0; >>>> + unsigned char *data = __data; >>>> + const u8 ackid = (u8)atomic_inc_return(&sp->ackid); >>>> + const int command = sp->variant->cmd.translate(data[0]); >>>> + struct rave_sp_reply reply = { >>>> + .code = rave_sp_reply_code((u8)command), >>>> + .ackid = ackid, >>>> + .data = reply_data, >>>> + .length = reply_data_size, >>>> + .received = COMPLETION_INITIALIZER_ONSTACK(reply.received), >>>> + }; >>>> + >>> >>>> + if (command < 0) >>>> + return command; >>> >>> >>> Shouldn't be like >>> >>> command = sp->variant->cmd.translate(data[0]); >>> if (command < 0) >>> return command; >>> >>> reply.code = rave_sp_reply_code((u8)command); >>> >>> ? >> >> Shouldn't really make any difference, rave_sp_reply_code() will either >> return -EINVAL or some ACK code but in either case it would not be >> used due to early return on "command" being less that 0. > > So, why then run a code that will be thrown away? >
Because this way "reply" is initialized in a single place instead of that code being spread around.
Thanks, Andrey Smirnov
| |