Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1321167 > unrolled thread

Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A Mark Brown <broonie@kernel.org> - 2016-01-29 01:20 +0100
    RE: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-01-29 03:20 +0100
      Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A Mark Brown <broonie@kernel.org> - 2016-01-29 12:40 +0100
        Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A Javier Martinez Canillas <javier@dowhile0.org> - 2016-01-29 13:00 +0100
          Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A Mark Brown <broonie@kernel.org> - 2016-01-29 13:10 +0100
          RE: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-02-01 07:00 +0100
            Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A Javier Martinez Canillas <javier@dowhile0.org> - 2016-02-01 14:20 +0100
              RE: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-02-02 03:30 +0100
              RE: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-02-03 03:40 +0100
                Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A Mark Brown <broonie@kernel.org> - 2016-02-03 18:50 +0100
                  RE: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-02-04 14:30 +0100
        Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A Peter Korsgaard <peter@korsgaard.com> - 2016-02-02 18:50 +0100
          RE: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-02-03 03:30 +0100
            Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for  ACT8945A Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-02-03 03:50 +0100

#1321167 — Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jan 27, 2016 at 10:57:42AM +0800, Wenyou Yang wrote:

> +static const struct of_device_id act8945a_pmic_of_match[] = {
> +	{ .compatible = "active-semi,act8945a-regulator" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);

This seems mostly OK but why do we have a compatible string here -
shouldn't the MFD be able to instantiate the regulator function without
needing this?

[toc] | [next] | [standalone]

#1321383

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 2016年1月29日 8:16
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>;
> Pawel Moll <pawel.moll@arm.com>; Ian Campbell
> <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Krzysztof Kozlowski <k.kozlowski@samsung.com>; Javier Martinez Canillas
> <javier@dowhile0.org>; Lee Jones <lee.jones@linaro.org>; Peter Korsgaard
> <jacmet@gmail.com>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> On Wed, Jan 27, 2016 at 10:57:42AM +0800, Wenyou Yang wrote:
> 
> > +static const struct of_device_id act8945a_pmic_of_match[] = {
> > +	{ .compatible = "active-semi,act8945a-regulator" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
> 
> This seems mostly OK but why do we have a compatible string here - shouldn't
> the MFD be able to instantiate the regulator function without needing this?

Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
He said missing the OF match table will cause the module autoloading broken. 

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html

What do you think about it?


Best Regards,
Wenyou Yang

[toc] | [prev] | [next] | [standalone]

#1321687

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:

> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
> > > +	{ .compatible = "active-semi,act8945a-regulator" },
> > > +	{ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);

> > This seems mostly OK but why do we have a compatible string here - shouldn't
> > the MFD be able to instantiate the regulator function without needing this?

> Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
> He said missing the OF match table will cause the module autoloading broken. 

> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html

> What do you think about it?

If then device is not being loaded from the DT (and it shouldn't be, the
device looks like it should be instantiated directly by the MFD as it
can't exist separately to that MFD) an OF table will do nothing.  

[toc] | [prev] | [next] | [standalone]

#1321712 — Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

Hello Mark,

On Fri, Jan 29, 2016 at 8:35 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
>
>> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
>> > > + { .compatible = "active-semi,act8945a-regulator" },
>> > > + { },
>> > > +};
>> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
>
>> > This seems mostly OK but why do we have a compatible string here - shouldn't
>> > the MFD be able to instantiate the regulator function without needing this?
>
>> Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
>> He said missing the OF match table will cause the module autoloading broken.
>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html
>
>> What do you think about it?
>
> If then device is not being loaded from the DT (and it shouldn't be, the
> device looks like it should be instantiated directly by the MFD as it
> can't exist separately to that MFD) an OF table will do nothing.

Then he should remove the .of_compatible from the MFD cell definition.

Otherwise the MFD core will assign an of_node to this device in
mfd_add_device(), making the platform core to believe the device was
instantiated by OF. So an OF modalias uevent will be reported to load
the module and the OF table should be used for matching the device
with the driver.

IOW, the MFD and regulator driver have to agree on this. Either
everything should have OF or platform information, both work but
mixing these as it was on a previous version doesn't.

Best regards,
Javier

[toc] | [prev] | [next] | [standalone]

#1321716

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 29, 2016 at 08:50:19AM -0300, Javier Martinez Canillas wrote:

> Then he should remove the .of_compatible from the MFD cell definition.

Yes, obviously.

[toc] | [prev] | [next] | [standalone]

#1322829

Hi Javier & Mark,

I am educated. Thank you all.

> -----Original Message-----
> From: Javier Martinez Canillas [mailto:javier@dowhile0.org]
> Sent: 2016年1月29日 19:50
> To: Mark Brown <broonie@kernel.org>
> Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Liam Girdwood
> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
> <k.kozlowski@samsung.com>; Lee Jones <lee.jones@linaro.org>; Peter
> Korsgaard <jacmet@gmail.com>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> Hello Mark,
> 
> On Fri, Jan 29, 2016 at 8:35 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
> >
> >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {  {
> >> > > +.compatible = "active-semi,act8945a-regulator" },  { }, };
> >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
> >
> >> > This seems mostly OK but why do we have a compatible string here -
> >> > shouldn't the MFD be able to instantiate the regulator function without
> needing this?
> >
> >> Because I got feedback from Javier for the act8945a-charger patches
> >> of this MFD series, He said missing the OF match table will cause the module
> autoloading broken.
> >
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/39
> >> 8113.html
> >
> >> What do you think about it?
> >
> > If then device is not being loaded from the DT (and it shouldn't be,
> > the device looks like it should be instantiated directly by the MFD as
> > it can't exist separately to that MFD) an OF table will do nothing.
> 
> Then he should remove the .of_compatible from the MFD cell definition.

I tried it,

But if removed this .of_compatible, and reserved the OF table. 
the &pdev->dev->of_node is NULL, the driver fails to get the configuration value from DT, 

It seems the OF table still doesn't work. Where is wrong?

Could you help give some suggestion?

> 
> Otherwise the MFD core will assign an of_node to this device in mfd_add_device(),
> making the platform core to believe the device was instantiated by OF. So an OF
> modalias uevent will be reported to load the module and the OF table should be
> used for matching the device with the driver.
> 
> IOW, the MFD and regulator driver have to agree on this. Either everything should
> have OF or platform information, both work but mixing these as it was on a
> previous version doesn't.
> 
> Best regards,
> Javier


Best Regards,
Wenyou Yang

[toc] | [prev] | [next] | [standalone]

#1323093 — Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

Hello Wenyou,

[snip]

>> >
>> > If then device is not being loaded from the DT (and it shouldn't be,
>> > the device looks like it should be instantiated directly by the MFD as
>> > it can't exist separately to that MFD) an OF table will do nothing.
>>
>> Then he should remove the .of_compatible from the MFD cell definition.
>
> I tried it,
>
> But if removed this .of_compatible, and reserved the OF table.

What we said is that you should remove both the mfd cell
.of_compatible field and the OF table. Of course removing one but
leaving the other is not correct.

> the &pdev->dev->of_node is NULL, the driver fails to get the configuration value from DT,
>

I see, you mean that you are not able to lookup the
"active-semi,vsel-high" property.

> It seems the OF table still doesn't work. Where is wrong?
>

I think the problem is with your DT binding. You have a PMIC dev node
with compatible "active-semi,act8945a" that has a child node with
compatible "active-semi,act8945a-regulator" which in turn has the
"regulators" node.

I believe what Mark says is that there shouldn't be a compatible and
node for the regulators IC since is part of the PMIC. IOW, you can't
have it as a standalone node in the DTS.

> Could you help give some suggestion?
>

I would just remove the "active-semi,act8945a-regulator" node and make
"active-semi,vsel-high" a property of the "active-semi,act8945a" node.
That way you can remove the mfd cell .of_compatible and OF table in
the regulator driver and read the "active-semi,vsel-high" using the
platform device's parent of_node.

But it's better if you wait for Mark's opinions before re-spining your patches.

Best regards,
Javier

[toc] | [prev] | [next] | [standalone]

#1323709

Hi Javier,

> -----Original Message-----
> From: Javier Martinez Canillas [mailto:javier@dowhile0.org]
> Sent: 2016年2月1日 21:15
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Mark Brown <broonie@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Rob Herring <robh+dt@kernel.org>; Pawel Moll <pawel.moll@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Krzysztof Kozlowski <k.kozlowski@samsung.com>; Lee Jones
> <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre, Nicolas
> <Nicolas.FERRE@atmel.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> Hello Wenyou,
> 
> [snip]
> 
> >> >
> >> > If then device is not being loaded from the DT (and it shouldn't
> >> > be, the device looks like it should be instantiated directly by the
> >> > MFD as it can't exist separately to that MFD) an OF table will do nothing.
> >>
> >> Then he should remove the .of_compatible from the MFD cell definition.
> >
> > I tried it,
> >
> > But if removed this .of_compatible, and reserved the OF table.
> 
> What we said is that you should remove both the mfd cell .of_compatible field and
> the OF table. Of course removing one but leaving the other is not correct.

I am confused :)

> 
> > the &pdev->dev->of_node is NULL, the driver fails to get the
> > configuration value from DT,
> >
> 
> I see, you mean that you are not able to lookup the "active-semi,vsel-high"
> property.

No only this property,

The regulator init_datas are also not gotten from DT due to the &pdev->dev->of_node is NULL.

> 
> > It seems the OF table still doesn't work. Where is wrong?
> >
> 
> I think the problem is with your DT binding. You have a PMIC dev node with
> compatible "active-semi,act8945a" that has a child node with compatible "active-
> semi,act8945a-regulator" which in turn has the "regulators" node.
> 
> I believe what Mark says is that there shouldn't be a compatible and node for the
> regulators IC since is part of the PMIC. IOW, you can't have it as a standalone
> node in the DTS.
> 
> > Could you help give some suggestion?
> >
> 
> I would just remove the "active-semi,act8945a-regulator" node and make "active-
> semi,vsel-high" a property of the "active-semi,act8945a" node.
> That way you can remove the mfd cell .of_compatible and OF table in the
> regulator driver and read the "active-semi,vsel-high" using the platform device's
> parent of_node.
> 
> But it's better if you wait for Mark's opinions before re-spining your patches.

All right.

> 
> Best regards,
> Javier

Thanks 


Best Regards,
Wenyou Yang

[toc] | [prev] | [next] | [standalone]

#1324847

> -----Original Message-----
> From: Javier Martinez Canillas [mailto:javier@dowhile0.org]
> Sent: 2016年2月1日 21:15
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Mark Brown <broonie@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Rob Herring <robh+dt@kernel.org>; Pawel Moll <pawel.moll@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Krzysztof Kozlowski <k.kozlowski@samsung.com>; Lee Jones
> <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre, Nicolas
> <Nicolas.FERRE@atmel.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> Hello Wenyou,
> 
> [snip]
> 
> >> >
> >> > If then device is not being loaded from the DT (and it shouldn't
> >> > be, the device looks like it should be instantiated directly by the
> >> > MFD as it can't exist separately to that MFD) an OF table will do nothing.
> >>
> >> Then he should remove the .of_compatible from the MFD cell definition.
> >
> > I tried it,
> >
> > But if removed this .of_compatible, and reserved the OF table.
> 
> What we said is that you should remove both the mfd cell .of_compatible field and
> the OF table. Of course removing one but leaving the other is not correct.
> 
> > the &pdev->dev->of_node is NULL, the driver fails to get the
> > configuration value from DT,
> >
> 
> I see, you mean that you are not able to lookup the "active-semi,vsel-high"
> property.
> 
> > It seems the OF table still doesn't work. Where is wrong?
> >
> 
> I think the problem is with your DT binding. You have a PMIC dev node with
> compatible "active-semi,act8945a" that has a child node with compatible "active-
> semi,act8945a-regulator" which in turn has the "regulators" node.
> 
> I believe what Mark says is that there shouldn't be a compatible and node for the
> regulators IC since is part of the PMIC. IOW, you can't have it as a standalone
> node in the DTS.
> 
> > Could you help give some suggestion?
> >
> 
> I would just remove the "active-semi,act8945a-regulator" node and make "active-
> semi,vsel-high" a property of the "active-semi,act8945a" node.
> That way you can remove the mfd cell .of_compatible and OF table in the
> regulator driver and read the "active-semi,vsel-high" using the platform device's
> parent of_node.
> 
> But it's better if you wait for Mark's opinions before re-spining your patches.

Hi Mark, what is your opinions?

> 
> Best regards,
> Javier


Best Regards,
Wenyou Yang

[toc] | [prev] | [next] | [standalone]

#1325739

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 03, 2016 at 02:33:11AM +0000, Yang, Wenyou wrote:

> > I would just remove the "active-semi,act8945a-regulator" node and make "active-
> > semi,vsel-high" a property of the "active-semi,act8945a" node.
> > That way you can remove the mfd cell .of_compatible and OF table in the
> > regulator driver and read the "active-semi,vsel-high" using the platform device's
> > parent of_node.

> > But it's better if you wait for Mark's opinions before re-spining your patches.

> Hi Mark, what is your opinions?

Like Javier says you should make your DT binding look like the DT
bindings for other MFDs and not use of_compatible.

[toc] | [prev] | [next] | [standalone]

#1326795

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 2016年2月4日 1:48
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
> <k.kozlowski@samsung.com>; Lee Jones <lee.jones@linaro.org>; Peter
> Korsgaard <jacmet@gmail.com>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> On Wed, Feb 03, 2016 at 02:33:11AM +0000, Yang, Wenyou wrote:
> 
> > > I would just remove the "active-semi,act8945a-regulator" node and
> > > make "active- semi,vsel-high" a property of the "active-semi,act8945a" node.
> > > That way you can remove the mfd cell .of_compatible and OF table in
> > > the regulator driver and read the "active-semi,vsel-high" using the
> > > platform device's parent of_node.
> 
> > > But it's better if you wait for Mark's opinions before re-spining your patches.
> 
> > Hi Mark, what is your opinions?
> 
> Like Javier says you should make your DT binding look like the DT bindings for
> other MFDs and not use of_compatible.

All right.

Thank you, Mark, Javier.


Best Regards,
Wenyou Yang

[toc] | [prev] | [next] | [standalone]

#1324334 — Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

>>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:

 > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
 >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
 >> > > +	{ .compatible = "active-semi,act8945a-regulator" },
 >> > > +	{ },
 >> > > +};
 >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);

 >> > This seems mostly OK but why do we have a compatible string here - shouldn't
 >> > the MFD be able to instantiate the regulator function without needing this?

 >> Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
 >> He said missing the OF match table will cause the module autoloading broken. 

 >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html

 >> What do you think about it?

 > If then device is not being loaded from the DT (and it shouldn't be, the
 > device looks like it should be instantiated directly by the MFD as it
 > can't exist separately to that MFD) an OF table will do nothing.  

To add to the confusion, the regulator part of the chip is actually
identical to act8865, so it could use the existing regulator driver /
compatible, except that it binds to the platform bus instead of i2c.

-- 
Bye, Peter Korsgaard

[toc] | [prev] | [next] | [standalone]

#1324835

Hi Peter,

> -----Original Message-----
> From: Peter Korsgaard [mailto:jacmet@gmail.com] On Behalf Of Peter Korsgaard
> Sent: 2016年2月3日 1:42
> To: Mark Brown <broonie@kernel.org>
> Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Liam Girdwood
> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
> <k.kozlowski@samsung.com>; Javier Martinez Canillas <javier@dowhile0.org>;
> Lee Jones <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre,
> Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> >>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:
> 
>  > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
>  >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
>  >> > > +	{ .compatible = "active-semi,act8945a-regulator" },
>  >> > > +	{ },
>  >> > > +};
>  >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
> 
>  >> > This seems mostly OK but why do we have a compatible string here -
> shouldn't  >> > the MFD be able to instantiate the regulator function without
> needing this?
> 
>  >> Because I got feedback from Javier for the act8945a-charger patches of this
> MFD series,  >> He said missing the OF match table will cause the module
> autoloading broken.
> 
>  >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html
> 
>  >> What do you think about it?
> 
>  > If then device is not being loaded from the DT (and it shouldn't be, the  > device
> looks like it should be instantiated directly by the MFD as it  > can't exist
> separately to that MFD) an OF table will do nothing.
> 
> To add to the confusion, the regulator part of the chip is actually identical to
> act8865, so it could use the existing regulator driver / compatible, except that it
> binds to the platform bus instead of i2c.

Thank you for your opinion.

But I think It is better to make it a separate driver, the driver is simpler. 

> 
> --
> Bye, Peter Korsgaard


Best Regards,
Wenyou Yang

[toc] | [prev] | [next] | [standalone]

#1324849

On 03.02.2016 11:29, Yang, Wenyou wrote:
> Hi Peter,
> 
>> -----Original Message-----
>> From: Peter Korsgaard [mailto:jacmet@gmail.com] On Behalf Of Peter Korsgaard
>> Sent: 2016年2月3日 1:42
>> To: Mark Brown <broonie@kernel.org>
>> Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Liam Girdwood
>> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
>> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
>> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
>> <k.kozlowski@samsung.com>; Javier Martinez Canillas <javier@dowhile0.org>;
>> Lee Jones <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre,
>> Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
>> ACT8945A
>>
>>>>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:
>>
>>  > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
>>  >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
>>  >> > > +	{ .compatible = "active-semi,act8945a-regulator" },
>>  >> > > +	{ },
>>  >> > > +};
>>  >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
>>
>>  >> > This seems mostly OK but why do we have a compatible string here -
>> shouldn't  >> > the MFD be able to instantiate the regulator function without
>> needing this?
>>
>>  >> Because I got feedback from Javier for the act8945a-charger patches of this
>> MFD series,  >> He said missing the OF match table will cause the module
>> autoloading broken.
>>
>>  >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html
>>
>>  >> What do you think about it?
>>
>>  > If then device is not being loaded from the DT (and it shouldn't be, the  > device
>> looks like it should be instantiated directly by the MFD as it  > can't exist
>> separately to that MFD) an OF table will do nothing.
>>
>> To add to the confusion, the regulator part of the chip is actually identical to
>> act8865, so it could use the existing regulator driver / compatible, except that it
>> binds to the platform bus instead of i2c.
> 
> Thank you for your opinion.
> 
> But I think It is better to make it a separate driver, the driver is simpler. 

Do I understand correctly that you are creating a new device driver for
the same logical device (regulator block)?

By duplicating the code you are not making kernel simpler. Maybe your
new driver will be simple but still this adds a NEW driver instead of
re-using existing code.

Best regards,
Krzysztof

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web