Re: [PATCH v3 1/2] platform: Add driver for RAVE Supervisory Processor

[Andrey Smirnov]

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