[PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Keerthy]

Currently the driver boots only via device tree hence add a
dependency on CONFIG_OF. This leaves with a bunch of unused code
so clean that up.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---

Changes in v2:

  * Cleaned up chip_id and data attached to the match.
  * Cleaned up i2c_dev_id
  * dropped the rest of the patches in series for now

 drivers/mfd/Kconfig                    |  2 +-
 drivers/mfd/tps65217.c                 | 35 +++++-----------------------------
 drivers/regulator/tps65217-regulator.c |  5 -----
 drivers/video/backlight/tps65217_bl.c  | 14 +++-----------
 include/linux/mfd/tps65217.h           |  6 ------
 5 files changed, 9 insertions(+), 53 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 95e8683..8177cbc 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1326,7 +1326,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
diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index f769c7d..e5a1a57 100644
--- a/drivers/mfd/tps65217.c
+++ b/drivers/mfd/tps65217.c
@@ -311,37 +311,20 @@ static bool tps65217_volatile_reg(struct device *dev, unsigned int reg)
 };
 
 static const struct of_device_id tps65217_of_match[] = {
-	{ .compatible = "ti,tps65217", .data = (void *)TPS65217 },
+	{ .compatible = "ti,tps65217"},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, tps65217_of_match);
 
-static int tps65217_probe(struct i2c_client *client,
-				const struct i2c_device_id *ids)
+static int tps65217_probe(struct i2c_client *client)
 {
 	struct tps65217 *tps;
 	unsigned int version;
-	unsigned long chip_id = ids->driver_data;
-	const struct of_device_id *match;
 	bool status_off = false;
 	int ret;
 
-	if (client->dev.of_node) {
-		match = of_match_device(tps65217_of_match, &client->dev);
-		if (!match) {
-			dev_err(&client->dev,
-				"Failed to find matching dt id\n");
-			return -EINVAL;
-		}
-		chip_id = (unsigned long)match->data;
-		status_off = of_property_read_bool(client->dev.of_node,
-					"ti,pmic-shutdown-controller");
-	}
-
-	if (!chip_id) {
-		dev_err(&client->dev, "id is null.\n");
-		return -ENODEV;
-	}
+	status_off = of_property_read_bool(client->dev.of_node,
+					   "ti,pmic-shutdown-controller");
 
 	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
 	if (!tps)
@@ -349,7 +332,6 @@ static int tps65217_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, tps);
 	tps->dev = &client->dev;
-	tps->id = chip_id;
 
 	tps->regmap = devm_regmap_init_i2c(client, &tps65217_regmap_config);
 	if (IS_ERR(tps->regmap)) {
@@ -418,19 +400,12 @@ static int tps65217_remove(struct i2c_client *client)
 	return 0;
 }
 
