| Message ID | 20220921230439.768185-4-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
| Series | Add multiple-consumer support to int3472-tps68470 driver | expand |
Hi, On 9/22/22 01:04, Daniel Scally wrote: > At present, the tps68470.c only supports a single clock consumer when > passing platform data to the clock driver. In some devices multiple > sensors depend on the clock provided by a single TPS68470 and so all > need to be able to acquire the clock. Support passing multiple > consumers as platform data. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> Note this one already has a review + ack from Stephen Boyd for merging this through the pdx86 tree (from v1 of the series): https://lore.kernel.org/platform-driver-x86/20220225004943.AA8EDC340EF@smtp.kernel.org/ Reviewed-by: Stephen Boyd <sboyd@kernel.org> Acked-by: Stephen Boyd <sboyd@kernel.org> Regards, Hans > --- > Changes since v2: > > - None > > drivers/clk/clk-tps68470.c | 13 ++-- > drivers/platform/x86/intel/int3472/tps68470.c | 59 ++++++++++++++++--- > include/linux/platform_data/tps68470.h | 7 ++- > 3 files changed, 67 insertions(+), 12 deletions(-) > > diff --git a/drivers/clk/clk-tps68470.c b/drivers/clk/clk-tps68470.c > index e5fbefd6ac2d..38f44b5b9b1b 100644 > --- a/drivers/clk/clk-tps68470.c > +++ b/drivers/clk/clk-tps68470.c > @@ -200,7 +200,9 @@ static int tps68470_clk_probe(struct platform_device *pdev) > .flags = CLK_SET_RATE_GATE, > }; > struct tps68470_clkdata *tps68470_clkdata; > + struct tps68470_clk_consumer *consumer; > int ret; > + int i; > > tps68470_clkdata = devm_kzalloc(&pdev->dev, sizeof(*tps68470_clkdata), > GFP_KERNEL); > @@ -223,10 +225,13 @@ static int tps68470_clk_probe(struct platform_device *pdev) > return ret; > > if (pdata) { > - ret = devm_clk_hw_register_clkdev(&pdev->dev, > - &tps68470_clkdata->clkout_hw, > - pdata->consumer_con_id, > - pdata->consumer_dev_name); > + for (i = 0; i < pdata->n_consumers; i++) { > + consumer = &pdata->consumers[i]; > + ret = devm_clk_hw_register_clkdev(&pdev->dev, > + &tps68470_clkdata->clkout_hw, > + consumer->consumer_con_id, > + consumer->consumer_dev_name); > + } > } > > return ret; > diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c > index 22f61b47f9e5..8a684030933d 100644 > --- a/drivers/platform/x86/intel/int3472/tps68470.c > +++ b/drivers/platform/x86/intel/int3472/tps68470.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Author: Dan Scally <djrscally@gmail.com> */ > > +#include <linux/acpi.h> > #include <linux/i2c.h> > #include <linux/kernel.h> > #include <linux/mfd/core.h> > @@ -95,20 +96,64 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev) > return DESIGNED_FOR_WINDOWS; > } > > +/* > + * Return the size of the flexible array member, because we'll need that later > + * on to pass .pdata_size to cells. > + */ > +static int > +skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data **clk_pdata) > +{ > + struct acpi_device *adev = ACPI_COMPANION(dev); > + struct acpi_device *consumer; > + unsigned int n_consumers = 0; > + const char *sensor_name; > + unsigned int i = 0; > + > + for_each_acpi_consumer_dev(adev, consumer) > + n_consumers++; > + > + if (!n_consumers) { > + dev_err(dev, "INT3472 seems to have no dependents\n"); > + return -ENODEV; > + } > + > + *clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers), > + GFP_KERNEL); > + if (!*clk_pdata) > + return -ENOMEM; > + > + (*clk_pdata)->n_consumers = n_consumers; > + i = 0; > + > + for_each_acpi_consumer_dev(adev, consumer) { > + sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, > + acpi_dev_name(consumer)); > + if (!sensor_name) > + return -ENOMEM; > + > + (*clk_pdata)->consumers[i].consumer_dev_name = sensor_name; > + i++; > + } > + > + acpi_dev_put(consumer); > + > + return n_consumers; > +} > + > static int skl_int3472_tps68470_probe(struct i2c_client *client) > { > struct acpi_device *adev = ACPI_COMPANION(&client->dev); > const struct int3472_tps68470_board_data *board_data; > - struct tps68470_clk_platform_data clk_pdata = {}; > + struct tps68470_clk_platform_data *clk_pdata; > struct mfd_cell *cells; > struct regmap *regmap; > + int n_consumers; > int device_type; > int ret; > > - ret = skl_int3472_get_sensor_adev_and_name(&client->dev, NULL, > - &clk_pdata.consumer_dev_name); > - if (ret) > - return ret; > + n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata); > + if (n_consumers < 0) > + return n_consumers; > > regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); > if (IS_ERR(regmap)) { > @@ -142,8 +187,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) > * the clk + regulators must be ready when this happens. > */ > cells[0].name = "tps68470-clk"; > - cells[0].platform_data = &clk_pdata; > - cells[0].pdata_size = sizeof(clk_pdata); > + cells[0].platform_data = clk_pdata; > + cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers); > cells[1].name = "tps68470-regulator"; > cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; > cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); > diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h > index 126d082c3f2e..e605a2cab07f 100644 > --- a/include/linux/platform_data/tps68470.h > +++ b/include/linux/platform_data/tps68470.h > @@ -27,9 +27,14 @@ struct tps68470_regulator_platform_data { > const struct regulator_init_data *reg_init_data[TPS68470_NUM_REGULATORS]; > }; > > -struct tps68470_clk_platform_data { > +struct tps68470_clk_consumer { > const char *consumer_dev_name; > const char *consumer_con_id; > }; > > +struct tps68470_clk_platform_data { > + unsigned int n_consumers; > + struct tps68470_clk_consumer consumers[]; > +}; > + > #endif
On Thu, Sep 22, 2022 at 12:04:37AM +0100, Daniel Scally wrote: > At present, the tps68470.c only supports a single clock consumer when > passing platform data to the clock driver. In some devices multiple > sensors depend on the clock provided by a single TPS68470 and so all > need to be able to acquire the clock. Support passing multiple > consumers as platform data. ... > +static int > +skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data **clk_pdata) > +{ > + struct acpi_device *adev = ACPI_COMPANION(dev); > + struct acpi_device *consumer; > + unsigned int n_consumers = 0; > + const char *sensor_name; > + unsigned int i = 0; > + > + for_each_acpi_consumer_dev(adev, consumer) > + n_consumers++; Here no put for consumer (and IIUC it's correct). > + (Also no need to have a blank line here, the condition is tighten to the for-loop.) > + if (!n_consumers) { > + dev_err(dev, "INT3472 seems to have no dependents\n"); > + return -ENODEV; > + } > + > + *clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers), > + GFP_KERNEL); > + if (!*clk_pdata) > + return -ENOMEM; > + > + (*clk_pdata)->n_consumers = n_consumers; > + i = 0; > + > + for_each_acpi_consumer_dev(adev, consumer) { > + sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, > + acpi_dev_name(consumer)); > + if (!sensor_name) > + return -ENOMEM; > + > + (*clk_pdata)->consumers[i].consumer_dev_name = sensor_name; > + i++; > + } > + acpi_dev_put(consumer); Why is it here? > + return n_consumers; > +}
On Tue, Sep 27, 2022 at 03:47:17PM +0300, Andy Shevchenko wrote: > On Thu, Sep 22, 2022 at 12:04:37AM +0100, Daniel Scally wrote: > > At present, the tps68470.c only supports a single clock consumer when > > passing platform data to the clock driver. In some devices multiple > > sensors depend on the clock provided by a single TPS68470 and so all > > need to be able to acquire the clock. Support passing multiple > > consumers as platform data. ... > > +static int > > +skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data **clk_pdata) > > +{ > > + struct acpi_device *adev = ACPI_COMPANION(dev); > > + struct acpi_device *consumer; > > + unsigned int n_consumers = 0; > > + const char *sensor_name; > > + unsigned int i = 0; > > + > > + for_each_acpi_consumer_dev(adev, consumer) > > + n_consumers++; > > Here no put for consumer (and IIUC it's correct). > > > + > > (Also no need to have a blank line here, the condition is tighten to > the for-loop.) > > > + if (!n_consumers) { > > + dev_err(dev, "INT3472 seems to have no dependents\n"); > > + return -ENODEV; > > + } > > + > > + *clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers), > > + GFP_KERNEL); > > + if (!*clk_pdata) > > + return -ENOMEM; > > + > > + (*clk_pdata)->n_consumers = n_consumers; > > + i = 0; > > + > > + for_each_acpi_consumer_dev(adev, consumer) { > > + sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, > > + acpi_dev_name(consumer)); > > + if (!sensor_name) > > + return -ENOMEM; > > + > > + (*clk_pdata)->consumers[i].consumer_dev_name = sensor_name; > > + i++; > > + } > > > + acpi_dev_put(consumer); > Why is it here? Now I got it, you need to move it to the error path before returning from inside the for-loop. > > + return n_consumers; > > +}
diff --git a/drivers/clk/clk-tps68470.c b/drivers/clk/clk-tps68470.c index e5fbefd6ac2d..38f44b5b9b1b 100644 --- a/drivers/clk/clk-tps68470.c +++ b/drivers/clk/clk-tps68470.c @@ -200,7 +200,9 @@ static int tps68470_clk_probe(struct platform_device *pdev) .flags = CLK_SET_RATE_GATE, }; struct tps68470_clkdata *tps68470_clkdata; + struct tps68470_clk_consumer *consumer; int ret; + int i; tps68470_clkdata = devm_kzalloc(&pdev->dev, sizeof(*tps68470_clkdata), GFP_KERNEL); @@ -223,10 +225,13 @@ static int tps68470_clk_probe(struct platform_device *pdev) return ret; if (pdata) { - ret = devm_clk_hw_register_clkdev(&pdev->dev, - &tps68470_clkdata->clkout_hw, - pdata->consumer_con_id, - pdata->consumer_dev_name); + for (i = 0; i < pdata->n_consumers; i++) { + consumer = &pdata->consumers[i]; + ret = devm_clk_hw_register_clkdev(&pdev->dev, + &tps68470_clkdata->clkout_hw, + consumer->consumer_con_id, + consumer->consumer_dev_name); + } } return ret; diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c index 22f61b47f9e5..8a684030933d 100644 --- a/drivers/platform/x86/intel/int3472/tps68470.c +++ b/drivers/platform/x86/intel/int3472/tps68470.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 /* Author: Dan Scally <djrscally@gmail.com> */ +#include <linux/acpi.h> #include <linux/i2c.h> #include <linux/kernel.h> #include <linux/mfd/core.h> @@ -95,20 +96,64 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev) return DESIGNED_FOR_WINDOWS; } +/* + * Return the size of the flexible array member, because we'll need that later + * on to pass .pdata_size to cells. + */ +static int +skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data **clk_pdata) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + struct acpi_device *consumer; + unsigned int n_consumers = 0; + const char *sensor_name; + unsigned int i = 0; + + for_each_acpi_consumer_dev(adev, consumer) + n_consumers++; + + if (!n_consumers) { + dev_err(dev, "INT3472 seems to have no dependents\n"); + return -ENODEV; + } + + *clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers), + GFP_KERNEL); + if (!*clk_pdata) + return -ENOMEM; + + (*clk_pdata)->n_consumers = n_consumers; + i = 0; + + for_each_acpi_consumer_dev(adev, consumer) { + sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, + acpi_dev_name(consumer)); + if (!sensor_name) + return -ENOMEM; + + (*clk_pdata)->consumers[i].consumer_dev_name = sensor_name; + i++; + } + + acpi_dev_put(consumer); + + return n_consumers; +} + static int skl_int3472_tps68470_probe(struct i2c_client *client) { struct acpi_device *adev = ACPI_COMPANION(&client->dev); const struct int3472_tps68470_board_data *board_data; - struct tps68470_clk_platform_data clk_pdata = {}; + struct tps68470_clk_platform_data *clk_pdata; struct mfd_cell *cells; struct regmap *regmap; + int n_consumers; int device_type; int ret; - ret = skl_int3472_get_sensor_adev_and_name(&client->dev, NULL, - &clk_pdata.consumer_dev_name); - if (ret) - return ret; + n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata); + if (n_consumers < 0) + return n_consumers; regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); if (IS_ERR(regmap)) { @@ -142,8 +187,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) * the clk + regulators must be ready when this happens. */ cells[0].name = "tps68470-clk"; - cells[0].platform_data = &clk_pdata; - cells[0].pdata_size = sizeof(clk_pdata); + cells[0].platform_data = clk_pdata; + cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers); cells[1].name = "tps68470-regulator"; cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h index 126d082c3f2e..e605a2cab07f 100644 --- a/include/linux/platform_data/tps68470.h +++ b/include/linux/platform_data/tps68470.h @@ -27,9 +27,14 @@ struct tps68470_regulator_platform_data { const struct regulator_init_data *reg_init_data[TPS68470_NUM_REGULATORS]; }; -struct tps68470_clk_platform_data { +struct tps68470_clk_consumer { const char *consumer_dev_name; const char *consumer_con_id; }; +struct tps68470_clk_platform_data { + unsigned int n_consumers; + struct tps68470_clk_consumer consumers[]; +}; + #endif