Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
Hi Vladimir, > > I'll try to find some time to review the whole changeset carefully, > > in fact I briefly reviewed it two months ago, but I didn't find > > anything obviously wrong that time. > > Please put that on hold until I have rebased ontop of v4.6-rc1 and > changed a couple of other things. I'd hate for you to waste your > time on outdated patches. v6 is out now. Any help with reviewing/testing is much appreciated! Thanks, Wolfram signature.asc Description: PGP signature
Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
Hi Vladimir, On 2016-03-24 15:24, Vladimir Zapolskiy wrote: > On 24.03.2016 13:05, Peter Rosin wrote: >> On 2016-03-24 10:50, Vladimir Zapolskiy wrote: >>> On 05.01.2016 17:57, Peter Rosin wrote: @@ -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); >>> >>> why do you prefer here to use "unlocked" version of API? >>> >>> Foe example would it be safe/possible to unload an I2C bus device driver >>> module or unbind I2C device itself in runtime? >> >> I think you ask why I change from of_get_i2c_... to of_find_i2c_..., and that >> change was not intentional. It was the result of a bad merge during an early >> rebase. >> >> Does that cover it? >> > > Yep, thank you for clarification, please account this in v3. Oh , v3 is old news, v4 was sent out some weeks ago, and there is a v5 on a github branch. This bad rebase was fixed in v4. > I'll try to find some time to review the whole changeset carefully, > in fact I briefly reviewed it two months ago, but I didn't find > anything obviously wrong that time. Please put that on hold until I have rebased ontop of v4.6-rc1 and changed a couple of other things. I'd hate for you to waste your time on outdated patches. Cheers, Peter
Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
Hi Peter, On 24.03.2016 13:05, Peter Rosin wrote: > Hi Vladimir, > > On 2016-03-24 10:50, Vladimir Zapolskiy wrote: >> Hi Peter, >> >> On 05.01.2016 17:57, Peter Rosin wrote: >>> From: Peter Rosin >>> >>> 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 >> >> is it still under review? If yes, please find one question from me below :) > > Yes, the series is still under review/testing, with an update planned in a > week or so. > >> [snip] >> >>> @@ -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); >> >> why do you prefer here to use "unlocked" version of API? >> >> Foe example would it be safe/possible to unload an I2C bus device driver >> module or unbind I2C device itself in runtime? > > I think you ask why I change from of_get_i2c_... to of_find_i2c_..., and that > change was not intentional. It was the result of a bad merge during an early > rebase. > > Does that cover it? > Yep, thank you for clarification, please account this in v3. I'll try to find some time to review the whole changeset carefully, in fact I briefly reviewed it two months ago, but I didn't find anything obviously wrong that time. -- With best wishes, Vladimir
Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
Hi Vladimir, On 2016-03-24 10:50, Vladimir Zapolskiy wrote: > Hi Peter, > > On 05.01.2016 17:57, Peter Rosin wrote: >> From: Peter Rosin >> >> 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 > > is it still under review? If yes, please find one question from me below :) Yes, the series is still under review/testing, with an update planned in a week or so. > [snip] > >> @@ -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); > > why do you prefer here to use "unlocked" version of API? > > Foe example would it be safe/possible to unload an I2C bus device driver > module or unbind I2C device itself in runtime? I think you ask why I change from of_get_i2c_... to of_find_i2c_..., and that change was not intentional. It was the result of a bad merge during an early rebase. Does that cover it? Cheers, Peter
Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
Hi Peter,
On 05.01.2016 17:57, Peter Rosin wrote:
> From: Peter Rosin
>
> 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
is it still under review? If yes, please find one question from me below :)
[snip]
> @@ -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);
why do you prefer here to use "unlocked" version of API?
Foe example would it be safe/possible to unload an I2C bus device driver
module or unbind I2C device itself in runtime?
> 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;
--
With best wishes,
Vladimir
Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
Hi Guenter,
[BTW, if anyone feels spammed by this series, please drop me a note]
On 2016-01-05 17:42, Guenter Roeck wrote:
> On 01/05/2016 07:57 AM, Peter Rosin wrote:
>> From: Peter Rosin
>>
>> 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
>> ---
>> 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[
Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
On 01/05/2016 07:57 AM, Peter Rosin wrote:
From: Peter Rosin
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
---
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
