RE: [PATCH 4/4 v4] GPIO: gpio-dwapb: Suspend & Resume PM enabling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> > Reviewed-by: Hock Leong Kweh <hock.leong.kweh@xxxxxxxxx>
> 
> You still keep that guy as reviewer in a whole series, however I didn't see a
> word from him here. How is it possible?
In our internal review, he gave me a lot of suggestions. 

> > +	for (i = 0; i < gpio->nr_ports; i++) {
> > +		unsigned int offset;
> > +		unsigned int idx = gpio->ports[i].idx;
> > +		struct dwapb_context *ctx = gpio->ports[i].ctx;
> > +
> > +		if (!ctx) {
> > +			ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +			gpio->ports[i].ctx = ctx;
> > +		}
> 
> I don't think it's a right place to allocate this resource, especially with devm_
> helper. Can you move this to probe() stage?
> 
> Or you even can embed contex structure inside chip one with #ifdef
> CONFIG_PM_SLEEP.
> 

OK, I will improve it.

> Maybe others could comment on this.
> 
> > +
> > +		offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
> 
> No need to have parentheses here. Check the code below as well.
OK. I will remove them.
��.n��������+%������w��{.n�����{��
b���ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f





[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux