| Message ID | 20210118003428.568892-5-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce intel_skl_int3472 driver | expand |
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 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.
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?
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(+)