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

[Andy Shevchenko]

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.

>>> +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.

>>> +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?

-- 
With Best Regards,
Andy Shevchenko