Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Daniel Scally <djrscally@gmail.com>,
	linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, devel@acpica.org,
	rjw@rjwysocki.net, lenb@kernel.org, andy@kernel.org,
	mika.westerberg@linux.intel.com, linus.walleij@linaro.org,
	bgolaszewski@baylibre.com, wsa@kernel.org, lee.jones@linaro.org,
	hdegoede@redhat.com, mgross@linux.intel.com,
	robert.moore@intel.com, erik.kaneda@intel.com,
	sakari.ailus@linux.intel.com, kieran.bingham@ideasonboard.com
Subject: Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Date: Wed, 20 Jan 2021 06:18:53 +0200	[thread overview]
Message-ID: <YAevLTVlUSXMylWL@pendragon.ideasonboard.com> (raw)
In-Reply-To: <YAcaM9Tcif1rS3V/@smile.fi.intel.com>

Hi Andy,

On Tue, Jan 19, 2021 at 07:43:15PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote:
> > On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote:
> > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote:
> > > > On 18/01/2021 21:19, Daniel Scally wrote:
> 
> ...
> 
> > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of
> > > methods to be able to use it as a library.
> > 
> > That seems really overkill given the very simple implementation of the
> > clock provided here.
> 
> Less code in the end is called an overkill? Hmm...
> I think since we in Linux it's better to utilize what it provides. Do you want
> me to prepare a patch to show that there is no overkill at all?

The amount of code we would save it very small. It's not necessarily a
bad idea, but I think such an improvement could be made on top, it
shouldn't block this series.

> ...
> 
> > > > (also, Laurent, if we did it this way we wouldn't be able to also handle
> > > > the led-indicator GPIO here without some fairly major rework)
> > > 
> > > LED indicators are done as LED class devices (see plenty of examples in PDx86
> > > drivers: drivers/platform/x86/)
> > 
> > How do you expose the link between the sensor and its indicator LED to
> > userspace ? Isn't it better to handle it in the kernel to avoid rogue
> > userspace turning the camera on without notifying the user ?
> 
> I didn't get this. It's completely a LED handling driver business. We may
> expose it to user space or not, but it's orthogonal to the usage of LED class
> IIUC. Am I mistaken here?

If it stays internal to the kernel and is solely controlled from the
int3472 driver, there's no need to involve the LED class. If we want to
expose the privacy LED to userspace then the LED framework is the way to
go, but we will also need to find a way to expose the link between the
camera sensor and the LED to userspace. If there are two privacy LEDs,
one for the front sensor and one for the back sensor, userspace will
need to know which is which.

-- 
Regards,

Laurent Pinchart