On Tue, Jul 25, 2017 at 3:25 AM, Andy Shevchenko
<andy.shevche...@gmail.com> wrote:
> On Mon, Jul 24, 2017 at 10:53 PM, Andrey Smirnov
> <andrew.smir...@gmail.com> wrote:
>> On Mon, Jul 24, 2017 at 10:25 AM, Andy Shevchenko
>> <andy.shevche...@gmail.com> wrote:
>>> On Mon, Jul 24, 2017 at 6:09 PM, Andrey Smirnov
>>> <andrew.smir...@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
