Re: [PATCH v4 5/6] at24: Support probing while off

From: Sakari Ailus <sakari.ailus@iki.fi>
To: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	linux-i2c <linux-i2c@vger.kernel.org>,
	Wolfram Sang <wsa@the-dreams.de>,
	linux-acpi@vger.kernel.org, Bingbu Cao <bingbu.cao@intel.com>,
	linux-media <linux-media@vger.kernel.org>,
	Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>,
	Hyungwoo Yang <hyungwoo.yang@intel.com>,
	Arnd Bergmann <arnd@arndb.de>,
	LKML <linux-kernel@vger.kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rajmohan Mani <rajmohan.mani@intel.com>,
	Tomasz Figa <tfiga@chromium.org>
Subject: Re: [PATCH v4 5/6] at24: Support probing while off
Date: Tue, 11 Aug 2020 11:00:09 +0300	[thread overview]
Message-ID: <20200811080009.GE840@valkosipuli.retiisi.org.uk> (raw)
In-Reply-To: <CAMpxmJUKSR-oCGnV1E5XiAMA2nYBy5f_f8=VSoMn0zf+qF39vg@mail.gmail.com>

Hi Bartosz,

On Mon, Aug 10, 2020 at 08:12:00PM +0200, Bartosz Golaszewski wrote:
> On Mon, Aug 10, 2020 at 10:26 AM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> 
> [snip]
> 
> > >
> > > Rafael: I think that there are two issues with patch 1/5:
> > > 1. It adds a very specific boolean flag to a structure that's meant to
> > > be very general. As I pointed out in the i2c patch: at the very least
> > > this could be made into an int storing flag values, instead of a
> > > boolean field. But rather than that - it looks to me more like a
> > > device (or bus) feature than a driver feature. Is there any ACPI flag
> > > we could use to pass this information to the driver model without
> > > changing the driver structure?
> >
> > To my knowledge there isn't. The fact that I²C devices are powered on for
> > probe in ACPI based systems is specific to Linux kernel and not ACPI as
> > such.
> >
> > The reason this needs to be in a generic struct is that the device's power
> > state will be changed before any interaction with the driver takes place as
> > it's the I²C framework that powers on the device.
> >
> 
> I'm not sure I'm following. Looking at patch 1/6 struct device already
> exists so why can't this information be conveyed "per device" as
> opposed to "per driver"?

It's both driver and device.

Suppose there's no indication of driver support. If you add the property
telling the device shouldn't be powered on for probe, it won't be. And if
the driver doesn't support that, probe will fail. That could happen e.g.
when running an older kernel on a system that happens to specify this
property for a given device.

You could view this as a driver bug of course. I still think it's better to
make driver support for this explicit, and avoid making this a practical
problem anywhere.

-- 
Kind regards,

Sakari Ailus