Re: [PATCH] soc: add polarfire soc system controller

[<Conor.Dooley@microchip.com>]

resending, think i accidentally clicked a formatting option and the mail 
got converted to html.

On 21/10/2021 19:34, Arnd Bergmann wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Oct 21, 2021 at 6:00 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> On Thu, 21 Oct 2021 06:13:35 PDT (-0700), Conor.Dooley@microchip.com wrote:
>
>> +Arnd, Olof, and the SOC list.  They probably understand this better
>> than I do, we're kind of new to having SOCs in RISC-V land.
>>
>> I guess I was assuming that someone maintained drivers/soc, but from
>> poking around it seems like there's no entry for it and instead it's
>> just a bunch of entries for the sub-directories.  As a result the
>> scripts aren't picking up anyone to send these too, and I'd assuming
>> that because they're not in arch/riscv that they're not for the RISC-V
>> tree.
>>
>> That said, it looks like I put the Kendryte stuff in there (so sorry if
>> I screwed anything up).  I'm happy to take these via the RISC-V tree as
>> well, I'm assuming that means there should be a MAINTAINERS entry for
>> this new sub-directory so changes to it are less likely to get lost.
>> Sorry if I was confusing before, I guess I forgot about how this fits
>> together.
>>
>> Arnd: aside from the lack of a maintainer, these generally look fine to
>> me.  LMK if you were expecting this kind of stuff to go through the
>> RISC-V tree.
> 
> It probably helps avoid merge conflicts to go through the soc tree,
> as there are generally more changes for arm specific socs in there.
Yeah, that sounds like a good idea. Spoke to Nicolas this morning and 
will send a revised version of this w/ your comments addressed and 
future polarfire soc additions via the at91/sam tree.
> 
> However, we usually take pull requests from platform maintainers,
> not individual patches. For Microchip's ARM based platforms, those
> patches would go through the AT91/SAMA5 maintainers (added to
> Cc). You can ask them if they are willing to take future patches for
> the polarfire soc as well and forward them to soc@kernel.org along
> with the other stuff.
> 
>>>> +int mpfs_blocking_transaction(struct mpfs_sys_controller *mpfs_client, void *msg)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    mutex_lock_interruptible(&transaction_lock);
> 
> When you do a mutex_lock_interruptible(), you have to check its return code and
> handle the interruption, usually by passing down -EINTR to the caller.
> 
>>>> +struct mpfs_sys_controller *
>>>> +mpfs_sys_controller_get(struct device_node *mss_node)
>>>> +{
>>>> +    struct platform_device *pdev = of_find_device_by_node(mss_node);
>>>> +
>>>> +    if (!pdev)
>>>> +            return NULL;
>>>> +
>>>> +    return platform_get_drvdata(pdev);
>>>> +}
>>>> +EXPORT_SYMBOL(mpfs_sys_controller_get);
> 
> There should probably be a check in here to ensure that this is actually
> a system controller and it's bound do this driver, rather than returning a
> random device's driver data.
> 
> It might also help to make this take a phandle instead of a device node
> for lookup, to spare the client the extra phandle to node conversion.
> 
>         Arnd
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv