| Message ID | 20210118003428.568892-5-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce intel_skl_int3472 driver | expand |
Hi Daniel, Thank you for the patch. On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > We want to refer to an i2c device by name before it has been s/i2c device/acpi i2c device/ ? > created by the kernel; add a function that constructs the name > from the acpi device instead. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Stopped using devm_kasprintf() > > drivers/i2c/i2c-core-acpi.c | 16 ++++++++++++++++ > include/linux/i2c.h | 5 +++++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 37c510d9347a..98c3ba9a2350 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > } > EXPORT_SYMBOL_GPL(i2c_acpi_new_device); > > +/** > + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI > + * @adev: ACPI device to construct the name for > + * > + * Constructs the name of an i2c device matching the format used by > + * i2c_dev_set_name() to allow users to refer to an i2c device by name even > + * before they have been instantiated. > + * > + * The caller is responsible for freeing the returned pointer. > + */ > +char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); There's a real danger of a memory leak, as the function name sounds very similar to dev_name() or acpi_dev_name() and those don't allocate memory. I'm not sure what a better name would be, but given that this function is only used in patch 6/7 and not in the I2C subsystem itself, I wonder if we should inline this kasprintf() call in the caller and drop this patch. > +} > +EXPORT_SYMBOL_GPL(i2c_acpi_dev_name); > + > #ifdef CONFIG_ACPI_I2C_OPREGION > static int acpi_gsb_i2c_read_bytes(struct i2c_client *client, > u8 cmd, u8 *data, u8 data_len) > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 4d40a4b46810..b82aac05b17f 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -998,6 +998,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > u32 i2c_acpi_find_bus_speed(struct device *dev); > struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > struct i2c_board_info *info); > +char *i2c_acpi_dev_name(struct acpi_device *adev); > struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle); > #else > static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > @@ -1014,6 +1015,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev, > { > return ERR_PTR(-ENODEV); > } > +static inline char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return NULL; > +} > static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle) > { > return NULL;
On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > We want to refer to an i2c device by name before it has been I²C > created by the kernel; add a function that constructs the name > from the acpi device instead. acpi -> ACPI Prefix: "i2c: core: " With above Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Stopped using devm_kasprintf() > > drivers/i2c/i2c-core-acpi.c | 16 ++++++++++++++++ > include/linux/i2c.h | 5 +++++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 37c510d9347a..98c3ba9a2350 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > } > EXPORT_SYMBOL_GPL(i2c_acpi_new_device); > > +/** > + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI > + * @adev: ACPI device to construct the name for > + * > + * Constructs the name of an i2c device matching the format used by > + * i2c_dev_set_name() to allow users to refer to an i2c device by name even > + * before they have been instantiated. > + * > + * The caller is responsible for freeing the returned pointer. > + */ > +char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > +} > +EXPORT_SYMBOL_GPL(i2c_acpi_dev_name); > + > #ifdef CONFIG_ACPI_I2C_OPREGION > static int acpi_gsb_i2c_read_bytes(struct i2c_client *client, > u8 cmd, u8 *data, u8 data_len) > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 4d40a4b46810..b82aac05b17f 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -998,6 +998,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > u32 i2c_acpi_find_bus_speed(struct device *dev); > struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > struct i2c_board_info *info); > +char *i2c_acpi_dev_name(struct acpi_device *adev); > struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle); > #else > static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > @@ -1014,6 +1015,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev, > { > return ERR_PTR(-ENODEV); > } > +static inline char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return NULL; > +} > static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle) > { > return NULL; > -- > 2.25.1 >
On Mon, Jan 18, 2021 at 11:18:55AM +0200, Laurent Pinchart wrote: > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: ... > > +/** > > + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI > > + * @adev: ACPI device to construct the name for > > + * > > + * Constructs the name of an i2c device matching the format used by > > + * i2c_dev_set_name() to allow users to refer to an i2c device by name even > > + * before they have been instantiated. > > + * > > + * The caller is responsible for freeing the returned pointer. > > + */ > > +char *i2c_acpi_dev_name(struct acpi_device *adev) > > +{ > > + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > > There's a real danger of a memory leak, as the function name sounds very > similar to dev_name() or acpi_dev_name() and those don't allocate > memory. I'm not sure what a better name would be, but given that this > function is only used in patch 6/7 and not in the I2C subsystem itself, > I wonder if we should inline this kasprintf() call in the caller and > drop this patch. Dan, I'm fine with either choice. > > +}
On Mon, 2021-01-18 at 15:39 +0200, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > > We want to refer to an i2c device by name before it has been > > I²C Andy, are you next going to suggest renaming all the files with i2c? $ git ls-files | grep i2c | wc -l 953 Please do not use the pedantic I²C, 7 bit ascii is just fine here. My keyboard does not have a superscripted 2 key, and yes, I know how to use it with multiple keypresses but it's irrelevant. > > created by the kernel; add a function that constructs the name > > from the acpi device instead. > > acpi -> ACPI Same deal with acpi filenames. Everyone already recognizes acpi is actually ACPI and there isn't any confusion in anyone's mind. $ git ls-files | grep acpi | wc -l 533 > Prefix: "i2c: core: " Please stop being a pedant on these trivial things. It's unimportant and has almost no value.
On Mon, Jan 18, 2021 at 10:43:14AM -0800, Joe Perches wrote: > On Mon, 2021-01-18 at 15:39 +0200, Andy Shevchenko wrote: > > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > > > We want to refer to an i2c device by name before it has been > > > > I²C > > Andy, are you next going to suggest renaming all the files with i2c? Where did you get this from? > Please do not use the pedantic I²C, 7 bit ascii is just fine here. > > My keyboard does not have a superscripted 2 key, and yes, I know > how to use it with multiple keypresses but it's irrelevant. I2C is fine for me as well. ... > > > created by the kernel; add a function that constructs the name > > > from the acpi device instead. > > > > acpi -> ACPI > > Same deal with acpi filenames. Where did you get about *file* names, really? Maybe you read again above? ... > > Prefix: "i2c: core: " > > Please stop being a pedant on these trivial things. > It's unimportant and has almost no value. This series is going to have a new version, that's why I did those comments. If it had been the only comments, I would have not posted them.
On Mon, 2021-01-18 at 20:56 +0200, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 10:43:14AM -0800, Joe Perches wrote: > > On Mon, 2021-01-18 at 15:39 +0200, Andy Shevchenko wrote: > > > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > > > > We want to refer to an i2c device by name before it has been > > > > > > I²C > > > > Andy, are you next going to suggest renaming all the files with i2c? > > Where did you get this from? By extension from the request to use I²C. And it's a leading question not a statement of fact.
On Mon, Jan 18, 2021 at 08:56:44PM +0200, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 10:43:14AM -0800, Joe Perches wrote: > > On Mon, 2021-01-18 at 15:39 +0200, Andy Shevchenko wrote: > > > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: ... > > > Prefix: "i2c: core: " > > > > Please stop being a pedant on these trivial things. > > It's unimportant and has almost no value. > > This series is going to have a new version, that's why I did those comments. > If it had been the only comments, I would have not posted them. And actually to the rationale: it makes slightly easier to grep for patches against same driver / group of drivers / subsystem. I bet the `... --grep 'i2c: core:' will give different results to the `... -- drivers/i2c/i2c-* include/i2c*` besides being shorter.
On Mon, Jan 18, 2021 at 9:55 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > Thank you for the patch. > > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > > We want to refer to an i2c device by name before it has been > > s/i2c device/acpi i2c device/ ? > > > created by the kernel; add a function that constructs the name > > from the acpi device instead. > > > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > > --- > > Changes in v2: > > > > - Stopped using devm_kasprintf() > > > > drivers/i2c/i2c-core-acpi.c | 16 ++++++++++++++++ > > include/linux/i2c.h | 5 +++++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > > index 37c510d9347a..98c3ba9a2350 100644 > > --- a/drivers/i2c/i2c-core-acpi.c > > +++ b/drivers/i2c/i2c-core-acpi.c > > @@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > > } > > EXPORT_SYMBOL_GPL(i2c_acpi_new_device); > > > > +/** > > + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI > > + * @adev: ACPI device to construct the name for > > + * > > + * Constructs the name of an i2c device matching the format used by > > + * i2c_dev_set_name() to allow users to refer to an i2c device by name even > > + * before they have been instantiated. > > + * > > + * The caller is responsible for freeing the returned pointer. > > + */ > > +char *i2c_acpi_dev_name(struct acpi_device *adev) > > +{ > > + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > > There's a real danger of a memory leak, as the function name sounds very > similar to dev_name() or acpi_dev_name() and those don't allocate > memory. I'm not sure what a better name would be, but given that this > function is only used in patch 6/7 and not in the I2C subsystem itself, > I wonder if we should inline this kasprintf() call in the caller and > drop this patch. IMO if this is a one-off usage, it's better to open-code it.
> > There's a real danger of a memory leak, as the function name sounds very > > similar to dev_name() or acpi_dev_name() and those don't allocate > > memory. I'm not sure what a better name would be, but given that this > > function is only used in patch 6/7 and not in the I2C subsystem itself, > > I wonder if we should inline this kasprintf() call in the caller and > > drop this patch. > > IMO if this is a one-off usage, it's better to open-code it. Sounds reasonable to me, too.
On 28/01/2021 09:00, Wolfram Sang wrote: >>> There's a real danger of a memory leak, as the function name sounds very >>> similar to dev_name() or acpi_dev_name() and those don't allocate >>> memory. I'm not sure what a better name would be, but given that this >>> function is only used in patch 6/7 and not in the I2C subsystem itself, >>> I wonder if we should inline this kasprintf() call in the caller and >>> drop this patch. >> IMO if this is a one-off usage, it's better to open-code it. > Sounds reasonable to me, too. > Just to clarify; "open-code" meaning inline it in the caller like Laurent said, right?
> Just to clarify; "open-code" meaning inline it in the caller like > Laurent said, right? Yes.
On 28/01/2021 09:17, Wolfram Sang wrote: >> Just to clarify; "open-code" meaning inline it in the caller like >> Laurent said, right? > Yes. > Thanks - will do that and drop this one then
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c index 37c510d9347a..98c3ba9a2350 100644 --- a/drivers/i2c/i2c-core-acpi.c +++ b/drivers/i2c/i2c-core-acpi.c @@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, } EXPORT_SYMBOL_GPL(i2c_acpi_new_device); +/** + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI + * @adev: ACPI device to construct the name for + * + * Constructs the name of an i2c device matching the format used by + * i2c_dev_set_name() to allow users to refer to an i2c device by name even + * before they have been instantiated. + * + * The caller is responsible for freeing the returned pointer. + */ +char *i2c_acpi_dev_name(struct acpi_device *adev) +{ + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); +} +EXPORT_SYMBOL_GPL(i2c_acpi_dev_name); + #ifdef CONFIG_ACPI_I2C_OPREGION static int acpi_gsb_i2c_read_bytes(struct i2c_client *client, u8 cmd, u8 *data, u8 data_len) diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 4d40a4b46810..b82aac05b17f 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -998,6 +998,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, u32 i2c_acpi_find_bus_speed(struct device *dev); struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, struct i2c_board_info *info); +char *i2c_acpi_dev_name(struct acpi_device *adev); struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle); #else static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, @@ -1014,6 +1015,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev, { return ERR_PTR(-ENODEV); } +static inline char *i2c_acpi_dev_name(struct acpi_device *adev) +{ + return NULL; +} static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle) { return NULL;
We want to refer to an i2c device by name before it has been created by the kernel; add a function that constructs the name from the acpi device instead. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Stopped using devm_kasprintf() drivers/i2c/i2c-core-acpi.c | 16 ++++++++++++++++ include/linux/i2c.h | 5 +++++ 2 files changed, 21 insertions(+)