On Tue, 16 Jun 2020, Baolin Wang wrote:
> On Mon, Jun 15, 2020 at 11:08 PM Orson Zhai <orsonz...@gmail.com> wrote:
> >
> > On Mon, Jun 15, 2020 at 10:12 PM Baolin Wang <baolin.wa...@gmail.com> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 11:47 AM Chunyan Zhang <zhang.l...@gmail.com>
> > > wrote:
> > > >
> > > > From: Chunyan Zhang <chunyan.zh...@unisoc.com>
> > > >
> > > > SC27XX-SPI added subdevices according to a pre-defined mfd_cell array,
> > > > no matter these devices were really included on board. So with this
> > > > patch we switch to a new way of detecting subdevices which are
> > > > defined in the devicetree.
> > > >
> > > > Signed-off-by: Chunyan Zhang <chunyan.zh...@unisoc.com>
> > > > ---
> > > > drivers/mfd/sprd-sc27xx-spi.c | 102 +++++++++++-----------------------
> > > > 1 file changed, 31 insertions(+), 71 deletions(-)
> > > >
> > > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c
> > > > b/drivers/mfd/sprd-sc27xx-spi.c
> > > > index 33336cde4724..aa3daa0cfcf5 100644
> > > > --- a/drivers/mfd/sprd-sc27xx-spi.c
> > > > +++ b/drivers/mfd/sprd-sc27xx-spi.c
> > > > @@ -93,73 +93,6 @@ enum usb_charger_type
> > > > sprd_pmic_detect_charger_type(struct device *dev)
> > > > }
> > > > EXPORT_SYMBOL_GPL(sprd_pmic_detect_charger_type);
> > > >
> > > > -static const struct mfd_cell sprd_pmic_devs[] = {
> > > > - {
> > > > - .name = "sc27xx-wdt",
> > > > - .of_compatible = "sprd,sc2731-wdt",
> > > > - }, {
> > > > - .name = "sc27xx-rtc",
> > > > - .of_compatible = "sprd,sc2731-rtc",
> > > > - }, {
> > > > - .name = "sc27xx-charger",
> > > > - .of_compatible = "sprd,sc2731-charger",
> > > > - }, {
> > > > - .name = "sc27xx-chg-timer",
> > > > - .of_compatible = "sprd,sc2731-chg-timer",
> > > > - }, {
> > > > - .name = "sc27xx-fast-chg",
> > > > - .of_compatible = "sprd,sc2731-fast-chg",
> > > > - }, {
> > > > - .name = "sc27xx-chg-wdt",
> > > > - .of_compatible = "sprd,sc2731-chg-wdt",
> > > > - }, {
> > > > - .name = "sc27xx-typec",
> > > > - .of_compatible = "sprd,sc2731-typec",
> > > > - }, {
> > > > - .name = "sc27xx-flash",
> > > > - .of_compatible = "sprd,sc2731-flash",
> > > > - }, {
> > > > - .name = "sc27xx-eic",
> > > > - .of_compatible = "sprd,sc2731-eic",
> > > > - }, {
> > > > - .name = "sc27xx-efuse",
> > > > - .of_compatible = "sprd,sc2731-efuse",
> > > > - }, {
> > > > - .name = "sc27xx-thermal",
> > > > - .of_compatible = "sprd,sc2731-thermal",
> > > > - }, {
> > > > - .name = "sc27xx-adc",
> > > > - .of_compatible = "sprd,sc2731-adc",
> > > > - }, {
> > > > - .name = "sc27xx-audio-codec",
> > > > - .of_compatible = "sprd,sc2731-audio-codec",
> > > > - }, {
> > > > - .name = "sc27xx-regulator",
> > > > - .of_compatible = "sprd,sc2731-regulator",
> > > > - }, {
> > > > - .name = "sc27xx-vibrator",
> > > > - .of_compatible = "sprd,sc2731-vibrator",
> > > > - }, {
> > > > - .name = "sc27xx-keypad-led",
> > > > - .of_compatible = "sprd,sc2731-keypad-led",
> > > > - }, {
> > > > - .name = "sc27xx-bltc",
> > > > - .of_compatible = "sprd,sc2731-bltc",
> > > > - }, {
> > > > - .name = "sc27xx-fgu",
> > > > - .of_compatible = "sprd,sc2731-fgu",
> > > > - }, {
> > > > - .name = "sc27xx-7sreset",
> > > > - .of_compatible = "sprd,sc2731-7sreset",
> > > > - }, {
> > > > - .name = "sc27xx-poweroff",
> > > > - .of_compatible = "sprd,sc2731-poweroff",
> > > > - }, {
> > > > - .name = "sc27xx-syscon",
> > > > - .of_compatible = "sprd,sc2731-syscon",
> > > > - },
> > > > -};
> > > > -
> > > > static int sprd_pmic_spi_write(void *context, const void *data, size_t
> > > > count)
> > > > {
> > > > struct device *dev = context;
> > > > @@ -205,6 +138,35 @@ static const struct regmap_config sprd_pmic_config
> > > > = {
> > > > .max_register = 0xffff,
> > > > };
> > > >
> > > > +static int sprd_pmic_add_subdevices(struct device *dev, int id,
> > > > + struct irq_domain *domain)
> > > > +{
> > > > + int ret = 0;
> > > > + struct device_node *child, *parent = dev->of_node;
> > > > + struct mfd_cell cell = {0};
> > > > + const char *comp;
> > > > + unsigned int prefix_len = strlen("sprd,");
> > > > + char buf[30];
> > > > +
> > > > + for_each_child_of_node(parent, child) {
> > > > + comp = of_get_property(child, "compatible", NULL);
> > > > + if (!comp || strncmp("sprd,", comp, prefix_len))
> > > > + return -EINVAL;
> > > > +
> > > > + memcpy(buf, comp, strlen(comp) + 1);
> > > > + cell.of_compatible = buf;
> > > > + cell.name = buf + prefix_len;
> > >
> > > I feel it is a little hackish, you can create any device nodes from DT
> > > even the PMIC does not support.
> >
> > In the old code, if sub cells have wrong compatible strings, he will not get
> > an opportunity to be probed.
>
> Yes, that's correct, and that's what PMIC asks.
>
> > It is confused to user for there is no any error and warning message.
>
> The sub-nodes of PMIC should describe their standard compatible string
> in their DT bindings.
>
> > I think it maybe better to let them being probed even when they might be
> > wrong.
> > The cell will check by itself -- return success in probing if in the
> > right place or return error if any
> > thing is wrong there.
>
> I do not think this is reasonable, since I can hack the DT to create
> any device nodes that PMIC does not support. But if Lee agrees with
> Chunyan's change, I am fine with that.
Obviously I do not agree to this patch, as it's one big hack.
Why not just use of_platform_populate()?
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
