Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance

[Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>]

On 01/05/2016 07:57 AM, Peter Rosin wrote:
> From: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>
> The initial core mux structure starts off small with only the parent
> adapter pointer, which all muxes have, and a priv pointer for mux
> driver private data.
>
> Add i2c_mux_alloc function to unify the creation of a mux.
>
> Where appropriate, pass around the mux core structure instead of the
> parent adapter or the driver private data.
>
> Remove the parent adapter pointer from the driver private data for all
> mux drivers.
>
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> ---
>   drivers/i2c/i2c-mux.c                        | 28 +++++++++++++++++-----
>   drivers/i2c/muxes/i2c-arb-gpio-challenge.c   | 24 +++++++++----------
>   drivers/i2c/muxes/i2c-mux-gpio.c             | 20 ++++++++--------
>   drivers/i2c/muxes/i2c-mux-pca9541.c          | 35 ++++++++++++++--------------
>   drivers/i2c/muxes/i2c-mux-pca954x.c          | 19 ++++++++++-----
>   drivers/i2c/muxes/i2c-mux-pinctrl.c          | 23 +++++++++---------
>   drivers/i2c/muxes/i2c-mux-reg.c              | 23 ++++++++++--------
>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c   | 10 +++++++-
>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h    |  1 +
>   drivers/media/dvb-frontends/m88ds3103.c      | 10 +++++++-
>   drivers/media/dvb-frontends/m88ds3103_priv.h |  1 +
>   drivers/media/dvb-frontends/rtl2830.c        | 10 +++++++-
>   drivers/media/dvb-frontends/rtl2830_priv.h   |  1 +
>   drivers/media/dvb-frontends/rtl2832.c        | 10 +++++++-
>   drivers/media/dvb-frontends/rtl2832_priv.h   |  1 +
>   drivers/media/dvb-frontends/si2168.c         | 10 +++++++-
>   drivers/media/dvb-frontends/si2168_priv.h    |  1 +
>   drivers/media/usb/cx231xx/cx231xx-core.c     |  3 +++
>   drivers/media/usb/cx231xx/cx231xx-i2c.c      | 13 +++++++++--
>   drivers/media/usb/cx231xx/cx231xx.h          |  2 ++
>   drivers/of/unittest.c                        | 16 +++++++------
>   include/linux/i2c-mux.h                      | 14 ++++++++++-
>   22 files changed, 187 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 00fc5b1c7b66..c2163f6b51d5 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -31,8 +31,8 @@
>   struct i2c_mux_priv {
>   	struct i2c_adapter adap;
>   	struct i2c_algorithm algo;
> +	struct i2c_mux_core *muxc;
>
> -	struct i2c_adapter *parent;
>   	struct device *mux_dev;
>   	void *mux_priv;
>   	u32 chan_id;
> @@ -45,7 +45,8 @@ static int i2c_mux_master_xfer(struct i2c_adapter *adap,
>   			       struct i2c_msg msgs[], int num)
>   {
>   	struct i2c_mux_priv *priv = adap->algo_data;
> -	struct i2c_adapter *parent = priv->parent;
> +	struct i2c_mux_core *muxc = priv->muxc;
> +	struct i2c_adapter *parent = muxc->parent;
>   	int ret;
>
>   	/* Switch to the right mux port and perform the transfer. */
> @@ -65,7 +66,8 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
>   			      int size, union i2c_smbus_data *data)
>   {
>   	struct i2c_mux_priv *priv = adap->algo_data;
> -	struct i2c_adapter *parent = priv->parent;
> +	struct i2c_mux_core *muxc = priv->muxc;
> +	struct i2c_adapter *parent = muxc->parent;
>   	int ret;
>
>   	/* Select the right mux port and perform the transfer. */
> @@ -84,7 +86,7 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
>   static u32 i2c_mux_functionality(struct i2c_adapter *adap)
>   {
>   	struct i2c_mux_priv *priv = adap->algo_data;
> -	struct i2c_adapter *parent = priv->parent;
> +	struct i2c_adapter *parent = priv->muxc->parent;
>
>   	return parent->algo->functionality(parent);
>   }
> @@ -102,7 +104,20 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
>   	return class;
>   }
>
> -struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
> +struct i2c_mux_core *i2c_mux_alloc(struct device *dev, int sizeof_priv)
> +{
> +	struct i2c_mux_core *muxc;
> +
> +	muxc = devm_kzalloc(dev, sizeof(*muxc) + sizeof_priv, GFP_KERNEL);
> +	if (!muxc)
> +		return NULL;
> +	if (sizeof_priv)
> +		muxc->priv = muxc + 1;
> +	return muxc;
> +}
> +EXPORT_SYMBOL_GPL(i2c_mux_alloc);
> +
> +struct i2c_adapter *i2c_add_mux_adapter(struct i2c_mux_core *muxc,
>   				struct device *mux_dev,
>   				void *mux_priv, u32 force_nr, u32 chan_id,
>   				unsigned int class,
> @@ -111,6 +126,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>   				int (*deselect) (struct i2c_adapter *,
>   						 void *, u32))
>   {
> +	struct i2c_adapter *parent = muxc->parent;
>   	struct i2c_mux_priv *priv;
>   	char symlink_name[20];
>   	int ret;
> @@ -120,7 +136,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>   		return NULL;
>
>   	/* Set up private adapter data */
> -	priv->parent = parent;
> +	priv->muxc = muxc;
>   	priv->mux_dev = mux_dev;
>   	priv->mux_priv = mux_priv;
>   	priv->chan_id = chan_id;
> diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> index 402e3a6c671a..6e27ea4fb25a 100644
> --- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> +++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> @@ -42,7 +42,6 @@
>    */
>
>   struct i2c_arbitrator_data {
> -	struct i2c_adapter *parent;
>   	struct i2c_adapter *child;
>   	int our_gpio;
>   	int our_gpio_release;
> @@ -119,6 +118,7 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>   	struct device *dev = &pdev->dev;
>   	struct device_node *np = dev->of_node;
>   	struct device_node *parent_np;
> +	struct i2c_mux_core *muxc;
>   	struct i2c_arbitrator_data *arb;
>   	enum of_gpio_flags gpio_flags;
>   	unsigned long out_init;
> @@ -134,13 +134,12 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>   		return -EINVAL;
>   	}
>
> -	arb = devm_kzalloc(dev, sizeof(*arb), GFP_KERNEL);
> -	if (!arb) {
> -		dev_err(dev, "Cannot allocate i2c_arbitrator_data\n");
> +	muxc = i2c_mux_alloc(dev, sizeof(*arb));
> +	if (!muxc)
>   		return -ENOMEM;
> -	}
> -	platform_set_drvdata(pdev, arb);
> +	arb = i2c_mux_priv(muxc);
>
> +	platform_set_drvdata(pdev, muxc);
>   	/* Request GPIOs */
>   	ret = of_get_named_gpio_flags(np, "our-claim-gpio", 0, &gpio_flags);
>   	if (!gpio_is_valid(ret)) {
> @@ -196,21 +195,21 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>   		dev_err(dev, "Cannot parse i2c-parent\n");
>   		return -EINVAL;
>   	}
> -	arb->parent = of_get_i2c_adapter_by_node(parent_np);
> +	muxc->parent = of_find_i2c_adapter_by_node(parent_np);
>   	of_node_put(parent_np);
> -	if (!arb->parent) {
> +	if (!muxc->parent) {
>   		dev_err(dev, "Cannot find parent bus\n");
>   		return -EPROBE_DEFER;
>   	}
>
>   	/* Actually add the mux adapter */
> -	arb->child = i2c_add_mux_adapter(arb->parent, dev, arb, 0, 0, 0,
> +	arb->child = i2c_add_mux_adapter(muxc, dev, arb, 0, 0, 0,
>   					 i2c_arbitrator_select,
>   					 i2c_arbitrator_deselect);
>   	if (!arb->child) {
>   		dev_err(dev, "Failed to add adapter\n");
>   		ret = -ENODEV;
> -		i2c_put_adapter(arb->parent);
> +		i2c_put_adapter(muxc->parent);
>   	}
>
>   	return ret;
> @@ -218,10 +217,11 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>
>   static int i2c_arbitrator_remove(struct platform_device *pdev)
>   {
> -	struct i2c_arbitrator_data *arb = platform_get_drvdata(pdev);
> +	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> +	struct i2c_arbitrator_data *arb = i2c_mux_priv(muxc);
>
>   	i2c_del_mux_adapter(arb->child);
> -	i2c_put_adapter(arb->parent);
> +	i2c_put_adapter(muxc->parent);
>
>   	return 0;
>   }
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index b8e11c16d98c..ee43dd76a4d7 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -18,7 +18,6 @@
>   #include <linux/of_gpio.h>
>
>   struct gpiomux {
> -	struct i2c_adapter *parent;
>   	struct i2c_adapter **adap; /* child busses */
>   	struct i2c_mux_gpio_platform_data data;
>   	unsigned gpio_base;
> @@ -136,19 +135,19 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>
>   static int i2c_mux_gpio_probe(struct platform_device *pdev)
>   {
> +	struct i2c_mux_core *muxc;
>   	struct gpiomux *mux;
>   	struct i2c_adapter *parent;
>   	int (*deselect) (struct i2c_adapter *, void *, u32);
>   	unsigned initial_state, gpio_base;
>   	int i, ret;
>
> -	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> -	if (!mux) {
> -		dev_err(&pdev->dev, "Cannot allocate gpiomux structure");
> +	muxc = i2c_mux_alloc(&pdev->dev, sizeof(*mux));
> +	if (!muxc)
>   		return -ENOMEM;
> -	}
> +	mux = i2c_mux_priv(muxc);
>
> -	platform_set_drvdata(pdev, mux);
> +	platform_set_drvdata(pdev, muxc);
>
>   	if (!dev_get_platdata(&pdev->dev)) {
>   		ret = i2c_mux_gpio_probe_dt(mux, pdev);
> @@ -180,7 +179,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>   	if (!parent)
>   		return -EPROBE_DEFER;
>
> -	mux->parent = parent;
> +	muxc->parent = parent;
>   	mux->gpio_base = gpio_base;
>
>   	mux->adap = devm_kzalloc(&pdev->dev,
> @@ -223,7 +222,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>   		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
>   		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
>
> -		mux->adap[i] = i2c_add_mux_adapter(parent, &pdev->dev, mux, nr,
> +		mux->adap[i] = i2c_add_mux_adapter(muxc, &pdev->dev, mux, nr,
>   						   mux->data.values[i], class,
>   						   i2c_mux_gpio_select, deselect);
>   		if (!mux->adap[i]) {
> @@ -253,7 +252,8 @@ alloc_failed:
>
>   static int i2c_mux_gpio_remove(struct platform_device *pdev)
>   {
> -	struct gpiomux *mux = platform_get_drvdata(pdev);
> +	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> +	struct gpiomux *mux = i2c_mux_priv(muxc);
>   	int i;
>
>   	for (i = 0; i < mux->data.n_values; i++)
> @@ -262,7 +262,7 @@ static int i2c_mux_gpio_remove(struct platform_device *pdev)
>   	for (i = 0; i < mux->data.n_gpios; i++)
>   		gpio_free(mux->gpio_base + mux->data.gpios[i]);
>
> -	i2c_put_adapter(mux->parent);
> +	i2c_put_adapter(muxc->parent);
>
>   	return 0;
>   }
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index d0ba424adebc..47ae2259d1ca 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -73,6 +73,7 @@
>   #define SELECT_DELAY_LONG	1000
>
>   struct pca9541 {
> +	struct i2c_client *client;

I fail to see where this is being used.

>   	struct i2c_adapter *mux_adap;
>   	unsigned long select_timeout;
>   	unsigned long arb_timeout;
> @@ -217,7 +218,8 @@ static const u8 pca9541_control[16] = {
>    */
>   static int pca9541_arbitrate(struct i2c_client *client)
>   {
> -	struct pca9541 *data = i2c_get_clientdata(client);
> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +	struct pca9541 *data = i2c_mux_priv(muxc);
>   	int reg;
>
>   	reg = pca9541_reg_read(client, PCA9541_CONTROL);
> @@ -324,20 +326,22 @@ static int pca9541_probe(struct i2c_client *client,
>   {
>   	struct i2c_adapter *adap = client->adapter;
>   	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct i2c_mux_core *muxc;
>   	struct pca9541 *data;
>   	int force;
> -	int ret = -ENODEV;
>
>   	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
> -		goto err;
> +		return -ENODEV;
>
> -	data = kzalloc(sizeof(struct pca9541), GFP_KERNEL);
> -	if (!data) {
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> +	muxc = i2c_mux_alloc(&client->dev, sizeof(*data));
> +	if (!muxc)
> +		return -ENOMEM;
> +	data = i2c_mux_priv(muxc);
> +
> +	i2c_set_clientdata(client, muxc);
>
> -	i2c_set_clientdata(client, data);
> +	data->client = client;
> +	muxc->parent = adap;
>
>   	/*
>   	 * I2C accesses are unprotected here.
> @@ -352,34 +356,29 @@ static int pca9541_probe(struct i2c_client *client,
>   	force = 0;
>   	if (pdata)
>   		force = pdata->modes[0].adap_id;
> -	data->mux_adap = i2c_add_mux_adapter(adap, &client->dev, client,
> +	data->mux_adap = i2c_add_mux_adapter(muxc, &client->dev, client,
>   					     force, 0, 0,
>   					     pca9541_select_chan,
>   					     pca9541_release_chan);
>
>   	if (data->mux_adap == NULL) {
>   		dev_err(&client->dev, "failed to register master selector\n");
> -		goto exit_free;
> +		return -ENODEV;
>   	}
>
>   	dev_info(&client->dev, "registered master selector for I2C %s\n",
>   		 client->name);
>
>   	return 0;
> -
> -exit_free:
> -	kfree(data);
> -err:
> -	return ret;
>   }
>
>   static int pca9541_remove(struct i2c_client *client)
>   {
> -	struct pca9541 *data = i2c_get_clientdata(client);
> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +	struct pca9541 *data = i2c_mux_priv(muxc);
>
>   	i2c_del_mux_adapter(data->mux_adap);
>
> -	kfree(data);
>   	return 0;
>   }
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index acfcef3d4068..a4df831fae9d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -63,6 +63,7 @@ struct pca954x {
>   	struct i2c_adapter *virt_adaps[PCA954X_MAX_NCHANS];
>
>   	u8 last_chan;		/* last register value */
> +	struct i2c_client *client;

Same here.

Maybe it starts to be used in a later patch (I did not check), but then it should be defined
where it is used.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html