[1/2] dt-bindings: Add vendor prefix and trivial device for BluTek BPA-RS600

Add vendor prefix "blutek" for BluTek Power.
Add trivial device entry for BPA-RS600.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 2 files changed, 4 insertions(+)

On 16/03/21 3:35 pm, Chris Packham wrote:
> The BPA-RS600 is a compact 600W AC to DC removable power supply module.

>

> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

>

> +static const struct of_device_id __maybe_unused bpa_rs600_of_match[] = {

> +	{ .compatible = "blutek,bpa-rs600" },

> +	{},

> +};


I see this will fall foul of the name check in 
__hwmon_device_register(). How should I name things to avoid this?

On 16/03/21 4:35 pm, Guenter Roeck wrote:
> On 3/15/21 8:30 PM, Chris Packham wrote:

>> On 16/03/21 3:35 pm, Chris Packham wrote:

>>> The BPA-RS600 is a compact 600W AC to DC removable power supply module.

>>>

>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

>>>

>>> +static const struct of_device_id __maybe_unused bpa_rs600_of_match[] = {

>>> +	{ .compatible = "blutek,bpa-rs600" },

>>> +	{},

>>> +};

>> I see this will fall foul of the name check in

>> __hwmon_device_register(). How should I name things to avoid this?

>>

> It isn't the binding. The driver name should not have a '-' in it.

> You could just name it bpa_rs600 instead.


Sold.

I'll give the world a chance to turn so people can look at the rest of 
the patch before I send a v2.

On 3/15/21 8:41 PM, Chris Packham wrote:
> 
> On 16/03/21 4:35 pm, Guenter Roeck wrote:
>> On 3/15/21 8:30 PM, Chris Packham wrote:
>>> On 16/03/21 3:35 pm, Chris Packham wrote:
>>>> The BPA-RS600 is a compact 600W AC to DC removable power supply module.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>
>>>> +static const struct of_device_id __maybe_unused bpa_rs600_of_match[] = {
>>>> +	{ .compatible = "blutek,bpa-rs600" },
>>>> +	{},
>>>> +};
>>> I see this will fall foul of the name check in
>>> __hwmon_device_register(). How should I name things to avoid this?
>>>
>> It isn't the binding. The driver name should not have a '-' in it.
>> You could just name it bpa_rs600 instead.
> 
> Sold.
> 

Maybe not. Maybe the client name is derived from the bindings name.
If so, we'll have to work around it. Please give it a try and let me know.

Guenter

On 16/03/21 4:43 pm, Guenter Roeck wrote:
> On 3/15/21 7:35 PM, Chris Packham wrote:

>> The BPA-RS600 is a compact 600W AC to DC removable power supply module.

>>

>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

>> ---

<snip>
>> +

>> +static int bpa_rs600_read_word_data(struct i2c_client *client, int page,

>> +		int phase, int reg)+{

>> +	int ret;

>> +

>> +	if (page > 0)

>> +		return -ENXIO;

>> +

>> +	switch (reg) {

>> +	case PMBUS_VIN_UV_FAULT_LIMIT:

>> +	case PMBUS_VIN_OV_FAULT_LIMIT:

>> +	case PMBUS_VOUT_UV_FAULT_LIMIT:

>> +	case PMBUS_VOUT_OV_FAULT_LIMIT:

>> +		ret = -ENXIO;

> Is that needed ? Why not -ENODATA ?


Basically these commands get responses on the bus but they don't have 
valid data (nor are they documented in the datasheet). I'll add a 
comment to that effect.

If I'm reading things correctly -ENODATA is a signal to 
_pmbus_read_word_data use the "normal" read operation. So I need to 
return something other than that. I found another driver (mp2975.c) 
doing the same thing for what I assume are similar reasons so I went 
with -ENXIO.

>

>> +		break;

<snip>
> +

>> +static const struct i2c_device_id bpa_rs600_id[] = {

>> +	{ "bpa_rs600", 0 },

> Hmm, no, this has an underscore. Guess you'll have to use the trick from

> iio_hwmon.c or similar to generate a valid name.

>

> Oh, wait, this is a pmbus driver, and the pmbus core uses client->name.

> Maybe we need to add an optional strreplace() to the pmbus core.

Looking into this now.

On 3/15/21 9:21 PM, Chris Packham wrote:
> 
> On 16/03/21 4:43 pm, Guenter Roeck wrote:
>> On 3/15/21 7:35 PM, Chris Packham wrote:
>>> The BPA-RS600 is a compact 600W AC to DC removable power supply module.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
> <snip>
>>> +
>>> +static int bpa_rs600_read_word_data(struct i2c_client *client, int page,
>>> +		int phase, int reg)+{
>>> +	int ret;
>>> +
>>> +	if (page > 0)
>>> +		return -ENXIO;
>>> +
>>> +	switch (reg) {
>>> +	case PMBUS_VIN_UV_FAULT_LIMIT:
>>> +	case PMBUS_VIN_OV_FAULT_LIMIT:
>>> +	case PMBUS_VOUT_UV_FAULT_LIMIT:
>>> +	case PMBUS_VOUT_OV_FAULT_LIMIT:
>>> +		ret = -ENXIO;
>> Is that needed ? Why not -ENODATA ?
> 
> Basically these commands get responses on the bus but they don't have 
> valid data (nor are they documented in the datasheet). I'll add a 
> comment to that effect.
> 
> If I'm reading things correctly -ENODATA is a signal to 
> _pmbus_read_word_data use the "normal" read operation. So I need to 

Correct.

> return something other than that. I found another driver (mp2975.c) 
> doing the same thing for what I assume are similar reasons so I went 
> with -ENXIO.
> 
-ENXIO is ok, but please document it.

Thanks,
Guenter