| Message ID | 1496760340-27959-4-git-send-email-j-keerthy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Keerthy, By change I was looking at this. Some comments below that I think can be applied to all patches in this series 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: > Currently the driver boots only via device tree hence add a > dependency on OF. > > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > drivers/mfd/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 75b59f1..2d1425d 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1297,7 +1297,7 @@ config MFD_TPS65090 > > config MFD_TPS65217 > tristate "TI TPS65217 Power Management / White LED chips" > - depends on I2C > + depends on I2C && OF Shouldn't you add || COMPILE_TEST here ? > select MFD_CORE > select REGMAP_I2C > select IRQ_DOMAIN > -- > 1.9.1 > I think you can remove the of_match_device checks in some drivers too i.e: http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 Regards, Enric -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 06 June 2017 08:34 PM, Enric Balletbo Serra wrote: > Hi Keerthy, > > By change I was looking at this. Some comments below that I think can > be applied to all patches in this series > > 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: >> Currently the driver boots only via device tree hence add a >> dependency on OF. >> >> Signed-off-by: Keerthy <j-keerthy@ti.com> >> --- >> drivers/mfd/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index 75b59f1..2d1425d 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -1297,7 +1297,7 @@ config MFD_TPS65090 >> >> config MFD_TPS65217 >> tristate "TI TPS65217 Power Management / White LED chips" >> - depends on I2C >> + depends on I2C && OF > > Shouldn't you add || COMPILE_TEST here ? Sure. > >> select MFD_CORE >> select REGMAP_I2C >> select IRQ_DOMAIN >> -- >> 1.9.1 >> > > I think you can remove the of_match_device checks in some drivers too > > i.e: > > http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 Yes that and removal of unused i2c_device_id. I will follow it up once this OF dependency is in. > > Regards, > Enric > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 07 Jun 2017, Keerthy wrote: > > > On Tuesday 06 June 2017 08:34 PM, Enric Balletbo Serra wrote: > > Hi Keerthy, > > > > By change I was looking at this. Some comments below that I think can > > be applied to all patches in this series > > > > 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: > >> Currently the driver boots only via device tree hence add a > >> dependency on OF. > >> > >> Signed-off-by: Keerthy <j-keerthy@ti.com> > >> --- > >> drivers/mfd/Kconfig | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >> index 75b59f1..2d1425d 100644 > >> --- a/drivers/mfd/Kconfig > >> +++ b/drivers/mfd/Kconfig > >> @@ -1297,7 +1297,7 @@ config MFD_TPS65090 > >> > >> config MFD_TPS65217 > >> tristate "TI TPS65217 Power Management / White LED chips" > >> - depends on I2C > >> + depends on I2C && OF > > > > Shouldn't you add || COMPILE_TEST here ? > > Sure. > > > > >> select MFD_CORE > >> select REGMAP_I2C > >> select IRQ_DOMAIN > >> > > > > I think you can remove the of_match_device checks in some drivers too > > > > i.e: > > > > http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 > > Yes that and removal of unused i2c_device_id. I will follow it up once > this OF dependency is in. The of_match_device() checks should be removed with the OF patch.
On Wed, 07 Jun 2017, Lee Jones wrote: > On Wed, 07 Jun 2017, Keerthy wrote: > > > > > > > On Tuesday 06 June 2017 08:34 PM, Enric Balletbo Serra wrote: > > > Hi Keerthy, > > > > > > By change I was looking at this. Some comments below that I think can > > > be applied to all patches in this series > > > > > > 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: > > >> Currently the driver boots only via device tree hence add a > > >> dependency on OF. > > >> > > >> Signed-off-by: Keerthy <j-keerthy@ti.com> > > >> --- > > >> drivers/mfd/Kconfig | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > >> index 75b59f1..2d1425d 100644 > > >> --- a/drivers/mfd/Kconfig > > >> +++ b/drivers/mfd/Kconfig > > >> @@ -1297,7 +1297,7 @@ config MFD_TPS65090 > > >> > > >> config MFD_TPS65217 > > >> tristate "TI TPS65217 Power Management / White LED chips" > > >> - depends on I2C > > >> + depends on I2C && OF > > > > > > Shouldn't you add || COMPILE_TEST here ? > > > > Sure. > > > > > > > >> select MFD_CORE > > >> select REGMAP_I2C > > >> select IRQ_DOMAIN > > >> > > > > > > I think you can remove the of_match_device checks in some drivers too > > > > > > i.e: > > > > > > http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 > > > > Yes that and removal of unused i2c_device_id. I will follow it up once > > this OF dependency is in. > > The of_match_device() checks should be removed with the OF patch. In fact, just squash these changes into the I2C removal patches.
On Wednesday 07 June 2017 04:07 PM, Lee Jones wrote: > On Wed, 07 Jun 2017, Keerthy wrote: > >> >> >> On Tuesday 06 June 2017 08:34 PM, Enric Balletbo Serra wrote: >>> Hi Keerthy, >>> >>> By change I was looking at this. Some comments below that I think can >>> be applied to all patches in this series >>> >>> 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: >>>> Currently the driver boots only via device tree hence add a >>>> dependency on OF. >>>> >>>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>>> --- >>>> drivers/mfd/Kconfig | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>> index 75b59f1..2d1425d 100644 >>>> --- a/drivers/mfd/Kconfig >>>> +++ b/drivers/mfd/Kconfig >>>> @@ -1297,7 +1297,7 @@ config MFD_TPS65090 >>>> >>>> config MFD_TPS65217 >>>> tristate "TI TPS65217 Power Management / White LED chips" >>>> - depends on I2C >>>> + depends on I2C && OF >>> >>> Shouldn't you add || COMPILE_TEST here ? >> >> Sure. >> >>> >>>> select MFD_CORE >>>> select REGMAP_I2C >>>> select IRQ_DOMAIN >>>> >>> >>> I think you can remove the of_match_device checks in some drivers too >>> >>> i.e: >>> >>> http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 >> >> Yes that and removal of unused i2c_device_id. I will follow it up once >> this OF dependency is in. > > The of_match_device() checks should be removed with the OF patch. okay > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 07 June 2017 04:08 PM, Lee Jones wrote: > On Wed, 07 Jun 2017, Lee Jones wrote: > >> On Wed, 07 Jun 2017, Keerthy wrote: >> >>> >>> >>> On Tuesday 06 June 2017 08:34 PM, Enric Balletbo Serra wrote: >>>> Hi Keerthy, >>>> >>>> By change I was looking at this. Some comments below that I think can >>>> be applied to all patches in this series >>>> >>>> 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: >>>>> Currently the driver boots only via device tree hence add a >>>>> dependency on OF. >>>>> >>>>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>>>> --- >>>>> drivers/mfd/Kconfig | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>> index 75b59f1..2d1425d 100644 >>>>> --- a/drivers/mfd/Kconfig >>>>> +++ b/drivers/mfd/Kconfig >>>>> @@ -1297,7 +1297,7 @@ config MFD_TPS65090 >>>>> >>>>> config MFD_TPS65217 >>>>> tristate "TI TPS65217 Power Management / White LED chips" >>>>> - depends on I2C >>>>> + depends on I2C && OF >>>> >>>> Shouldn't you add || COMPILE_TEST here ? >>> >>> Sure. >>> >>>> >>>>> select MFD_CORE >>>>> select REGMAP_I2C >>>>> select IRQ_DOMAIN >>>>> >>>> >>>> I think you can remove the of_match_device checks in some drivers too >>>> >>>> i.e: >>>> >>>> http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 >>> >>> Yes that and removal of unused i2c_device_id. I will follow it up once >>> this OF dependency is in. >> >> The of_match_device() checks should be removed with the OF patch. Lee Jones/ Enric, IIUC of_match_device call is still needed to obtain a match and in case there are multiple compatibles with different match data then this call is definitely needed. There is no need to check for return value as we will find one match for sure and that can be removed. Even checks like 'if (client->dev.of_node) {' can surely be removed with depends on OF. Please correct me if i am wrong. Regards, Keerthy > > In fact, just squash these changes into the I2C removal patches. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-06-07 13:24 GMT+02:00 Keerthy <j-keerthy@ti.com>: > > > On Wednesday 07 June 2017 04:08 PM, Lee Jones wrote: >> On Wed, 07 Jun 2017, Lee Jones wrote: >> >>> On Wed, 07 Jun 2017, Keerthy wrote: >>> >>>> >>>> >>>> On Tuesday 06 June 2017 08:34 PM, Enric Balletbo Serra wrote: >>>>> Hi Keerthy, >>>>> >>>>> By change I was looking at this. Some comments below that I think can >>>>> be applied to all patches in this series >>>>> >>>>> 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: >>>>>> Currently the driver boots only via device tree hence add a >>>>>> dependency on OF. >>>>>> >>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>>>>> --- >>>>>> drivers/mfd/Kconfig | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>> index 75b59f1..2d1425d 100644 >>>>>> --- a/drivers/mfd/Kconfig >>>>>> +++ b/drivers/mfd/Kconfig >>>>>> @@ -1297,7 +1297,7 @@ config MFD_TPS65090 >>>>>> >>>>>> config MFD_TPS65217 >>>>>> tristate "TI TPS65217 Power Management / White LED chips" >>>>>> - depends on I2C >>>>>> + depends on I2C && OF >>>>> >>>>> Shouldn't you add || COMPILE_TEST here ? >>>> >>>> Sure. >>>> >>>>> >>>>>> select MFD_CORE >>>>>> select REGMAP_I2C >>>>>> select IRQ_DOMAIN >>>>>> >>>>> >>>>> I think you can remove the of_match_device checks in some drivers too >>>>> >>>>> i.e: >>>>> >>>>> http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 >>>> >>>> Yes that and removal of unused i2c_device_id. I will follow it up once >>>> this OF dependency is in. >>> >>> The of_match_device() checks should be removed with the OF patch. > > Lee Jones/ Enric, > > IIUC of_match_device call is still needed to obtain a match and in case > there are multiple compatibles with different match data then this call > is definitely needed. > Not sure if I follow you. My understanding is that with DT the probe of this driver is only called if there is a node with the compatible = "ti,tps65217" string. So if probe is called there is always a match and the call to of_match_device is redundant. > There is no need to check for return value as we will find one match for > sure and that can be removed. > > Even checks like 'if (client->dev.of_node) {' can surely be removed with > depends on OF. > Yes I think this should be removed too. > Please correct me if i am wrong. > > Regards, > Keerthy >> >> In fact, just squash these changes into the I2C removal patches. >> Regards, Enric -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 07 June 2017 07:07 PM, Enric Balletbo Serra wrote: > 2017-06-07 13:24 GMT+02:00 Keerthy <j-keerthy@ti.com>: >> >> >> On Wednesday 07 June 2017 04:08 PM, Lee Jones wrote: >>> On Wed, 07 Jun 2017, Lee Jones wrote: >>> >>>> On Wed, 07 Jun 2017, Keerthy wrote: >>>> >>>>> >>>>> >>>>> On Tuesday 06 June 2017 08:34 PM, Enric Balletbo Serra wrote: >>>>>> Hi Keerthy, >>>>>> >>>>>> By change I was looking at this. Some comments below that I think can >>>>>> be applied to all patches in this series >>>>>> >>>>>> 2017-06-06 16:45 GMT+02:00 Keerthy <j-keerthy@ti.com>: >>>>>>> Currently the driver boots only via device tree hence add a >>>>>>> dependency on OF. >>>>>>> >>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>>>>>> --- >>>>>>> drivers/mfd/Kconfig | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>>> index 75b59f1..2d1425d 100644 >>>>>>> --- a/drivers/mfd/Kconfig >>>>>>> +++ b/drivers/mfd/Kconfig >>>>>>> @@ -1297,7 +1297,7 @@ config MFD_TPS65090 >>>>>>> >>>>>>> config MFD_TPS65217 >>>>>>> tristate "TI TPS65217 Power Management / White LED chips" >>>>>>> - depends on I2C >>>>>>> + depends on I2C && OF >>>>>> >>>>>> Shouldn't you add || COMPILE_TEST here ? >>>>> >>>>> Sure. >>>>> >>>>>> >>>>>>> select MFD_CORE >>>>>>> select REGMAP_I2C >>>>>>> select IRQ_DOMAIN >>>>>>> >>>>>> >>>>>> I think you can remove the of_match_device checks in some drivers too >>>>>> >>>>>> i.e: >>>>>> >>>>>> http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 >>>>> >>>>> Yes that and removal of unused i2c_device_id. I will follow it up once >>>>> this OF dependency is in. >>>> >>>> The of_match_device() checks should be removed with the OF patch. >> >> Lee Jones/ Enric, >> >> IIUC of_match_device call is still needed to obtain a match and in case >> there are multiple compatibles with different match data then this call >> is definitely needed. >> > > Not sure if I follow you. My understanding is that with DT the probe > of this driver is only called if there is a node with the compatible = > "ti,tps65217" string. So if probe is called there is always a match > and the call to of_match_device is redundant. How will you get the matching data? For the tps65217 case you mentioned we need the match pointer to get the chip_id right? chip_id = (unsigned long)match->data; Also one more case of when we have multiple compatibles with different matching data. Ex: http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/palmas.c#L522 You need the match pointer to get the corresponding data. Hope i am clear. > >> There is no need to check for return value as we will find one match for >> sure and that can be removed. >> >> Even checks like 'if (client->dev.of_node) {' can surely be removed with >> depends on OF. >> > > Yes I think this should be removed too. > >> Please correct me if i am wrong. >> >> Regards, >> Keerthy >>> >>> In fact, just squash these changes into the I2C removal patches. >>> > > Regards, > Enric > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Keerthy, On Wed, Jun 7, 2017 at 3:45 PM, Keerthy <j-keerthy@ti.com> wrote: [snip] >>>>>>>> >>>>>>> >>>>>>> I think you can remove the of_match_device checks in some drivers too >>>>>>> >>>>>>> i.e: >>>>>>> >>>>>>> http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 >>>>>> >>>>>> Yes that and removal of unused i2c_device_id. I will follow it up once >>>>>> this OF dependency is in. >>>>> >>>>> The of_match_device() checks should be removed with the OF patch. >>> >>> Lee Jones/ Enric, >>> >>> IIUC of_match_device call is still needed to obtain a match and in case >>> there are multiple compatibles with different match data then this call >>> is definitely needed. >>> That's correct... >> >> Not sure if I follow you. My understanding is that with DT the probe >> of this driver is only called if there is a node with the compatible = >> "ti,tps65217" string. So if probe is called there is always a match >> and the call to of_match_device is redundant. > > How will you get the matching data? > > For the tps65217 case you mentioned we need the match pointer to get the > chip_id right? > > chip_id = (unsigned long)match->data; > ...but this particular driver only has a single entry in the OF table and so you can just do: tps->id = TPS65217; Later if there's a variant for this chip, then you can add the logic to query the struct of_device_id .data. But for now I think that's better to just remove as Enric proposes and also remove the .data field from the struct of_device_id entry. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 7, 2017 at 4:10 PM, Javier Martinez Canillas <javier@dowhile0.org> wrote: [snip] >> >> chip_id = (unsigned long)match->data; >> > > ...but this particular driver only has a single entry in the OF table > and so you can just do: > > tps->id = TPS65217; > In fact, it seems that the whole chip id logic can go away since no one is using it. I see that the regulator driver checks for the chip id value but I don't think that makes sense since the regulator driver's probe function won't be called for other chips since they won't match. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 07 June 2017 07:40 PM, Javier Martinez Canillas wrote: > Hello Keerthy, > > On Wed, Jun 7, 2017 at 3:45 PM, Keerthy <j-keerthy@ti.com> wrote: > > [snip] > >>>>>>>>> >>>>>>>> >>>>>>>> I think you can remove the of_match_device checks in some drivers too >>>>>>>> >>>>>>>> i.e: >>>>>>>> >>>>>>>> http://elixir.free-electrons.com/linux/latest/source/drivers/mfd/tps65217.c#L330 >>>>>>> >>>>>>> Yes that and removal of unused i2c_device_id. I will follow it up once >>>>>>> this OF dependency is in. >>>>>> >>>>>> The of_match_device() checks should be removed with the OF patch. >>>> >>>> Lee Jones/ Enric, >>>> >>>> IIUC of_match_device call is still needed to obtain a match and in case >>>> there are multiple compatibles with different match data then this call >>>> is definitely needed. >>>> > > That's correct... That is what i wanted to know. Thanks. > >>> >>> Not sure if I follow you. My understanding is that with DT the probe >>> of this driver is only called if there is a node with the compatible = >>> "ti,tps65217" string. So if probe is called there is always a match >>> and the call to of_match_device is redundant. >> >> How will you get the matching data? >> >> For the tps65217 case you mentioned we need the match pointer to get the >> chip_id right? >> >> chip_id = (unsigned long)match->data; >> > > ...but this particular driver only has a single entry in the OF table > and so you can just do: > > tps->id = TPS65217; > > Later if there's a variant for this chip, then you can add the logic > to query the struct of_device_id .data. But for now I think that's > better to just remove as Enric proposes and also remove the .data > field from the struct of_device_id entry. okay agreed for tps65217. > > Best regards, > Javier > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 75b59f1..2d1425d 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1297,7 +1297,7 @@ config MFD_TPS65090 config MFD_TPS65217 tristate "TI TPS65217 Power Management / White LED chips" - depends on I2C + depends on I2C && OF select MFD_CORE select REGMAP_I2C select IRQ_DOMAIN
Currently the driver boots only via device tree hence add a dependency on OF. Signed-off-by: Keerthy <j-keerthy@ti.com> --- drivers/mfd/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)