Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Laurent Pinchart]

Hi Kieran,

On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote:
> On 29/01/18 10:26, Laurent Pinchart wrote:
> > On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
> >> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
> >> ports. Each map has it own I²C address and acts as a standard slave
> >> device on the I²C bus.
> >> 
> >> Allow a device tree node to override the default addresses so that
> >> address conflicts with other devices on the same bus may be resolved at
> >> the board description level.
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> >> ---
> >> 
> >>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
> > 
> > I don't mind personally, but device tree maintainers usually ask for DT
> > bindings changes to be split to a separate patch.
> > 
> >>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
> >>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++-----
> >>  3 files changed, 37 insertions(+), 13 deletions(-)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> index 0047b1394c70..f6bb9f6d3f48 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> 
> >> @@ -70,6 +70,9 @@ Optional properties:
> >>    rather than generate its own timings for HDMI output.
> >>  
> >>  - clocks: from common clock binding: reference to the CEC clock.
> >>  - clock-names: from common clock binding: must be "cec".
> >> 
> >> +- reg-names : Names of maps with programmable addresses.
> >> +	It can contain any map needing a non-default address.
> >> +	Possible maps names are : "main", "edid", "cec", "packet"
> > 
> > Is the reg-names property (and the additional maps) mandatory or optional
> > ? If mandatory you should also update the existing DT sources that use
> > those bindings.
> 
> They are currently optional. I do prefer it that way - but perhaps due to an
> issue mentioned below we might need to make this driver mandatory ?
> 
> > If optional you should define which I2C addresses will be used when
> > the maps are not specified (and in that case I think we should go for
> > the addresses listed as default in the datasheet, which correspond to the
> > current driver implementation when the main address is 0x3d/0x7a).
> 
> The current addresses do not correspond to the datasheet, even when the
> implementation main address is set to 0x3d.

Don't they ? The driver currently uses the following (8-bit) I2C addresses:

EDID:   main + 4  = 0x7e (0x3f)
Packet: main - 10 = 0x70 (0x38)
CEC:    main - 2  = 0x78 (0x3c)

Those are the default addresses according to section 4.1 of the ADV7511W 
programming guide (rev. B), and they match the ones you set in this patch.

> Thus, in my opinion - they are currently 'wrong' - but perhaps changing them
> is considered breakage too.
> 
> A particular issue will arise here too - as on this device - when the device
> is in low-power mode (after probe, before use) - the maps will respond on
> their *hardware default* addresses (the ones implemented in this patch),
> and thus consume that address on the I2C bus regardless of their settings
> in the driver.

We've discussed this previously and I share you concern. Just to make sure I 
remember correctly, did all the secondary maps reset to their default 
addresses, or just the EDID map ?

> > You should also update the definition of the reg property that currently
> > just states
> > 
> > - reg: I2C slave address
> > 
> > And finally you might want to define the term "map" in this context.
> > Here's a proposal (if we make all maps mandatory).
> > 
> > The ADV7511 internal registers are split into four pages exposed through
> > different I2C addresses, creating four register maps. The I2C addresses of
> > all four maps shall be specified by the reg and reg-names property.
> > 
> > - reg: I2C slave addresses, one per reg-names entry
> > - reg-names: map names, shall be "main", "edid", "cec", "packet"
> > 
> >>  Required nodes:
> >> @@ -88,7 +91,12 @@ Example
> >> 
> >>  	adv7511w: hdmi@39 {
> >>  		compatible = "adi,adv7511w";
> >> -		reg = <39>;
> >> +		/*
> >> +		 * The EDID page will be accessible on address 0x66 on the i2c
> >> +		 * bus. All other maps continue to use their default addresses.
> >> +		 */
> >> +		reg = <0x39 0x66>;
> >> +		reg-names = "main", "edid";
> >>  		interrupt-parent = <&gpio3>;
> >>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> >>  		clocks = <&cec_clock>;

[snip]

> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> index efa29db5fc2b..7ec33837752b 100644
> >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c

[snip]

> >> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
> >> const struct i2c_device_id *id)
> >>  	if (ret)
> >>  		goto uninit_regulators;
> >> 
> >> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> >> edid_i2c_addr);
> >> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> >> -		     main_i2c_addr - 0xa);
> >> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> >> -		     main_i2c_addr - 2);
> >> -
> >>  	adv7511_packet_disable(adv7511, 0xffff);
> >> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
> >> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> >> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
> >>  	if (!adv7511->i2c_edid) {
> >>  		ret = -ENOMEM;
> > 
> > I wonder if this is the right error code. Maybe -EINVAL ? In most cases
> > errors will be caused by invalid addresses (out of memory and
> > device_register() failures can happen too but should be less common).
> 
> It's arbitrary, as multiple error paths simply return NULL, but I think you
> are right - the 'most common' fault here is likely to be an in use address.
> 
> -ENOMEM was chosen here to mirror the equivalent code returned from the
> adv7604 driver.
> 
> Either way - -EINVAL does feel better at the moment. Although when mirrored
> back to the ADV7604 that becomes a distinct change there.
> 
> I don't think that matters though - so I'll apply this fix to both patches.
> 
> > It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but
> > that's out of scope.
> 
> I was about to say - well I'll add this, but the reality is it might require
> updating all users of i2c_new_device, and i2c_new_dummy. Updating
> i2c_new_secondary_device() on it's own seems a bit pointless otherwise.
> 
> And that is definitely out of scope for the amount of time I currently have
> :D
> 
> >>  		goto uninit_regulators;
> >>  	}
> >> 
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> >> +		     adv7511->i2c_edid->addr << 1);
> >> +
> >>  	ret = adv7511_init_cec_regmap(adv7511);
> >>  	if (ret)
> >>  		goto err_i2c_unregister_edid;
> >> 
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> >> +		     adv7511->i2c_cec->addr << 1);
> >> +
> >> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> >> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
> >> +	if (!adv7511->i2c_packet) {
> >> +		ret = -ENOMEM;
> >> +		goto err_unregister_cec;
> >> +	}
> >> +
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> >> +		     adv7511->i2c_packet->addr << 1);
> >> +
> >>  	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
> >>  	
> >>  	if (i2c->irq) {

[snip]

-- 
Regards,

Laurent Pinchart