Hi Steven, > Apologize for the late reply. I am out of work for a long time > because of my personal emergency. I am back at work now. No problem at all; Sorry to hear it, I hope things are okay now. Thanks for getting back to me though! > We are working on the following items after the first submit of the > patch: > 1. Modify the binding file to make it focused on the device > description instead of driver description. > 2. Fix the codes which doesn't following the kernel development > rules. > 3. Re-implement the SMBus Agent function with IBI mechanism instead > of polling. > SMBus Agent function requires the driver to check the status > register of the I3C hub. Polling the status register over I3C bus > exhausts too much bus resources. I3C Hub supports reporting status > changes with IBI. This shall be supported in the driver. > 4. Remove the i2c slave-mqueue module. > The slave-mqueue module is used to provide a user interface for > SMBus Agent slave function. But it has not been included in upstream. > > The first two items have been finished. And we are working on the #3 > and #4. OK, sounds good. I have a few structural suggestions; some of these have been covered by others' reviews too, but to summarise: Specificity: it looks like you're proposing this as a generic i3c hub driver, but I don't think that's the case; as far as I can tell, it's a Renesas-specific design (and IO interface), so you want to reflect that in the naming & config options. which leads to: Binding: the driver cannot match on the I3C hub class BCR, as that doesn't specify any sort of register/behavioural interface. If another hardware hub was implemented, using the same class BCR, this driver would conflict. You'll need to match on vendor/device IDs instead. Existing infrastructure: you're re-implementing a few common bits of kernel infrastructure with this change; for example the LDO settings can use the regulator devices instead. Is this included in your (2) point above? Configuration: as you've noted, the run-time configuration of the device doesn't belong in the device tree binding in most cases. In fixing those, there are a couple of approaches: * when moving to existing infrastructure (for example, the regulator and/or pinctrl devices), the consumers of those devices can request the correct configuration anyway, so you don't need to worry about the specific configuration chosen * for other runtime things, such as the downstream port configuration (in i3c mode vs. SMBus Agent mode) may be better exposed via configfs * for things that do belong as new properties in the device tree, you want numeric values (with an appropriately named property) rather than strings. * there may be other configuration settings that don't sit well in above categories, best to discuss those here. SMBus Agent i2c interface: the controller side looks fine, but the device side should be implemented as i2c_slave_event() events on the same controllers rather than the i2c-mqueue device (which isn't upstream, and doesn't look likely to be). Happy to provide specific feedback as you go, but it's the maintainers (Alexandre and Krzysztof) that will have the final say in design and style decisions. Cheers, Jeremy
