| Message ID | 20201130133129.1024662-17-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
Hi Daniel, Thank you for the patch. On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote: > From: Dan Scally <djrscally@gmail.com> > > To make sure the new i2c_acpi_dev_name() always reflects the name of i2c > devices sourced from ACPI, use it in i2c_set_dev_name(). > > Signed-off-by: Dan Scally <djrscally@gmail.com> I'd squash this with 15/18, which would make it clear there's a memory leak :-) > --- > Changes since RFC v3: > > - Patch introduced > > drivers/i2c/i2c-core-base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 573b5da145d1..a6d4ceb01077 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -814,7 +814,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, > } > > if (adev) { > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); > + dev_set_name(&client->dev, i2c_acpi_dev_name(adev)); > return; > } >
On Mon, Nov 30, 2020 at 07:12:41PM +0200, Laurent Pinchart wrote: > On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote: > > From: Dan Scally <djrscally@gmail.com> > > > > To make sure the new i2c_acpi_dev_name() always reflects the name of i2c > > devices sourced from ACPI, use it in i2c_set_dev_name(). > > > > Signed-off-by: Dan Scally <djrscally@gmail.com> > > I'd squash this with 15/18, which would make it clear there's a memory > leak :-) ... > > if (adev) { > > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); > > + dev_set_name(&client->dev, i2c_acpi_dev_name(adev)); > > return; But you split pattern used in i2c_dev_set_name(). What you need is to provide something like this #define I2C_DEV_NAME_FORMAT "i2c-%s" const char *i2c_acpi_get_dev_name(...) { return kasprintf(..., I2C_DEV_NAME_FORMAT, ...); } (Possible in the future if anybody needs const char *i2c_dev_get_name_by_bus_and_addr(int bus, unsigned short addr) ) And here - dev_set_name(&client->dev, "i2c-%s", info->dev_name); + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name); - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev));
On 30/11/2020 17:12, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote: >> From: Dan Scally <djrscally@gmail.com> >> >> To make sure the new i2c_acpi_dev_name() always reflects the name of i2c >> devices sourced from ACPI, use it in i2c_set_dev_name(). >> >> Signed-off-by: Dan Scally <djrscally@gmail.com> > > I'd squash this with 15/18, which would make it clear there's a memory > leak :-) Ah - that was sloppy...switched from devm_ and forgot to go fix that. I'll add the kfree into i2c_unregister_device() and squash to 15/18 >> --- >> Changes since RFC v3: >> >> - Patch introduced >> >> drivers/i2c/i2c-core-base.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 573b5da145d1..a6d4ceb01077 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -814,7 +814,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, >> } >> >> if (adev) { >> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); >> + dev_set_name(&client->dev, i2c_acpi_dev_name(adev)); >> return; >> } >> >
Dan, does this mail among other my replies reach you? It seems you answered to Laurent's mails and leaving mine ignored. Just wondering if our servers have an issue again... On Mon, Nov 30, 2020 at 09:18:56PM +0200, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 07:12:41PM +0200, Laurent Pinchart wrote: > > On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote: > > > From: Dan Scally <djrscally@gmail.com> > > > > > > To make sure the new i2c_acpi_dev_name() always reflects the name of i2c > > > devices sourced from ACPI, use it in i2c_set_dev_name(). > > > > > > Signed-off-by: Dan Scally <djrscally@gmail.com> > > > > I'd squash this with 15/18, which would make it clear there's a memory > > leak :-) > > ... > > > > if (adev) { > > > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); > > > + dev_set_name(&client->dev, i2c_acpi_dev_name(adev)); > > > return; > > But you split pattern used in i2c_dev_set_name(). > What you need is to provide something like this > > #define I2C_DEV_NAME_FORMAT "i2c-%s" > > const char *i2c_acpi_get_dev_name(...) > { > return kasprintf(..., I2C_DEV_NAME_FORMAT, ...); > } > > (Possible in the future if anybody needs > const char *i2c_dev_get_name_by_bus_and_addr(int bus, unsigned short addr) > ) > > And here > > - dev_set_name(&client->dev, "i2c-%s", info->dev_name); > + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name); > > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); > + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > > -- > With Best Regards, > Andy Shevchenko; > >
On 02/12/2020 09:35, Andy Shevchenko wrote: > Dan, does this mail among other my replies reach you? > It seems you answered to Laurent's mails and leaving mine ignored. Just > wondering if our servers have an issue again... Morning - I got it, sorry. I just read Laurent's first and then called it a night > On Mon, Nov 30, 2020 at 09:18:56PM +0200, Andy Shevchenko wrote: >> On Mon, Nov 30, 2020 at 07:12:41PM +0200, Laurent Pinchart wrote: >>> On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote: >>>> From: Dan Scally <djrscally@gmail.com> >>>> >>>> To make sure the new i2c_acpi_dev_name() always reflects the name of i2c >>>> devices sourced from ACPI, use it in i2c_set_dev_name(). >>>> >>>> Signed-off-by: Dan Scally <djrscally@gmail.com> >>> I'd squash this with 15/18, which would make it clear there's a memory >>> leak :-) >> ... >> >>>> if (adev) { >>>> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); >>>> + dev_set_name(&client->dev, i2c_acpi_dev_name(adev)); >>>> return; >> But you split pattern used in i2c_dev_set_name(). >> What you need is to provide something like this >> >> #define I2C_DEV_NAME_FORMAT "i2c-%s" >> >> const char *i2c_acpi_get_dev_name(...) >> { >> return kasprintf(..., I2C_DEV_NAME_FORMAT, ...); >> } >> >> (Possible in the future if anybody needs >> const char *i2c_dev_get_name_by_bus_and_addr(int bus, unsigned short addr) >> ) >> >> And here >> >> - dev_set_name(&client->dev, "i2c-%s", info->dev_name); >> + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name); >> >> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); >> + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); >> Yeah ok, I like this approach much better, I'll switch to that.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 573b5da145d1..a6d4ceb01077 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -814,7 +814,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, } if (adev) { - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); + dev_set_name(&client->dev, i2c_acpi_dev_name(adev)); return; }