Hi Biju, Thank you for the patch. On Mon, May 22, 2023 at 11:18:39AM +0100, Biju Das wrote: > Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main > device and another for rtc device. > > Enhance i2c_new_ancillary_device() to instantiate a real device. Doesn't it already instantiate a real device ? > (eg: Instantiate rtc device from PMIC driver) > > Added helper function __i2c_new_dummy_device to share the code > between i2c_new_dummy_device and i2c_new_ancillary_device(). > > Also added helper function __i2c_new_client_device() to pass parent dev > parameter, so that the ancillary device can assign its parent during > creation. > > Suggested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > --- > v4->v5: > * Replaced parameter dev->parent in __i2c_new_client_device() and > __i2c_new_dummy_device(). > * Improved error message in __i2c_new_dummy_device() by printing device name. > * Updated comment for ancillary's device parent > * Dropped aux_device_name check in i2c_new_ancillary_device(). > v3->v4: > * Dropped Rb tag from Geert as there are new changes. > * Introduced __i2c_new_dummy_device() to share the code between > i2c_new_dummy_device and i2c_new_ancillary_device(). > * Introduced __i2c_new_client_device() to pass parent dev > parameter, so that the ancillary device can assign its parent during > creation. > v3: > * New patch > > Ref: > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das.jz@xxxxxxxxxxxxxx/ > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +- > drivers/i2c/i2c-core-base.c | 92 +++++++++++++------- > drivers/media/i2c/adv748x/adv748x-core.c | 2 +- > drivers/media/i2c/adv7604.c | 3 +- > include/linux/i2c.h | 3 +- > 5 files changed, 69 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index ddceafa7b637..86306b010a0a 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) > int ret; > > adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, "cec", > - ADV7511_CEC_I2C_ADDR_DEFAULT); > + ADV7511_CEC_I2C_ADDR_DEFAULT, NULL); > if (IS_ERR(adv->i2c_cec)) > return PTR_ERR(adv->i2c_cec); > > @@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client *i2c) > adv7511_packet_disable(adv7511, 0xffff); > > adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid", > - ADV7511_EDID_I2C_ADDR_DEFAULT); > + ADV7511_EDID_I2C_ADDR_DEFAULT, NULL); > if (IS_ERR(adv7511->i2c_edid)) { > ret = PTR_ERR(adv7511->i2c_edid); > goto uninit_regulators; > @@ -1271,7 +1271,7 @@ static int adv7511_probe(struct i2c_client *i2c) > adv7511->i2c_edid->addr << 1); > > adv7511->i2c_packet = i2c_new_ancillary_device(i2c, "packet", > - ADV7511_PACKET_I2C_ADDR_DEFAULT); > + ADV7511_PACKET_I2C_ADDR_DEFAULT, NULL); > if (IS_ERR(adv7511->i2c_packet)) { > ret = PTR_ERR(adv7511->i2c_packet); > goto err_i2c_unregister_edid; > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index ae3af738b03f..3442aa80290f 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct resource *resources, > return 0; > } > > -/** > - * i2c_new_client_device - instantiate an i2c device > - * @adap: the adapter managing the device > - * @info: describes one I2C device; bus_num is ignored > - * Context: can sleep > - * > - * Create an i2c device. Binding is handled through driver model > - * probe()/remove() methods. A driver may be bound to this device when we > - * return from this function, or any later moment (e.g. maybe hotplugging will > - * load the driver module). This call is not appropriate for use by mainboard > - * initialization logic, which usually runs during an arch_initcall() long > - * before any i2c_adapter could exist. > - * > - * This returns the new i2c client, which may be saved for later use with > - * i2c_unregister_device(); or an ERR_PTR to describe the error. > - */ > -struct i2c_client * > -i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info) > +static struct i2c_client * > +__i2c_new_client_device(struct i2c_adapter *adap, > + struct i2c_board_info const *info, > + struct device *parent) > { > struct i2c_client *client; > int status; > @@ -944,7 +930,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf > if (status) > goto out_err; > > - client->dev.parent = &client->adapter->dev; > + client->dev.parent = parent ? parent : &client->adapter->dev; > client->dev.bus = &i2c_bus_type; > client->dev.type = &i2c_client_type; > client->dev.of_node = of_node_get(info->of_node); > @@ -984,6 +970,28 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf > kfree(client); > return ERR_PTR(status); > } > + > +/** > + * i2c_new_client_device - instantiate an i2c device > + * @adap: the adapter managing the device > + * @info: describes one I2C device; bus_num is ignored > + * Context: can sleep > + * > + * Create an i2c device. Binding is handled through driver model > + * probe()/remove() methods. A driver may be bound to this device when we > + * return from this function, or any later moment (e.g. maybe hotplugging will > + * load the driver module). This call is not appropriate for use by mainboard > + * initialization logic, which usually runs during an arch_initcall() long > + * before any i2c_adapter could exist. > + * > + * This returns the new i2c client, which may be saved for later use with > + * i2c_unregister_device(); or an ERR_PTR to describe the error. > + */ > +struct i2c_client * > +i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info) > +{ > + return __i2c_new_client_device(adap, info, NULL); > +} > EXPORT_SYMBOL_GPL(i2c_new_client_device); > > /** > @@ -1054,6 +1062,26 @@ static struct i2c_driver dummy_driver = { > .id_table = dummy_id, > }; > > +static struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter *adapter, > + u16 address, const char *name, > + struct device *parent) > +{ > + struct i2c_board_info info = { > + I2C_BOARD_INFO("dummy", address), > + }; > + > + if (name) { > + ssize_t ret = strscpy(info.type, name, sizeof(info.type)); > + > + if (ret < 0) > + return ERR_PTR(dev_err_probe(&adapter->dev, ret, > + "Invalid device name: %s\n", > + name)); > + } > + > + return __i2c_new_client_device(adapter, &info, parent); > +} > + > /** > * i2c_new_dummy_device - return a new i2c device bound to a dummy driver > * @adapter: the adapter managing the device > @@ -1074,11 +1102,7 @@ static struct i2c_driver dummy_driver = { > */ > struct i2c_client *i2c_new_dummy_device(struct i2c_adapter *adapter, u16 address) > { > - struct i2c_board_info info = { > - I2C_BOARD_INFO("dummy", address), > - }; > - > - return i2c_new_client_device(adapter, &info); > + return __i2c_new_dummy_device(adapter, address, NULL, NULL); > } > EXPORT_SYMBOL_GPL(i2c_new_dummy_device); > > @@ -1122,15 +1146,19 @@ EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_device); > * @client: Handle to the primary client > * @name: Handle to specify which secondary address to get > * @default_addr: Used as a fallback if no secondary address was specified > + * @aux_device_name: Ancillary device name > * Context: can sleep > * > * I2C clients can be composed of multiple I2C slaves bound together in a single > * component. The I2C client driver then binds to the master I2C slave and needs > - * to create I2C dummy clients to communicate with all the other slaves. > + * to create I2C ancillary clients to communicate with all the other slaves. > * > - * This function creates and returns an I2C dummy client whose I2C address is > - * retrieved from the platform firmware based on the given slave name. If no > - * address is specified by the firmware default_addr is used. > + * This function creates and returns an I2C ancillary client whose I2C address > + * is retrieved from the platform firmware based on the given slave name. if > + * aux_device_name is specified by the firmware, Unless I'm missing something, aux_device_name isn't specified by the firmware, it's a function parameter. > the ancillary's device parent > + * will be set to the primary device. This doesn't seem to match the implementation. With this patch the ancillary device's parent is always the primary device. Are you sure this won't cause any regression ? And why do you need this ? > If no address is specified by the firmware > + * default_addr is used. If no aux_device_name is specified by the firmware, it Same here regarding firmware. > + * will create an I2C dummy client. > * > * On DT-based platforms the address is retrieved from the "reg" property entry > * cell whose "reg-names" value matches the slave name. > @@ -1139,8 +1167,9 @@ EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_device); > * i2c_unregister_device(); or an ERR_PTR to describe the error. > */ > struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client, > - const char *name, > - u16 default_addr) > + const char *name, > + u16 default_addr, > + const char *aux_device_name) > { > struct device_node *np = client->dev.of_node; > u32 addr = default_addr; > @@ -1153,7 +1182,8 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client, > } > > dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr); > - return i2c_new_dummy_device(client->adapter, addr); > + return __i2c_new_dummy_device(client->adapter, addr, aux_device_name, > + &client->dev); > } > EXPORT_SYMBOL_GPL(i2c_new_ancillary_device); > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > index 4498d78a2357..5bdf7b0c6bf3 100644 > --- a/drivers/media/i2c/adv748x/adv748x-core.c > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > @@ -186,7 +186,7 @@ static int adv748x_initialise_clients(struct adv748x_state *state) > state->i2c_clients[i] = i2c_new_ancillary_device( > state->client, > adv748x_default_addresses[i].name, > - adv748x_default_addresses[i].default_addr); > + adv748x_default_addresses[i].default_addr, NULL); > > if (IS_ERR(state->i2c_clients[i])) { > adv_err(state, "failed to create i2c client %u\n", i); > diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c > index 3d0898c4175e..63fa44c9d27c 100644 > --- a/drivers/media/i2c/adv7604.c > +++ b/drivers/media/i2c/adv7604.c > @@ -2935,7 +2935,8 @@ static struct i2c_client *adv76xx_dummy_client(struct v4l2_subdev *sd, > else > new_client = i2c_new_ancillary_device(client, > adv76xx_default_addresses[page].name, > - adv76xx_default_addresses[page].default_addr); > + adv76xx_default_addresses[page].default_addr, > + NULL); > > if (!IS_ERR(new_client)) > io_write(sd, io_reg, new_client->addr << 1); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 13a1ce38cb0c..0ce344724209 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -489,7 +489,8 @@ devm_i2c_new_dummy_device(struct device *dev, struct i2c_adapter *adap, u16 addr > struct i2c_client * > i2c_new_ancillary_device(struct i2c_client *client, > const char *name, > - u16 default_addr); > + u16 default_addr, > + const char *aux_device_name); > > void i2c_unregister_device(struct i2c_client *client); > -- Regards, Laurent Pinchart