-static const struct i2c_device_id tps65217_id_table[] = {
-	{"tps65217", TPS65217},
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
-
 static struct i2c_driver tps65217_driver = {
 	.driver		= {
 		.name	= "tps65217",
 		.of_match_table = tps65217_of_match,
 	},
-	.id_table	= tps65217_id_table,
-	.probe		= tps65217_probe,
+	.probe_new	= tps65217_probe,
 	.remove		= tps65217_remove,
 };
 
diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
index 5324dc9..7b12e88 100644
--- a/drivers/regulator/tps65217-regulator.c
+++ b/drivers/regulator/tps65217-regulator.c
@@ -228,11 +228,6 @@ static int tps65217_regulator_probe(struct platform_device *pdev)
 	int i, ret;
 	unsigned int val;
 
-	if (tps65217_chip_id(tps) != TPS65217) {
-		dev_err(&pdev->dev, "Invalid tps chip version\n");
-		return -ENODEV;
-	}
-
 	/* Allocate memory for strobes */
 	tps->strobes = devm_kzalloc(&pdev->dev, sizeof(u8) *
 				    TPS65217_NUM_REGULATOR, GFP_KERNEL);
diff --git a/drivers/video/backlight/tps65217_bl.c b/drivers/video/backlight/tps65217_bl.c
index fd524ad..5ce8791 100644
--- a/drivers/video/backlight/tps65217_bl.c
+++ b/drivers/video/backlight/tps65217_bl.c
@@ -275,17 +275,9 @@ static int tps65217_bl_probe(struct platform_device *pdev)
 	struct tps65217_bl_pdata *pdata;
 	struct backlight_properties bl_props;
 
-	if (tps->dev->of_node) {
-		pdata = tps65217_bl_parse_dt(pdev);
-		if (IS_ERR(pdata))
-			return PTR_ERR(pdata);
-	} else {
-		pdata = dev_get_platdata(&pdev->dev);
-		if (!pdata) {
-			dev_err(&pdev->dev, "no platform data provided\n");
-			return -EINVAL;
-		}
-	}
+	pdata = tps65217_bl_parse_dt(pdev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
 
 	tps65217_bl = devm_kzalloc(&pdev->dev, sizeof(*tps65217_bl),
 				GFP_KERNEL);
diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
index eac2857..b5dd108 100644
--- a/include/linux/mfd/tps65217.h
+++ b/include/linux/mfd/tps65217.h
@@ -263,7 +263,6 @@ struct tps65217_board {
 struct tps65217 {
 	struct device *dev;
 	struct tps65217_board *pdata;
-	unsigned long id;
 	struct regulator_desc desc[TPS65217_NUM_REGULATOR];
 	struct regmap *regmap;
 	u8 *strobes;
@@ -278,11 +277,6 @@ static inline struct tps65217 *dev_to_tps65217(struct device *dev)
 	return dev_get_drvdata(dev);
 }
 
-static inline unsigned long tps65217_chip_id(struct tps65217 *tps65217)
-{
-	return tps65217->id;
-}
-
 int tps65217_reg_read(struct tps65217 *tps, unsigned int reg,
 					unsigned int *val);
 int tps65217_reg_write(struct tps65217 *tps, unsigned int reg,
-- 
1.9.1

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Enric Balletbo Serra]

Hi Keerthy:

2017-06-08 12:46 GMT+02:00 Keerthy <j-keerthy@ti.com>:
> Currently the driver boots only via device tree hence add a
> dependency on CONFIG_OF. This leaves with a bunch of unused code
> so clean that up.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>

nit: I think will be good if you can also mention the change to the
probe_new in the commit message.

> ---
>
> Changes in v2:
>
>   * Cleaned up chip_id and data attached to the match.
>   * Cleaned up i2c_dev_id
>   * dropped the rest of the patches in series for now
>
>  drivers/mfd/Kconfig                    |  2 +-
>  drivers/mfd/tps65217.c                 | 35 +++++-----------------------------
>  drivers/regulator/tps65217-regulator.c |  5 -----
>  drivers/video/backlight/tps65217_bl.c  | 14 +++-----------
>  include/linux/mfd/tps65217.h           |  6 ------
>  5 files changed, 9 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 95e8683..8177cbc 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1326,7 +1326,7 @@ config MFD_TPS65090
>
>  config MFD_TPS65217
>         tristate "TI TPS65217 Power Management / White LED chips"
> -       depends on I2C
> +       depends on I2C && OF

I think you should append || COMPILE_TEST here.

>         select MFD_CORE
>         select REGMAP_I2C
>         select IRQ_DOMAIN
> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index f769c7d..e5a1a57 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -311,37 +311,20 @@ static bool tps65217_volatile_reg(struct device *dev, unsigned int reg)
>  };
>
>  static const struct of_device_id tps65217_of_match[] = {
> -       { .compatible = "ti,tps65217", .data = (void *)TPS65217 },
> +       { .compatible = "ti,tps65217"},
>         { /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, tps65217_of_match);
>
> -static int tps65217_probe(struct i2c_client *client,
> -                               const struct i2c_device_id *ids)
> +static int tps65217_probe(struct i2c_client *client)
>  {
>         struct tps65217 *tps;
>         unsigned int version;
> -       unsigned long chip_id = ids->driver_data;
> -       const struct of_device_id *match;
>         bool status_off = false;
>         int ret;
>
> -       if (client->dev.of_node) {
> -               match = of_match_device(tps65217_of_match, &client->dev);
> -               if (!match) {
> -                       dev_err(&client->dev,
> -                               "Failed to find matching dt id\n");
> -                       return -EINVAL;
> -               }
> -               chip_id = (unsigned long)match->data;
> -               status_off = of_property_read_bool(client->dev.of_node,
> -                                       "ti,pmic-shutdown-controller");
> -       }
> -
> -       if (!chip_id) {
> -               dev_err(&client->dev, "id is null.\n");
> -               return -ENODEV;
> -       }
> +       status_off = of_property_read_bool(client->dev.of_node,
> +                                          "ti,pmic-shutdown-controller");
>
>         tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
>         if (!tps)
> @@ -349,7 +332,6 @@ static int tps65217_probe(struct i2c_client *client,
>
>         i2c_set_clientdata(client, tps);
>         tps->dev = &client->dev;
> -       tps->id = chip_id;
>
>         tps->regmap = devm_regmap_init_i2c(client, &tps65217_regmap_config);
>         if (IS_ERR(tps->regmap)) {
> @@ -418,19 +400,12 @@ static int tps65217_remove(struct i2c_client *client)
>         return 0;
>  }
>
> -static const struct i2c_device_id tps65217_id_table[] = {
> -       {"tps65217", TPS65217},
> -       { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
> -
>  static struct i2c_driver tps65217_driver = {
>         .driver         = {
>                 .name   = "tps65217",
>                 .of_match_table = tps65217_of_match,
>         },
> -       .id_table       = tps65217_id_table,
> -       .probe          = tps65217_probe,
> +       .probe_new      = tps65217_probe,
>         .remove         = tps65217_remove,
>  };
>
> diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
> index 5324dc9..7b12e88 100644
> --- a/drivers/regulator/tps65217-regulator.c
> +++ b/drivers/regulator/tps65217-regulator.c
> @@ -228,11 +228,6 @@ static int tps65217_regulator_probe(struct platform_device *pdev)
>         int i, ret;
>         unsigned int val;
>
> -       if (tps65217_chip_id(tps) != TPS65217) {
> -               dev_err(&pdev->dev, "Invalid tps chip version\n");
> -               return -ENODEV;
> -       }
> -
>         /* Allocate memory for strobes */
>         tps->strobes = devm_kzalloc(&pdev->dev, sizeof(u8) *
>                                     TPS65217_NUM_REGULATOR, GFP_KERNEL);
> diff --git a/drivers/video/backlight/tps65217_bl.c b/drivers/video/backlight/tps65217_bl.c
> index fd524ad..5ce8791 100644
> --- a/drivers/video/backlight/tps65217_bl.c
> +++ b/drivers/video/backlight/tps65217_bl.c
> @@ -275,17 +275,9 @@ static int tps65217_bl_probe(struct platform_device *pdev)
>         struct tps65217_bl_pdata *pdata;
>         struct backlight_properties bl_props;
>
> -       if (tps->dev->of_node) {
> -               pdata = tps65217_bl_parse_dt(pdev);
> -               if (IS_ERR(pdata))
> -                       return PTR_ERR(pdata);
> -       } else {
> -               pdata = dev_get_platdata(&pdev->dev);
> -               if (!pdata) {
> -                       dev_err(&pdev->dev, "no platform data provided\n");
> -                       return -EINVAL;
> -               }
> -       }
> +       pdata = tps65217_bl_parse_dt(pdev);
> +       if (IS_ERR(pdata))
> +               return PTR_ERR(pdata);
>
>         tps65217_bl = devm_kzalloc(&pdev->dev, sizeof(*tps65217_bl),
>                                 GFP_KERNEL);
> diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
> index eac2857..b5dd108 100644
> --- a/include/linux/mfd/tps65217.h
> +++ b/include/linux/mfd/tps65217.h
> @@ -263,7 +263,6 @@ struct tps65217_board {
>  struct tps65217 {
>         struct device *dev;
>         struct tps65217_board *pdata;
> -       unsigned long id;
>         struct regulator_desc desc[TPS65217_NUM_REGULATOR];
>         struct regmap *regmap;
>         u8 *strobes;
> @@ -278,11 +277,6 @@ static inline struct tps65217 *dev_to_tps65217(struct device *dev)
>         return dev_get_drvdata(dev);
>  }
>
> -static inline unsigned long tps65217_chip_id(struct tps65217 *tps65217)
> -{
> -       return tps65217->id;
> -}
> -
>  int tps65217_reg_read(struct tps65217 *tps, unsigned int reg,
>                                         unsigned int *val);
>  int tps65217_reg_write(struct tps65217 *tps, unsigned int reg,
> --
> 1.9.1
>

Apart from the above minor comments:

Tested-and-reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Cheers,
 Enric

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Javier Martinez Canillas]

On Thu, Jun 8, 2017 at 3:32 PM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> Hi Keerthy:
>
> 2017-06-08 12:46 GMT+02:00 Keerthy <j-keerthy@ti.com>:
>> Currently the driver boots only via device tree hence add a
>> dependency on CONFIG_OF. This leaves with a bunch of unused code
>> so clean that up.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>

[snip]

>>
>>  config MFD_TPS65217
>>         tristate "TI TPS65217 Power Management / White LED chips"
>> -       depends on I2C
>> +       depends on I2C && OF
>
> I think you should append || COMPILE_TEST here.
>

For me it should be a separate patch, it's nice to have COMPILE_TEST
but not related to this change IMHO.

>>
>> -static const struct i2c_device_id tps65217_id_table[] = {
>> -       {"tps65217", TPS65217},
>> -       { /* sentinel */ }
>> -};
>> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);

Unfortunately you can't get rid of this table (yet) since the I2C
subsystem always reports a MODALIAS of the form "i2c:tps65217" even
when the devices have been registered via OF. There are only a couple
of drivers more to clean-up and then I'll post a patch that fixes the
I2C core to report a proper OF modalias. But for now, removing will
mean that module autoload will be broken for this driver.

Best regards,
Javier

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Keerthy]

On Thursday 08 June 2017 07:41 PM, Javier Martinez Canillas wrote:
> On Thu, Jun 8, 2017 at 3:32 PM, Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
>> Hi Keerthy:
>>
>> 2017-06-08 12:46 GMT+02:00 Keerthy <j-keerthy@ti.com>:
>>> Currently the driver boots only via device tree hence add a
>>> dependency on CONFIG_OF. This leaves with a bunch of unused code
>>> so clean that up.
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>
> 
> [snip]
> 
>>>
>>>  config MFD_TPS65217
>>>         tristate "TI TPS65217 Power Management / White LED chips"
>>> -       depends on I2C
>>> +       depends on I2C && OF
>>
>> I think you should append || COMPILE_TEST here.
>>
> 
> For me it should be a separate patch, it's nice to have COMPILE_TEST
> but not related to this change IMHO.

Yes. I will do that as a separate patch.

> 
>>>
>>> -static const struct i2c_device_id tps65217_id_table[] = {
>>> -       {"tps65217", TPS65217},
>>> -       { /* sentinel */ }
>>> -};
>>> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
> 
> Unfortunately you can't get rid of this table (yet) since the I2C
> subsystem always reports a MODALIAS of the form "i2c:tps65217" even
> when the devices have been registered via OF. There are only a couple
> of drivers more to clean-up and then I'll post a patch that fixes the
> I2C core to report a proper OF modalias. But for now, removing will
> mean that module autoload will be broken for this driver.

So this means whole logic of probe_new without i2c_device_id is not
ready? I will have to revert all that logic right?

Lee Jones,

Does that mean even for LP87565 driver we need MODULE_DEVICE_TABLE for
module autoload?

Regards,
Keerthy
> 
> Best regards,
> Javier
> 

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Javier Martinez Canillas]

On Fri, Jun 9, 2017 at 1:18 AM, Keerthy <j-keerthy@ti.com> wrote:

[snip]

>>
>>>>
>>>> -static const struct i2c_device_id tps65217_id_table[] = {
>>>> -       {"tps65217", TPS65217},
>>>> -       { /* sentinel */ }
>>>> -};
>>>> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
>>
>> Unfortunately you can't get rid of this table (yet) since the I2C
>> subsystem always reports a MODALIAS of the form "i2c:tps65217" even
>> when the devices have been registered via OF. There are only a couple
>> of drivers more to clean-up and then I'll post a patch that fixes the
>> I2C core to report a proper OF modalias. But for now, removing will
>> mean that module autoload will be broken for this driver.
>
> So this means whole logic of probe_new without i2c_device_id is not
> ready? I will have to revert all that logic right?
>

No, that's not what I meant.

It's absolutely correct for drivers that can't be build as a module
(i.e: have a boolean instead of tristate Kconfig symbol) or if you
want to get rid of the struct i2c_device_id pointed passed to your
probe callback since isn't used in the driver.

But it's not enough to get rid of the struct i2c_device_id table for
the reason I mentioned before.

> Lee Jones,
>
> Does that mean even for LP87565 driver we need MODULE_DEVICE_TABLE for
> module autoload?
>

I guess you are talking about [0], right?

Yes, it's needed because the driver can be built as a module.

[0]: https://lkml.org/lkml/2017/5/19/394

Best regards,
Javier

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Keerthy]

On Friday 09 June 2017 05:18 AM, Javier Martinez Canillas wrote:
> On Fri, Jun 9, 2017 at 1:18 AM, Keerthy <j-keerthy@ti.com> wrote:
> 
> [snip]
> 
>>>
>>>>>
>>>>> -static const struct i2c_device_id tps65217_id_table[] = {
>>>>> -       {"tps65217", TPS65217},
>>>>> -       { /* sentinel */ }
>>>>> -};
>>>>> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
>>>
>>> Unfortunately you can't get rid of this table (yet) since the I2C
>>> subsystem always reports a MODALIAS of the form "i2c:tps65217" even
>>> when the devices have been registered via OF. There are only a couple
>>> of drivers more to clean-up and then I'll post a patch that fixes the
>>> I2C core to report a proper OF modalias. But for now, removing will
>>> mean that module autoload will be broken for this driver.
>>
>> So this means whole logic of probe_new without i2c_device_id is not
>> ready? I will have to revert all that logic right?
>>
> 
> No, that's not what I meant.
> 
> It's absolutely correct for drivers that can't be build as a module
> (i.e: have a boolean instead of tristate Kconfig symbol) or if you
> want to get rid of the struct i2c_device_id pointed passed to your
> probe callback since isn't used in the driver.
> 
> But it's not enough to get rid of the struct i2c_device_id table for
> the reason I mentioned before.

Thanks for clarifying!

> 
>> Lee Jones,
>>
>> Does that mean even for LP87565 driver we need MODULE_DEVICE_TABLE for
>> module autoload?
>>
> 
> I guess you are talking about [0], right?
> 
> Yes, it's needed because the driver can be built as a module.

Okay.

> 
> [0]: https://lkml.org/lkml/2017/5/19/394
> 
> Best regards,
> Javier
> 

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Keerthy]

On Sunday 11 June 2017 10:50 AM, Keerthy wrote:
> 
> 
> On Friday 09 June 2017 05:18 AM, Javier Martinez Canillas wrote:
>> On Fri, Jun 9, 2017 at 1:18 AM, Keerthy <j-keerthy@ti.com> wrote:
>>
>> [snip]
>>
>>>>
>>>>>>
>>>>>> -static const struct i2c_device_id tps65217_id_table[] = {
>>>>>> -       {"tps65217", TPS65217},
>>>>>> -       { /* sentinel */ }
>>>>>> -};
>>>>>> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
>>>>
>>>> Unfortunately you can't get rid of this table (yet) since the I2C
>>>> subsystem always reports a MODALIAS of the form "i2c:tps65217" even
>>>> when the devices have been registered via OF. There are only a couple
>>>> of drivers more to clean-up and then I'll post a patch that fixes the
>>>> I2C core to report a proper OF modalias. But for now, removing will
>>>> mean that module autoload will be broken for this driver.
>>>
>>> So this means whole logic of probe_new without i2c_device_id is not
>>> ready? I will have to revert all that logic right?
>>>
>>
>> No, that's not what I meant.
>>
>> It's absolutely correct for drivers that can't be build as a module
>> (i.e: have a boolean instead of tristate Kconfig symbol) or if you
>> want to get rid of the struct i2c_device_id pointed passed to your
>> probe callback since isn't used in the driver.
>>
>> But it's not enough to get rid of the struct i2c_device_id table for
>> the reason I mentioned before.
> 
> Thanks for clarifying!
> 
>>
>>> Lee Jones,
>>>
>>> Does that mean even for LP87565 driver we need MODULE_DEVICE_TABLE for
>>> module autoload?
>>>
>>
>> I guess you are talking about [0], right?
>>
>> Yes, it's needed because the driver can be built as a module.
> 
> Okay.

Javier,

I found one instance where in the config is tristate MFD_MAX77686.
The driver already seems to be using probe_new.

mfd: max77686: Use the struct i2c_driver .probe_new instead of .probe

So for the above module auto probe would be broken?

- Keerthy
> 
>>
>> [0]: https://lkml.org/lkml/2017/5/19/394
>>
>> 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
> 

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Javier Martinez Canillas]

Hello Keerthy,

On Sun, Jun 11, 2017 at 8:12 PM, Keerthy <j-keerthy@ti.com> wrote:
>
>
> On Sunday 11 June 2017 10:50 AM, Keerthy wrote:
>>
>>
>> On Friday 09 June 2017 05:18 AM, Javier Martinez Canillas wrote:
>>> On Fri, Jun 9, 2017 at 1:18 AM, Keerthy <j-keerthy@ti.com> wrote:
>>>
>>> [snip]
>>>
>>>>>
>>>>>>>
>>>>>>> -static const struct i2c_device_id tps65217_id_table[] = {
>>>>>>> -       {"tps65217", TPS65217},
>>>>>>> -       { /* sentinel */ }
>>>>>>> -};
>>>>>>> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
>>>>>
>>>>> Unfortunately you can't get rid of this table (yet) since the I2C
>>>>> subsystem always reports a MODALIAS of the form "i2c:tps65217" even
>>>>> when the devices have been registered via OF. There are only a couple
>>>>> of drivers more to clean-up and then I'll post a patch that fixes the
>>>>> I2C core to report a proper OF modalias. But for now, removing will
>>>>> mean that module autoload will be broken for this driver.
>>>>
>>>> So this means whole logic of probe_new without i2c_device_id is not
>>>> ready? I will have to revert all that logic right?
>>>>
>>>
>>> No, that's not what I meant.
>>>
>>> It's absolutely correct for drivers that can't be build as a module
>>> (i.e: have a boolean instead of tristate Kconfig symbol) or if you
>>> want to get rid of the struct i2c_device_id pointed passed to your
>>> probe callback since isn't used in the driver.
>>>
>>> But it's not enough to get rid of the struct i2c_device_id table for
>>> the reason I mentioned before.
>>
>> Thanks for clarifying!
>>
>>>
>>>> Lee Jones,
>>>>
>>>> Does that mean even for LP87565 driver we need MODULE_DEVICE_TABLE for
>>>> module autoload?
>>>>
>>>
>>> I guess you are talking about [0], right?
>>>
>>> Yes, it's needed because the driver can be built as a module.
>>
>> Okay.
>
> Javier,
>
> I found one instance where in the config is tristate MFD_MAX77686.
> The driver already seems to be using probe_new.
>
> mfd: max77686: Use the struct i2c_driver .probe_new instead of .probe
>
> So for the above module auto probe would be broken?
>

Yes, and to make things even worse I see that I was the author of that
commit... so I missed that when posting the series :(

Now, arguably no one would build the MAX77686 as a module since the
regulators provided by that PMIC need to be enabled very early in the
boot process or the system will hang. Both exynos_defconfig and
multi_v7_defconfig have it as built-in. But yes, this is a regression
and the patch should be reverted (or maybe just wait for the I2C core
to report a proper modalias since no one complained, not sure what's
better for this driver).

> - Keerthy
>>
>>>

Best regards,
Javier

Re: [PATCH v2] mfd: tps65217: Introduce dependency on CONFIG_OF

[Keerthy]

On Thursday 08 June 2017 07:41 PM, Javier Martinez Canillas wrote:
> On Thu, Jun 8, 2017 at 3:32 PM, Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
>> Hi Keerthy:
>>
>> 2017-06-08 12:46 GMT+02:00 Keerthy <j-keerthy@ti.com>:
>>> Currently the driver boots only via device tree hence add a
>>> dependency on CONFIG_OF. This leaves with a bunch of unused code
>>> so clean that up.
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>
> 
> [snip]
> 
>>>
>>>  config MFD_TPS65217
>>>         tristate "TI TPS65217 Power Management / White LED chips"
>>> -       depends on I2C
>>> +       depends on I2C && OF
>>
>> I think you should append || COMPILE_TEST here.
>>
> 
> For me it should be a separate patch, it's nice to have COMPILE_TEST
> but not related to this change IMHO.

Resurrecting this patch.

> 
>>>
>>> -static const struct i2c_device_id tps65217_id_table[] = {
>>> -       {"tps65217", TPS65217},
>>> -       { /* sentinel */ }
>>> -};
>>> -MODULE_DEVICE_TABLE(i2c, tps65217_id_table);
> 
> Unfortunately you can't get rid of this table (yet) since the I2C
> subsystem always reports a MODALIAS of the form "i2c:tps65217" even
> when the devices have been registered via OF. There are only a couple
> of drivers more to clean-up and then I'll post a patch that fixes the
> I2C core to report a proper OF modalias. But for now, removing will
> mean that module autoload will be broken for this driver.

I believe now all the dependent patches are in to get rid of the above
table.

> 
> Best regards,
> Javier
>