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

[Arnd Bergmann <arnd@arndb.de>]

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:
> > On 05/10/2021 13:47, conor.dooley@microchip.com wrote:
> >> From: Conor Dooley <conor.dooley@microchip.com>
> >>
> >> This driver provides an interface for other drivers to access the
> >> functions of the system controller on the Microchip PolarFire SoC.
> >>
> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > Is there some extra CCs that I am missing on this patch that weren't
> > picked up by getmaintainers, and/or am I mistaken in thinking that
> > the soc tree is arm only?

We can handle non-Arm SoCs as well, though most architecture
maintainers prefer to keep them merged through their trees.

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

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