Re: [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	linux-acpi@vger.kernel.org, Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Len Brown <lenb@kernel.org>
Subject: Re: [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
Date: Tue, 8 Jan 2019 19:33:44 +0200	[thread overview]
Message-ID: <20190108173344.GE9170@smile.fi.intel.com> (raw)
In-Reply-To: <ed2fda33-3728-8585-e56d-7857d9217cc0@redhat.com>

On Tue, Jan 08, 2019 at 04:35:45PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-01-19 15:51, Andy Shevchenko wrote:
> > On Tue, Jan 08, 2019 at 02:45:39PM +0100, Hans de Goede wrote:
> > > On 07-01-19 16:46, Andy Shevchenko wrote:
> > > > On Mon, Jan 07, 2019 at 12:15:55PM +0100, Hans de Goede wrote:
> > 
> > > > > +	} else if (d->pmic_i2c_address) {
> > > > > +		if (i2c_address == d->pmic_i2c_address) {
> > > > > +			ret = regmap_update_bits(intel_pmic_opregion->regmap,
> > > > > +						 reg_address, mask, value);
> > > > > +		} else {
> > > > > +			pr_err("%s: Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n",
> > > > > +			       __func__, i2c_address, reg_address, value, mask);
> > > > > +			ret = -ENXIO;
> > > > > +		}
> > > > 
> > > > > --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> > > > > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> > > > > +	.pmic_i2c_address = 0x34,
> > > > 
> > > > Can we just have a hook here instead of exposing PMIC I2C address?
> > > > Am I missing something in case it's not possible?
> > > 
> > > We already have a hook, but it isn't really necessary to implement
> > > that for each model PMIC. The MFD device which is the PMIC's parent
> > > in most cases will give us a regmap to access the PMIC registers and
> > > that allows us to do a generic implementation.
> > > 
> > > But the MIPI PMIC sequence includes an i2c-address as some PMICs
> > > span multiple i2c-addresses. For the simple single i2c-address case
> > > the regmap gives us access to the registers behind that single address
> > > and we can use a generic solution. In this case we should verify the
> > > i2c-addr is what we expect, which is where the pmic_i2c_address comes in.
> > 
> > But we also can have a generic hook in intel_pmic.c and assign it whenever
> > it's the case?
> 
> We could, but then we still need the pmic_i2c_address member and now the
> hook would need to passed both the regmap and the intel_pmic_opregion_data
> pointer so that it can verify the i2c address so handling the generic case
> directly inside intel_soc_pmic_exec_mipi_pmic_seq_element is easier.

I see.

One more question, can we unify somehow error messages and do something like

if (...) {
	...
} else if (pmic_i2c_address && i2c_address == pmic_i2c_address) {
	ret = regmap_update_bits(...);
} else {
	...
}

?

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx