Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Cristian Marussi <cristian.marussi@arm.com>]

On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> Hi Andy,
> 

Hi Peng,


> > Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> > protocol basic support
> > 
> > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> > 
> > ...
> > 
> > >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > > system.o voltage.o powercap.o
> > 
> > Actually you want to have := here.
> > 
> > > +scmi-protocols-y += pinctrl.o
> > 
> > 
> > 
> > >  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > > $(scmi-transport-y)
> > 
> > Side note: The -objs has to be -y
> > 
> > ...
> > 
> > > +#include <linux/module.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> > 
> > This is semi-random list of headers. Please, follow IWYU principle (include
> > what you use). There are a lot of inclusions I see missing (just in the context of
> > this page I see bits.h, types.h, and  asm/byteorder.h).
> 
> Is there any documentation about this requirement?
> Some headers are already included by others.
> 

Andy made (mostly) the same remarks on this same patch ~1-year ago on
this same patch while it was posted by Oleksii.

And I told that time that most of the remarks around devm_ usage were
wrong due to how the SCMI core handles protocol initialization (using a
devres group transparently).

This is what I answered that time.

https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t

I wont repeat myself, but, in a nutshell the memory allocation like it
is now is fine: a bit happens via devm_ at protocol initialization, the
other is doe via explicit kmalloc at runtime and freed via kfree at
remove time (if needed...i.e. checking the present flag of some structs)

I'll made further remarks on v7 that you just posted.

Thanks,
Cristian