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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 7 Feb 2024 16:53:19 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Matti Vaittinen <mazziesaccount@...il.com>,
	Marek Vasut <marex@...x.de>, Anshul Dalal <anshulusr@...il.com>,
	Javier Carrasco <javier.carrasco.cruz@...il.com>,
	Matt Ranostay <matt@...ostay.sg>,
	Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 5/5] iio: light: Add support for APDS9306 Light Sensor

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