| Message ID | 20210703084224.31623-3-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | mfd: rn5t618: Extend ADC support | expand |
On Sat, 3 Jul 2021 10:42:22 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > This allows having devicetree nodes for the subdevices. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/mfd/rn5t618.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > index 384acb459427..b916c7471ca3 100644 > --- a/drivers/mfd/rn5t618.c > +++ b/drivers/mfd/rn5t618.c > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > }; > > static const struct mfd_cell rc5t619_cells[] = { > - { .name = "rn5t618-adc" }, > - { .name = "rn5t618-power" }, > + { .name = "rn5t618-adc", > + .of_compatible = "ricoh,rc5t619-adc" }, Odd to have a name of 618 and a compatible of 619. Why? Definitely deserves a comment if this is necessary for some reason! > + { .name = "rn5t618-power", > + .of_compatible = "ricoh,rc5t619-power" }, > { .name = "rn5t618-regulator" }, > { .name = "rc5t619-rtc" }, > { .name = "rn5t618-wdt" },
Hi, On Sat, 3 Jul 2021 17:04:05 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 3 Jul 2021 10:42:22 +0200 > Andreas Kemnade <andreas@kemnade.info> wrote: > > > This allows having devicetree nodes for the subdevices. > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > drivers/mfd/rn5t618.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > index 384acb459427..b916c7471ca3 100644 > > --- a/drivers/mfd/rn5t618.c > > +++ b/drivers/mfd/rn5t618.c > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > }; > > > > static const struct mfd_cell rc5t619_cells[] = { > > - { .name = "rn5t618-adc" }, > > - { .name = "rn5t618-power" }, > > + { .name = "rn5t618-adc", > > + .of_compatible = "ricoh,rc5t619-adc" }, > > Odd to have a name of 618 and a compatible of 619. Why? > Definitely deserves a comment if this is necessary for some reason! > Background of this whole thing: Both RN5T618 and RC5T619 have an ADC. I would expect the driver to work for both but I cannot prove that. No RN5T618 here to test. Maybe it requires some quirks, probably not. The only hint I have is that a) I use register definitions added to the kernel for RN5T618 support b) public datasheets looks same regarding the ADC. c) out-of-tree code for the RN5T618 does not give a clear picture, it shows no differences but is not complete enough to judge. To avoid introducing untested things, I am only adding it to the rc5t619_cell list. I would really hope that someone does some rn5t618 tests... Because everything else which is both for the RN5T618 and RC5T619 uses rn5t618 as a name, I continued that practise. The subdevice driver also gets the information whether it is a rn5t618 or rc5t619 via rn5t618->variant, so it can handle things. > > + { .name = "rn5t618-power", > > + .of_compatible = "ricoh,rc5t619-power" }, Similar situation here. > > { .name = "rn5t618-regulator" }, > > { .name = "rc5t619-rtc" }, and this one definitively only exists in the rc5t619. > > { .name = "rn5t618-wdt" }, > > Regards, Andreas
On Sat, 3 Jul 2021 18:31:11 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > Hi, > > On Sat, 3 Jul 2021 17:04:05 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sat, 3 Jul 2021 10:42:22 +0200 > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > This allows having devicetree nodes for the subdevices. > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > --- > > > drivers/mfd/rn5t618.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > index 384acb459427..b916c7471ca3 100644 > > > --- a/drivers/mfd/rn5t618.c > > > +++ b/drivers/mfd/rn5t618.c > > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > > }; > > > > > > static const struct mfd_cell rc5t619_cells[] = { > > > - { .name = "rn5t618-adc" }, > > > - { .name = "rn5t618-power" }, > > > + { .name = "rn5t618-adc", > > > + .of_compatible = "ricoh,rc5t619-adc" }, > > > > Odd to have a name of 618 and a compatible of 619. Why? > > Definitely deserves a comment if this is necessary for some reason! > > > Background of this whole thing: > Both RN5T618 and RC5T619 have an ADC. I would expect the driver to work > for both but I cannot prove that. No RN5T618 here to test. Maybe it > requires some quirks, probably not. The only hint I have is that > a) I use register definitions added to the kernel for RN5T618 support > b) public datasheets looks same regarding the ADC. > c) out-of-tree code for the RN5T618 does not give a clear picture, it > shows no differences but is not complete enough to judge. > > To avoid introducing untested things, I am only adding it to the > rc5t619_cell list. I would really hope that someone does some rn5t618 > tests... Because everything else which is both for the RN5T618 and > RC5T619 uses rn5t618 as a name, I continued that practise. > > The subdevice driver also gets the information whether it is a rn5t618 > or rc5t619 via rn5t618->variant, so it can handle things. > > > > + { .name = "rn5t618-power", > > > + .of_compatible = "ricoh,rc5t619-power" }, > > Similar situation here. > > > > { .name = "rn5t618-regulator" }, > > > { .name = "rc5t619-rtc" }, > and this one definitively only exists in the rc5t619. All sounds reasonable to me. Dt binding wise we'd normally handle this with a double compatible, with the more part specific one first. That way we can diverge later if we need to, but don't need to care about it until an identified need has occurred. compatible = "ricoh,rc5t619-adc", "ricoh,rc5t618-adc"; (or something like that) Anyhow, for now perhaps a comment to express briefly what you state above. > > > > { .name = "rn5t618-wdt" }, > > > > > > Regards, > Andreas
On Sat, 03 Jul 2021, Jonathan Cameron wrote: > On Sat, 3 Jul 2021 10:42:22 +0200 > Andreas Kemnade <andreas@kemnade.info> wrote: > > > This allows having devicetree nodes for the subdevices. > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > drivers/mfd/rn5t618.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > index 384acb459427..b916c7471ca3 100644 > > --- a/drivers/mfd/rn5t618.c > > +++ b/drivers/mfd/rn5t618.c > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > }; > > > > static const struct mfd_cell rc5t619_cells[] = { > > - { .name = "rn5t618-adc" }, > > - { .name = "rn5t618-power" }, > > + { .name = "rn5t618-adc", > > + .of_compatible = "ricoh,rc5t619-adc" }, > > Odd to have a name of 618 and a compatible of 619. Why? > Definitely deserves a comment if this is necessary for some reason! Actually this is the norm. We have lots of drivers named after the *first* device they supported before expansion. > > + { .name = "rn5t618-power", > > + .of_compatible = "ricoh,rc5t619-power" }, > > { .name = "rn5t618-regulator" }, > > { .name = "rc5t619-rtc" }, > > { .name = "rn5t618-wdt" }, >
On Sat, 03 Jul 2021, Andreas Kemnade wrote: > This allows having devicetree nodes for the subdevices. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/mfd/rn5t618.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > index 384acb459427..b916c7471ca3 100644 > --- a/drivers/mfd/rn5t618.c > +++ b/drivers/mfd/rn5t618.c > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > }; > > static const struct mfd_cell rc5t619_cells[] = { > - { .name = "rn5t618-adc" }, > - { .name = "rn5t618-power" }, > + { .name = "rn5t618-adc", > + .of_compatible = "ricoh,rc5t619-adc" }, > + { .name = "rn5t618-power", > + .of_compatible = "ricoh,rc5t619-power" }, If you're converting entries from single to multi-line, you should place the coding lines one different ones to the brackets. Also, if ordering is unimportant, could you move the multi-line entries to the bottom please? > { .name = "rn5t618-regulator" }, > { .name = "rc5t619-rtc" }, > { .name = "rn5t618-wdt" },
On Mon, 5 Jul 2021 08:36:08 +0100 Lee Jones <lee.jones@linaro.org> wrote: > On Sat, 03 Jul 2021, Jonathan Cameron wrote: > > > On Sat, 3 Jul 2021 10:42:22 +0200 > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > This allows having devicetree nodes for the subdevices. > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > --- > > > drivers/mfd/rn5t618.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > index 384acb459427..b916c7471ca3 100644 > > > --- a/drivers/mfd/rn5t618.c > > > +++ b/drivers/mfd/rn5t618.c > > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > > }; > > > > > > static const struct mfd_cell rc5t619_cells[] = { > > > - { .name = "rn5t618-adc" }, > > > - { .name = "rn5t618-power" }, > > > + { .name = "rn5t618-adc", > > > + .of_compatible = "ricoh,rc5t619-adc" }, > > > > Odd to have a name of 618 and a compatible of 619. Why? > > Definitely deserves a comment if this is necessary for some reason! > > Actually this is the norm. We have lots of drivers named after the > *first* device they supported before expansion. Ah. I'd missed that this cells array is specific to the 5t619, though if the driver is the same I'd also expect it to be needed for the 5t618 entry. > > > > + { .name = "rn5t618-power", > > > + .of_compatible = "ricoh,rc5t619-power" }, > > > { .name = "rn5t618-regulator" }, > > > { .name = "rc5t619-rtc" }, > > > { .name = "rn5t618-wdt" }, > > >
On Mon, 5 Jul 2021 09:31:29 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 5 Jul 2021 08:36:08 +0100 > Lee Jones <lee.jones@linaro.org> wrote: > > > On Sat, 03 Jul 2021, Jonathan Cameron wrote: > > > > > On Sat, 3 Jul 2021 10:42:22 +0200 > > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > > > This allows having devicetree nodes for the subdevices. > > > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > > --- > > > > drivers/mfd/rn5t618.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > > index 384acb459427..b916c7471ca3 100644 > > > > --- a/drivers/mfd/rn5t618.c > > > > +++ b/drivers/mfd/rn5t618.c > > > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > > > }; > > > > > > > > static const struct mfd_cell rc5t619_cells[] = { > > > > - { .name = "rn5t618-adc" }, > > > > - { .name = "rn5t618-power" }, > > > > + { .name = "rn5t618-adc", > > > > + .of_compatible = "ricoh,rc5t619-adc" }, > > > > > > Odd to have a name of 618 and a compatible of 619. Why? > > > Definitely deserves a comment if this is necessary for some reason! > > > > Actually this is the norm. We have lots of drivers named after the > > *first* device they supported before expansion. > > Ah. I'd missed that this cells array is specific to the 5t619, though if > the driver is the same I'd also expect it to be needed for the 5t618 entry. > Well, yes, it is needed for the 5t618 also. But if I would add it, it would be untested. And that second shorter array is also used for the rn5t567 which does not have an ADC, So I we need three arrays there. Regards, Andreas
diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c index 384acb459427..b916c7471ca3 100644 --- a/drivers/mfd/rn5t618.c +++ b/drivers/mfd/rn5t618.c @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { }; static const struct mfd_cell rc5t619_cells[] = { - { .name = "rn5t618-adc" }, - { .name = "rn5t618-power" }, + { .name = "rn5t618-adc", + .of_compatible = "ricoh,rc5t619-adc" }, + { .name = "rn5t618-power", + .of_compatible = "ricoh,rc5t619-power" }, { .name = "rn5t618-regulator" }, { .name = "rc5t619-rtc" }, { .name = "rn5t618-wdt" },
This allows having devicetree nodes for the subdevices. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- drivers/mfd/rn5t618.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)