Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > 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 ? No, as per the code it instantiates dummy device[1] [1] https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1156 > > > (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: > > > > --- > > 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. It is specified in the platform firmware(device tree firmware). > > > 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 ? There is no regression as existing users only instantiate dummy device. > And why do you need this ? As per Krzysztof [2], The DT schema allows multiple addresses for children. But we lack implementation of parent child relationship, As parent owns the resources. Child device needs to parse parent node to get some resource like clocks. [2] https://lore.kernel.org/linux-renesas-soc/TYCPR01MB5933BFFD4EB556F5FB4EA82186729@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ Cheers, Biju > > > 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
