Re: [PATCH v4 1/3] mfd: intel_soc_pmic: Core driver

[Lee Jones <lee.jones@linaro.org>]

> >> +static int intel_soc_pmic_find_gpio_irq(struct device *dev)
> >> +{
> >> +	struct gpio_desc *desc;
> >> +	int irq;
> >> +
> >> +	desc = devm_gpiod_get_index(dev, KBUILD_MODNAME, 0);
> > 
> > What does "KBUILD_MODNAME" translate to?
> 
> It translates into "intel_soc_pmic".

Can you just put that instead?

> >> +	if (IS_ERR(desc)) {
> >> +		dev_dbg(dev, "Not using GPIO as interrupt.\n");
> > 
> > You can't have a debug print, then return an err - use dev_err().
> 
> Actually returning ENOENT here is just a hardware difference. On some
> boards the PMIC interrupt is from a GPIO line exposed by the CPU, on the
> rest (e.g. Asus T100TA) it's not. When -ENOENT is returned, probe() will
> simply use the IRQ provided by the I2C.
> 
> I will remove this line completely, and put a comment before the function.

That'll do, thanks.

> >> +static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
> >> +	{"INT33FD:00", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> >> +	{ }
> >> +};
> >> +MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
> >> +
> >> +static struct acpi_device_id intel_soc_pmic_acpi_match[] = {
> >> +	{"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_crc},
> >> +	{ },
> >> +};
> >> +MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
> > 
> > Does ACPI have a match function to extact it's .driver_data attribute?
> > 
> > If so, are you using it here? If not, why not?
> > 
> 
> The ACPI table is used in i2c_device_match(), and the i2c table is used
> in i2c_device_probe(), so the id in the i2c table is actually fed to
> intel_soc_pmic_probe(). But I only found out now that having the i2c
> table alone is enough, because i2c_device_match will fallback to the i2c
> table if there's no ACPI table. So to keep it simple, I'll remove the
> ACPI table completely.

Actually, can you do it the other way round?  Minimise the i2c table
and populate the ACPI one.  I'm just about to work on a separate
patch-set which deprecates the use of the i2c table on DT and/or ACPI
only registered devices.

> By the way, the GPIO child driver got reviewed-by from Linus Walleij,
> but can't be merged because it depends on intel_soc_pmic.h. May I
> include it in next version of the patch set and have it merged along
> with the MFD driver?

Yes, if it's okay with Linus and you aapply his Ack.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog