| Message ID | 20240529162958.18081-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic | expand |
On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > Rework the pm8008 driver to match the new binding which no longer > describes internal details like interrupts and register offsets > (including which of the two consecutive I2C addresses the registers > belong to). > > Instead make the interrupt controller implementation internal and pass > interrupts to the subdrivers using MFD cell resources. > > Note that subdrivers may either get their resources, like register block > offsets, from the parent MFD or this can be included in the subdrivers > directly. > > In the current implementation, the temperature alarm driver is generic > enough to just get its base address and alarm interrupt from the parent > driver, which already uses this information to implement the interrupt > controller. > > The regulator driver, however, needs additional information like parent > supplies and regulator characteristics so in that case it is easier to > just augment its table with the regulator register base addresses. > > Similarly, the current GPIO driver already holds the number of pins and > that lookup table can therefore also be extended with register offsets. > > Note that subdrivers can now access the two regmaps by name, even if the > primary regmap is registered last so that it is returned by default when > no name is provided in lookups. > > Finally, note that the temperature alarm and GPIO subdrivers need some > minor rework before they can be used with non-SPMI devices like the > PM8008. The temperature alarm MFD cell name specifically uses a "qpnp" > rather than "spmi" prefix to prevent binding until the driver has been > updated. ... > + dummy = devm_i2c_new_dummy_device(dev, client->adapter, client->addr + 1); > + if (IS_ERR(dummy)) { > + ret = PTR_ERR(dummy); > + dev_err(dev, "failed to claim second address: %d\n", ret); > + return ret; > + } > + ret = devm_regmap_add_irq_chip_fwnode(dev, fwnode, regmap, client->irq, > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data); > + if (ret) { > + dev_err(dev, "failed to add IRQ chip: %d\n", ret); > + return ret; > } I believe there is no harm to use return dev_err_probe(...); for these. But it seems you don't like that API. Whatever, no-one will die, just additional work for the future :-)
On Wed, May 29, 2024 at 10:53:09PM +0300, Andy Shevchenko wrote: > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > + ret = devm_regmap_add_irq_chip_fwnode(dev, fwnode, regmap, client->irq, > > IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data); > > + if (ret) { > > + dev_err(dev, "failed to add IRQ chip: %d\n", ret); > > + return ret; > > } > > I believe there is no harm to use > > return dev_err_probe(...); > > for these. But it seems you don't like that API. Whatever, no-one will > die, just additional work for the future :-) Correct. And there's no additional work for the future here either. Johan
On Thu, May 30, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote: > On Wed, May 29, 2024 at 10:45:40PM +0300, Andy Shevchenko wrote: > > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > > > Request and deassert any (optional) reset gpio during probe in case it > > > has been left asserted by the boot firmware. > > > > > > Note the reset line is not asserted to avoid reverting to the default > > > I2C address in case the firmware has configured an alternate address. ... > > > + /* > > > + * The PMIC does not appear to require a post-reset delay, but wait > > > + * for a millisecond for now anyway. > > > + */ > > > > > + usleep_range(1000, 2000); > > > > fsleep() ? > > No, I'd only use fsleep() when the argument is variable. Okay, this is basically the same issue as with use of dev_err_probe() with known errors. fsleep() hides the choice between let's say msleep() / usleep_range() / udelay() from the caller. This, in particular, might allow shifting constraints if the timer core is changed or becomes more granular. It's independent to the variable or constant parameter(s). Whatever, I'm not going to insist.
On Wed, 29 May 2024, Johan Hovold wrote: > The Qualcomm PM8008 PMIC is a so called QPNP PMIC with seven LDO > regulators, a temperature alarm block and two GPIO pins (which are also > used for interrupt signalling and reset). I don't see any issues with the MFD commits. When you submit this, would you do me a favour and change the subject lines to match that of the subsystem. I usually silently change them, but this is a large set and it'll become annoying real quick. `git log --oneline -- <subsystem>` is your friend. Thanks.