[v3,3/5] platform/x86: int3472: Support multiple clock consumers

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>
---
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(-)

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;
> > +}