Re: [PATCH v6 5/5] iio: light: Add support for APDS9306 Light Sensor

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Matti Vaittinen <mazziesaccount@gmail.com>,
	Marek Vasut <marex@denx.de>, Anshul Dalal <anshulusr@gmail.com>,
	Javier Carrasco <javier.carrasco.cruz@gmail.com>,
	Matt Ranostay <matt@ranostay.sg>,
	Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 5/5] iio: light: Add support for APDS9306 Light Sensor
Date: Wed, 7 Feb 2024 16:53:19 +0200	[thread overview]
Message-ID: <ZcOZX8mWTozC2EAc@smile.fi.intel.com> (raw)
In-Reply-To: <43e01493-1f26-414b-b2eb-7fb959b9b542@tweaklogic.com>

On Wed, Feb 07, 2024 at 09:37:37PM +1030, Subhajit Ghosh wrote:

...

> > > +static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
> > > +		APDS9306_NUM_REPEAT_RATES);
> > 
> > Just make that define to be inside [] in the respective array and drop this
> > static assert. The assertion might make sense to have different arrays to be
> > synchronized and when their maximums are different due to semantics (not your
> > case AFAICS).

...

> > > +static_assert(ARRAY_SIZE(apds9306_repeat_rate_period) ==
> > > +		APDS9306_NUM_REPEAT_RATES);
> > 
> > Ditto.

> I apologize for this. You pointed me out in an earlier review, I misunderstood
> it and used the macro in two static asserts! It will be fixed.

It might be used, but not must be, use your common sense!
In this case it's easier to have all defined properly from the beginning.

...

> > > +static int apds9306_runtime_power_on(struct device *dev)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = pm_runtime_resume_and_get(dev);
> > > +	if (ret < 0)
> > > +		dev_err_ratelimited(dev, "runtime resume failed: %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int apds9306_runtime_power_off(struct device *dev)
> > > +{
> > > +	pm_runtime_mark_last_busy(dev);
> > > +	pm_runtime_put_autosuspend(dev);
> > > +
> > > +	return 0;
> > > +}
> > 
> > Seems to me like useless wrappers. Why do you need that message?
> No specific need for that message, however the wrapper was suggested in a previous review:
> https://lore.kernel.org/all/ZTuuUl0PBklbVjb9@smile.fi.intel.com/

I see, what I meant in previous review is to split to separate helpers.
Now, when we see how it looks like and how many actual users, we may
go further and drop them.

> Do you still want me to use the pm functions directly from the calling functions?

It seems a good move forward.

> > Btw, it's used only twice, open coding saves the LoCs!
> Yes, it makes sense.

-- 
With Best Regards,
Andy Shevchenko