| Message ID | 6954178a8e40879a613caac60e94d08eb595cc77.1633529948.git.michal.simek@xilinx.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v3,1/3] dm: core: Bind another driver with the same compatible string | expand |
On 10/6/21 10:19 AM, Michal Simek wrote: > When DT node has pwm-cells property it shouldn't be bind as timer driver > but as PWM driver. That's why make sure that this property is checked. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > Changes in v3: > - New patch in series > > drivers/timer/cadence-ttc.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c > index 2f95d45ecd7a..2eff45060ad6 100644 > --- a/drivers/timer/cadence-ttc.c > +++ b/drivers/timer/cadence-ttc.c > @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev) > return 0; > } > > +static int cadence_ttc_bind(struct udevice *dev) > +{ > + const char *cells; > + > + cells = dev_read_prop(dev, "#pwm-cells", NULL); > + if (cells) > + return -ENODEV; > + > + return 0; > +} > + > static const struct timer_ops cadence_ttc_ops = { > .get_count = cadence_ttc_get_count, > }; > @@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = { > .priv_auto = sizeof(struct cadence_ttc_priv), > .probe = cadence_ttc_probe, > .ops = &cadence_ttc_ops, > + .bind = cadence_ttc_bind, > }; > Reviewed-by: Sean Anderson <sean.anderson@seco.com>
Hi Michal, On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote: > > When DT node has pwm-cells property it shouldn't be bind as timer driver > but as PWM driver. That's why make sure that this property is checked. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > Changes in v3: > - New patch in series > > drivers/timer/cadence-ttc.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > Why not have two compatible strings? > diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c > index 2f95d45ecd7a..2eff45060ad6 100644 > --- a/drivers/timer/cadence-ttc.c > +++ b/drivers/timer/cadence-ttc.c > @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev) > return 0; > } > > +static int cadence_ttc_bind(struct udevice *dev) > +{ > + const char *cells; > + > + cells = dev_read_prop(dev, "#pwm-cells", NULL); > + if (cells) > + return -ENODEV; We can read properties in bind() when necessary, but this is very strange...it seems like the bindings are messed up? > + > + return 0; > +} > + > static const struct timer_ops cadence_ttc_ops = { > .get_count = cadence_ttc_get_count, > }; > @@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = { > .priv_auto = sizeof(struct cadence_ttc_priv), > .probe = cadence_ttc_probe, > .ops = &cadence_ttc_ops, > + .bind = cadence_ttc_bind, > }; > -- > 2.33.0 > Regards, Simon
On 10/14/21 11:09 AM, Simon Glass wrote: > Hi Michal, > > On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote: >> >> When DT node has pwm-cells property it shouldn't be bind as timer driver >> but as PWM driver. That's why make sure that this property is checked. >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> Changes in v3: >> - New patch in series >> >> drivers/timer/cadence-ttc.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> > > Why not have two compatible strings? As I understand it, because the hardware is the same, it should have the same compatible string. >> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c >> index 2f95d45ecd7a..2eff45060ad6 100644 >> --- a/drivers/timer/cadence-ttc.c >> +++ b/drivers/timer/cadence-ttc.c >> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev) >> return 0; >> } >> >> +static int cadence_ttc_bind(struct udevice *dev) >> +{ >> + const char *cells; >> + >> + cells = dev_read_prop(dev, "#pwm-cells", NULL); >> + if (cells) >> + return -ENODEV; > > We can read properties in bind() when necessary, but this is very > strange...it seems like the bindings are messed up? This is the preferred way to select between a PWM and a timer driver. See e.g. Rob's comment for xlnx,pwm at [1]. --Sean [1] https://lore.kernel.org/linux-pwm/20210513021631.GA878860@robh.at.kernel.org/ >> + >> + return 0; >> +} >> + >> static const struct timer_ops cadence_ttc_ops = { >> .get_count = cadence_ttc_get_count, >> }; >> @@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = { >> .priv_auto = sizeof(struct cadence_ttc_priv), >> .probe = cadence_ttc_probe, >> .ops = &cadence_ttc_ops, >> + .bind = cadence_ttc_bind, >> }; >> -- >> 2.33.0 >> > > Regards, > Simon >
Hi Sean, On Thu, 14 Oct 2021 at 09:36, Sean Anderson <sean.anderson@seco.com> wrote: > > > > On 10/14/21 11:09 AM, Simon Glass wrote: > > Hi Michal, > > > > On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote: > >> > >> When DT node has pwm-cells property it shouldn't be bind as timer driver > >> but as PWM driver. That's why make sure that this property is checked. > >> > >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >> --- > >> > >> Changes in v3: > >> - New patch in series > >> > >> drivers/timer/cadence-ttc.c | 12 ++++++++++++ > >> 1 file changed, 12 insertions(+) > >> > > > > Why not have two compatible strings? > > As I understand it, because the hardware is the same, it should have the > same compatible string. > > >> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c > >> index 2f95d45ecd7a..2eff45060ad6 100644 > >> --- a/drivers/timer/cadence-ttc.c > >> +++ b/drivers/timer/cadence-ttc.c > >> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev) > >> return 0; > >> } > >> > >> +static int cadence_ttc_bind(struct udevice *dev) > >> +{ > >> + const char *cells; > >> + > >> + cells = dev_read_prop(dev, "#pwm-cells", NULL); > >> + if (cells) > >> + return -ENODEV; > > > > We can read properties in bind() when necessary, but this is very > > strange...it seems like the bindings are messed up? > > This is the preferred way to select between a PWM and a timer driver. > See e.g. Rob's comment for xlnx,pwm at [1]. I actually don't understand his comment. Is it the 'No...' one? Anyway, it seems you understand what you are doing, and this is supposed to be supported by driver model. Reviewed-by: Simon Glass <sjg@chromium.org> Regards, Simon
On 10/14/21 2:24 PM, Simon Glass wrote: > Hi Sean, > > On Thu, 14 Oct 2021 at 09:36, Sean Anderson <sean.anderson@seco.com> wrote: >> >> >> >> On 10/14/21 11:09 AM, Simon Glass wrote: >> > Hi Michal, >> > >> > On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote: >> >> >> >> When DT node has pwm-cells property it shouldn't be bind as timer driver >> >> but as PWM driver. That's why make sure that this property is checked. >> >> >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> >> --- >> >> >> >> Changes in v3: >> >> - New patch in series >> >> >> >> drivers/timer/cadence-ttc.c | 12 ++++++++++++ >> >> 1 file changed, 12 insertions(+) >> >> >> > >> > Why not have two compatible strings? >> >> As I understand it, because the hardware is the same, it should have the >> same compatible string. >> >> >> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c >> >> index 2f95d45ecd7a..2eff45060ad6 100644 >> >> --- a/drivers/timer/cadence-ttc.c >> >> +++ b/drivers/timer/cadence-ttc.c >> >> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev) >> >> return 0; >> >> } >> >> >> >> +static int cadence_ttc_bind(struct udevice *dev) >> >> +{ >> >> + const char *cells; >> >> + >> >> + cells = dev_read_prop(dev, "#pwm-cells", NULL); >> >> + if (cells) >> >> + return -ENODEV; >> > >> > We can read properties in bind() when necessary, but this is very >> > strange...it seems like the bindings are messed up? >> >> This is the preferred way to select between a PWM and a timer driver. >> See e.g. Rob's comment for xlnx,pwm at [1]. > > I actually don't understand his comment. Is it the 'No...' one? No, it's the > If a PWM, perhaps you want a '#pwm-cells' property which can serve as > the hint to configure as a PWM. --Sean > > Anyway, it seems you understand what you are doing, and this is > supposed to be supported by driver model. > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Regards, > Simon >
diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c index 2f95d45ecd7a..2eff45060ad6 100644 --- a/drivers/timer/cadence-ttc.c +++ b/drivers/timer/cadence-ttc.c @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev) return 0; } +static int cadence_ttc_bind(struct udevice *dev) +{ + const char *cells; + + cells = dev_read_prop(dev, "#pwm-cells", NULL); + if (cells) + return -ENODEV; + + return 0; +} + static const struct timer_ops cadence_ttc_ops = { .get_count = cadence_ttc_get_count, }; @@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = { .priv_auto = sizeof(struct cadence_ttc_priv), .probe = cadence_ttc_probe, .ops = &cadence_ttc_ops, + .bind = cadence_ttc_bind, };
When DT node has pwm-cells property it shouldn't be bind as timer driver but as PWM driver. That's why make sure that this property is checked. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- Changes in v3: - New patch in series drivers/timer/cadence-ttc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)